[PATCH 00/14] nfsd: some updates and cleanups

[PATCH 00/14] nfsd: some updates and cleanups

[Kinglong Mee]

The first five are some bugfix and update,
the last nine are some cleanups.

Kinglong Mee (14):
  nfsd: Add layouts checking for state resources
  nfsd: Add missing gen_confirm nfsd4_setclientid()
  nfsd: Fix memory leak of so_owner.data in nfs4_stateowner
  nfsd: Fix a memory leak of struct file_lock
  nfsd: Use check_stateid_generation() for generation checking
  nfsd: Drop duplicate locks_init_lock()
  nfsd: Remove unneeded values in nfsd4_open()
  nfsd: Drop duplicate checking of seqid in nfsd4_create_session()
  nfsd: Remove nfs4_set_claim_prev()
  nfsd: Remove unused values in nfs4_setlease()
  nfsd: Remove duplicate checking of nfsd_net in nfs4_laundromat()
  nfsd: Remove macro LOFF_OVERFLOW
  nfsd: Use lk_new_xxx instead of v.new.xxx for nfs4_lockowner
  nfsd: Remove unused clientid arguments from find_lockowner_str{_locked}

 fs/nfsd/nfs4layouts.c |  7 +++--
 fs/nfsd/nfs4proc.c    |  4 +--
 fs/nfsd/nfs4state.c   | 74 ++++++++++++++++++++++-----------------------------
 fs/nfsd/state.h       |  1 +
 4 files changed, 37 insertions(+), 49 deletions(-)

-- 
2.4.3

[PATCH 01/14] nfsd: Add layouts checking for state resources

[Kinglong Mee]

Layout is a state resource, nfsd should check it too.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..e0a4556 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
 	 * Also note we should probably be using this in 4.0 case too.
 	 */
 	return !list_empty(&clp->cl_openowners)
+		|| !list_empty(&clp->cl_lo_states)
 		|| !list_empty(&clp->cl_delegations)
 		|| !list_empty(&clp->cl_sessions);
 }
@@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	clp = cstate->clp;
 	status = nfserr_cb_path_down;
-	if (!list_empty(&clp->cl_delegations)
-			&& clp->cl_cb_state != NFSD4_CB_UP)
+	if ((!list_empty(&clp->cl_delegations)
+	     || !list_empty(&clp->cl_lo_states))
+	    && clp->cl_cb_state != NFSD4_CB_UP)
 		goto out;
 	status = nfs_ok;
 out:
-- 
2.4.3

[PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[Kinglong Mee]

Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
have moved gen_confirm() to gen_clid().

After it, setclientid will return a bad reply with all zero confirms
after copy_clid().

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e0a4556..b1f84fc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	unconf = find_unconfirmed_client_by_name(&clname, nn);
 	if (unconf)
 		unhash_client_locked(unconf);
-	if (conf && same_verf(&conf->cl_verifier, &clverifier))
+	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
 		/* case 1: probable callback update */
 		copy_clid(new, conf);
-	else /* case 4 (new client) or cases 2, 3 (client reboot): */
+		gen_confirm(new, nn);
+	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
 		gen_clid(new, nn);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);
-- 
2.4.3

[PATCH 03/14] nfsd: Fix memory leak of so_owner.data in nfs4_stateowner

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b1f84fc..e5e14fa 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3316,8 +3316,10 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 	if (ret == NULL) {
 		hash_openowner(oo, clp, strhashval);
 		ret = oo;
-	} else
+	} else {
+		kfree(oo->oo_owner.so_owner.data);
 		nfs4_free_openowner(&oo->oo_owner);
+	}
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }
@@ -5217,8 +5219,10 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 		list_add(&lo->lo_owner.so_strhash,
 			 &clp->cl_ownerstr_hashtbl[strhashval]);
 		ret = lo;
-	} else
-		nfs4_free_lockowner(&lo->lo_owner);
+	} else {
+		kfree(lo->lo_owner.so_owner.data);
+		nfs4_free_openowner(&lo->lo_owner);
+	}
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }
-- 
2.4.3

[PATCH 04/14] nfsd: Fix a memory leak of struct file_lock

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e5e14fa..998166d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3938,6 +3938,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	if (!filp) {
 		/* We should always have a readable file here */
 		WARN_ON_ONCE(1);
+		locks_free_lock(fl);
 		return -EBADF;
 	}
 	fl->fl_file = filp;
-- 
2.4.3

[PATCH 05/14] nfsd: Use check_stateid_generation() for generation checking

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4layouts.c | 5 ++---
 fs/nfsd/nfs4state.c   | 2 +-
 fs/nfsd/state.h       | 1 +
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 6904213..5c0f1d6 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 			goto out;
 	} else {
 		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
-
-		status = nfserr_bad_stateid;
-		if (stateid->si_generation > stid->sc_stateid.si_generation)
+		status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
+		if (status)
 			goto out_put_stid;
 		if (layout_type != ls->ls_layout_type)
 			goto out_put_stid;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 998166d..2edfedc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
 	return (s32)(a->si_generation - b->si_generation) > 0;
 }
 
-static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
 {
 	/*
 	 * When sessions are used the stateid generation number is ignored
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 4874ce5..18d015d 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn);
+__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
 struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
 		struct kmem_cache *slab);
 void nfs4_unhash_stid(struct nfs4_stid *s);
-- 
2.4.3

[PATCH 06/14] nfsd: Drop duplicate locks_init_lock()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4layouts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 5c0f1d6..c5af4ccb 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -162,7 +162,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
 	fl = locks_alloc_lock();
 	if (!fl)
 		return -ENOMEM;
-	locks_init_lock(fl);
+
 	fl->fl_lmops = &nfsd4_layouts_lm_ops;
 	fl->fl_flags = FL_LAYOUT;
 	fl->fl_type = F_RDLCK;
-- 
2.4.3

[PATCH 07/14] nfsd: Remove unneeded values in nfsd4_open()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4proc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 90cfda7..5d99111 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -362,7 +362,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 {
 	__be32 status;
 	struct svc_fh *resfh = NULL;
-	struct nfsd4_compoundres *resp;
 	struct net *net = SVC_NET(rqstp);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
@@ -389,8 +388,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		copy_clientid(&open->op_clientid, cstate->session);
 
 	/* check seqid for replay. set nfs4_owner */
-	resp = rqstp->rq_resp;
-	status = nfsd4_process_open1(&resp->cstate, open, nn);
+	status = nfsd4_process_open1(cstate, open, nn);
 	if (status == nfserr_replay_me) {
 		struct nfs4_replay *rp = &open->op_openowner->oo_owner.so_replay;
 		fh_put(&cstate->current_fh);
-- 
2.4.3

[PATCH 08/14] nfsd: Drop duplicate checking of seqid in nfsd4_create_session()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2edfedc..c94d1ef 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2548,11 +2548,9 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 			goto out_free_conn;
 		cs_slot = &conf->cl_cs_slot;
 		status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
-		if (status == nfserr_replay_cache) {
-			status = nfsd4_replay_create_session(cr_ses, cs_slot);
-			goto out_free_conn;
-		} else if (cr_ses->seqid != cs_slot->sl_seqid + 1) {
-			status = nfserr_seq_misordered;
+		if (status) {
+			if (status == nfserr_replay_cache)
+				status = nfsd4_replay_create_session(cr_ses, cs_slot);
 			goto out_free_conn;
 		}
 	} else if (unconf) {
-- 
2.4.3

[PATCH 09/14] nfsd: Remove nfs4_set_claim_prev()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c94d1ef..f3c38e3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3887,12 +3887,6 @@ nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *c
 	return status;
 }
 
-static void
-nfs4_set_claim_prev(struct nfsd4_open *open, bool has_session)
-{
-	open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
-}
-
 /* Should we give out recallable state?: */
 static bool nfsd4_cb_channel_good(struct nfs4_client *clp)
 {
@@ -4212,7 +4206,7 @@ out:
 	if (fp)
 		put_nfs4_file(fp);
 	if (status == 0 && open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS)
-		nfs4_set_claim_prev(open, nfsd4_has_session(&resp->cstate));
+		open->op_openowner->oo_flags |= NFS4_OO_CONFIRMED;
 	/*
 	* To finish the open response, we just need to set the rflags.
 	*/
-- 
2.4.3

[PATCH 10/14] nfsd: Remove unused values in nfs4_setlease()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f3c38e3..8205542 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3919,7 +3919,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
 static int nfs4_setlease(struct nfs4_delegation *dp)
 {
 	struct nfs4_file *fp = dp->dl_stid.sc_file;
-	struct file_lock *fl, *ret;
+	struct file_lock *fl;
 	struct file *filp;
 	int status = 0;
 
@@ -3934,7 +3934,6 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 		return -EBADF;
 	}
 	fl->fl_file = filp;
-	ret = fl;
 	status = vfs_setlease(filp, fl->fl_type, &fl, NULL);
 	if (fl)
 		locks_free_lock(fl);
-- 
2.4.3

[PATCH 11/14] nfsd: Remove duplicate checking of nfsd_net in nfs4_laundromat()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8205542..d342769 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4335,8 +4335,6 @@ nfs4_laundromat(struct nfsd_net *nn)
 	spin_lock(&state_lock);
 	list_for_each_safe(pos, next, &nn->del_recall_lru) {
 		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
-		if (net_generic(dp->dl_stid.sc_client->net, nfsd_net_id) != nn)
-			continue;
 		if (time_after((unsigned long)dp->dl_time, (unsigned long)cutoff)) {
 			t = dp->dl_time - cutoff;
 			new_timeo = min(new_timeo, t);
-- 
2.4.3

[PATCH 12/14] nfsd: Remove macro LOFF_OVERFLOW

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d342769..13df582 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5040,9 +5040,6 @@ out:
 	return status;
 }
 
-
-#define LOFF_OVERFLOW(start, len)      ((u64)(len) > ~(u64)(start))
-
 static inline u64
 end_offset(u64 start, u64 len)
 {
@@ -5295,8 +5292,8 @@ find_or_create_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fi,
 static int
 check_lock_length(u64 offset, u64 length)
 {
-	return ((length == 0)  || ((length != NFS4_MAX_UINT64) &&
-	     LOFF_OVERFLOW(offset, length)));
+	return ((length == 0) || ((length != NFS4_MAX_UINT64) &&
+		(length > ~offset)));
 }
 
 static void get_lock_access(struct nfs4_ol_stateid *lock_stp, u32 access)
-- 
2.4.3

[PATCH 13/14] nfsd: Use lk_new_xxx instead of v.new.xxx for nfs4_lockowner

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 13df582..2704270 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5322,9 +5322,9 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 	struct nfs4_lockowner *lo;
 	unsigned int strhashval;
 
-	lo = find_lockowner_str(&cl->cl_clientid, &lock->v.new.owner, cl);
+	lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
 	if (!lo) {
-		strhashval = ownerstr_hashval(&lock->v.new.owner);
+		strhashval = ownerstr_hashval(&lock->lk_new_owner);
 		lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
 		if (lo == NULL)
 			return nfserr_jukebox;
@@ -5385,7 +5385,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (lock->lk_is_new) {
 		if (nfsd4_has_session(cstate))
 			/* See rfc 5661 18.10.3: given clientid is ignored: */
-			memcpy(&lock->v.new.clientid,
+			memcpy(&lock->lk_new_clientid,
 				&cstate->session->se_client->cl_clientid,
 				sizeof(clientid_t));
 
@@ -5403,7 +5403,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		open_sop = openowner(open_stp->st_stateowner);
 		status = nfserr_bad_stateid;
 		if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
-						&lock->v.new.clientid))
+						&lock->lk_new_clientid))
 			goto out;
 		status = lookup_or_create_lock_state(cstate, open_stp, lock,
 							&lock_stp, &new);
-- 
2.4.3

[PATCH 14/14] nfsd: Remove unused clientid arguments from, find_lockowner_str{_locked}

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2704270..b6c134d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5131,8 +5131,7 @@ nevermind:
 }
 
 static struct nfs4_lockowner *
-find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
-		struct nfs4_client *clp)
+find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
 {
 	unsigned int strhashval = ownerstr_hashval(owner);
 	struct nfs4_stateowner *so;
@@ -5150,13 +5149,12 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
 }
 
 static struct nfs4_lockowner *
-find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
-		struct nfs4_client *clp)
+find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
 {
 	struct nfs4_lockowner *lo;
 
 	spin_lock(&clp->cl_lock);
-	lo = find_lockowner_str_locked(clid, owner, clp);
+	lo = find_lockowner_str_locked(clp, owner);
 	spin_unlock(&clp->cl_lock);
 	return lo;
 }
@@ -5200,8 +5198,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 	lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
 	lo->lo_owner.so_ops = &lockowner_ops;
 	spin_lock(&clp->cl_lock);
-	ret = find_lockowner_str_locked(&clp->cl_clientid,
-			&lock->lk_new_owner, clp);
+	ret = find_lockowner_str_locked(clp, &lock->lk_new_owner);
 	if (ret == NULL) {
 		list_add(&lo->lo_owner.so_strhash,
 			 &clp->cl_ownerstr_hashtbl[strhashval]);
@@ -5322,7 +5319,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
 	struct nfs4_lockowner *lo;
 	unsigned int strhashval;
 
-	lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
+	lo = find_lockowner_str(cl, &lock->lk_new_owner);
 	if (!lo) {
 		strhashval = ownerstr_hashval(&lock->lk_new_owner);
 		lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
@@ -5597,8 +5594,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
-				cstate->clp);
+	lo = find_lockowner_str(cstate->clp, &lockt->lt_owner);
 	if (lo)
 		file_lock->fl_owner = (fl_owner_t)lo;
 	file_lock->fl_pid = current->tgid;
-- 
2.4.3

Re: [PATCH 01/14] nfsd: Add layouts checking for state resources

[J. Bruce Fields]

On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
> Layout is a state resource, nfsd should check it too.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 61dfb33..e0a4556 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
>  	 * Also note we should probably be using this in 4.0 case too.
>  	 */
>  	return !list_empty(&clp->cl_openowners)
> +		|| !list_empty(&clp->cl_lo_states)
>  		|| !list_empty(&clp->cl_delegations)
>  		|| !list_empty(&clp->cl_sessions);
>  }
> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	clp = cstate->clp;
>  	status = nfserr_cb_path_down;
> -	if (!list_empty(&clp->cl_delegations)
> -			&& clp->cl_cb_state != NFSD4_CB_UP)
> +	if ((!list_empty(&clp->cl_delegations)
> +	     || !list_empty(&clp->cl_lo_states))

nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.

That said, it's harmless--why don't we just call client_has_state() here
instead?

--b.

> +	    && clp->cl_cb_state != NFSD4_CB_UP)
>  		goto out;
>  	status = nfs_ok;
>  out:
> -- 
> 2.4.3

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[J. Bruce Fields]

On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> have moved gen_confirm() to gen_clid().

This means the statement in that earlier commit is wrong:

	
	With this, there's no need to keep two counters as they'd always
	be in sync anyway, so just use the clientid_counter for both.

Looks to me like this may need a separate counter to eliminate the
possibibility of returning the same confirm twice for a one clientid?

--b.

> 
> After it, setclientid will return a bad reply with all zero confirms
> after copy_clid().
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e0a4556..b1f84fc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	unconf = find_unconfirmed_client_by_name(&clname, nn);
>  	if (unconf)
>  		unhash_client_locked(unconf);
> -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
> +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
>  		/* case 1: probable callback update */
>  		copy_clid(new, conf);
> -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
> +		gen_confirm(new, nn);
> +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
>  		gen_clid(new, nn);
>  	new->cl_minorversion = 0;
>  	gen_callback(new, setclid, rqstp);
> -- 
> 2.4.3

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[J. Bruce Fields]

On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> > Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> > have moved gen_confirm() to gen_clid().
> 
> This means the statement in that earlier commit is wrong:
> 
> 	
> 	With this, there's no need to keep two counters as they'd always
> 	be in sync anyway, so just use the clientid_counter for both.
> 
> Looks to me like this may need a separate counter to eliminate the
> possibibility of returning the same confirm twice for a one clientid?

(but frankly I can never completely review changes to the
setclientid/setclientid_confirm behavior without rereading RFC 7530
16.33.5 every time, which is a slog.  Might help to contrive a pynfs
test derived from that text which tests for this particular behavior.)

> 
> --b.
> 
> > 
> > After it, setclientid will return a bad reply with all zero confirms
> > after copy_clid().
> > 
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> >  fs/nfsd/nfs4state.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e0a4556..b1f84fc 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	unconf = find_unconfirmed_client_by_name(&clname, nn);
> >  	if (unconf)
> >  		unhash_client_locked(unconf);
> > -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
> > +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> >  		/* case 1: probable callback update */
> >  		copy_clid(new, conf);
> > -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > +		gen_confirm(new, nn);
> > +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >  		gen_clid(new, nn);
> >  	new->cl_minorversion = 0;
> >  	gen_callback(new, setclid, rqstp);
> > -- 
> > 2.4.3

Re: [PATCH 03/14] nfsd: Fix memory leak of so_owner.data in nfs4_stateowner

[J. Bruce Fields]

Good catch, but could we make a common nfs4_free_stateowner() helper
called both from here and nfs4_put_stateowner() so we only have to do
the kfree() in that one place?

--b.

On Mon, Jul 13, 2015 at 05:30:21PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b1f84fc..e5e14fa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3316,8 +3316,10 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
>  	if (ret == NULL) {
>  		hash_openowner(oo, clp, strhashval);
>  		ret = oo;
> -	} else
> +	} else {
> +		kfree(oo->oo_owner.so_owner.data);
>  		nfs4_free_openowner(&oo->oo_owner);
> +	}
>  	spin_unlock(&clp->cl_lock);
>  	return ret;
>  }
> @@ -5217,8 +5219,10 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
>  		list_add(&lo->lo_owner.so_strhash,
>  			 &clp->cl_ownerstr_hashtbl[strhashval]);
>  		ret = lo;
> -	} else
> -		nfs4_free_lockowner(&lo->lo_owner);
> +	} else {
> +		kfree(lo->lo_owner.so_owner.data);
> +		nfs4_free_openowner(&lo->lo_owner);
> +	}
>  	spin_unlock(&clp->cl_lock);
>  	return ret;
>  }
> -- 
> 2.4.3

Re: [PATCH 04/14] nfsd: Fix a memory leak of struct file_lock

[J. Bruce Fields]

Thanks, applying.--b.

On Mon, Jul 13, 2015 at 05:30:51PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e5e14fa..998166d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3938,6 +3938,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  	if (!filp) {
>  		/* We should always have a readable file here */
>  		WARN_ON_ONCE(1);
> +		locks_free_lock(fl);
>  		return -EBADF;
>  	}
>  	fl->fl_file = filp;
> -- 
> 2.4.3

Re: [PATCH 01/14] nfsd: Add layouts checking for state resources

[Kinglong Mee]

On 7/15/2015 23:03, J. Bruce Fields wrote:
> On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
>> Layout is a state resource, nfsd should check it too.
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/nfsd/nfs4state.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 61dfb33..e0a4556 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
>>  	 * Also note we should probably be using this in 4.0 case too.
>>  	 */
>>  	return !list_empty(&clp->cl_openowners)
>> +		|| !list_empty(&clp->cl_lo_states)
>>  		|| !list_empty(&clp->cl_delegations)
>>  		|| !list_empty(&clp->cl_sessions);
>>  }
>> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  		goto out;
>>  	clp = cstate->clp;
>>  	status = nfserr_cb_path_down;
>> -	if (!list_empty(&clp->cl_delegations)
>> -			&& clp->cl_cb_state != NFSD4_CB_UP)
>> +	if ((!list_empty(&clp->cl_delegations)
>> +	     || !list_empty(&clp->cl_lo_states))
> 
> nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.
> 
> That said, it's harmless--why don't we just call client_has_state() here
> instead?

I don't think so.

NFSv4.0's callback path is used for delegation recalling only,
without delegation, callback path is useless.

Also, describing in rfc3530,

   When the client holds delegations, it needs to use RENEW to detect
   when the server has determined that the callback path is down.  When
   the server has made such a determination, only the RENEW operation
   will renew the lease on delegations.  If the server determines the
   callback path is down, it returns NFS4ERR_CB_PATH_DOWN.  Even though
   it returns NFS4ERR_CB_PATH_DOWN, the server MUST renew the lease on
   the record locks and share reservations that the client has
   established on the server.

I will drop the second modify in version 2.

thanks,
Kinglong Mee

[PATCH v2] nfsd: Add layouts checking in client_has_state()

[Kinglong Mee]

Layout is a state resource, nfsd should check it too.

v2,
drop unneeded updating in nfsd4_renew()

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..00a0f8f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
 	 * Also note we should probably be using this in 4.0 case too.
 	 */
 	return !list_empty(&clp->cl_openowners)
+		|| !list_empty(&clp->cl_lo_states)
 		|| !list_empty(&clp->cl_delegations)
 		|| !list_empty(&clp->cl_sessions);
 }
-- 
2.4.3

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[Kinglong Mee]

On 7/16/2015 04:49, J. Bruce Fields wrote:
> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
>>> have moved gen_confirm() to gen_clid().
>>
>> This means the statement in that earlier commit is wrong:
>>
>> 	
>> 	With this, there's no need to keep two counters as they'd always
>> 	be in sync anyway, so just use the clientid_counter for both.
>>
>> Looks to me like this may need a separate counter to eliminate the
>> possibibility of returning the same confirm twice for a one clientid?

Yes, nfsd will generate same confirm for one clientid in one second.

 verf[0] = (__force __be32)jiffies;
 verf[1] = (__force __be32)nn->clientid_counter;

for case 1: probable callback update, the new unconf client needs
a different confirm.

Rereading rfc7530,
   x  be the value of the client.id subfield of the SETCLIENTID4args
      structure.

   v  be the value of the client.verifier subfield of the
      SETCLIENTID4args structure.

   c  be the value of the client ID field returned in the
      SETCLIENTID4resok structure.

   k  represent the value combination of the callback and callback_ident
      fields of the SETCLIENTID4args structure.

   s  be the setclientid_confirm value returned in the SETCLIENTID4resok
      structure.

   { v, x, c, k, s }  be a quintuple for a client record.  A client
      record is confirmed if there has been a SETCLIENTID_CONFIRM
      operation to confirm it.  Otherwise, it is unconfirmed.  An
      unconfirmed record is established by a SETCLIENTID call.

... /* case 1: probable callback update */ ... 

   o  The server checks if it has recorded a confirmed record for { v,
      x, c, l, s }, where l may or may not equal k.  If so, and since
      the id verifier v of the request matches that which is confirmed
      and recorded, the server treats this as a probable callback
      information update and records an unconfirmed { v, x, c, k, t }
      and leaves the confirmed { v, x, c, l, s } in place, such that
      t != s.  It does not matter whether k equals l or not.  Any
      pre-existing unconfirmed { v, x, c, *, * } is removed.

      The server returns { c, t }.  It is indeed returning the old
      clientid4 value c, because the client apparently only wants to
      update callback value k to value l.  It's possible this request is
      one from the Byzantine router that has stale callback information,
      but this is not a problem.  The callback information update is
      only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.

      The server awaits confirmation of k via SETCLIENTID_CONFIRM
      { c, t }.

      The server does NOT remove client (lock/share/delegation) state
      for x.

> 
> (but frankly I can never completely review changes to the
> setclientid/setclientid_confirm behavior without rereading RFC 7530
> 16.33.5 every time, which is a slog.  Might help to contrive a pynfs
> test derived from that text which tests for this particular behavior.)
> 

Make sense.
I will make it later.

thanks,
Kinglong Mee


>>
>> --b.
>>
>>>
>>> After it, setclientid will return a bad reply with all zero confirms
>>> after copy_clid().
>>>
>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>> ---
>>>  fs/nfsd/nfs4state.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index e0a4556..b1f84fc 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>  	unconf = find_unconfirmed_client_by_name(&clname, nn);
>>>  	if (unconf)
>>>  		unhash_client_locked(unconf);
>>> -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
>>> +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
>>>  		/* case 1: probable callback update */
>>>  		copy_clid(new, conf);
>>> -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>> +		gen_confirm(new, nn);
>>> +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>>  		gen_clid(new, nn);
>>>  	new->cl_minorversion = 0;
>>>  	gen_callback(new, setclid, rqstp);
>>> -- 
>>> 2.4.3
> 

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[Kinglong Mee]

On 7/16/2015 11:36, Kinglong Mee wrote:
> On 7/16/2015 04:49, J. Bruce Fields wrote:
>> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
>>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
>>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
>>>> have moved gen_confirm() to gen_clid().
>>>
>>> This means the statement in that earlier commit is wrong:
>>>
>>> 	
>>> 	With this, there's no need to keep two counters as they'd always
>>> 	be in sync anyway, so just use the clientid_counter for both.
>>>
>>> Looks to me like this may need a separate counter to eliminate the
>>> possibibility of returning the same confirm twice for a one clientid?
> 
> Yes, nfsd will generate same confirm for one clientid in one second.
> 
>  verf[0] = (__force __be32)jiffies;
>  verf[1] = (__force __be32)nn->clientid_counter;
> 
> for case 1: probable callback update, the new unconf client needs
> a different confirm.

Ignore this patch, and just revert commit 294ac32e99 
"nfsd: protect clid and verifier generation with client_lock"
is a better solve.

thanks,
Kinglong Mee

> 
> Rereading rfc7530,
>    x  be the value of the client.id subfield of the SETCLIENTID4args
>       structure.
> 
>    v  be the value of the client.verifier subfield of the
>       SETCLIENTID4args structure.
> 
>    c  be the value of the client ID field returned in the
>       SETCLIENTID4resok structure.
> 
>    k  represent the value combination of the callback and callback_ident
>       fields of the SETCLIENTID4args structure.
> 
>    s  be the setclientid_confirm value returned in the SETCLIENTID4resok
>       structure.
> 
>    { v, x, c, k, s }  be a quintuple for a client record.  A client
>       record is confirmed if there has been a SETCLIENTID_CONFIRM
>       operation to confirm it.  Otherwise, it is unconfirmed.  An
>       unconfirmed record is established by a SETCLIENTID call.
> 
> ... /* case 1: probable callback update */ ... 
> 
>    o  The server checks if it has recorded a confirmed record for { v,
>       x, c, l, s }, where l may or may not equal k.  If so, and since
>       the id verifier v of the request matches that which is confirmed
>       and recorded, the server treats this as a probable callback
>       information update and records an unconfirmed { v, x, c, k, t }
>       and leaves the confirmed { v, x, c, l, s } in place, such that
>       t != s.  It does not matter whether k equals l or not.  Any
>       pre-existing unconfirmed { v, x, c, *, * } is removed.
> 
>       The server returns { c, t }.  It is indeed returning the old
>       clientid4 value c, because the client apparently only wants to
>       update callback value k to value l.  It's possible this request is
>       one from the Byzantine router that has stale callback information,
>       but this is not a problem.  The callback information update is
>       only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
> 
>       The server awaits confirmation of k via SETCLIENTID_CONFIRM
>       { c, t }.
> 
>       The server does NOT remove client (lock/share/delegation) state
>       for x.
> 
>>
>> (but frankly I can never completely review changes to the
>> setclientid/setclientid_confirm behavior without rereading RFC 7530
>> 16.33.5 every time, which is a slog.  Might help to contrive a pynfs
>> test derived from that text which tests for this particular behavior.)
>>
> 
> Make sense.
> I will make it later.
> 
> thanks,
> Kinglong Mee
> 
> 
>>>
>>> --b.
>>>
>>>>
>>>> After it, setclientid will return a bad reply with all zero confirms
>>>> after copy_clid().
>>>>
>>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>>>> ---
>>>>  fs/nfsd/nfs4state.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index e0a4556..b1f84fc 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>  	unconf = find_unconfirmed_client_by_name(&clname, nn);
>>>>  	if (unconf)
>>>>  		unhash_client_locked(unconf);
>>>> -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
>>>> +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
>>>>  		/* case 1: probable callback update */
>>>>  		copy_clid(new, conf);
>>>> -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>>> +		gen_confirm(new, nn);
>>>> +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
>>>>  		gen_clid(new, nn);
>>>>  	new->cl_minorversion = 0;
>>>>  	gen_callback(new, setclid, rqstp);
>>>> -- 
>>>> 2.4.3
>>
> 

[PATCH v2] nfsd: Fix memory leak of so_owner.data in nfs4_stateowner

[Kinglong Mee]

v2, new helper nfs4_free_stateowner for freeing so_owner.data and sop

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4state.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 00a0f8f..3c24c72 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -990,6 +990,12 @@ release_all_access(struct nfs4_ol_stateid *stp)
 	}
 }
 
+static inline void nfs4_free_stateowner(struct nfs4_stateowner *sop)
+{
+	kfree(sop->so_owner.data);
+	sop->so_ops->so_free(sop);
+}
+
 static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
 {
 	struct nfs4_client *clp = sop->so_client;
@@ -1000,8 +1006,7 @@ static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
 		return;
 	sop->so_ops->so_unhash(sop);
 	spin_unlock(&clp->cl_lock);
-	kfree(sop->so_owner.data);
-	sop->so_ops->so_free(sop);
+	nfs4_free_stateowner(sop);
 }
 
 static void unhash_ol_stateid(struct nfs4_ol_stateid *stp)
@@ -3316,7 +3321,8 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
 		hash_openowner(oo, clp, strhashval);
 		ret = oo;
 	} else
-		nfs4_free_openowner(&oo->oo_owner);
+		nfs4_free_stateowner(&oo->oo_owner);
+
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }
@@ -5216,7 +5222,8 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
 			 &clp->cl_ownerstr_hashtbl[strhashval]);
 		ret = lo;
 	} else
-		nfs4_free_lockowner(&lo->lo_owner);
+		nfs4_free_stateowner(&lo->lo_owner);
+
 	spin_unlock(&clp->cl_lock);
 	return ret;
 }
-- 
2.4.3

Re: [PATCH 01/14] nfsd: Add layouts checking for state resources

[J. Bruce Fields]

On Thu, Jul 16, 2015 at 10:30:37AM +0800, Kinglong Mee wrote:
> On 7/15/2015 23:03, J. Bruce Fields wrote:
> > On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
> >> Layout is a state resource, nfsd should check it too.
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/nfsd/nfs4state.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 61dfb33..e0a4556 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
> >>  	 * Also note we should probably be using this in 4.0 case too.
> >>  	 */
> >>  	return !list_empty(&clp->cl_openowners)
> >> +		|| !list_empty(&clp->cl_lo_states)
> >>  		|| !list_empty(&clp->cl_delegations)
> >>  		|| !list_empty(&clp->cl_sessions);
> >>  }
> >> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>  		goto out;
> >>  	clp = cstate->clp;
> >>  	status = nfserr_cb_path_down;
> >> -	if (!list_empty(&clp->cl_delegations)
> >> -			&& clp->cl_cb_state != NFSD4_CB_UP)
> >> +	if ((!list_empty(&clp->cl_delegations)
> >> +	     || !list_empty(&clp->cl_lo_states))
> > 
> > nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.
> > 
> > That said, it's harmless--why don't we just call client_has_state() here
> > instead?
> 
> I don't think so.
> 
> NFSv4.0's callback path is used for delegation recalling only,
> without delegation, callback path is useless.
> 
> Also, describing in rfc3530,
> 
>    When the client holds delegations, it needs to use RENEW to detect
>    when the server has determined that the callback path is down.  When
>    the server has made such a determination, only the RENEW operation
>    will renew the lease on delegations.  If the server determines the
>    callback path is down, it returns NFS4ERR_CB_PATH_DOWN.  Even though
>    it returns NFS4ERR_CB_PATH_DOWN, the server MUST renew the lease on
>    the record locks and share reservations that the client has
>    established on the server.
> 
> I will drop the second modify in version 2.

Oh, you're right, the cl_openowners check would be wrong for the renew
case.

Applying v2, thanks.--b.

> 
> thanks,
> Kinglong Mee

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[J. Bruce Fields]

On Thu, Jul 16, 2015 at 11:50:55AM +0800, Kinglong Mee wrote:
> On 7/16/2015 11:36, Kinglong Mee wrote:
> > On 7/16/2015 04:49, J. Bruce Fields wrote:
> >> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> >>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> >>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> >>>> have moved gen_confirm() to gen_clid().
> >>>
> >>> This means the statement in that earlier commit is wrong:
> >>>
> >>> 	
> >>> 	With this, there's no need to keep two counters as they'd always
> >>> 	be in sync anyway, so just use the clientid_counter for both.
> >>>
> >>> Looks to me like this may need a separate counter to eliminate the
> >>> possibibility of returning the same confirm twice for a one clientid?
> > 
> > Yes, nfsd will generate same confirm for one clientid in one second.
> > 
> >  verf[0] = (__force __be32)jiffies;
> >  verf[1] = (__force __be32)nn->clientid_counter;
> > 
> > for case 1: probable callback update, the new unconf client needs
> > a different confirm.
> 
> Ignore this patch, and just revert commit 294ac32e99 
> "nfsd: protect clid and verifier generation with client_lock"
> is a better solve.

We can't revert that completely, it does fix a real locking bug at
least, I think.

I'd agree to reinstating a separate counter for the verifier.  That
verifier probably also needs to be per-network namespace to make the
per-network-namespace locking correct.

--b.

> 
> thanks,
> Kinglong Mee
> 
> > 
> > Rereading rfc7530,
> >    x  be the value of the client.id subfield of the SETCLIENTID4args
> >       structure.
> > 
> >    v  be the value of the client.verifier subfield of the
> >       SETCLIENTID4args structure.
> > 
> >    c  be the value of the client ID field returned in the
> >       SETCLIENTID4resok structure.
> > 
> >    k  represent the value combination of the callback and callback_ident
> >       fields of the SETCLIENTID4args structure.
> > 
> >    s  be the setclientid_confirm value returned in the SETCLIENTID4resok
> >       structure.
> > 
> >    { v, x, c, k, s }  be a quintuple for a client record.  A client
> >       record is confirmed if there has been a SETCLIENTID_CONFIRM
> >       operation to confirm it.  Otherwise, it is unconfirmed.  An
> >       unconfirmed record is established by a SETCLIENTID call.
> > 
> > ... /* case 1: probable callback update */ ... 
> > 
> >    o  The server checks if it has recorded a confirmed record for { v,
> >       x, c, l, s }, where l may or may not equal k.  If so, and since
> >       the id verifier v of the request matches that which is confirmed
> >       and recorded, the server treats this as a probable callback
> >       information update and records an unconfirmed { v, x, c, k, t }
> >       and leaves the confirmed { v, x, c, l, s } in place, such that
> >       t != s.  It does not matter whether k equals l or not.  Any
> >       pre-existing unconfirmed { v, x, c, *, * } is removed.
> > 
> >       The server returns { c, t }.  It is indeed returning the old
> >       clientid4 value c, because the client apparently only wants to
> >       update callback value k to value l.  It's possible this request is
> >       one from the Byzantine router that has stale callback information,
> >       but this is not a problem.  The callback information update is
> >       only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
> > 
> >       The server awaits confirmation of k via SETCLIENTID_CONFIRM
> >       { c, t }.
> > 
> >       The server does NOT remove client (lock/share/delegation) state
> >       for x.
> > 
> >>
> >> (but frankly I can never completely review changes to the
> >> setclientid/setclientid_confirm behavior without rereading RFC 7530
> >> 16.33.5 every time, which is a slog.  Might help to contrive a pynfs
> >> test derived from that text which tests for this particular behavior.)
> >>
> > 
> > Make sense.
> > I will make it later.
> > 
> > thanks,
> > Kinglong Mee
> > 
> > 
> >>>
> >>> --b.
> >>>
> >>>>
> >>>> After it, setclientid will return a bad reply with all zero confirms
> >>>> after copy_clid().
> >>>>
> >>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >>>> ---
> >>>>  fs/nfsd/nfs4state.c | 5 +++--
> >>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >>>> index e0a4556..b1f84fc 100644
> >>>> --- a/fs/nfsd/nfs4state.c
> >>>> +++ b/fs/nfsd/nfs4state.c
> >>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>>>  	unconf = find_unconfirmed_client_by_name(&clname, nn);
> >>>>  	if (unconf)
> >>>>  		unhash_client_locked(unconf);
> >>>> -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
> >>>> +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> >>>>  		/* case 1: probable callback update */
> >>>>  		copy_clid(new, conf);
> >>>> -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >>>> +		gen_confirm(new, nn);
> >>>> +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >>>>  		gen_clid(new, nn);
> >>>>  	new->cl_minorversion = 0;
> >>>>  	gen_callback(new, setclid, rqstp);
> >>>> -- 
> >>>> 2.4.3
> >>
> > 

Re: [PATCH v2] nfsd: Fix memory leak of so_owner.data in nfs4_stateowner

[J. Bruce Fields]

On Thu, Jul 16, 2015 at 12:05:07PM +0800, Kinglong Mee wrote:
> v2, new helper nfs4_free_stateowner for freeing so_owner.data and sop

Got it, thanks.--b.

> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 00a0f8f..3c24c72 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -990,6 +990,12 @@ release_all_access(struct nfs4_ol_stateid *stp)
>  	}
>  }
>  
> +static inline void nfs4_free_stateowner(struct nfs4_stateowner *sop)
> +{
> +	kfree(sop->so_owner.data);
> +	sop->so_ops->so_free(sop);
> +}
> +
>  static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
>  {
>  	struct nfs4_client *clp = sop->so_client;
> @@ -1000,8 +1006,7 @@ static void nfs4_put_stateowner(struct nfs4_stateowner *sop)
>  		return;
>  	sop->so_ops->so_unhash(sop);
>  	spin_unlock(&clp->cl_lock);
> -	kfree(sop->so_owner.data);
> -	sop->so_ops->so_free(sop);
> +	nfs4_free_stateowner(sop);
>  }
>  
>  static void unhash_ol_stateid(struct nfs4_ol_stateid *stp)
> @@ -3316,7 +3321,8 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
>  		hash_openowner(oo, clp, strhashval);
>  		ret = oo;
>  	} else
> -		nfs4_free_openowner(&oo->oo_owner);
> +		nfs4_free_stateowner(&oo->oo_owner);
> +
>  	spin_unlock(&clp->cl_lock);
>  	return ret;
>  }
> @@ -5216,7 +5222,8 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
>  			 &clp->cl_ownerstr_hashtbl[strhashval]);
>  		ret = lo;
>  	} else
> -		nfs4_free_lockowner(&lo->lo_owner);
> +		nfs4_free_stateowner(&lo->lo_owner);
> +
>  	spin_unlock(&clp->cl_lock);
>  	return ret;
>  }
> -- 
> 2.4.3

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[Jeff Layton]

On Fri, 17 Jul 2015 11:58:46 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Jul 16, 2015 at 11:50:55AM +0800, Kinglong Mee wrote:
> > On 7/16/2015 11:36, Kinglong Mee wrote:
> > > On 7/16/2015 04:49, J. Bruce Fields wrote:
> > >> On Wed, Jul 15, 2015 at 04:47:48PM -0400, J. Bruce Fields wrote:
> > >>> On Mon, Jul 13, 2015 at 05:29:41PM +0800, Kinglong Mee wrote:
> > >>>> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> > >>>> have moved gen_confirm() to gen_clid().
> > >>>
> > >>> This means the statement in that earlier commit is wrong:
> > >>>
> > >>> 	
> > >>> 	With this, there's no need to keep two counters as they'd always
> > >>> 	be in sync anyway, so just use the clientid_counter for both.
> > >>>
> > >>> Looks to me like this may need a separate counter to eliminate the
> > >>> possibibility of returning the same confirm twice for a one clientid?
> > > 
> > > Yes, nfsd will generate same confirm for one clientid in one second.
> > > 
> > >  verf[0] = (__force __be32)jiffies;
> > >  verf[1] = (__force __be32)nn->clientid_counter;
> > > 
> > > for case 1: probable callback update, the new unconf client needs
> > > a different confirm.
> > 
> > Ignore this patch, and just revert commit 294ac32e99 
> > "nfsd: protect clid and verifier generation with client_lock"
> > is a better solve.
> 
> We can't revert that completely, it does fix a real locking bug at
> least, I think.
> 
> I'd agree to reinstating a separate counter for the verifier.  That
> verifier probably also needs to be per-network namespace to make the
> per-network-namespace locking correct.
> 
> --b.
> 

Sorry, just getting caught up on this. At the time I was just
"following the code" and not necessarily paying much attention to the
spec.

Yes, I agree -- a separate counter sounds like the right fix for now,
in conjunction with Kinglong's patch (or something like it).

> > 
> > thanks,
> > Kinglong Mee
> > 
> > > 
> > > Rereading rfc7530,
> > >    x  be the value of the client.id subfield of the SETCLIENTID4args
> > >       structure.
> > > 
> > >    v  be the value of the client.verifier subfield of the
> > >       SETCLIENTID4args structure.
> > > 
> > >    c  be the value of the client ID field returned in the
> > >       SETCLIENTID4resok structure.
> > > 
> > >    k  represent the value combination of the callback and callback_ident
> > >       fields of the SETCLIENTID4args structure.
> > > 
> > >    s  be the setclientid_confirm value returned in the SETCLIENTID4resok
> > >       structure.
> > > 
> > >    { v, x, c, k, s }  be a quintuple for a client record.  A client
> > >       record is confirmed if there has been a SETCLIENTID_CONFIRM
> > >       operation to confirm it.  Otherwise, it is unconfirmed.  An
> > >       unconfirmed record is established by a SETCLIENTID call.
> > > 
> > > ... /* case 1: probable callback update */ ... 
> > > 
> > >    o  The server checks if it has recorded a confirmed record for { v,
> > >       x, c, l, s }, where l may or may not equal k.  If so, and since
> > >       the id verifier v of the request matches that which is confirmed
> > >       and recorded, the server treats this as a probable callback
> > >       information update and records an unconfirmed { v, x, c, k, t }
> > >       and leaves the confirmed { v, x, c, l, s } in place, such that
> > >       t != s.  It does not matter whether k equals l or not.  Any
> > >       pre-existing unconfirmed { v, x, c, *, * } is removed.
> > > 
> > >       The server returns { c, t }.  It is indeed returning the old
> > >       clientid4 value c, because the client apparently only wants to
> > >       update callback value k to value l.  It's possible this request is
> > >       one from the Byzantine router that has stale callback information,
> > >       but this is not a problem.  The callback information update is
> > >       only confirmed if followed up by a SETCLIENTID_CONFIRM { c, t }.
> > > 
> > >       The server awaits confirmation of k via SETCLIENTID_CONFIRM
> > >       { c, t }.
> > > 
> > >       The server does NOT remove client (lock/share/delegation) state
> > >       for x.
> > > 
> > >>
> > >> (but frankly I can never completely review changes to the
> > >> setclientid/setclientid_confirm behavior without rereading RFC 7530
> > >> 16.33.5 every time, which is a slog.  Might help to contrive a pynfs
> > >> test derived from that text which tests for this particular behavior.)
> > >>
> > > 
> > > Make sense.
> > > I will make it later.
> > > 
> > > thanks,
> > > Kinglong Mee
> > > 
> > > 
> > >>>
> > >>> --b.
> > >>>
> > >>>>
> > >>>> After it, setclientid will return a bad reply with all zero confirms
> > >>>> after copy_clid().
> > >>>>
> > >>>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > >>>> ---
> > >>>>  fs/nfsd/nfs4state.c | 5 +++--
> > >>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > >>>> index e0a4556..b1f84fc 100644
> > >>>> --- a/fs/nfsd/nfs4state.c
> > >>>> +++ b/fs/nfsd/nfs4state.c
> > >>>> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >>>>  	unconf = find_unconfirmed_client_by_name(&clname, nn);
> > >>>>  	if (unconf)
> > >>>>  		unhash_client_locked(unconf);
> > >>>> -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
> > >>>> +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> > >>>>  		/* case 1: probable callback update */
> > >>>>  		copy_clid(new, conf);
> > >>>> -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > >>>> +		gen_confirm(new, nn);
> > >>>> +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > >>>>  		gen_clid(new, nn);
> > >>>>  	new->cl_minorversion = 0;
> > >>>>  	gen_callback(new, setclid, rqstp);
> > >>>> -- 
> > >>>> 2.4.3
> > >>
> > > 
> --
> 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


-- 
Jeff Layton <jlayton@poochiereds.net>

[PATCH] nfsd: New counter for generating client confirm verifier

[Kinglong Mee]

If using clientid_counter, gen_confirm will generate same verifier
in one second.

A new counter for client confirm verifier make sure gen_confirm
generating different each calling for the same clientid.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/netns.h     | 1 +
 fs/nfsd/nfs4state.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ea6749a..d8b16c2 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -110,6 +110,7 @@ struct nfsd_net {
 	unsigned int max_connections;
 
 	u32 clientid_counter;
+	u32 clverifier_counter;
 
 	struct svc_serv *nfsd_serv;
 };
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..5a64757e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
 	 * __force to keep sparse happy
 	 */
 	verf[0] = (__force __be32)get_seconds();
-	verf[1] = (__force __be32)nn->clientid_counter;
+	verf[1] = (__force __be32)nn->clverifier_counter++;
 	memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
 }
 
-- 
2.4.3

Re: [PATCH] nfsd: New counter for generating client confirm verifier

[Jeff Layton]

On Sat, 18 Jul 2015 07:33:31 +0800
Kinglong Mee <kinglongmee@gmail.com> wrote:

> If using clientid_counter, gen_confirm will generate same verifier
> in one second.
> 
> A new counter for client confirm verifier make sure gen_confirm
> generating different each calling for the same clientid.
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/netns.h     | 1 +
>  fs/nfsd/nfs4state.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index ea6749a..d8b16c2 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -110,6 +110,7 @@ struct nfsd_net {
>  	unsigned int max_connections;
>  
>  	u32 clientid_counter;
> +	u32 clverifier_counter;
>  
>  	struct svc_serv *nfsd_serv;
>  };
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 61dfb33..5a64757e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
>  	 * __force to keep sparse happy
>  	 */
>  	verf[0] = (__force __be32)get_seconds();
> -	verf[1] = (__force __be32)nn->clientid_counter;
> +	verf[1] = (__force __be32)nn->clverifier_counter++;
>  	memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
>  }
>  

Reviewed-by: Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: New counter for generating client confirm verifier

[J. Bruce Fields]

On Sat, Jul 18, 2015 at 08:16:08AM -0400, Jeff Layton wrote:
> On Sat, 18 Jul 2015 07:33:31 +0800
> Kinglong Mee <kinglongmee@gmail.com> wrote:
> 
> > If using clientid_counter, gen_confirm will generate same verifier
> > in one second.
> > 
> > A new counter for client confirm verifier make sure gen_confirm
> > generating different each calling for the same clientid.
> > 
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> >  fs/nfsd/netns.h     | 1 +
> >  fs/nfsd/nfs4state.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index ea6749a..d8b16c2 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -110,6 +110,7 @@ struct nfsd_net {
> >  	unsigned int max_connections;
> >  
> >  	u32 clientid_counter;
> > +	u32 clverifier_counter;
> >  
> >  	struct svc_serv *nfsd_serv;
> >  };
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 61dfb33..5a64757e 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1894,7 +1894,7 @@ static void gen_confirm(struct nfs4_client *clp, struct nfsd_net *nn)
> >  	 * __force to keep sparse happy
> >  	 */
> >  	verf[0] = (__force __be32)get_seconds();
> > -	verf[1] = (__force __be32)nn->clientid_counter;
> > +	verf[1] = (__force __be32)nn->clverifier_counter++;
> >  	memcpy(clp->cl_confirm.data, verf, sizeof(clp->cl_confirm.data));
> >  }
> >  
> 
> Reviewed-by: Jeff Layton <jlayton@poochiereds.net>

Thanks.--b.

Re: [PATCH 05/14] nfsd: Use check_stateid_generation() for generation checking

[J. Bruce Fields]

I don't think layout stateid handling is the same as handling of other
stateid's.  See

	https://tools.ietf.org/html/rfc5661#section-12.5.3

I haven't reviewed it in detail yet.

--b.

On Mon, Jul 13, 2015 at 05:31:21PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4layouts.c | 5 ++---
>  fs/nfsd/nfs4state.c   | 2 +-
>  fs/nfsd/state.h       | 1 +
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 6904213..5c0f1d6 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>  			goto out;
>  	} else {
>  		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
> -
> -		status = nfserr_bad_stateid;
> -		if (stateid->si_generation > stid->sc_stateid.si_generation)
> +		status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
> +		if (status)
>  			goto out_put_stid;
>  		if (layout_type != ls->ls_layout_type)
>  			goto out_put_stid;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 998166d..2edfedc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
>  	return (s32)(a->si_generation - b->si_generation) > 0;
>  }
>  
> -static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
>  {
>  	/*
>  	 * When sessions are used the stateid generation number is ignored
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 4874ce5..18d015d 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		     stateid_t *stateid, unsigned char typemask,
>  		     struct nfs4_stid **s, struct nfsd_net *nn);
> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
>  struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
>  		struct kmem_cache *slab);
>  void nfs4_unhash_stid(struct nfs4_stid *s);
> -- 
> 2.4.3

Re: [PATCH 06/14] nfsd: Drop duplicate locks_init_lock()

[J. Bruce Fields]

OK.--b.

On Mon, Jul 13, 2015 at 05:31:42PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4layouts.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 5c0f1d6..c5af4ccb 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -162,7 +162,7 @@ nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
>  	fl = locks_alloc_lock();
>  	if (!fl)
>  		return -ENOMEM;
> -	locks_init_lock(fl);
> +
>  	fl->fl_lmops = &nfsd4_layouts_lm_ops;
>  	fl->fl_flags = FL_LAYOUT;
>  	fl->fl_type = F_RDLCK;
> -- 
> 2.4.3

Re: [PATCH 14/14] nfsd: Remove unused clientid arguments from, find_lockowner_str{_locked}

[J. Bruce Fields]

OK, applied the rest of these.--b.

On Mon, Jul 13, 2015 at 05:35:37PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2704270..b6c134d 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5131,8 +5131,7 @@ nevermind:
>  }
>  
>  static struct nfs4_lockowner *
> -find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
> -		struct nfs4_client *clp)
> +find_lockowner_str_locked(struct nfs4_client *clp, struct xdr_netobj *owner)
>  {
>  	unsigned int strhashval = ownerstr_hashval(owner);
>  	struct nfs4_stateowner *so;
> @@ -5150,13 +5149,12 @@ find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
>  }
>  
>  static struct nfs4_lockowner *
> -find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
> -		struct nfs4_client *clp)
> +find_lockowner_str(struct nfs4_client *clp, struct xdr_netobj *owner)
>  {
>  	struct nfs4_lockowner *lo;
>  
>  	spin_lock(&clp->cl_lock);
> -	lo = find_lockowner_str_locked(clid, owner, clp);
> +	lo = find_lockowner_str_locked(clp, owner);
>  	spin_unlock(&clp->cl_lock);
>  	return lo;
>  }
> @@ -5200,8 +5198,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
>  	lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
>  	lo->lo_owner.so_ops = &lockowner_ops;
>  	spin_lock(&clp->cl_lock);
> -	ret = find_lockowner_str_locked(&clp->cl_clientid,
> -			&lock->lk_new_owner, clp);
> +	ret = find_lockowner_str_locked(clp, &lock->lk_new_owner);
>  	if (ret == NULL) {
>  		list_add(&lo->lo_owner.so_strhash,
>  			 &clp->cl_ownerstr_hashtbl[strhashval]);
> @@ -5322,7 +5319,7 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
>  	struct nfs4_lockowner *lo;
>  	unsigned int strhashval;
>  
> -	lo = find_lockowner_str(&cl->cl_clientid, &lock->lk_new_owner, cl);
> +	lo = find_lockowner_str(cl, &lock->lk_new_owner);
>  	if (!lo) {
>  		strhashval = ownerstr_hashval(&lock->lk_new_owner);
>  		lo = alloc_init_lock_stateowner(strhashval, cl, ost, lock);
> @@ -5597,8 +5594,7 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	}
>  
> -	lo = find_lockowner_str(&lockt->lt_clientid, &lockt->lt_owner,
> -				cstate->clp);
> +	lo = find_lockowner_str(cstate->clp, &lockt->lt_owner);
>  	if (lo)
>  		file_lock->fl_owner = (fl_owner_t)lo;
>  	file_lock->fl_pid = current->tgid;
> -- 
> 2.4.3

Re: [PATCH 05/14] nfsd: Use check_stateid_generation() for generation checking

[Kinglong Mee]

On 7/23/2015 02:22, J. Bruce Fields wrote:
> I don't think layout stateid handling is the same as handling of other
> stateid's.  See
> 
> 	https://tools.ietf.org/html/rfc5661#section-12.5.3

Yes,

  "Given the design goal of pNFS to provide parallelism, the layout
   stateid differs from other stateid types in that the client is
   expected to send LAYOUTGET and LAYOUTRETURN operations in parallel."

Please ignore this patch.

thanks,
Kinglong Mee

> 
> I haven't reviewed it in detail yet.
> 
> --b.
> 
> On Mon, Jul 13, 2015 at 05:31:21PM +0800, Kinglong Mee wrote:
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/nfsd/nfs4layouts.c | 5 ++---
>>  fs/nfsd/nfs4state.c   | 2 +-
>>  fs/nfsd/state.h       | 1 +
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
>> index 6904213..5c0f1d6 100644
>> --- a/fs/nfsd/nfs4layouts.c
>> +++ b/fs/nfsd/nfs4layouts.c
>> @@ -263,9 +263,8 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>>  			goto out;
>>  	} else {
>>  		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
>> -
>> -		status = nfserr_bad_stateid;
>> -		if (stateid->si_generation > stid->sc_stateid.si_generation)
>> +		status = check_stateid_generation(stateid, &stid->sc_stateid, 1);
>> +		if (status)
>>  			goto out_put_stid;
>>  		if (layout_type != ls->ls_layout_type)
>>  			goto out_put_stid;
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 998166d..2edfedc 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4474,7 +4474,7 @@ static bool stateid_generation_after(stateid_t *a, stateid_t *b)
>>  	return (s32)(a->si_generation - b->si_generation) > 0;
>>  }
>>  
>> -static __be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
>> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session)
>>  {
>>  	/*
>>  	 * When sessions are used the stateid generation number is ignored
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 4874ce5..18d015d 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -589,6 +589,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>>  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>>  		     stateid_t *stateid, unsigned char typemask,
>>  		     struct nfs4_stid **s, struct nfsd_net *nn);
>> +__be32 check_stateid_generation(stateid_t *in, stateid_t *ref, bool has_session);
>>  struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
>>  		struct kmem_cache *slab);
>>  void nfs4_unhash_stid(struct nfs4_stid *s);
>> -- 
>> 2.4.3
> 

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[Kinglong Mee]

This one needs be applied with
"nfsd: New counter for generating client confirm verifier"
that is only update client confirm verifier without fix this problem.

thanks,
Kinglong Mee

On 7/13/2015 17:29, Kinglong Mee wrote:
> Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> have moved gen_confirm() to gen_clid().
> 
> After it, setclientid will return a bad reply with all zero confirms
> after copy_clid().
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4state.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e0a4556..b1f84fc 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	unconf = find_unconfirmed_client_by_name(&clname, nn);
>  	if (unconf)
>  		unhash_client_locked(unconf);
> -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
> +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
>  		/* case 1: probable callback update */
>  		copy_clid(new, conf);
> -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
> +		gen_confirm(new, nn);
> +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
>  		gen_clid(new, nn);
>  	new->cl_minorversion = 0;
>  	gen_callback(new, setclid, rqstp);
> 

Re: [PATCH 02/14] nfsd: Add missing gen_confirm in nfsd4_setclientid()

[J. Bruce Fields]

On Thu, Jul 23, 2015 at 09:16:27AM +0800, Kinglong Mee wrote:
> This one needs be applied with
> "nfsd: New counter for generating client confirm verifier"
> that is only update client confirm verifier without fix this problem.

Oh, right, I forgot the original patch after the discussion; applying
"nfsd: Add missing gen_confirm...".

--b.

> 
> thanks,
> Kinglong Mee
> 
> On 7/13/2015 17:29, Kinglong Mee wrote:
> > Commit 294ac32e99 "nfsd: protect clid and verifier generation with client_lock"
> > have moved gen_confirm() to gen_clid().
> > 
> > After it, setclientid will return a bad reply with all zero confirms
> > after copy_clid().
> > 
> > Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> > ---
> >  fs/nfsd/nfs4state.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index e0a4556..b1f84fc 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3042,10 +3042,11 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	unconf = find_unconfirmed_client_by_name(&clname, nn);
> >  	if (unconf)
> >  		unhash_client_locked(unconf);
> > -	if (conf && same_verf(&conf->cl_verifier, &clverifier))
> > +	if (conf && same_verf(&conf->cl_verifier, &clverifier)) {
> >  		/* case 1: probable callback update */
> >  		copy_clid(new, conf);
> > -	else /* case 4 (new client) or cases 2, 3 (client reboot): */
> > +		gen_confirm(new, nn);
> > +	} else /* case 4 (new client) or cases 2, 3 (client reboot): */
> >  		gen_clid(new, nn);
> >  	new->cl_minorversion = 0;
> >  	gen_callback(new, setclid, rqstp);
> > 
> --
> 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