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

[Juergen Lock]

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
> > )
> 
> Hmm, when I asked Bruce, he actually said it was a feature that you didn't use 
> vn_isdisk().  He also suggested that the proper way to fix lseek on devices is 
> to fix stat in devfs to return a non-zero size.  I have a possible fix to do 
> that but haven't tested it yet:
> 
> Index: devfs_vnops.c
> ===================================================================
> --- devfs_vnops.c	(revision 204207)
> +++ devfs_vnops.c	(working copy)
> @@ -44,6 +44,7 @@
>  #include <sys/systm.h>
>  #include <sys/conf.h>
>  #include <sys/dirent.h>
> +#include <sys/disk.h>
>  #include <sys/fcntl.h>
>  #include <sys/file.h>
>  #include <sys/filedesc.h>
> @@ -564,7 +565,11 @@
>  	struct vattr *vap = ap->a_vap;
>  	int error = 0;
>  	struct devfs_dirent *de;
> +	struct cdevsw *dsw;
> +	struct thread *td;
> +	struct file *fpop;
>  	struct cdev *dev;
> +	off_t size;
>  
>  	de = vp->v_data;
>  	KASSERT(de != NULL, ("Null dirent in devfs_getattr vp=%p", vp));
> @@ -612,6 +617,18 @@
>  		vap->va_ctime = dev->si_ctime;
>  
>  		vap->va_rdev = cdev2priv(dev)->cdp_inode;
> +
> +		dsw = dev_refthread(dev);
> +		if (dsw != NULL) {
> +			td = curthread;
> +			fpop = td->td_fpop;
> +			td->td_fpop = NULL;
> +			if (dsw->d_ioctl(dev, DIOCGMEDIASIZE, (caddr_t)&size,
> +			    FREAD, td) == 0)
> +				vap->va_size = size;
> +			td->td_fpop = fpop;
> +			dev_relthread(dev);
> +		}
>  	}
>  	vap->va_gen = 0;
>  	vap->va_flags = 0;
> 

I had to add an D_DISK check, else it would panic at boot in a revoke
syscall in ttydev_ioctl in ttydev_enter, apparently in the tty_lock call:
(I didn't get a dump as it panic'd way too early in boot)
	http://fxr.watson.org/fxr/source/kern/tty.c#L159

 Maybe dev->si_drv1 is null here?
	http://fxr.watson.org/fxr/source/kern/tty.c#L486

 Anyway, with the D_DISK check added the patch seems to work:

Index: src/sys/fs/devfs/devfs_vnops.c
===================================================================
RCS file: /home/scvs/src/sys/fs/devfs/devfs_vnops.c,v
retrieving revision 1.181.2.1
diff -u -p -p -u -r1.181.2.1 devfs_vnops.c
--- src/sys/fs/devfs/devfs_vnops.c	3 Aug 2009 08:13:06 -0000	1.181.2.1
+++ src/sys/fs/devfs/devfs_vnops.c	26 Feb 2010 20:46:17 -0000
@@ -44,6 +44,7 @@
 #include <sys/systm.h>
 #include <sys/conf.h>
 #include <sys/dirent.h>
+#include <sys/disk.h>
 #include <sys/fcntl.h>
 #include <sys/file.h>
 #include <sys/filedesc.h>
@@ -563,7 +564,11 @@ devfs_getattr(struct vop_getattr_args *a
 	struct vattr *vap = ap->a_vap;
 	int error = 0;
 	struct devfs_dirent *de;
+	struct cdevsw *dsw;
+	struct thread *td;
+	struct file *fpop;
 	struct cdev *dev;
+	off_t size;
 
 	de = vp->v_data;
 	KASSERT(de != NULL, ("Null dirent in devfs_getattr vp=%p", vp));
@@ -611,6 +616,19 @@ devfs_getattr(struct vop_getattr_args *a
 		vap->va_ctime = dev->si_ctime;
 
 		vap->va_rdev = cdev2priv(dev)->cdp_inode;
+
+		dsw = dev_refthread(dev);
+		if (dsw != NULL) {
+			td = curthread;
+			fpop = td->td_fpop;
+			td->td_fpop = NULL;
+			if ((dsw->d_flags & D_DISK) &&
+			    dsw->d_ioctl(dev, DIOCGMEDIASIZE, (caddr_t)&size,
+			    FREAD, td) == 0)
+				vap->va_size = size;
+			td->td_fpop = fpop;
+			dev_relthread(dev);
+		}
 	}
 	vap->va_gen = 0;
 	vap->va_flags = 0;