32 bit Linux lseek missing overflow check (was: Re: Linuxolator patches: stat and lseek SEEK_END for disk devices)

[Juergen Lock]

In article <201003020750.20845.jhb at freebsd.org> you write:
>On Monday 01 March 2010 1:25:53 pm John Baldwin wrote:
>> On Friday 26 February 2010 4:09:52 pm Juergen Lock wrote:
>> > On Fri, Feb 26, 2010 at 11:21:05AM -0500, John Baldwin wrote:
>> > > On Thursday 25 February 2010 3:28:50 pm Juergen Lock wrote:
>> > > > On Tue, Feb 23, 2010 at 10:50:10PM +0100, Juergen Lock wrote:
>> > > > > Hi!
>> > > > > 
>> > > > >  Before this gets buried on -hackers in another thead... :)
>> > > > > 
>> > > > >  I now have disks appear as block devices for Linux processes (there
>> > > > > already was commented out code for that in linux_stats.c, I hope my
>> > > > > version is now `correct enough' to be usable [1]), and I made a simple
>> > > > > patch to make lseek SEEK_END (L_XTND in the source) dtrt on disk
>> > > > > devices too by simply invoking the DIOCGMEDIASIZE ioctl there; [2]
>> > > > > both of these things are what (some) Linux processes expect.
>> > > > > 
>> > > > >  Patches are here: (made on stable/8, if they don't apply on head
>> > > > > I'll have to make extra versions for that...)
>> > > > > 	http://people.freebsd.org/~nox/linuxdisk-blk.patch [1]
>> > > > > 	http://people.freebsd.org/~nox/lseek-seek_end.patch [2]
>> > > > > 
>> > > > >  And yes, with these patches the Linux bsdtar mentioned on -hackers
>> > > > > in the `"tar tfv /dev/cd0" speedup patch' thread now also runs fast
>> > > > > on FreeBSD. :)
>> > > > 
>> > > > I now added an vn_isdisk() check to the second patch after comments from
>> > > > julian, and I made a new patch that adds an overflow check to the 32 bit
>> > > > linux lseek: (also at
>> > > > 	http://people.freebsd.org/~nox/linux-lseek-overflow.patch
>> > > > )
>>
>> Unfortunately it panics in g_dev_ioctl() with INVARIANTS enabled.  I will bug 
>> phk about that.
>
>After talking to phk, it isn't safe to invoke the ioctl from stat(), only if
>the device is known to be opened (e.g. lseek() or fstat()).  This patch is a
>simpler version of your original patch (with some simple error checking
>added).  I wasn't able to test it due to weird issues with igb(4) on my test
>machine, but it does compile:
>
Does work for me too! :)

 Btw, has anyone looked at my other two patches since?

>Index: vfs_syscalls.c
>===================================================================
>--- vfs_syscalls.c	(revision 204518)
>+++ vfs_syscalls.c	(working copy)
>@@ -45,6 +45,7 @@
> #include <sys/systm.h>
> #include <sys/bio.h>
> #include <sys/buf.h>
>+#include <sys/disk.h>
> #include <sys/sysent.h>
> #include <sys/malloc.h>
> #include <sys/mount.h>
>@@ -1920,7 +1921,7 @@
> 	struct file *fp;
> 	struct vnode *vp;
> 	struct vattr vattr;
>-	off_t offset;
>+	off_t offset, size;
> 	int error, noneg;
> 	int vfslocked;
> 
>@@ -1951,6 +1952,15 @@
> 		VOP_UNLOCK(vp, 0);
> 		if (error)
> 			break;
>+
>+		/*
>+		 * If the file references a disk device, then fetch
>+		 * the media size and use that to determine the ending
>+		 * offset.
>+		 */
>+		if (vattr.va_size == 0 && vp->v_type == VCHR &&
>+		    fo_ioctl(fp, DIOCGMEDIASIZE, &size, cred, td) == 0)
>+			vattr.va_size = size;
> 		if (noneg &&
> 		    (vattr.va_size > OFF_MAX ||
> 		    (offset > 0 && vattr.va_size > OFF_MAX - offset))) {
>
>-- 
>John Baldwin
>_______________________________________________
>freebsd-hackers at freebsd.org mailing list
>http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
>To unsubscribe, send any mail to "freebsd-hackers-unsubscribe at freebsd.org"
>

 Cheers,
	Juergen