make NFS client side WRITE more defensive
Subsystems (2)
PATCH 1/1 make NFS client side WRITE more defensive
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.
Thanx...
ps
write.c renamed from write.c.org +50 -26
@@ -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 ;
Quoted 7 lines · Peter Staubach
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
fs/nfs/write.c renamed from fs/nfs/write.c.org +50 -26
@@ -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 renamed from fs/nfs/direct.c.org +25 -13
@@ -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;