All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] NFSD patches for 3.5
@ 2012-05-15 21:41 Chuck Lever
  2012-05-15 21:42 ` [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often Chuck Lever
  2012-05-15 21:42 ` [PATCH 2/2] NFSD: Use __be32 for variable holding return value Chuck Lever
  0 siblings, 2 replies; 7+ messages in thread
From: Chuck Lever @ 2012-05-15 21:41 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

Hi Bruce-

Here's the SETCLIENTID_CONFIRM fix from a couple of weeks ago, and a
separate clean-up patch.

I haven't been able to reproduce the other issue, where the server
hands out different clientid4's for the same nfs_client_id4 string.
The logic in nfsd4_setclientid() appears to be working as designed.

I may have mistaken the clientid4 verifier for the clientid4, but
I didn't save the original network capture, unfortunately.

---

Chuck Lever (2):
      NFSD: Use __be32 for variable holding return value
      NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often


 fs/nfsd/nfs4state.c |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)

-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often
  2012-05-15 21:41 [PATCH 0/2] NFSD patches for 3.5 Chuck Lever
@ 2012-05-15 21:42 ` Chuck Lever
  2012-05-16  1:32   ` J. Bruce Fields
  2012-05-15 21:42 ` [PATCH 2/2] NFSD: Use __be32 for variable holding return value Chuck Lever
  1 sibling, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2012-05-15 21:42 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

According to RFC 3530bis, the only items SETCLIENTID_CONFIRM
processing should be concerned with is the clientid, clientid
verifier, and authentication flavor.  The client's IP address
is not supposed to be interesting.

And, NFS4ERR_CLID_INUSE is meant only for authentication flavor
mismatches.

I triggered this logic with a prototype UCS client -- one that
uses the same nfs_client_id4 string for all servers.  The client
mounted our server via its IPv4, then via its IPv6 address.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfsd/nfs4state.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7f71c69..27e6401 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2213,7 +2213,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			 struct nfsd4_compound_state *cstate,
 			 struct nfsd4_setclientid_confirm *setclientid_confirm)
 {
-	struct sockaddr *sa = svc_addr(rqstp);
 	struct nfs4_client *conf, *unconf;
 	nfs4_verifier confirm = setclientid_confirm->sc_confirm; 
 	clientid_t * clid = &setclientid_confirm->sc_clientid;
@@ -2231,17 +2230,12 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 	conf = find_confirmed_client(clid);
 	unconf = find_unconfirmed_client(clid);
 
-	status = nfserr_clid_inuse;
-	if (conf && !rpc_cmp_addr((struct sockaddr *) &conf->cl_addr, sa))
-		goto out;
-	if (unconf && !rpc_cmp_addr((struct sockaddr *) &unconf->cl_addr, sa))
-		goto out;
-
 	/*
 	 * section 14.2.34 of RFC 3530 has a description of
 	 * SETCLIENTID_CONFIRM request processing consisting
 	 * of 4 bullet points, labeled as CASE1 - CASE4 below.
 	 */
+	status = nfserr_clid_inuse;
 	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
 		/*
 		 * RFC 3530 14.2.34 CASE 1:
@@ -2254,7 +2248,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 			nfsd4_probe_callback(conf);
 			expire_client(unconf);
 			status = nfs_ok;
-
 		}
 	} else if (conf && !unconf) {
 		/*
@@ -2296,11 +2289,8 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 		 * Client probably hasn't noticed that we rebooted yet.
 		 */
 		status = nfserr_stale_clientid;
-	} else {
-		/* check that we have hit one of the cases...*/
-		status = nfserr_clid_inuse;
 	}
-out:
+
 	nfs4_unlock_state();
 	return status;
 }


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [PATCH 2/2] NFSD: Use __be32 for variable holding return value
  2012-05-15 21:41 [PATCH 0/2] NFSD patches for 3.5 Chuck Lever
  2012-05-15 21:42 ` [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often Chuck Lever
@ 2012-05-15 21:42 ` Chuck Lever
  2012-05-16 12:52   ` J. Bruce Fields
  1 sibling, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2012-05-15 21:42 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

Clean up: Use __be32 constants and variables consistently when
handling pre-encoded NFS error values.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfsd/nfs4state.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 27e6401..9235cfa 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -885,7 +885,7 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
 	struct nfsd4_session *new;
 	struct nfsd4_channel_attrs *fchan = &cses->fore_channel;
 	int numslots, slotsize;
-	int status;
+	__be32 status;
 	int idx;
 
 	/*
@@ -1476,12 +1476,12 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 		  struct nfsd4_exchange_id *exid)
 {
 	struct nfs4_client *unconf, *conf, *new;
-	int status;
 	unsigned int		strhashval;
 	char			dname[HEXDIR_LEN];
 	char			addr_str[INET6_ADDRSTRLEN];
 	nfs4_verifier		verf = exid->verifier;
 	struct sockaddr		*sa = svc_addr(rqstp);
+	__be32 status;
 
 	rpc_ntop(sa, addr_str, sizeof(addr_str));
 	dprintk("%s rqstp=%p exid=%p clname.len=%u clname.data=%p "
@@ -1673,7 +1673,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 	struct nfsd4_session *new;
 	struct nfsd4_clid_slot *cs_slot = NULL;
 	bool confirm_me = false;
-	int status = 0;
+	__be32 status = nfs_ok;
 
 	if (cr_ses->flags & ~SESSION4_FLAG_MASK_A)
 		return nfserr_inval;
@@ -1914,7 +1914,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
 	struct nfsd4_session *session;
 	struct nfsd4_slot *slot;
 	struct nfsd4_conn *conn;
-	int status;
+	__be32 status;
 
 	if (resp->opcnt != 1)
 		return nfserr_sequence_pos;
@@ -2019,7 +2019,7 @@ __be32
 nfsd4_destroy_clientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_destroy_clientid *dc)
 {
 	struct nfs4_client *conf, *unconf, *clp;
-	int status = 0;
+	__be32 status = nfs_ok;
 
 	nfs4_lock_state();
 	unconf = find_unconfirmed_client(&dc->clientid);
@@ -2055,7 +2055,7 @@ out:
 __be32
 nfsd4_reclaim_complete(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_reclaim_complete *rc)
 {
-	int status = 0;
+	__be32 status = nfs_ok;
 
 	if (rc->rca_one_fs) {
 		if (!cstate->current_fh.fh_dentry)
@@ -3330,7 +3330,8 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
 	return (s32)a->si_generation - (s32)b->si_generation > 0;
 }
 
-static int check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
+static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref,
+				       bool has_session)
 {
 	/*
 	 * When sessions are used the stateid generation number is ignored
@@ -4049,7 +4050,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct file *filp = NULL;
 	struct file_lock file_lock;
 	struct file_lock conflock;
-	__be32 status = 0;
+	__be32 status = nfs_ok;
 	bool new_state = false;
 	int lkflg;
 	int err;
@@ -4172,7 +4173,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		update_stateid(&lock_stp->st_stid.sc_stateid);
 		memcpy(&lock->lk_resp_stateid, &lock_stp->st_stid.sc_stateid, 
 				sizeof(stateid_t));
-		status = 0;
+		status = nfs_ok;
 		break;
 	case (EAGAIN):		/* conflock holds conflicting lock */
 		status = nfserr_denied;


^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often
  2012-05-15 21:42 ` [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often Chuck Lever
@ 2012-05-16  1:32   ` J. Bruce Fields
  2012-05-16  2:34     ` Chuck Lever
  0 siblings, 1 reply; 7+ messages in thread
From: J. Bruce Fields @ 2012-05-16  1:32 UTC (permalink / raw)
  To: Chuck Lever; +Cc: linux-nfs

On Tue, May 15, 2012 at 05:42:08PM -0400, Chuck Lever wrote:
> According to RFC 3530bis, the only items SETCLIENTID_CONFIRM
> processing should be concerned with is the clientid, clientid
> verifier, and authentication flavor.

Nit: the spec says "principal", not "authentication flavor".

(I was looking at this yesterday because our current code only compares
uid's, which is pretty meaningless--any client using a krb5 service
principal, for example, will likely get mapped to the same uid--so I
have patches that instead compare flavor, principal name (when we have
it--our current userspace only passes it down for service principals)
and groups and auxiliary groups as well as uid.  But not IP address...)

Anyway, this patch looks reasonable, thanks.

--b.

> The client's IP address
> is not supposed to be interesting.
> 
> And, NFS4ERR_CLID_INUSE is meant only for authentication flavor
> mismatches.
> 
> I triggered this logic with a prototype UCS client -- one that
> uses the same nfs_client_id4 string for all servers.  The client
> mounted our server via its IPv4, then via its IPv6 address.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfsd/nfs4state.c |   14 ++------------
>  1 files changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7f71c69..27e6401 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2213,7 +2213,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>  			 struct nfsd4_compound_state *cstate,
>  			 struct nfsd4_setclientid_confirm *setclientid_confirm)
>  {
> -	struct sockaddr *sa = svc_addr(rqstp);
>  	struct nfs4_client *conf, *unconf;
>  	nfs4_verifier confirm = setclientid_confirm->sc_confirm; 
>  	clientid_t * clid = &setclientid_confirm->sc_clientid;
> @@ -2231,17 +2230,12 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>  	conf = find_confirmed_client(clid);
>  	unconf = find_unconfirmed_client(clid);
>  
> -	status = nfserr_clid_inuse;
> -	if (conf && !rpc_cmp_addr((struct sockaddr *) &conf->cl_addr, sa))
> -		goto out;
> -	if (unconf && !rpc_cmp_addr((struct sockaddr *) &unconf->cl_addr, sa))
> -		goto out;
> -
>  	/*
>  	 * section 14.2.34 of RFC 3530 has a description of
>  	 * SETCLIENTID_CONFIRM request processing consisting
>  	 * of 4 bullet points, labeled as CASE1 - CASE4 below.
>  	 */
> +	status = nfserr_clid_inuse;
>  	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
>  		/*
>  		 * RFC 3530 14.2.34 CASE 1:
> @@ -2254,7 +2248,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>  			nfsd4_probe_callback(conf);
>  			expire_client(unconf);
>  			status = nfs_ok;
> -
>  		}
>  	} else if (conf && !unconf) {
>  		/*
> @@ -2296,11 +2289,8 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>  		 * Client probably hasn't noticed that we rebooted yet.
>  		 */
>  		status = nfserr_stale_clientid;
> -	} else {
> -		/* check that we have hit one of the cases...*/
> -		status = nfserr_clid_inuse;
>  	}
> -out:
> +
>  	nfs4_unlock_state();
>  	return status;
>  }
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often
  2012-05-16  1:32   ` J. Bruce Fields
@ 2012-05-16  2:34     ` Chuck Lever
  2012-05-16 12:52       ` J. Bruce Fields
  0 siblings, 1 reply; 7+ messages in thread
From: Chuck Lever @ 2012-05-16  2:34 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs


On May 15, 2012, at 9:32 PM, J. Bruce Fields wrote:

> On Tue, May 15, 2012 at 05:42:08PM -0400, Chuck Lever wrote:
>> According to RFC 3530bis, the only items SETCLIENTID_CONFIRM
>> processing should be concerned with is the clientid, clientid
>> verifier, and authentication flavor.
> 
> Nit: the spec says "principal", not "authentication flavor".

Fair enough... feel free to correct the description.

> (I was looking at this yesterday because our current code only compares
> uid's, which is pretty meaningless--any client using a krb5 service
> principal, for example, will likely get mapped to the same uid--so I
> have patches that instead compare flavor, principal name (when we have
> it--our current userspace only passes it down for service principals)
> and groups and auxiliary groups as well as uid.  But not IP address...)
> 
> Anyway, this patch looks reasonable, thanks.
> 
> --b.
> 
>> The client's IP address
>> is not supposed to be interesting.
>> 
>> And, NFS4ERR_CLID_INUSE is meant only for authentication flavor
>> mismatches.
>> 
>> I triggered this logic with a prototype UCS client -- one that
>> uses the same nfs_client_id4 string for all servers.  The client
>> mounted our server via its IPv4, then via its IPv6 address.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> fs/nfsd/nfs4state.c |   14 ++------------
>> 1 files changed, 2 insertions(+), 12 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 7f71c69..27e6401 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2213,7 +2213,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>> 			 struct nfsd4_compound_state *cstate,
>> 			 struct nfsd4_setclientid_confirm *setclientid_confirm)
>> {
>> -	struct sockaddr *sa = svc_addr(rqstp);
>> 	struct nfs4_client *conf, *unconf;
>> 	nfs4_verifier confirm = setclientid_confirm->sc_confirm; 
>> 	clientid_t * clid = &setclientid_confirm->sc_clientid;
>> @@ -2231,17 +2230,12 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>> 	conf = find_confirmed_client(clid);
>> 	unconf = find_unconfirmed_client(clid);
>> 
>> -	status = nfserr_clid_inuse;
>> -	if (conf && !rpc_cmp_addr((struct sockaddr *) &conf->cl_addr, sa))
>> -		goto out;
>> -	if (unconf && !rpc_cmp_addr((struct sockaddr *) &unconf->cl_addr, sa))
>> -		goto out;
>> -
>> 	/*
>> 	 * section 14.2.34 of RFC 3530 has a description of
>> 	 * SETCLIENTID_CONFIRM request processing consisting
>> 	 * of 4 bullet points, labeled as CASE1 - CASE4 below.
>> 	 */
>> +	status = nfserr_clid_inuse;
>> 	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
>> 		/*
>> 		 * RFC 3530 14.2.34 CASE 1:
>> @@ -2254,7 +2248,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>> 			nfsd4_probe_callback(conf);
>> 			expire_client(unconf);
>> 			status = nfs_ok;
>> -
>> 		}
>> 	} else if (conf && !unconf) {
>> 		/*
>> @@ -2296,11 +2289,8 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>> 		 * Client probably hasn't noticed that we rebooted yet.
>> 		 */
>> 		status = nfserr_stale_clientid;
>> -	} else {
>> -		/* check that we have hit one of the cases...*/
>> -		status = nfserr_clid_inuse;
>> 	}
>> -out:
>> +
>> 	nfs4_unlock_state();
>> 	return status;
>> }
>> 
> --
> 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





^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] NFSD: Use __be32 for variable holding return value
  2012-05-15 21:42 ` [PATCH 2/2] NFSD: Use __be32 for variable holding return value Chuck Lever
@ 2012-05-16 12:52   ` J. Bruce Fields
  0 siblings, 0 replies; 7+ messages in thread
From: J. Bruce Fields @ 2012-05-16 12:52 UTC (permalink / raw)
  To: Chuck Lever; +Cc: bfields, linux-nfs

On Tue, May 15, 2012 at 05:42:16PM -0400, Chuck Lever wrote:
> Clean up: Use __be32 constants and variables consistently when
> handling pre-encoded NFS error values.

I already have patches for these committed to my for-3.5 branch.  If you
see anything in there I've missed, let me know.

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfsd/nfs4state.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 27e6401..9235cfa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -885,7 +885,7 @@ static struct nfsd4_session *alloc_init_session(struct svc_rqst *rqstp, struct n
>  	struct nfsd4_session *new;
>  	struct nfsd4_channel_attrs *fchan = &cses->fore_channel;
>  	int numslots, slotsize;
> -	int status;
> +	__be32 status;
>  	int idx;
>  
>  	/*
> @@ -1476,12 +1476,12 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
>  		  struct nfsd4_exchange_id *exid)
>  {
>  	struct nfs4_client *unconf, *conf, *new;
> -	int status;
>  	unsigned int		strhashval;
>  	char			dname[HEXDIR_LEN];
>  	char			addr_str[INET6_ADDRSTRLEN];
>  	nfs4_verifier		verf = exid->verifier;
>  	struct sockaddr		*sa = svc_addr(rqstp);
> +	__be32 status;
>  
>  	rpc_ntop(sa, addr_str, sizeof(addr_str));
>  	dprintk("%s rqstp=%p exid=%p clname.len=%u clname.data=%p "
> @@ -1673,7 +1673,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>  	struct nfsd4_session *new;
>  	struct nfsd4_clid_slot *cs_slot = NULL;
>  	bool confirm_me = false;
> -	int status = 0;
> +	__be32 status = nfs_ok;
>  
>  	if (cr_ses->flags & ~SESSION4_FLAG_MASK_A)
>  		return nfserr_inval;
> @@ -1914,7 +1914,7 @@ nfsd4_sequence(struct svc_rqst *rqstp,
>  	struct nfsd4_session *session;
>  	struct nfsd4_slot *slot;
>  	struct nfsd4_conn *conn;
> -	int status;
> +	__be32 status;
>  
>  	if (resp->opcnt != 1)
>  		return nfserr_sequence_pos;
> @@ -2019,7 +2019,7 @@ __be32
>  nfsd4_destroy_clientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_destroy_clientid *dc)
>  {
>  	struct nfs4_client *conf, *unconf, *clp;
> -	int status = 0;
> +	__be32 status = nfs_ok;
>  
>  	nfs4_lock_state();
>  	unconf = find_unconfirmed_client(&dc->clientid);
> @@ -2055,7 +2055,7 @@ out:
>  __be32
>  nfsd4_reclaim_complete(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_reclaim_complete *rc)
>  {
> -	int status = 0;
> +	__be32 status = nfs_ok;
>  
>  	if (rc->rca_one_fs) {
>  		if (!cstate->current_fh.fh_dentry)
> @@ -3330,7 +3330,8 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
>  	return (s32)a->si_generation - (s32)b->si_generation > 0;
>  }
>  
> -static int check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
> +static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref,
> +				       bool has_session)
>  {
>  	/*
>  	 * When sessions are used the stateid generation number is ignored
> @@ -4049,7 +4050,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct file *filp = NULL;
>  	struct file_lock file_lock;
>  	struct file_lock conflock;
> -	__be32 status = 0;
> +	__be32 status = nfs_ok;
>  	bool new_state = false;
>  	int lkflg;
>  	int err;
> @@ -4172,7 +4173,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		update_stateid(&lock_stp->st_stid.sc_stateid);
>  		memcpy(&lock->lk_resp_stateid, &lock_stp->st_stid.sc_stateid, 
>  				sizeof(stateid_t));
> -		status = 0;
> +		status = nfs_ok;
>  		break;
>  	case (EAGAIN):		/* conflock holds conflicting lock */
>  		status = nfserr_denied;
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often
  2012-05-16  2:34     ` Chuck Lever
@ 2012-05-16 12:52       ` J. Bruce Fields
  0 siblings, 0 replies; 7+ messages in thread
From: J. Bruce Fields @ 2012-05-16 12:52 UTC (permalink / raw)
  To: Chuck Lever; +Cc: J. Bruce Fields, linux-nfs

On Tue, May 15, 2012 at 10:34:34PM -0400, Chuck Lever wrote:
> 
> On May 15, 2012, at 9:32 PM, J. Bruce Fields wrote:
> 
> > On Tue, May 15, 2012 at 05:42:08PM -0400, Chuck Lever wrote:
> >> According to RFC 3530bis, the only items SETCLIENTID_CONFIRM
> >> processing should be concerned with is the clientid, clientid
> >> verifier, and authentication flavor.
> > 
> > Nit: the spec says "principal", not "authentication flavor".
> 
> Fair enough... feel free to correct the description.

OK, applying.  Thanks for fixing this.

--b.

> 
> > (I was looking at this yesterday because our current code only compares
> > uid's, which is pretty meaningless--any client using a krb5 service
> > principal, for example, will likely get mapped to the same uid--so I
> > have patches that instead compare flavor, principal name (when we have
> > it--our current userspace only passes it down for service principals)
> > and groups and auxiliary groups as well as uid.  But not IP address...)
> > 
> > Anyway, this patch looks reasonable, thanks.
> > 
> > --b.
> > 
> >> The client's IP address
> >> is not supposed to be interesting.
> >> 
> >> And, NFS4ERR_CLID_INUSE is meant only for authentication flavor
> >> mismatches.
> >> 
> >> I triggered this logic with a prototype UCS client -- one that
> >> uses the same nfs_client_id4 string for all servers.  The client
> >> mounted our server via its IPv4, then via its IPv6 address.
> >> 
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> 
> >> fs/nfsd/nfs4state.c |   14 ++------------
> >> 1 files changed, 2 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 7f71c69..27e6401 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -2213,7 +2213,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
> >> 			 struct nfsd4_compound_state *cstate,
> >> 			 struct nfsd4_setclientid_confirm *setclientid_confirm)
> >> {
> >> -	struct sockaddr *sa = svc_addr(rqstp);
> >> 	struct nfs4_client *conf, *unconf;
> >> 	nfs4_verifier confirm = setclientid_confirm->sc_confirm; 
> >> 	clientid_t * clid = &setclientid_confirm->sc_clientid;
> >> @@ -2231,17 +2230,12 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
> >> 	conf = find_confirmed_client(clid);
> >> 	unconf = find_unconfirmed_client(clid);
> >> 
> >> -	status = nfserr_clid_inuse;
> >> -	if (conf && !rpc_cmp_addr((struct sockaddr *) &conf->cl_addr, sa))
> >> -		goto out;
> >> -	if (unconf && !rpc_cmp_addr((struct sockaddr *) &unconf->cl_addr, sa))
> >> -		goto out;
> >> -
> >> 	/*
> >> 	 * section 14.2.34 of RFC 3530 has a description of
> >> 	 * SETCLIENTID_CONFIRM request processing consisting
> >> 	 * of 4 bullet points, labeled as CASE1 - CASE4 below.
> >> 	 */
> >> +	status = nfserr_clid_inuse;
> >> 	if (conf && unconf && same_verf(&confirm, &unconf->cl_confirm)) {
> >> 		/*
> >> 		 * RFC 3530 14.2.34 CASE 1:
> >> @@ -2254,7 +2248,6 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
> >> 			nfsd4_probe_callback(conf);
> >> 			expire_client(unconf);
> >> 			status = nfs_ok;
> >> -
> >> 		}
> >> 	} else if (conf && !unconf) {
> >> 		/*
> >> @@ -2296,11 +2289,8 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
> >> 		 * Client probably hasn't noticed that we rebooted yet.
> >> 		 */
> >> 		status = nfserr_stale_clientid;
> >> -	} else {
> >> -		/* check that we have hit one of the cases...*/
> >> -		status = nfserr_clid_inuse;
> >> 	}
> >> -out:
> >> +
> >> 	nfs4_unlock_state();
> >> 	return status;
> >> }
> >> 
> > --
> > 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
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-16 12:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-15 21:41 [PATCH 0/2] NFSD patches for 3.5 Chuck Lever
2012-05-15 21:42 ` [PATCH 1/2] NFSD: SETCLIENTID_CONFIRM returns NFS4ERR_CLID_INUSE too often Chuck Lever
2012-05-16  1:32   ` J. Bruce Fields
2012-05-16  2:34     ` Chuck Lever
2012-05-16 12:52       ` J. Bruce Fields
2012-05-15 21:42 ` [PATCH 2/2] NFSD: Use __be32 for variable holding return value Chuck Lever
2012-05-16 12:52   ` J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.