* Re: [PATCH] make NFS client side WRITE more defensive
2005-06-29 16:09 [PATCH] make NFS client side WRITE more defensive Peter Staubach
@ 2005-06-29 20:36 ` Peter Staubach
0 siblings, 0 replies; 2+ messages in thread
From: Peter Staubach @ 2005-06-29 20:36 UTC (permalink / raw)
To: Peter Staubach; +Cc: nfs
[-- 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;
^ permalink raw reply [flat|nested] 2+ messages in thread