Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 6 Jun 2012 16:30:16 +0000 (UTC)
From:      Konstantin Belousov <kib@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r236687 - head/sys/fs/nfsclient
Message-ID:  <201206061630.q56GUGH4025324@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: kib
Date: Wed Jun  6 16:30:16 2012
New Revision: 236687
URL: http://svn.freebsd.org/changeset/base/236687

Log:
  Improve handling of uiomove(9) errors for the NFS client.
  
  Do not brelse() the buffer unconditionally with BIO_ERROR set if
  uiomove() failed. The brelse() treats most buffers with BIO_ERROR as
  B_INVAL, dropping their content.  Instead, if the write request
  covered the whole buffer, remember the cached state and brelse() with
  BIO_ERROR set only if the buffer was not cached previously.
  
  Update the buffer dirtyoff/dirtyend based on the progress recorded by
  uiomove() in passed struct uio, even in the presence of
  error. Otherwise, usermode could see changed data in the backed pages,
  but later the buffer is destroyed without write-back.
  
  If uiomove() failed for IO_UNIT request, try to truncate the vnode
  back to the pre-write state, and rewind the progress in passed uio
  accordingly, following the FFS behaviour.
  
  Reviewed by:	rmacklem (some time ago)
  Tested by:	pho
  MFC after:	1 month

Modified:
  head/sys/fs/nfsclient/nfs_clbio.c

Modified: head/sys/fs/nfsclient/nfs_clbio.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clbio.c	Wed Jun  6 16:26:55 2012	(r236686)
+++ head/sys/fs/nfsclient/nfs_clbio.c	Wed Jun  6 16:30:16 2012	(r236687)
@@ -897,8 +897,9 @@ ncl_write(struct vop_write_args *ap)
 	struct nfsmount *nmp = VFSTONFS(vp->v_mount);
 	daddr_t lbn;
 	int bcount;
-	int n, on, error = 0;
-	off_t tmp_off;
+	int bp_cached, n, on, error = 0;
+	size_t orig_resid, local_resid;
+	off_t orig_size, tmp_off;
 
 	KASSERT(uio->uio_rw == UIO_WRITE, ("ncl_write mode"));
 	KASSERT(uio->uio_segflg != UIO_USERSPACE || uio->uio_td == curthread,
@@ -950,6 +951,11 @@ flush_and_restart:
 			mtx_unlock(&np->n_mtx);
 	}
 
+	orig_resid = uio->uio_resid;
+	mtx_lock(&np->n_mtx);
+	orig_size = np->n_size;
+	mtx_unlock(&np->n_mtx);
+
 	/*
 	 * If IO_APPEND then load uio_offset.  We restart here if we cannot
 	 * get the append lock.
@@ -1127,7 +1133,10 @@ again:
 		 * normally.
 		 */
 
+		bp_cached = 1;
 		if (on == 0 && n == bcount) {
+			if ((bp->b_flags & B_CACHE) == 0)
+				bp_cached = 0;
 			bp->b_flags |= B_CACHE;
 			bp->b_flags &= ~B_INVAL;
 			bp->b_ioflags &= ~BIO_ERROR;
@@ -1193,8 +1202,24 @@ again:
 			goto again;
 		}
 
+		local_resid = uio->uio_resid;
 		error = uiomove((char *)bp->b_data + on, n, uio);
 
+		if (error != 0 && !bp_cached) {
+			/*
+			 * This block has no other content then what
+			 * possibly was written by the faulty uiomove.
+			 * Release it, forgetting the data pages, to
+			 * prevent the leak of uninitialized data to
+			 * usermode.
+			 */
+			bp->b_ioflags |= BIO_ERROR;
+			brelse(bp);
+			uio->uio_offset -= local_resid - uio->uio_resid;
+			uio->uio_resid = local_resid;
+			break;
+		}
+
 		/*
 		 * Since this block is being modified, it must be written
 		 * again and not just committed.  Since write clustering does
@@ -1203,17 +1228,18 @@ again:
 		 */
 		bp->b_flags &= ~(B_NEEDCOMMIT | B_CLUSTEROK);
 
-		if (error) {
-			bp->b_ioflags |= BIO_ERROR;
-			brelse(bp);
-			break;
-		}
+		/*
+		 * Get the partial update on the progress made from
+		 * uiomove, if an error occured.
+		 */
+		if (error != 0)
+			n = local_resid - uio->uio_resid;
 
 		/*
 		 * Only update dirtyoff/dirtyend if not a degenerate
 		 * condition.
 		 */
-		if (n) {
+		if (n > 0) {
 			if (bp->b_dirtyend > 0) {
 				bp->b_dirtyoff = min(on, bp->b_dirtyoff);
 				bp->b_dirtyend = max((on + n), bp->b_dirtyend);
@@ -1242,8 +1268,22 @@ again:
 		} else {
 			bdwrite(bp);
 		}
+
+		if (error != 0)
+			break;
 	} while (uio->uio_resid > 0 && n > 0);
 
+	if (error != 0) {
+		if (ioflag & IO_UNIT) {
+			VATTR_NULL(&vattr);
+			vattr.va_size = orig_size;
+			/* IO_SYNC is handled implicitely */
+			(void)VOP_SETATTR(vp, &vattr, cred);
+			uio->uio_offset -= orig_resid - uio->uio_resid;
+			uio->uio_resid = orig_resid;
+		}
+	}
+
 	return (error);
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201206061630.q56GUGH4025324>