NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

Sorry not to notice this before--the below causes a regression against
the Linux server; something like:

	# mount -osec=krb5i pip1:/exports /mnt/
	# echo "test" >/mnt/test
	# umount /mnt/
	# mount -osec=krb5 pip1:/exports /mnt/
	# echo "test" >/mnt/test
	bash: /mnt/test: Operation not permitted

This fails after the below commit on the client, but not before, thanks
to the server rejecting the second setclientid with CLID_INUSE due to a
different security flavor.

I haven't really thought through who's at fault here.  The second
setclientid does lack some level of security that the former had, so I
think the server's within its rights to complain.

--b.

commit de734831224e74fcaf8917386e33644c4243db95
Author: Chuck Lever <chuck.lever@oracle.com>
Date:   Wed Jul 11 16:30:50 2012 -0400

    NFS: Treat NFS4ERR_CLID_INUSE as a fatal error
    
    For NFSv4 minor version 0, currently the cl_id_uniquifier allows the
    Linux client to generate a unique nfs_client_id4 string whenever a
    server replies with NFS4ERR_CLID_INUSE.
    
    This implementation seems to be based on a flawed reading of RFC
    3530.  NFS4ERR_CLID_INUSE actually means that the client has presented
    this nfs_client_id4 string with a different principal at some time in
    the past, and that lease is still in use on the server.
    
    For a Linux client this might be rather difficult to achieve: the
    authentication flavor is named right in the nfs_client_id4.id
    string.  If we change flavors, we change strings automatically.
    
    So, practically speaking, NFS4ERR_CLID_INUSE means there is some other
    client using our string.  There is not much that can be done to
    recover automatically.  Let's make it a permanent error.
    
    Remove the recovery logic in nfs4_proc_setclientid(), and remove the
    cl_id_uniquifier field from the nfs_client data structure.  And,
    remove the authentication flavor from the nfs_client_id4 string.
    
    Keeping the authentication flavor in the nfs_client_id4.id string
    means that we could have a separate lease for each authentication
    flavor used by mounts on the client.  But we want just one lease for
    all the mounts on this client.
    
    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 74dcd85..1148081 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4029,42 +4029,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
 		.rpc_resp = res,
 		.rpc_cred = cred,
 	};
-	int loop = 0;
-	int status;
 
+	/* nfs_client_id4 */
 	nfs4_init_boot_verifier(clp, &sc_verifier);
-
-	for(;;) {
-		rcu_read_lock();
-		setclientid.sc_name_len = scnprintf(setclientid.sc_name,
-				sizeof(setclientid.sc_name), "%s/%s %s %s %u",
-				clp->cl_ipaddr,
-				rpc_peeraddr2str(clp->cl_rpcclient,
-							RPC_DISPLAY_ADDR),
-				rpc_peeraddr2str(clp->cl_rpcclient,
-							RPC_DISPLAY_PROTO),
-				clp->cl_rpcclient->cl_auth->au_ops->au_name,
-				clp->cl_id_uniquifier);
-		setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
+	rcu_read_lock();
+	setclientid.sc_name_len = scnprintf(setclientid.sc_name,
+			sizeof(setclientid.sc_name), "%s/%s %s",
+			clp->cl_ipaddr,
+			rpc_peeraddr2str(clp->cl_rpcclient,
+						RPC_DISPLAY_ADDR),
+			rpc_peeraddr2str(clp->cl_rpcclient,
+						RPC_DISPLAY_PROTO));
+	/* cb_client4 */
+	setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
 				sizeof(setclientid.sc_netid),
 				rpc_peeraddr2str(clp->cl_rpcclient,
 							RPC_DISPLAY_NETID));
-		setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
+	rcu_read_unlock();
+	setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
 				sizeof(setclientid.sc_uaddr), "%s.%u.%u",
 				clp->cl_ipaddr, port >> 8, port & 255);
-		rcu_read_unlock();
 
-		status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
-		if (status != -NFS4ERR_CLID_INUSE)
-			break;
-		if (loop != 0) {
-			++clp->cl_id_uniquifier;
-			break;
-		}
-		++loop;
-		ssleep(clp->cl_lease_time / HZ + 1);
-	}
-	return status;
+	return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
 }
 
 int nfs4_proc_setclientid_confirm(struct nfs_client *clp,
@@ -5262,10 +5248,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)
 	nfs4_init_boot_verifier(clp, &verifier);
 
 	args.id_len = scnprintf(args.id, sizeof(args.id),
-				"%s/%s/%u",
+				"%s/%s",
 				clp->cl_ipaddr,
-				clp->cl_rpcclient->cl_nodename,
-				clp->cl_rpcclient->cl_auth->au_flavor);
+				clp->cl_rpcclient->cl_nodename);
 
 	res.server_owner = kzalloc(sizeof(struct nfs41_server_owner),
 					GFP_NOFS);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 1cfc460..81eabcd 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status)
 			return -ESERVERFAULT;
 		/* Lease confirmation error: retry after purging the lease */
 		ssleep(1);
-	case -NFS4ERR_CLID_INUSE:
 	case -NFS4ERR_STALE_CLIENTID:
 		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
 		break;
+	case -NFS4ERR_CLID_INUSE:
+		pr_err("NFS: Server %s reports our clientid is in use\n",
+			clp->cl_hostname);
+		nfs_mark_client_ready(clp, -EPERM);
+		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
+		return -EPERM;
 	case -EACCES:
 		if (clp->cl_machine_cred == NULL)
 			return -EACCES;
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index f58325a..6532765 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -69,10 +69,9 @@ struct nfs_client {
 	struct idmap *		cl_idmap;
 
 	/* Our own IP address, as a null-terminated string.
-	 * This is used to generate the clientid, and the callback address.
+	 * This is used to generate the mv0 callback address.
 	 */
 	char			cl_ipaddr[48];
-	unsigned char		cl_id_uniquifier;
 	u32			cl_cb_ident;	/* v4.0 callback identifier */
 	const struct nfs4_minor_version_ops *cl_mvops;
 

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:

> Sorry not to notice this before--the below causes a regression against
> the Linux server; something like:
> 
> 	# mount -osec=krb5i pip1:/exports /mnt/
> 	# echo "test" >/mnt/test
> 	# umount /mnt/
> 	# mount -osec=krb5 pip1:/exports /mnt/
> 	# echo "test" >/mnt/test
> 	bash: /mnt/test: Operation not permitted
> 
> This fails after the below commit on the client, but not before, thanks
> to the server rejecting the second setclientid with CLID_INUSE due to a
> different security flavor.

This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.

One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.

> I haven't really thought through who's at fault here.  The second
> setclientid does lack some level of security that the former had, so I
> think the server's within its rights to complain.

I don't think the second _lacks_ a level of security: I think the second is using a _different_ security flavor with the same nfs_client_id4.id string.  NFS4ERR_CLID_INUSE is the correct server response in this case.

> --b.
> 
> commit de734831224e74fcaf8917386e33644c4243db95
> Author: Chuck Lever <chuck.lever@oracle.com>
> Date:   Wed Jul 11 16:30:50 2012 -0400
> 
>    NFS: Treat NFS4ERR_CLID_INUSE as a fatal error
> 
>    For NFSv4 minor version 0, currently the cl_id_uniquifier allows the
>    Linux client to generate a unique nfs_client_id4 string whenever a
>    server replies with NFS4ERR_CLID_INUSE.
> 
>    This implementation seems to be based on a flawed reading of RFC
>    3530.  NFS4ERR_CLID_INUSE actually means that the client has presented
>    this nfs_client_id4 string with a different principal at some time in
>    the past, and that lease is still in use on the server.
> 
>    For a Linux client this might be rather difficult to achieve: the
>    authentication flavor is named right in the nfs_client_id4.id
>    string.  If we change flavors, we change strings automatically.
> 
>    So, practically speaking, NFS4ERR_CLID_INUSE means there is some other
>    client using our string.  There is not much that can be done to
>    recover automatically.  Let's make it a permanent error.
> 
>    Remove the recovery logic in nfs4_proc_setclientid(), and remove the
>    cl_id_uniquifier field from the nfs_client data structure.  And,
>    remove the authentication flavor from the nfs_client_id4 string.
> 
>    Keeping the authentication flavor in the nfs_client_id4.id string
>    means that we could have a separate lease for each authentication
>    flavor used by mounts on the client.  But we want just one lease for
>    all the mounts on this client.
> 
>    Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 74dcd85..1148081 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4029,42 +4029,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
> 		.rpc_resp = res,
> 		.rpc_cred = cred,
> 	};
> -	int loop = 0;
> -	int status;
> 
> +	/* nfs_client_id4 */
> 	nfs4_init_boot_verifier(clp, &sc_verifier);
> -
> -	for(;;) {
> -		rcu_read_lock();
> -		setclientid.sc_name_len = scnprintf(setclientid.sc_name,
> -				sizeof(setclientid.sc_name), "%s/%s %s %s %u",
> -				clp->cl_ipaddr,
> -				rpc_peeraddr2str(clp->cl_rpcclient,
> -							RPC_DISPLAY_ADDR),
> -				rpc_peeraddr2str(clp->cl_rpcclient,
> -							RPC_DISPLAY_PROTO),
> -				clp->cl_rpcclient->cl_auth->au_ops->au_name,
> -				clp->cl_id_uniquifier);
> -		setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
> +	rcu_read_lock();
> +	setclientid.sc_name_len = scnprintf(setclientid.sc_name,
> +			sizeof(setclientid.sc_name), "%s/%s %s",
> +			clp->cl_ipaddr,
> +			rpc_peeraddr2str(clp->cl_rpcclient,
> +						RPC_DISPLAY_ADDR),
> +			rpc_peeraddr2str(clp->cl_rpcclient,
> +						RPC_DISPLAY_PROTO));
> +	/* cb_client4 */
> +	setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
> 				sizeof(setclientid.sc_netid),
> 				rpc_peeraddr2str(clp->cl_rpcclient,
> 							RPC_DISPLAY_NETID));
> -		setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
> +	rcu_read_unlock();
> +	setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
> 				sizeof(setclientid.sc_uaddr), "%s.%u.%u",
> 				clp->cl_ipaddr, port >> 8, port & 255);
> -		rcu_read_unlock();
> 
> -		status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
> -		if (status != -NFS4ERR_CLID_INUSE)
> -			break;
> -		if (loop != 0) {
> -			++clp->cl_id_uniquifier;
> -			break;
> -		}
> -		++loop;
> -		ssleep(clp->cl_lease_time / HZ + 1);
> -	}
> -	return status;
> +	return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
> }
> 
> int nfs4_proc_setclientid_confirm(struct nfs_client *clp,
> @@ -5262,10 +5248,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)
> 	nfs4_init_boot_verifier(clp, &verifier);
> 
> 	args.id_len = scnprintf(args.id, sizeof(args.id),
> -				"%s/%s/%u",
> +				"%s/%s",
> 				clp->cl_ipaddr,
> -				clp->cl_rpcclient->cl_nodename,
> -				clp->cl_rpcclient->cl_auth->au_flavor);
> +				clp->cl_rpcclient->cl_nodename);
> 
> 	res.server_owner = kzalloc(sizeof(struct nfs41_server_owner),
> 					GFP_NOFS);
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 1cfc460..81eabcd 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status)
> 			return -ESERVERFAULT;
> 		/* Lease confirmation error: retry after purging the lease */
> 		ssleep(1);
> -	case -NFS4ERR_CLID_INUSE:
> 	case -NFS4ERR_STALE_CLIENTID:
> 		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
> 		break;
> +	case -NFS4ERR_CLID_INUSE:
> +		pr_err("NFS: Server %s reports our clientid is in use\n",
> +			clp->cl_hostname);
> +		nfs_mark_client_ready(clp, -EPERM);
> +		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
> +		return -EPERM;
> 	case -EACCES:
> 		if (clp->cl_machine_cred == NULL)
> 			return -EACCES;
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index f58325a..6532765 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -69,10 +69,9 @@ struct nfs_client {
> 	struct idmap *		cl_idmap;
> 
> 	/* Our own IP address, as a null-terminated string.
> -	 * This is used to generate the clientid, and the callback address.
> +	 * This is used to generate the mv0 callback address.
> 	 */
> 	char			cl_ipaddr[48];
> -	unsigned char		cl_id_uniquifier;
> 	u32			cl_cb_ident;	/* v4.0 callback identifier */
> 	const struct nfs4_minor_version_ops *cl_mvops;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote:
> 
> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:
> 
> > Sorry not to notice this before--the below causes a regression against
> > the Linux server; something like:
> > 
> > 	# mount -osec=krb5i pip1:/exports /mnt/
> > 	# echo "test" >/mnt/test
> > 	# umount /mnt/
> > 	# mount -osec=krb5 pip1:/exports /mnt/
> > 	# echo "test" >/mnt/test
> > 	bash: /mnt/test: Operation not permitted
> > 
> > This fails after the below commit on the client, but not before, thanks
> > to the server rejecting the second setclientid with CLID_INUSE due to a
> > different security flavor.
> 
> This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.
> 
> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.

That should work.

> > I haven't really thought through who's at fault here.  The second
> > setclientid does lack some level of security that the former had, so I
> > think the server's within its rights to complain.
> 
> I don't think the second _lacks_ a level of security: I think the second is using a _different_ security flavor with the same nfs_client_id4.id string.  NFS4ERR_CLID_INUSE is the correct server response in this case.

OK.

--b.

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote:
> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote:
> > 
> > On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:
> > 
> > > Sorry not to notice this before--the below causes a regression against
> > > the Linux server; something like:
> > > 
> > > 	# mount -osec=krb5i pip1:/exports /mnt/
> > > 	# echo "test" >/mnt/test
> > > 	# umount /mnt/
> > > 	# mount -osec=krb5 pip1:/exports /mnt/
> > > 	# echo "test" >/mnt/test
> > > 	bash: /mnt/test: Operation not permitted
> > > 
> > > This fails after the below commit on the client, but not before, thanks
> > > to the server rejecting the second setclientid with CLID_INUSE due to a
> > > different security flavor.
> > 
> > This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.
> > 
> > One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.
> 
> That should work.

Whoops, no, au_name is just "RPCSEC_GSS" in both cases.

--b.

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote:

> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote:
>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote:
>>> 
>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:
>>> 
>>>> Sorry not to notice this before--the below causes a regression against
>>>> the Linux server; something like:
>>>> 
>>>> 	# mount -osec=krb5i pip1:/exports /mnt/
>>>> 	# echo "test" >/mnt/test
>>>> 	# umount /mnt/
>>>> 	# mount -osec=krb5 pip1:/exports /mnt/
>>>> 	# echo "test" >/mnt/test
>>>> 	bash: /mnt/test: Operation not permitted
>>>> 
>>>> This fails after the below commit on the client, but not before, thanks
>>>> to the server rejecting the second setclientid with CLID_INUSE due to a
>>>> different security flavor.
>>> 
>>> This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.
>>> 
>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.
>> 
>> That should work.
> 
> Whoops, no, au_name is just "RPCSEC_GSS" in both cases.

Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier?

We'll need to distinguish the pseudoflavor as well for GSS, apparently.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 9, 2012, at 4:46 PM, Chuck Lever wrote:

> 
> On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote:
> 
>> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote:
>>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote:
>>>> 
>>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:
>>>> 
>>>>> Sorry not to notice this before--the below causes a regression against
>>>>> the Linux server; something like:
>>>>> 
>>>>> 	# mount -osec=krb5i pip1:/exports /mnt/
>>>>> 	# echo "test" >/mnt/test
>>>>> 	# umount /mnt/
>>>>> 	# mount -osec=krb5 pip1:/exports /mnt/
>>>>> 	# echo "test" >/mnt/test
>>>>> 	bash: /mnt/test: Operation not permitted
>>>>> 
>>>>> This fails after the below commit on the client, but not before, thanks
>>>>> to the server rejecting the second setclientid with CLID_INUSE due to a
>>>>> different security flavor.
>>>> 
>>>> This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.
>>>> 
>>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.
>>> 
>>> That should work.
>> 
>> Whoops, no, au_name is just "RPCSEC_GSS" in both cases.
> 
> Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier?
> 
> We'll need to distinguish the pseudoflavor as well for GSS, apparently.

The client should present the same principal on both SETCLIENTID requests, shouldn't it?  If the principal is the same, that's all that RFC 3530bis requires.  Maybe I don't understand how GSS principals work.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Thu, Aug 09, 2012 at 04:52:07PM -0400, Chuck Lever wrote:
> 
> On Aug 9, 2012, at 4:46 PM, Chuck Lever wrote:
> 
> > 
> > On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote:
> > 
> >> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote:
> >>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote:
> >>>> 
> >>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:
> >>>> 
> >>>>> Sorry not to notice this before--the below causes a regression against
> >>>>> the Linux server; something like:
> >>>>> 
> >>>>> 	# mount -osec=krb5i pip1:/exports /mnt/
> >>>>> 	# echo "test" >/mnt/test
> >>>>> 	# umount /mnt/
> >>>>> 	# mount -osec=krb5 pip1:/exports /mnt/
> >>>>> 	# echo "test" >/mnt/test
> >>>>> 	bash: /mnt/test: Operation not permitted
> >>>>> 
> >>>>> This fails after the below commit on the client, but not before, thanks
> >>>>> to the server rejecting the second setclientid with CLID_INUSE due to a
> >>>>> different security flavor.
> >>>> 
> >>>> This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.
> >>>> 
> >>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.
> >>> 
> >>> That should work.
> >> 
> >> Whoops, no, au_name is just "RPCSEC_GSS" in both cases.
> > 
> > Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier?

Confirmed.

> > We'll need to distinguish the pseudoflavor as well for GSS,
> > apparently.
> 
> The client should present the same principal on both SETCLIENTID
> requests, shouldn't it?  If the principal is the same, that's all that
> RFC 3530bis requires.  Maybe I don't understand how GSS principals
> work.

Which rfc3530bis language are you looking at?

I remember it being a bit vague.  Looking at the spec....

	"a deliberate change of the principal owner of the id string
	(such as the case of a client that changes security flavors, and
	under the new flavor, there is no mapping to the previous owner)
	will in rare cases result in NFS4ERR_CLID_INUSE."

Which makes it sound like the server can arbitrarily decide how to map
principals sent with different flavors--which doesn't offer much
guidance about what to do.

The server could just compare principal strings (and ignore
pseudoflavors) in the gss case.

If the intention is to ensure that a clientid can't be "hijacked" by
someone malicious, then you don't want to allow a krb5 setclientid to
blow away a clientid established with krb5i.  (If sending the
setclientid with krb5i indicates the client wants protection against
attacks which replace the body of the rpc, then a later krb5 setclientid
should be rejected, since it could be the product of such an attack.)

--b.

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote:

> On Thu, Aug 09, 2012 at 04:52:07PM -0400, Chuck Lever wrote:
>> 
>> On Aug 9, 2012, at 4:46 PM, Chuck Lever wrote:
>> 
>>> 
>>> On Aug 9, 2012, at 4:38 PM, J. Bruce Fields wrote:
>>> 
>>>> On Thu, Aug 09, 2012 at 03:37:43PM -0400, J. Bruce Fields wrote:
>>>>> On Thu, Aug 09, 2012 at 03:06:00PM -0400, Chuck Lever wrote:
>>>>>> 
>>>>>> On Aug 9, 2012, at 2:35 PM, J. Bruce Fields wrote:
>>>>>> 
>>>>>>> Sorry not to notice this before--the below causes a regression against
>>>>>>> the Linux server; something like:
>>>>>>> 
>>>>>>> 	# mount -osec=krb5i pip1:/exports /mnt/
>>>>>>> 	# echo "test" >/mnt/test
>>>>>>> 	# umount /mnt/
>>>>>>> 	# mount -osec=krb5 pip1:/exports /mnt/
>>>>>>> 	# echo "test" >/mnt/test
>>>>>>> 	bash: /mnt/test: Operation not permitted
>>>>>>> 
>>>>>>> This fails after the below commit on the client, but not before, thanks
>>>>>>> to the server rejecting the second setclientid with CLID_INUSE due to a
>>>>>>> different security flavor.
>>>>>> 
>>>>>> This was part of a series where the last few patches got dropped for other problems.  Testing with this patch by itself was never done since it was part of a series of patches that implement a particular feature.
>>>>>> 
>>>>>> One thought is to put the authentication flavor name back into nfs_client_id4.id string temporarily until we have worked through the issues with full UCS support.  That would prevent the regression, but we'd still have clients who use multiple authentication flavors maintaining multiple leases.
>>>>> 
>>>>> That should work.
>>>> 
>>>> Whoops, no, au_name is just "RPCSEC_GSS" in both cases.
>>> 
>>> Could you confirm that before that commit, the client had to send an additional SETCLIENTID with a new cl_id_uniquifier?
> 
> Confirmed.
> 
>>> We'll need to distinguish the pseudoflavor as well for GSS,
>>> apparently.
>> 
>> The client should present the same principal on both SETCLIENTID
>> requests, shouldn't it?  If the principal is the same, that's all that
>> RFC 3530bis requires.  Maybe I don't understand how GSS principals
>> work.
> 
> Which rfc3530bis language are you looking at?
> 
> I remember it being a bit vague.  Looking at the spec....
> 
> 	"a deliberate change of the principal owner of the id string
> 	(such as the case of a client that changes security flavors, and
> 	under the new flavor, there is no mapping to the previous owner)
> 	will in rare cases result in NFS4ERR_CLID_INUSE."
> 
> Which makes it sound like the server can arbitrarily decide how to map
> principals sent with different flavors--which doesn't offer much
> guidance about what to do.

Not sure it's that liberal.  We'd have to ask for some consensus about the interpretation of that paragraph.  But...

The authenticating principal is what matters, I'd think.  Kerberos itself is supposed to provide just strong authentication here, so krb5 == krb5i == krb5p.  I don't see how the same principal presented with krb5 and with krb5i could represent two different entities, but my understanding of this stuff is shallow.

> The server could just compare principal strings (and ignore
> pseudoflavors) in the gss case.

Barring a correction about how GSS Kerberos principals work, I think that's the correct approach.

> If the intention is to ensure that a clientid can't be "hijacked" by
> someone malicious, then you don't want to allow a krb5 setclientid to
> blow away a clientid established with krb5i.  (If sending the
> setclientid with krb5i indicates the client wants protection against
> attacks which replace the body of the rpc, then a later krb5 setclientid
> should be rejected, since it could be the product of such an attack.)

If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis).

The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string.  A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic.  I can't think of a good reason it should do this.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote:
> 
> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote:
> > The server could just compare principal strings (and ignore
> > pseudoflavors) in the gss case.
> 
> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach.

I also don't know if principals produced by other mechanisms are going
to be comparable.  Of course that's a bit academic given that kerberos
is the only mechanism for the forseeable future.

> > If the intention is to ensure that a clientid can't be "hijacked" by
> > someone malicious, then you don't want to allow a krb5 setclientid to
> > blow away a clientid established with krb5i.  (If sending the
> > setclientid with krb5i indicates the client wants protection against
> > attacks which replace the body of the rpc, then a later krb5 setclientid
> > should be rejected, since it could be the product of such an attack.)
> 
> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis).
> 
> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string.  A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic.  I can't think of a good reason it should do this.

You don't know it's the same client any more.  The attack is:

	client sends setclientid with krb5i.

	client later sends some other rpc with krb5.

	attacker intercepts that operation, takes the rpc header, strips
	out the body, and replaces the body by a setclientid operation
	destroying the client's state.

Since krb5 only protects the header, the server can't tell that's what's
happened.

--b.

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 9, 2012, at 5:38 PM, J. Bruce Fields wrote:

> On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote:
>> 
>> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote:
>>> The server could just compare principal strings (and ignore
>>> pseudoflavors) in the gss case.
>> 
>> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach.
> 
> I also don't know if principals produced by other mechanisms are going
> to be comparable.  Of course that's a bit academic given that kerberos
> is the only mechanism for the forseeable future.

Agreed, I would expect this to work only for pseudoflavors that share the same type of principal.

Let me be sure I understand... I think the text of RFC 3530bis allows this implementation choice, but you are suggesting below that such an implementation may allow an attack.  Does this concern warrant a change to RFC 3530bis or 5661?  Should we bring this up on nfsv4@ietf.org?

> 
>>> If the intention is to ensure that a clientid can't be "hijacked" by
>>> someone malicious, then you don't want to allow a krb5 setclientid to
>>> blow away a clientid established with krb5i.  (If sending the
>>> setclientid with krb5i indicates the client wants protection against
>>> attacks which replace the body of the rpc, then a later krb5 setclientid
>>> should be rejected, since it could be the product of such an attack.)
>> 
>> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis).
>> 
>> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string.  A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic.  I can't think of a good reason it should do this.
> 
> You don't know it's the same client any more.  The attack is:
> 
> 	client sends setclientid with krb5i.
> 
> 	client later sends some other rpc with krb5.
> 
> 	attacker intercepts that operation, takes the rpc header, strips
> 	out the body, and replaces the body by a setclientid operation
> 	destroying the client's state.
> 
> Since krb5 only protects the header, the server can't tell that's what's
> happened.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Fri, Aug 10, 2012 at 03:38:30PM -0400, Chuck Lever wrote:
> 
> On Aug 9, 2012, at 5:38 PM, J. Bruce Fields wrote:
> 
> > On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote:
> >> 
> >> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote:
> >>> The server could just compare principal strings (and ignore
> >>> pseudoflavors) in the gss case.
> >> 
> >> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach.
> > 
> > I also don't know if principals produced by other mechanisms are going
> > to be comparable.  Of course that's a bit academic given that kerberos
> > is the only mechanism for the forseeable future.
> 
> Agreed, I would expect this to work only for pseudoflavors that share the same type of principal.
> 
> Let me be sure I understand... I think the text of RFC 3530bis allows this implementation choice, but you are suggesting below that such an implementation may allow an attack.  Does this concern warrant a change to RFC 3530bis or 5661?  Should we bring this up on nfsv4@ietf.org?

This should be all completely settled in 4.1.  It's 4.0 that I'm
confused by.

What do you need the client to be able to do?  Does it need to be able
to perform setclientid's on the same client with different security
flavors?

In this particular case (mount/umount with one flavor, then mount/umount
with another), maybe the client should destroy the client state (with an
"I rebooted" setclientid) on the first umount?  (I thought you had
patches that did that?)

--b.

> 
> > 
> >>> If the intention is to ensure that a clientid can't be "hijacked" by
> >>> someone malicious, then you don't want to allow a krb5 setclientid to
> >>> blow away a clientid established with krb5i.  (If sending the
> >>> setclientid with krb5i indicates the client wants protection against
> >>> attacks which replace the body of the rpc, then a later krb5 setclientid
> >>> should be rejected, since it could be the product of such an attack.)
> >> 
> >> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis).
> >> 
> >> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string.  A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic.  I can't think of a good reason it should do this.
> > 
> > You don't know it's the same client any more.  The attack is:
> > 
> > 	client sends setclientid with krb5i.
> > 
> > 	client later sends some other rpc with krb5.
> > 
> > 	attacker intercepts that operation, takes the rpc header, strips
> > 	out the body, and replaces the body by a setclientid operation
> > 	destroying the client's state.
> > 
> > Since krb5 only protects the header, the server can't tell that's what's
> > happened.
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 10, 2012, at 4:13 PM, J. Bruce Fields wrote:

> On Fri, Aug 10, 2012 at 03:38:30PM -0400, Chuck Lever wrote:
>> 
>> On Aug 9, 2012, at 5:38 PM, J. Bruce Fields wrote:
>> 
>>> On Thu, Aug 09, 2012 at 05:29:47PM -0400, Chuck Lever wrote:
>>>> 
>>>> On Aug 9, 2012, at 5:13 PM, J. Bruce Fields wrote:
>>>>> The server could just compare principal strings (and ignore
>>>>> pseudoflavors) in the gss case.
>>>> 
>>>> Barring a correction about how GSS Kerberos principals work, I think that's the correct approach.
>>> 
>>> I also don't know if principals produced by other mechanisms are going
>>> to be comparable.  Of course that's a bit academic given that kerberos
>>> is the only mechanism for the forseeable future.
>> 
>> Agreed, I would expect this to work only for pseudoflavors that share the same type of principal.
>> 
>> Let me be sure I understand... I think the text of RFC 3530bis allows this implementation choice, but you are suggesting below that such an implementation may allow an attack.  Does this concern warrant a change to RFC 3530bis or 5661?  Should we bring this up on nfsv4@ietf.org?
> 
> This should be all completely settled in 4.1.  It's 4.0 that I'm
> confused by.

Then 3530bis only.

> What do you need the client to be able to do?  Does it need to be able
> to perform setclientid's on the same client with different security
> flavors?

The problem is what client implementations really do, not what they "should" do.  They should use a single SETCLIENTID with one flavor.  Linux currently doesn't, and I'm guessing others are in the same boat.  Since clients use different id strings for each flavor, the problem is avoided, but that isn't desirable client behavior.

My concern above is with the spec.  If it allows insecure implementations, maybe we should fix the spec.

> In this particular case (mount/umount with one flavor, then mount/umount
> with another), maybe the client should destroy the client state (with an
> "I rebooted" setclientid) on the first umount?  (I thought you had
> patches that did that?)

OK, I didn't notice that you were unmounting between the tests.  3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior.  So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended?

I think we agree that, whether the Linux server is compliant with what's currently in 3530bis or not, it's the NFS client changes in 3.6 that "introduced the regression" and should be fixed.  If you change the client to add the flavor and pseudoflavor name to the nfs_client_id4.id string, does that fix the regression satisfactorily?

Maybe we should just revert those two and try again in 3.7.

> --b.
> 
>> 
>>> 
>>>>> If the intention is to ensure that a clientid can't be "hijacked" by
>>>>> someone malicious, then you don't want to allow a krb5 setclientid to
>>>>> blow away a clientid established with krb5i.  (If sending the
>>>>> setclientid with krb5i indicates the client wants protection against
>>>>> attacks which replace the body of the rpc, then a later krb5 setclientid
>>>>> should be rejected, since it could be the product of such an attack.)
>>>> 
>>>> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis).
>>>> 
>>>> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string.  A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic.  I can't think of a good reason it should do this.
>>> 
>>> You don't know it's the same client any more.  The attack is:
>>> 
>>> 	client sends setclientid with krb5i.
>>> 
>>> 	client later sends some other rpc with krb5.
>>> 
>>> 	attacker intercepts that operation, takes the rpc header, strips
>>> 	out the body, and replaces the body by a setclientid operation
>>> 	destroying the client's state.
>>> 
>>> Since krb5 only protects the header, the server can't tell that's what's
>>> happened.
>> 
>> -- 
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Fri, Aug 10, 2012 at 04:33:46PM -0400, Chuck Lever wrote:
> OK, I didn't notice that you were unmounting between the tests.  3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior.  So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended?

I don't know.  (Why is that the right thing to do?  Telling the server
when you're not using the client any more seems reasonable to me.)

> I think we agree that, whether the Linux server is compliant with what's currently in 3530bis or not, it's the NFS client changes in 3.6 that "introduced the regression" and should be fixed.  If you change the client to add the flavor and pseudoflavor name to the nfs_client_id4.id string, does that fix the regression satisfactorily?

I haven't tested it, but yes, that should do the job.

(Though note that's not just a revert, since previously only the flavor
(not the pseudoflavor) was factored into the clientid string.)

--b.

> 
> Maybe we should just revert those two and try again in 3.7.
> 
> > --b.
> > 
> >> 
> >>> 
> >>>>> If the intention is to ensure that a clientid can't be "hijacked" by
> >>>>> someone malicious, then you don't want to allow a krb5 setclientid to
> >>>>> blow away a clientid established with krb5i.  (If sending the
> >>>>> setclientid with krb5i indicates the client wants protection against
> >>>>> attacks which replace the body of the rpc, then a later krb5 setclientid
> >>>>> should be rejected, since it could be the product of such an attack.)
> >>>> 
> >>>> If the client wants to avoid hijacking, it should start out with krb5i, as is recommended in the Security Considerations section of 3530(bis).
> >>>> 
> >>>> The intention of 3530(bis) is that one client instance uses just one nfs_client_id4.id string.  A client that attempts to change flavors on its SETCLIENTID/RENEW operations in mid-journey, on purpose, seems a little schizophrenic.  I can't think of a good reason it should do this.
> >>> 
> >>> You don't know it's the same client any more.  The attack is:
> >>> 
> >>> 	client sends setclientid with krb5i.
> >>> 
> >>> 	client later sends some other rpc with krb5.
> >>> 
> >>> 	attacker intercepts that operation, takes the rpc header, strips
> >>> 	out the body, and replaces the body by a setclientid operation
> >>> 	destroying the client's state.
> >>> 
> >>> Since krb5 only protects the header, the server can't tell that's what's
> >>> happened.
> >> 
> >> -- 
> >> Chuck Lever
> >> chuck[dot]lever[at]oracle[dot]com
> >> 
> >> 
> >> 
> >> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Fri, Aug 10, 2012 at 04:39:30PM -0400, J. Bruce Fields wrote:
> On Fri, Aug 10, 2012 at 04:33:46PM -0400, Chuck Lever wrote:
> > OK, I didn't notice that you were unmounting between the tests.  3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior.  So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended?
> 
> I don't know.  (Why is that the right thing to do?  Telling the server
> when you're not using the client any more seems reasonable to me.)

And anyway can't the trick described in

	2c820d9a97f07b273b2c8a5960bd52b1b5864c68 "NFS: Force server to
	drop NFSv4 state"

be used to do that without changing the boot verifier?

--b.

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 10, 2012, at 4:39 PM, J. Bruce Fields wrote:

> On Fri, Aug 10, 2012 at 04:33:46PM -0400, Chuck Lever wrote:
>> OK, I didn't notice that you were unmounting between the tests.  3.5 and before did update the boot verifier after the last mount of a server goes away, but that's also not desirable behavior.  So I guess 3.6 also has my patch which makes the NFS client use the same boot verifier until it reboots, as God and the authors of 3530 intended?
> 
> I don't know.  (Why is that the right thing to do?  Telling the server
> when you're not using the client any more seems reasonable to me.)

The pre-3.6 Linux NFS client uses a different nfs_client_id4.id string for every server address, even server addresses that happen to be endpoints for the same server instance (multi-homed servers).

That means the id and verifier are associated with a struct nfs_client.  The id, because it gets the server's presentation address from the nfs_client; and the verifier, because the verifier is stored in the nfs_client itself.

That struct nfs_client goes away when the client unmounts the last share on a server address.  The next mount of that server address will get a new nfs_client and thus a fresh verifier.  The id string stays the same, but is unique for that server address, so the apparent reboot should not interfere with other leases on that server.

If the client uses the same id string for all server addresses (and I explain below why we want to go there) this changes things.  If the client completely unmounts and remounts one of the server's addresses, the new nfs_client will contain a fresh boot verifier, but the id string is the same as the one the client uses on all the other server addresses.  And that will result in the client's next SETCLIENTID canceling its own leases on the other server addresses for that server.

So, if the client uses one id string for all servers, it must also use one boot verifier for all servers, to avoid clobbering its own leases on multi-homed servers.

Having a single atomic client instance <id, verifier> is critical for making Transparent State Migration work properly.  A destination server can not recognize migrated leased state for a client if the id string and boot verifier can't be matched to it because the client uses a different boot verifier and id string for every server it talks to.


Can you explain why you think its a better idea for the client to remove its lease?  For 4.0, it's never had to do that before.  The case you've hit worked before by establishing an additional lease rather than by zapping the old one, for example.

Moreover, it shouldn't matter much that a client doesn't tell a server when it's done, since the client's leases will quickly expire.  Generally the client cleans up everything but the lease and clientid4 when it unmounts, unless the client crashes, which is hopefully rare (and couldn't result in an "I'm done" notification anyway).  So, I don't see how leaving a lease could be a significant server-side resource issue, or how it might interfere with the operation of other clients.


> 
>> I think we agree that, whether the Linux server is compliant with what's currently in 3530bis or not, it's the NFS client changes in 3.6 that "introduced the regression" and should be fixed.  If you change the client to add the flavor and pseudoflavor name to the nfs_client_id4.id string, does that fix the regression satisfactorily?
> 
> I haven't tested it, but yes, that should do the job.
> 
> (Though note that's not just a revert, since previously only the flavor
> (not the pseudoflavor) was factored into the clientid string.)

Correct, that fix is not a revert.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[J. Bruce Fields]

On Fri, Aug 10, 2012 at 05:22:17PM -0400, Chuck Lever wrote:
> Can you explain why you think its a better idea for the client to
> remove its lease?  For 4.0, it's never had to do that before.  The
> case you've hit worked before by establishing an additional lease
> rather than by zapping the old one, for example.
> 
> Moreover, it shouldn't matter much that a client doesn't tell a server
> when it's done, since the client's leases will quickly expire.
> Generally the client cleans up everything but the lease and clientid4
> when it unmounts, unless the client crashes, which is hopefully rare
> (and couldn't result in an "I'm done" notification anyway).  So, I
> don't see how leaving a lease could be a significant server-side
> resource issue, or how it might interfere with the operation of other
> clients.

Well, I've only seen the problem in this case.  I'm not seeing why the
client couldn't clean up its clientid as well, but there's also a server
problem here: I don't believe it should be returning INUSE when the
preexisting lease actually has no state associated with it any more.  So
my server's detection of when a clientid has no state is broken.  I'll
work on that.

For now I'm also dropping the flavor comparison, if this look OK.

--b.

commit d81a6e088c8c884f9be58dfe3647cbff22e1f112
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Aug 21 12:48:30 2012 -0400

    nfsd4: don't pin clientids to pseudoflavors
    
    I added cr_flavor to the data compared in same_creds without any
    justification, in d5497fc693a446ce9100fcf4117c3f795ddfd0d2 "nfsd4: move
    rq_flavor into svc_cred".
    
    Recent client changes then started making
    
    	mount -osec=krb5 server:/export /mnt/
    	echo "hello" >/mnt/TMP
    	umount /mnt/
    	mount -osec=krb5i server:/export /mnt/
    	echo "hello" >/mnt/TMP
    
    to fail due to a clid_inuse on the second open.
    
    Mounting sequentially like this with different flavors probably isn't
    that common outside artificial tests.  Also, the real bug here may be
    that the server isn't just destroying the former clientid in this case
    (because it isn't good enough at recognizing when the old state is
    gone).  But it prompted some discussion and a look back at the spec, and
    I think the check was probably wrong.  Fix and document.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6872852..b5058c2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1223,10 +1223,26 @@ static bool groups_equal(struct group_info *g1, struct group_info *g2)
 	return true;
 }
 
+/*
+ * RFC 3530 language requires clid_inuse be returned when the
+ * "principal" associated with a requests differs from that previously
+ * used.  We use uid, gid's, and gss principal string as our best
+ * approximation.  We also don't want to allow non-gss use of a client
+ * established using gss: in theory cr_principal should catch that
+ * change, but in practice cr_principal can be null even in the gss case
+ * since gssd doesn't always pass down a principal string.
+ */
+static bool is_gss_cred(struct svc_cred *cr)
+{
+	/* Is cr_flavor one of the gss "pseudoflavors"?: */
+	return (cr->cr_flavor > RPC_AUTH_MAXFLAVOR);
+}
+
+
 static bool
 same_creds(struct svc_cred *cr1, struct svc_cred *cr2)
 {
-	if ((cr1->cr_flavor != cr2->cr_flavor)
+	if ((is_gss_cred(cr1) != is_gss_cred(cr2))
 		|| (cr1->cr_uid != cr2->cr_uid)
 		|| (cr1->cr_gid != cr2->cr_gid)
 		|| !groups_equal(cr1->cr_group_info, cr2->cr_group_info))

Re: NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

[Chuck Lever]

On Aug 21, 2012, at 2:24 PM, J. Bruce Fields wrote:

> On Fri, Aug 10, 2012 at 05:22:17PM -0400, Chuck Lever wrote:
>> Can you explain why you think its a better idea for the client to
>> remove its lease?  For 4.0, it's never had to do that before.  The
>> case you've hit worked before by establishing an additional lease
>> rather than by zapping the old one, for example.
>> 
>> Moreover, it shouldn't matter much that a client doesn't tell a server
>> when it's done, since the client's leases will quickly expire.
>> Generally the client cleans up everything but the lease and clientid4
>> when it unmounts, unless the client crashes, which is hopefully rare
>> (and couldn't result in an "I'm done" notification anyway).  So, I
>> don't see how leaving a lease could be a significant server-side
>> resource issue, or how it might interfere with the operation of other
>> clients.
> 
> Well, I've only seen the problem in this case.  I'm not seeing why the
> client couldn't clean up its clientid as well, but there's also a server
> problem here: I don't believe it should be returning INUSE when the
> preexisting lease actually has no state associated with it any more.  So
> my server's detection of when a clientid has no state is broken.  I'll
> work on that.

Have you tried this optimization with a client that has concurrent mounts with different GSS flavors?  The below patch may make the optimization moot.

> For now I'm also dropping the flavor comparison, if this look OK.

Very interesting, looks plausible.  Commit d5497fc6 is in only 3.5?

With the below change and the current 3.6-rc2 NFS client, does the second open succeed now?

> --b.
> 
> commit d81a6e088c8c884f9be58dfe3647cbff22e1f112
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Tue Aug 21 12:48:30 2012 -0400
> 
>    nfsd4: don't pin clientids to pseudoflavors
> 
>    I added cr_flavor to the data compared in same_creds without any
>    justification, in d5497fc693a446ce9100fcf4117c3f795ddfd0d2 "nfsd4: move
>    rq_flavor into svc_cred".
> 
>    Recent client changes then started making
> 
>    	mount -osec=krb5 server:/export /mnt/
>    	echo "hello" >/mnt/TMP
>    	umount /mnt/
>    	mount -osec=krb5i server:/export /mnt/
>    	echo "hello" >/mnt/TMP
> 
>    to fail due to a clid_inuse on the second open.
> 
>    Mounting sequentially like this with different flavors probably isn't
>    that common outside artificial tests.  Also, the real bug here may be
>    that the server isn't just destroying the former clientid in this case
>    (because it isn't good enough at recognizing when the old state is
>    gone).  But it prompted some discussion and a look back at the spec, and
>    I think the check was probably wrong.  Fix and document.
> 
>    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6872852..b5058c2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1223,10 +1223,26 @@ static bool groups_equal(struct group_info *g1, struct group_info *g2)
> 	return true;
> }
> 
> +/*
> + * RFC 3530 language requires clid_inuse be returned when the
> + * "principal" associated with a requests differs from that previously
> + * used.  We use uid, gid's, and gss principal string as our best
> + * approximation.  We also don't want to allow non-gss use of a client
> + * established using gss: in theory cr_principal should catch that
> + * change, but in practice cr_principal can be null even in the gss case
> + * since gssd doesn't always pass down a principal string.
> + */
> +static bool is_gss_cred(struct svc_cred *cr)
> +{
> +	/* Is cr_flavor one of the gss "pseudoflavors"?: */
> +	return (cr->cr_flavor > RPC_AUTH_MAXFLAVOR);
> +}
> +
> +
> static bool
> same_creds(struct svc_cred *cr1, struct svc_cred *cr2)
> {
> -	if ((cr1->cr_flavor != cr2->cr_flavor)
> +	if ((is_gss_cred(cr1) != is_gss_cred(cr2))
> 		|| (cr1->cr_uid != cr2->cr_uid)
> 		|| (cr1->cr_gid != cr2->cr_gid)
> 		|| !groups_equal(cr1->cr_group_info, cr2->cr_group_info))

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com