NFSD's CREATE_SESSION implementation does not follow spec. Here
are a couple of patches to get it back into compliance.
---
Chuck Lever (2):
NFSD: Fix the NFSv4.1 CREATE_SESSION operation
NFSD: Document the phases of CREATE_SESSION
fs/nfsd/nfs4state.c | 63 ++++++++++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 26 deletions(-)
--
Chuck Lever
From: Chuck Lever <[email protected]>
As described in RFC 8881 Section 18.36.4, CREATE_SESSION can be
split into four phases. NFSD's implementation now does it like that
description.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4state.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index bca2c2878ad6..3b52728d8cb9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3614,6 +3614,8 @@ nfsd4_create_session(struct svc_rqst *rqstp,
goto out_free_session;
spin_lock(&nn->client_lock);
+
+ /* RFC 8881 Section 18.36.4 Phase 1: Client record look-up. */
unconf = find_unconfirmed_client(&cr_ses->clientid, true, nn);
conf = find_confirmed_client(&cr_ses->clientid, true, nn);
if (!conf && !unconf) {
@@ -3621,6 +3623,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
goto out_free_conn;
}
+ /* RFC 8881 Section 18.36.4 Phase 2: Sequence ID processing. */
if (conf)
cs_slot = &conf->cl_cs_slot;
else
@@ -3636,6 +3639,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
cs_slot->sl_seqid++;
cr_ses->seqid = cs_slot->sl_seqid;
+ /* RFC 8881 Section 18.36.4 Phase 3: Client ID confirmation. */
if (conf) {
status = nfserr_wrong_cred;
if (!nfsd4_mach_creds_match(conf, rqstp))
@@ -3662,6 +3666,8 @@ nfsd4_create_session(struct svc_rqst *rqstp,
move_to_confirmed(unconf);
conf = unconf;
}
+
+ /* RFC 8881 Section 18.36.4 Phase 4: Session creation. */
status = nfs_ok;
/* Persistent sessions are not supported */
cr_ses->flags &= ~SESSION4_PERSIST;
From: Chuck Lever <[email protected]>
RFC 8881 Section 18.36.4 discusses the implementation of the NFSv4.1
CREATE_SESSION operation. The section defines four phases of
operation.
Phase 2 processes the CREATE_SESSION sequence ID. As a separate
step, Phase 3 evaluates the CREATE_SESSION arguments.
The problem we are concerned with is when phase 2 is successful but
phase 3 fails. The spec language in this case is "No changes are
made to any client records on the server."
RFC 8881 Section 18.35.4 defines a "client record", and it does
/not/ contain any details related to the special CREATE_SESSION
slot. Therefore NFSD is incorrect to skip incrementing the
CREATE_SESSION sequence id when phase 3 (see Section 18.36.4) of
CREATE_SESSION processing fails. In other words, even though NFSD
happens to store the cs_slot in a client record, in terms of the
protocol the slot is logically separate from the client record.
Three complications:
1. The world has moved on since commit 86c3e16cc7aa ("nfsd4: confirm
only on succesful create_session") broke this. So we can't simply
revert that commit.
2. NFSD's CREATE_SESSION implementation does not cleanly delineate
the logic of phases 2 and 3. So this won't be a surgical fix.
3. Because of the way it currently handles the CREATE_SESSION slot
sequence number, nfsd4_create_session() isn't caching error
responses in the CREATE_SESSION slot. Instead of replaying the
response cache in those cases, it's executing the transaction
again.
Reorganize the CREATE_SESSION slot sequence number accounting. This
requires that error responses are appropriately cached in the
CREATE_SESSION slot (once it is found).
Reported-by: Connor Smith <[email protected]>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218382
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs4state.c | 57 ++++++++++++++++++++++++++++-----------------------
1 file changed, 31 insertions(+), 26 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6dc6340e2852..bca2c2878ad6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3414,6 +3414,9 @@ nfsd4_exchange_id(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
new->cl_spo_must_allow.u.words[0] = exid->spo_must_allow[0];
new->cl_spo_must_allow.u.words[1] = exid->spo_must_allow[1];
+ /* Contrived initial CREATE_SESSION response */
+ new->cl_cs_slot.sl_status = nfserr_seq_misordered;
+
add_to_unconfirmed(new);
swap(new, conf);
out_copy:
@@ -3584,10 +3587,10 @@ nfsd4_create_session(struct svc_rqst *rqstp,
struct nfsd4_create_session *cr_ses = &u->create_session;
struct sockaddr *sa = svc_addr(rqstp);
struct nfs4_client *conf, *unconf;
+ struct nfsd4_clid_slot *cs_slot;
struct nfs4_client *old = NULL;
struct nfsd4_session *new;
struct nfsd4_conn *conn;
- struct nfsd4_clid_slot *cs_slot = NULL;
__be32 status = 0;
struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
@@ -3613,50 +3616,51 @@ nfsd4_create_session(struct svc_rqst *rqstp,
spin_lock(&nn->client_lock);
unconf = find_unconfirmed_client(&cr_ses->clientid, true, nn);
conf = find_confirmed_client(&cr_ses->clientid, true, nn);
- WARN_ON_ONCE(conf && unconf);
+ if (!conf && !unconf) {
+ status = nfserr_stale_clientid;
+ goto out_free_conn;
+ }
- if (conf) {
- status = nfserr_wrong_cred;
- if (!nfsd4_mach_creds_match(conf, rqstp))
- goto out_free_conn;
+ if (conf)
cs_slot = &conf->cl_cs_slot;
- status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
- if (status) {
- if (status == nfserr_replay_cache)
- status = nfsd4_replay_create_session(cr_ses, cs_slot);
+ else
+ cs_slot = &unconf->cl_cs_slot;
+ status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
+ if (status) {
+ if (status == nfserr_replay_cache) {
+ status = nfsd4_replay_create_session(cr_ses, cs_slot);
goto out_free_conn;
}
- } else if (unconf) {
+ goto out_cache_error;
+ }
+ cs_slot->sl_seqid++;
+ cr_ses->seqid = cs_slot->sl_seqid;
+
+ if (conf) {
+ status = nfserr_wrong_cred;
+ if (!nfsd4_mach_creds_match(conf, rqstp))
+ goto out_cache_error;
+ } else {
status = nfserr_clid_inuse;
if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
!rpc_cmp_addr(sa, (struct sockaddr *) &unconf->cl_addr)) {
trace_nfsd_clid_cred_mismatch(unconf, rqstp);
- goto out_free_conn;
+ goto out_cache_error;
}
status = nfserr_wrong_cred;
if (!nfsd4_mach_creds_match(unconf, rqstp))
- goto out_free_conn;
- cs_slot = &unconf->cl_cs_slot;
- status = check_slot_seqid(cr_ses->seqid, cs_slot->sl_seqid, 0);
- if (status) {
- /* an unconfirmed replay returns misordered */
- status = nfserr_seq_misordered;
- goto out_free_conn;
- }
+ goto out_cache_error;
old = find_confirmed_client_by_name(&unconf->cl_name, nn);
if (old) {
status = mark_client_expired_locked(old);
if (status) {
old = NULL;
- goto out_free_conn;
+ goto out_cache_error;
}
trace_nfsd_clid_replaced(&old->cl_clientid);
}
move_to_confirmed(unconf);
conf = unconf;
- } else {
- status = nfserr_stale_clientid;
- goto out_free_conn;
}
status = nfs_ok;
/* Persistent sessions are not supported */
@@ -3669,8 +3673,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
memcpy(cr_ses->sessionid.data, new->se_sessionid.data,
NFS4_MAX_SESSIONID_LEN);
- cs_slot->sl_seqid++;
- cr_ses->seqid = cs_slot->sl_seqid;
/* cache solo and embedded create sessions under the client_lock */
nfsd4_cache_create_session(cr_ses, cs_slot, status);
@@ -3683,6 +3685,9 @@ nfsd4_create_session(struct svc_rqst *rqstp,
if (old)
expire_client(old);
return status;
+
+out_cache_error:
+ nfsd4_cache_create_session(cr_ses, cs_slot, status);
out_free_conn:
spin_unlock(&nn->client_lock);
free_conn(conn);