[PATCH 0/5] knfsd miscellaneous cleanup

[PATCH 0/5] knfsd miscellaneous cleanup

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Some cleanup and one minor bugfix.

--b.

J. Bruce Fields (5):
  nfsd4: less confusing nfsd4_compound_in_session
  nfsd4: cleanup sessionid in nfsd4_destroy_session
  nfsd: update obselete comment referencing the BKL
  nfsd: clarify check_op_ordering
  nfsd: fix corrupted reply to badly ordered compound

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

-- 
2.17.1

[PATCH 1/5] nfsd4: less confusing nfsd4_compound_in_session

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Make the function prototype match the name a little better.

Signed-off-by: J. Bruce Fields <bfields@redhat.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 683a5f705d81..ac891b5b1d6a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2955,11 +2955,11 @@ __be32 nfsd4_bind_conn_to_session(struct svc_rqst *rqstp,
 	return status;
 }
 
-static bool nfsd4_compound_in_session(struct nfsd4_session *session, struct nfs4_sessionid *sid)
+static bool nfsd4_compound_in_session(struct nfsd4_compound_state *cstate, struct nfs4_sessionid *sid)
 {
-	if (!session)
+	if (!cstate->session)
 		return false;
-	return !memcmp(sid, &session->se_sessionid, sizeof(*sid));
+	return !memcmp(sid, &cstate->session->se_sessionid, sizeof(*sid));
 }
 
 __be32
@@ -2974,7 +2974,7 @@ nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate,
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	status = nfserr_not_only_op;
-	if (nfsd4_compound_in_session(cstate->session, &sessionid->sessionid)) {
+	if (nfsd4_compound_in_session(cstate, &sessionid->sessionid)) {
 		if (!nfsd4_last_compound_op(r))
 			goto out;
 		ref_held_by_me++;
-- 
2.17.1

[PATCH 2/5] nfsd4: cleanup sessionid in nfsd4_destroy_session

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

The name of this variable doesn't fit the type.  And we only ever use
one field of it.

Signed-off-by: J. Bruce Fields <bfields@redhat.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 ac891b5b1d6a..b762054377e0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2966,7 +2966,7 @@ __be32
 nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate,
 		union nfsd4_op_u *u)
 {
-	struct nfsd4_destroy_session *sessionid = &u->destroy_session;
+	struct nfs4_sessionid *sessionid = &u->destroy_session.sessionid;
 	struct nfsd4_session *ses;
 	__be32 status;
 	int ref_held_by_me = 0;
@@ -2974,14 +2974,14 @@ nfsd4_destroy_session(struct svc_rqst *r, struct nfsd4_compound_state *cstate,
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	status = nfserr_not_only_op;
-	if (nfsd4_compound_in_session(cstate, &sessionid->sessionid)) {
+	if (nfsd4_compound_in_session(cstate, sessionid)) {
 		if (!nfsd4_last_compound_op(r))
 			goto out;
 		ref_held_by_me++;
 	}
-	dump_sessionid(__func__, &sessionid->sessionid);
+	dump_sessionid(__func__, sessionid);
 	spin_lock(&nn->client_lock);
-	ses = find_in_sessionid_hashtbl(&sessionid->sessionid, net, &status);
+	ses = find_in_sessionid_hashtbl(sessionid, net, &status);
 	if (!ses)
 		goto out_client_lock;
 	status = nfserr_wrong_cred;
-- 
2.17.1

[PATCH 3/5] nfsd: update obselete comment referencing the BKL

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

It's inode->i_lock that's now taken in setlease and break_lease, instead
of the big kernel lock.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b762054377e0..294b9a433594 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3944,9 +3944,9 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
 	/*
 	 * We're assuming the state code never drops its reference
 	 * without first removing the lease.  Since we're in this lease
-	 * callback (and since the lease code is serialized by the kernel
-	 * lock) we know the server hasn't removed the lease yet, we know
-	 * it's safe to take a reference.
+	 * callback (and since the lease code is serialized by the
+	 * i_lock) we know the server hasn't removed the lease yet, and
+	 * we know it's safe to take a reference.
 	 */
 	refcount_inc(&dp->dl_stid.sc_count);
 	nfsd4_run_cb(&dp->dl_recall);
-- 
2.17.1

[PATCH 4/5] nfsd: clarify check_op_ordering

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Document a couple things that confused me on a recent reading.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 1929f85b8269..140b05c8a4be 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1603,7 +1603,7 @@ static const char *nfsd4_op_name(unsigned opnum);
  */
 static __be32 nfs41_check_op_ordering(struct nfsd4_compoundargs *args)
 {
-	struct nfsd4_op *op = &args->ops[0];
+	struct nfsd4_op *first_op = &args->ops[0];
 
 	/* These ordering requirements don't apply to NFSv4.0: */
 	if (args->minorversion == 0)
@@ -1611,12 +1611,17 @@ static __be32 nfs41_check_op_ordering(struct nfsd4_compoundargs *args)
 	/* This is weird, but OK, not our problem: */
 	if (args->opcnt == 0)
 		return nfs_ok;
-	if (op->status == nfserr_op_illegal)
+	if (first_op->status == nfserr_op_illegal)
 		return nfs_ok;
-	if (!(nfsd4_ops[op->opnum].op_flags & ALLOWED_AS_FIRST_OP))
+	if (!(nfsd4_ops[first_op->opnum].op_flags & ALLOWED_AS_FIRST_OP))
 		return nfserr_op_not_in_session;
-	if (op->opnum == OP_SEQUENCE)
+	if (first_op->opnum == OP_SEQUENCE)
 		return nfs_ok;
+	/*
+	 * So first_op is something allowed outside a session, like
+	 * EXCHANGE_ID; but then it has to be the only op in the
+	 * compound:
+	 */
 	if (args->opcnt != 1)
 		return nfserr_not_only_op;
 	return nfs_ok;
-- 
2.17.1

[PATCH 5/5] nfsd: fix corrupted reply to badly ordered compound

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

We're encoding a single op in the reply but leaving the number of ops
zero, so the reply makes no sense.

Somewhat academic as this isn't a case any real client will hit, though
in theory perhaps that could change in a future protocol extension.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 140b05c8a4be..3652f9b1fb68 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1735,6 +1735,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 	if (status) {
 		op = &args->ops[0];
 		op->status = status;
+		resp->opcnt = 1;
 		goto encode_op;
 	}
 
-- 
2.17.1

Re: [PATCH 0/5] knfsd miscellaneous cleanup

[Jeff Layton]

On Thu, 2018-06-14 at 15:32 -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Some cleanup and one minor bugfix.
> 
> --b.
> 
> J. Bruce Fields (5):
>   nfsd4: less confusing nfsd4_compound_in_session
>   nfsd4: cleanup sessionid in nfsd4_destroy_session
>   nfsd: update obselete comment referencing the BKL
>   nfsd: clarify check_op_ordering
>   nfsd: fix corrupted reply to badly ordered compound
> 
>  fs/nfsd/nfs4proc.c  | 14 ++++++++++----
>  fs/nfsd/nfs4state.c | 20 ++++++++++----------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 

These also seem reasonable.

Reviewed-by: Jeff Layton <jlayton@kernel.org>