NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

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 <[email protected]>
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 <[email protected]>

Signed-off-by Trond Myklebust <[email protected]>

fs/nfs/nfs4proc.c +16 -31

index 74dcd85..1148081 100644
@@ -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);

fs/nfs/nfs4state.c +6 -1

index 1cfc460..81eabcd 100644
@@ -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;

include/linux/nfs_fs_sb.h +1 -3

index f58325a..6532765 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · 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

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 17 lines · Chuck Lever

Quoted 13 lines · 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.

Quoted 3 lines

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.

Quoted 18 lines · J. Bruce Fields

Quoted 17 lines · Chuck Lever

Quoted 13 lines · 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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 23 lines · J. Bruce Fields

Quoted 22 lines · J. Bruce Fields

Quoted 20 lines · Chuck Lever

Quoted 10 lines · 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

Quoted 3 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 24 lines · Chuck Lever

Quoted 21 lines · J. Bruce Fields

Quoted 20 lines · J. Bruce Fields

Quoted 19 lines · Chuck Lever

Quoted 10 lines · 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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 27 lines · Chuck Lever

Quoted 24 lines · Chuck Lever

Quoted 21 lines · J. Bruce Fields

Quoted 20 lines · J. Bruce Fields

Quoted 19 lines · Chuck Lever

Quoted 10 lines · 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

Quoted 3 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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.)

Quoted 2 lines · J. Bruce Fields

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.

Quoted 6 lines

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.

Quoted 6 lines · Chuck Lever

Quoted 2 lines · J. Bruce Fields

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.

Quoted 11 lines

Quoted 7 lines

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.

Quoted 8 lines · J. Bruce Fields

Quoted 6 lines · Chuck Lever

Quoted 2 lines · J. Bruce Fields

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.

Quoted 3 lines

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 [email protected]?

Quoted 25 lines

Quoted 9 lines

Quoted 8 lines

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.)

Quoted 3 lines

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.

Signature · 2 lines

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

Quoted 15 lines · Chuck Lever

Quoted 11 lines · J. Bruce Fields

Quoted 6 lines · Chuck Lever

Quoted 2 lines · J. Bruce Fields

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.

Quoted 3 lines

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 [email protected]?

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.

Quoted 17 lines

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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.)

Quoted 3 lines

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.

Quoted 16 lines

Quoted 8 lines

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.

Signature · 2 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

Quoted 39 lines

--b.

Quoted 17 lines

Quoted 17 lines

Quoted 9 lines

Quoted 8 lines

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.)

Quoted 3 lines

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.

Quoted 16 lines

Quoted 8 lines

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.

Signature · 2 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

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

Quoted 1 line · Chuck Lever

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.)

Quoted 1 line · Chuck Lever

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.)

Quoted 2 lines

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.)

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.

Quoted 5 lines · Chuck Lever

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.

Quoted 8 lines

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 <[email protected]>
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 <[email protected]>

fs/nfsd/nfs4state.c +16 -0

index 6872852..b5058c2 100644
@@ -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) {

fs/nfsd/nfs4state.c +1 -1

-	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))