On Sun, Oct 27, 2002 at 05:19:45PM +0000, Christos Zoulas wrote:
> In article <1035713161.485419.541.nullmailer@yamt.dyndns.org>,
> YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote:
> >-=-=-=-=-=-
> >
> >hi.
> >
> >for nfs,
> >following may cause assertion failure (bytes != 0) in genfs_gop_write.
> >(originally reported by kay on a japanese mailing list)
> >
> >fd = open(.., O_RDWR|O_TRUNC);
> >write(fd, bad_pointer, 1);
> >close(fd);
> >
> >the problem is that it'll assign pages to vnode without bumping v_size.
> >panic can be avoided with attached patch, i think.
> >i'm not sure how to do for NMODIFIED, though.
> >
> >YAMAMOTO Takashi
> 
> Isn't a better way to:
> 
> 	if (uvm_useracc(bad_pointer, bad_size, B_READ) == FALSE)
> 		return EFAULT;

no, uvm_useracc() is pretty useless.  two reasons:

 (1) it just tells you whether there is a mapping which has the appropriate
     permissions, not whether the mapping is actually valid (eg. it might
     point to a vnode mapping past EOF) or whether faulting on the mapping
     will succeed (eg. reading pages from disk might get an i/o error).

 (2) uvm_useracc() must be called without any locks held, so the address
     space can change out from under the caller between the check and when
     the caller then tries to access that virtual address range for real.


as a side note, most of the current usage of uvm_useracc() is as a substitute
for copyin() / copyout(), which is completely bogus.  someone wrote some
really dumb framebuffer code long, long ago and it's been cut & pasted into
an unbelievable number of copies.  one of my low-priority work items is to
remove uvm_useracc() entirely.



so back to the original topic, I think the code below is correct.
I was thinking it'd be nice if was more similar to the FFS code,
but I decided I don't care, it's fine as-is.

-Chuck



> >Index: nfs_bio.c
> >===================================================================
> >RCS file: /cvs/NetBSD/syssrc/sys/nfs/nfs_bio.c,v
> >retrieving revision 1.84
> >diff -u -p -r1.84 nfs_bio.c
> >--- nfs_bio.c	2002/10/23 09:14:48	1.84
> >+++ nfs_bio.c	2002/10/27 09:38:20
> >@@ -591,6 +591,8 @@ nfs_write(v)
> > 
> > 	origoff = uio->uio_offset;
> > 	do {
> >+		boolean_t extending; /* if we are extending whole pages */
> >+		u_quad_t oldsize;
> > 		oldoff = uio->uio_offset;
> > 		bytelen = uio->uio_resid;
> > 
> >@@ -616,13 +618,15 @@ nfs_write(v)
> > #endif
> > 		nfsstats.biocache_writes++;
> > 
> >+		oldsize = np->n_size;
> > 		np->n_flag |= NMODIFIED;
> > 		if (np->n_size < uio->uio_offset + bytelen) {
> > 			np->n_size = uio->uio_offset + bytelen;
> > 		}
> >-		if ((uio->uio_offset & PAGE_MASK) == 0 &&
> >+		extending = ((uio->uio_offset & PAGE_MASK) == 0 &&
> > 		    (bytelen & PAGE_MASK) == 0 &&
> >-		    uio->uio_offset >= vp->v_size) {
> >+		    uio->uio_offset >= vp->v_size);
> >+		if (extending) {
> > 			win = ubc_alloc(&vp->v_uobj, uio->uio_offset, &bytelen,
> > 			    UBC_WRITE | UBC_FAULTBUSY);
> > 		} else {
> >@@ -632,6 +636,14 @@ nfs_write(v)
> > 		error = uiomove(win, bytelen, uio);
> > 		ubc_release(win, 0);
> > 		if (error) {
> >+			if (extending) {
> >+				/*
> >+				 * backout size and free pages past eof.
> >+				 */
> >+				np->n_size = oldsize;
> >+				(void)VOP_PUTPAGES(vp, round_page(vp->v_size),
> >+				    0, PGO_SYNCIO | PGO_FREE);
> >+			}
> > 			break;
> > 		}
> > 		wrotedta = 1;
> >
> >-=-=-=-=-=-
>