Re: [patch 2.6.13-rc6] Fix kmem read on 32-bit archs

Re: [patch 2.6.13-rc6] Fix kmem read on 32-bit archs

[Chuck Ebbert]

On Sun, 14 Aug 2005 09:24:02 -0700 (PDT), Linus Torvalds wrote:

> Yes, the thing needs to be opened with O_LARGEFILE and you need to use 
> "llseek()" to seek into it, but once you do that, everything should be 
> fine.

  GCC warns about using llseek and suggests lseek64 instead. That works
for me, but up till 2.6.11 plain lseek worked too.  I guess it really
shouldn't have?

__
Chuck

Re: [patch 2.6.13-rc6] Fix kmem read on 32-bit archs

[Linus Torvalds]

On Sun, 14 Aug 2005, Chuck Ebbert wrote:
> 
>   GCC warns about using llseek and suggests lseek64 instead. That works
> for me, but up till 2.6.11 plain lseek worked too.  I guess it really
> shouldn't have?

Well, we have had various special-cases for /dev/[k]mem to try to make it 
work even with 32-bit lseek in the past (or with 64-bit lseek on archs 
that need the high bit set). So I wouldn't say "shouldn't have" - I'd love 
it if it worked, but my ove for it working is somewhat tempered by the 
need for it not screwing up everything else in the kernel ;)

Now, it's entirely possible that we shoul djust make "sys_lseek()" work
differently: right now it _always_ sign-extends its offset from "off_t" to
"loff_t", but the thing is, for a SEEK_SET, it probably makes more sense
to zero-extend it (since a negative SEEK_SET just doesn't make any sense).

So a hacky alternative might be something like the appended, but I have to 
admit that it's pretty damn hacky even if it makes sense at some level..

Oh, btw, this will cause it to still return EOVERFLOW, you'd need to also 
change that error return logic.

		Linus

-----
diff --git a/fs/read_write.c b/fs/read_write.c
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -137,7 +137,11 @@ asmlinkage off_t sys_lseek(unsigned int 
 
 	retval = -EINVAL;
 	if (origin <= 2) {
-		loff_t res = vfs_llseek(file, offset, origin);
+		/* SEEK_SET zero-extends, others sign-extend */
+		loff_t res = offset;
+		if (!origin)
+			res = (unsigned long)offset;
+		res = vfs_llseek(file, res, origin);
 		retval = res;
 		if (res != (loff_t)retval)
 			retval = -EOVERFLOW;	/* LFS: should only happen on 32 bit platforms */

Re: [patch 2.6.13-rc6] Fix kmem read on 32-bit archs

[Linus Torvalds]

On Sun, 14 Aug 2005, Chuck Ebbert wrote:
> 
>   It's ugly but so is the existing code.  And it won't fix 64-bit
> archs AFAICT.  Tested on 2.6.11, patch offsets fixed up for 2.6.13-rc6.

Btw, if you really want to allow negative ("huge positive") loff_t, then
you should do it the way we did "file->f_maxcount", and just have a u64
"file->f_maxpos" that we initialize to LONG_LONG_MAX or something like
that by default.

Then, the kmem_open() routine could set it to ULONG_LONG_MAX, and the
rw_verify_area check would change from comparing pos for being negative to
an unsigned compare of pos against "f_maxpos".

(That would also allow filesystems to limit the position to 32-bit values
if they wanted to, and felt that they didn't want to even handle anything
else).

			Linus

Re: [patch 2.6.13-rc6] Fix kmem read on 32-bit archs

[Linus Torvalds]

On Sun, 14 Aug 2005, Chuck Ebbert wrote:
>
>   The first thing drivers/char/mem.c:read_kmem does is convert the
> loff_t it gets as the offset for reading into an unsigned int.  This
> patch makes the kmem driver's llseek operator do that up-front, so
> that fs/read_write.c:rw_verify_area doesn't return -EINVAL when
> we try to read from higher addresses.

Why would rw_verify_area() return -EINVAL? It does

        if (unlikely((pos < 0) || (loff_t) (pos + count) < 0))
                goto Einval;

but pos and "loff_t" is 64-bit, and if the thing is negative in 64 bits, 
then that thing really _should_ fail.

Yes, the thing needs to be opened with O_LARGEFILE and you need to use 
"llseek()" to seek into it, but once you do that, everything should be 
fine.

		Linus

Re: [patch 2.6.13-rc6] Fix kmem read on 32-bit archs

[Andi Kleen]

On Sun, Aug 14, 2005 at 12:12:52PM -0400, Chuck Ebbert wrote:
>   The first thing drivers/char/mem.c:read_kmem does is convert the
> loff_t it gets as the offset for reading into an unsigned int.  This
> patch makes the kmem driver's llseek operator do that up-front, so
> that fs/read_write.c:rw_verify_area doesn't return -EINVAL when
> we try to read from higher addresses.
> 
>   It's ugly but so is the existing code.  And it won't fix 64-bit
> archs AFAICT.  Tested on 2.6.11, patch offsets fixed up for 2.6.13-rc6.

I already have a full patch for this issue queued for 2.6.14.

-Andi

[patch 2.6.13-rc6] Fix kmem read on 32-bit archs

[Chuck Ebbert]

  The first thing drivers/char/mem.c:read_kmem does is convert the
loff_t it gets as the offset for reading into an unsigned int.  This
patch makes the kmem driver's llseek operator do that up-front, so
that fs/read_write.c:rw_verify_area doesn't return -EINVAL when
we try to read from higher addresses.

  It's ugly but so is the existing code.  And it won't fix 64-bit
archs AFAICT.  Tested on 2.6.11, patch offsets fixed up for 2.6.13-rc6.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

Index: 2.6.13-rc6/drivers/char/mem.c
===================================================================
--- 2.6.13-rc6.orig/drivers/char/mem.c	2005-08-14 11:49:11.000000000 -0400
+++ 2.6.13-rc6/drivers/char/mem.c	2005-08-14 11:51:15.000000000 -0400
@@ -747,6 +747,29 @@
 	return ret;
 }
 
+static loff_t kmem_lseek(struct file * file, loff_t offset, int orig)
+{
+	loff_t ret = -EINVAL;
+
+	if (orig == 2)  /* sys_seek/llseek has checked orig = {012} */
+		goto out;
+
+	down(&file->f_dentry->d_inode->i_sem);
+	switch (orig) {
+		case 0:
+			file->f_pos = offset;
+			break;
+		case 1:
+			file->f_pos += offset;
+	}
+	ret = file->f_pos;
+	file->f_pos = (loff_t)(unsigned long)file->f_pos;
+	force_successful_syscall_return();
+	up(&file->f_dentry->d_inode->i_sem);
+out:
+	return ret;
+}
+
 static int open_port(struct inode * inode, struct file * filp)
 {
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
@@ -769,7 +792,7 @@
 };
 
 static struct file_operations kmem_fops = {
-	.llseek		= memory_lseek,
+	.llseek		= kmem_lseek,
 	.read		= read_kmem,
 	.write		= write_kmem,
 	.mmap		= mmap_kmem,
__
Chuck