linux-kernel - Re: [ 131/149] NFSv4.1: Fix layoutcommit error handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <056307F7-5680-4520-AA8E-220F525C6506@netapp.com>
Date:	Sun, 1 Apr 2012 23:59:54 +0000
From:	"Myklebust, Trond" <Trond.Myklebust@...app.com>
To:	Ben Hutchings <ben@...adent.org.uk>
CC:	"Myklebust, Trond" <Trond.Myklebust@...app.com>,
	"<linux-kernel@...r.kernel.org>" <linux-kernel@...r.kernel.org>,
	"<stable@...r.kernel.org>" <stable@...r.kernel.org>,
	"<torvalds@...ux-foundation.org>" <torvalds@...ux-foundation.org>,
	"<akpm@...ux-foundation.org>" <akpm@...ux-foundation.org>,
	"<alan@...rguk.ukuu.org.uk>" <alan@...rguk.ukuu.org.uk>,
	Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [ 131/149] NFSv4.1: Fix layoutcommit error handling


On Apr 1, 2012, at 2:23 PM, Ben Hutchings wrote:

> On Fri, 2012-03-30 at 12:50 -0700, Greg KH wrote:
>> 3.2-stable review patch.  If anyone has any objections, please let me know.
>> 
>> ------------------
>> 
>> From: Trond Myklebust <Trond.Myklebust@...app.com>
>> 
>> commit e59d27e05a6435f8c04d5ad843f37fa795f2eaaa upstream.
>> 
>> Firstly, task->tk_status will always return negative error values,
>> so the current tests for 'NFS4ERR_DELEG_REVOKED' etc. are all being
>> ignored.
>> Secondly, clean up the code so that we only need to test
>> task->tk_status once!
>> 
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@...app.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> 
>> ---
>> fs/nfs/nfs4proc.c |   25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -5983,21 +5983,22 @@ nfs4_layoutcommit_done(struct rpc_task *
>> 		return;
>> 
>> 	switch (task->tk_status) { /* Just ignore these failures */
>> -	case NFS4ERR_DELEG_REVOKED: /* layout was recalled */
>> -	case NFS4ERR_BADIOMODE:     /* no IOMODE_RW layout for range */
>> -	case NFS4ERR_BADLAYOUT:     /* no layout */
>> -	case NFS4ERR_GRACE:	    /* loca_recalim always false */
>> +	case -NFS4ERR_DELEG_REVOKED: /* layout was recalled */
>> +	case -NFS4ERR_BADIOMODE:     /* no IOMODE_RW layout for range */
>> +	case -NFS4ERR_BADLAYOUT:     /* no layout */
>> +	case -NFS4ERR_GRACE:	    /* loca_recalim always false */
>> 		task->tk_status = 0;
>> -	}
>> -
>> -	if (nfs4_async_handle_error(task, server, NULL) == -EAGAIN) {
>> -		rpc_restart_call_prepare(task);
>> -		return;
>> -	}
>> -
>> -	if (task->tk_status == 0)
>> +		break;
> 
> It loooks like the previous intent was that for all those specific error
> codes we would end up calling nfs_post_op_update_inode_force_wcc().
> That didn't happen because of the incorrectly signed error codes.  But
> it still won't happen now, because of this break.  Do we really want to
> break here or fall through past 'case 0'?
> 

The 'break' there is deliberate. If the LAYOUTCOMMIT gets an error, then the server will never get to process the post-op GETATTR.

Trond

> Ben.
> 
>> +	case 0:
>> 		nfs_post_op_update_inode_force_wcc(data->args.inode,
>> 						   data->res.fattr);
>> +		break;
>> +	default:
>> +		if (nfs4_async_handle_error(task, server, NULL) == -EAGAIN) {
>> +			rpc_restart_call_prepare(task);
>> +			return;
>> +		}
>> +	}
>> }
>> 
>> static void nfs4_layoutcommit_release(void *calldata)
> 
> -- 
> Ben Hutchings
> I'm always amazed by the number of people who take up solipsism because
> they heard someone else explain it. - E*Borg on alt.fan.pratchett

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/