Re: [PATCH] make NFS client side WRITE more defensive

[Peter Staubach <staubach@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

Peter Staubach wrote:

> Hi.
>
> Attached are same changes to make the NFS client side WRITE processing 
> a little
> more defensive.  Currently, a response which indicates that the server 
> wrote
> more data than actually requested could cause bad things to happen.  
> Such a
> response could come from a broken or malicious server or from network 
> corruption
> that the normal checksumming fails to catch. 


Chuck Lever correctly pointed out that this patch did not address all of the
WRITE cases.  I hadn't incuded the direct I/O case.  The new, attached patch
should address it in addition to the sync and async cases already covered.

    Thanx...

       ps

[-- Attachment #2: devel --]
[-- Type: text/plain, Size: 4885 bytes --]

--- ./fs/nfs/write.c.org	2005-06-27 16:20:02.000000000 -0400
+++ ./fs/nfs/write.c	2005-06-27 17:07:52.000000000 -0400
@@ -197,15 +197,21 @@ static int nfs_writepage_sync(struct nfs
 
 		result = NFS_PROTO(inode)->write(wdata);
 
-		if (result < 0) {
+		if (result <= 0) {
 			/* Must mark the page invalid after I/O error */
 			ClearPageUptodate(page);
 			goto io_error;
 		}
-		if (result < wdata->args.count)
-			printk(KERN_WARNING "NFS: short write, count=%u, result=%d\n",
-					wdata->args.count, result);
-
+		if (result > wdata->args.count) {
+			dprintk("NFS: faulty NFS server %s:"
+				" (requested = %u) != (result = %d)\n",
+				NFS_SERVER(inode)->hostname,
+				wdata->args.count, result);
+			/* Treat as I/O error */
+			ClearPageUptodate(page);
+			result = -EIO;
+			goto io_error;
+		}
 		wdata->args.offset += result;
 	        wdata->args.pgbase += result;
 		written += result;
@@ -1165,32 +1171,50 @@ void nfs_writeback_done(struct rpc_task 
 		}
 	}
 #endif
-	/* Is this a short write? */
-	if (task->tk_status >= 0 && resp->count < argp->count) {
+	if (task->tk_status >= 0 && resp->count != argp->count) {
 		static unsigned long    complain;
 
-		/* Has the server at least made some progress? */
-		if (resp->count != 0) {
-			/* Was this an NFSv2 write or an NFSv3 stable write? */
-			if (resp->verf->committed != NFS_UNSTABLE) {
-				/* Resend from where the server left off */
-				argp->offset += resp->count;
-				argp->pgbase += resp->count;
-				argp->count -= resp->count;
-			} else {
-				/* Resend as a stable write in order to avoid
-				 * headaches in the case of a server crash.
+		/* Is this a short write? */
+		if (resp->count < argp->count) {
+			/* Has the server at least made some progress? */
+			if (resp->count != 0) {
+				/*
+				 * Was this an NFSv2 write or an NFSv3
+				 * stable write?
 				 */
-				argp->stable = NFS_FILE_SYNC;
+				if (resp->verf->committed != NFS_UNSTABLE) {
+					/*
+					 * Resend from where the server
+					 * left off
+					 */
+					argp->offset += resp->count;
+					argp->pgbase += resp->count;
+					argp->count -= resp->count;
+				} else {
+					/*
+					 * Resend as a stable write in
+					 * order to avoid headaches in
+					 * the case of a server crash.
+					 */
+					argp->stable = NFS_FILE_SYNC;
+				}
+				rpc_restart_call(task);
+				return;
 			}
-			rpc_restart_call(task);
-			return;
-		}
-		if (time_before(complain, jiffies)) {
-			printk(KERN_WARNING
-			       "NFS: Server wrote zero bytes, expected %u.\n",
+			if (time_before(complain, jiffies)) {
+				dprintk("NFS: Server wrote zero bytes,"
+				       " expected %u.\n",
 					argp->count);
-			complain = jiffies + 300 * HZ;
+				complain = jiffies + 300 * HZ;
+			}
+		} else {
+		/* Or an oversized write? */
+			if (time_before(complain, jiffies)) {
+				dprintk("NFS: Server wrote more than requested,"
+				       " expected %u.\n",
+					argp->count);
+				complain = jiffies + 300 * HZ;
+			}
 		}
 		/* Can't do anything about it except throw an error. */
 		task->tk_status = -EIO;
--- ./fs/nfs/direct.c.org	2005-06-27 16:20:02.000000000 -0400
+++ ./fs/nfs/direct.c	2005-06-29 14:29:29.833983712 -0400
@@ -469,28 +469,39 @@ retry:
 		result = NFS_PROTO(inode)->write(wdata);
 		unlock_kernel();
 
-		if (result <= 0) {
-			if (tot_bytes > 0)
-				break;
-			goto out;
+		/*
+		 * If an error occurred or the server did not write any
+		 * data, then stop looping.  Any data already written
+		 * which needs to be committed will be committed.  If
+		 * data was written, then return the amount of data
+		 * written, else return either the error or zero indicating
+		 * that no data was written.
+		 */
+		if (result <= 0)
+			break;
+
+		/*
+		 * The response indicated that more data was written
+		 * then was sent.  Oops.  It is unknown what the server
+		 * might have done, so stop looping and handle this as
+		 * an error as described above.
+		 */
+		if (result > wdata->args.count) {
+			result = -EIO;
+			break;
 		}
 
 		if (tot_bytes == 0)
 			memcpy(&first_verf.verifier, &wdata->verf.verifier,
 						sizeof(first_verf.verifier));
 		if (wdata->verf.committed != NFS_FILE_SYNC) {
-			need_commit = 1;
 			if (memcmp(&first_verf.verifier, &wdata->verf.verifier,
-					sizeof(first_verf.verifier)));
+					sizeof(first_verf.verifier)) != 0)
 				goto sync_retry;
+			need_commit = 1;
 		}
 
 		tot_bytes += result;
-
-		/* in case of a short write: stop now, let the app recover */
-		if (result < wdata->args.count)
-			break;
-
 		wdata->args.offset += result;
 		wdata->args.pgbase += result;
 		curpage += wdata->args.pgbase >> PAGE_SHIFT;
@@ -514,9 +525,10 @@ retry:
 					 sizeof(first_verf.verifier)) != 0)
 			goto sync_retry;
 	}
-	result = tot_bytes;
 
-out:
+	if (tot_bytes)
+		result = tot_bytes;
+
 	nfs_end_data_update(inode);
 	nfs_writedata_free(wdata);
 	return result;