[Bug 300130] nfscl: EXCHANGE_ID verifier bump on every recovery causes client-replacement churn against Linux nfsd

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=300130

--- Comment #1 from Nick <e@4779.name> ---
Proposed fix (inline below): keep the EXCHANGE_ID verifier stable on
the race-recovery fallback.

The nfsc_rev bump in nfsrpc_setclient() fires not only on identity
reset but also on the ExchangeID fallback taken by nfscl_recover()
when CreateSession on the extant ClientID fails. Guarding it with
odsp == NULL keeps the verifier stable on that fallback, so a server
implementing RFC 5661 s18.35.4 case 2 reuses the confirmed client
record instead of replacing it. Race-initiated recovery then no longer
destroys live state, and the self-sustaining loop (every in-flight op
failing BADSESSION/BADSTATEID, each failure re-arming recovery) stops.

The bump remains for first-setclient and for nfscl_hasexpired()
(retokp == NULL there), so lease loss still rotates the verifier once.
The v4.0 SETCLIENTID path is intentionally untouched (same disease,
no BADSESSION amplifier; noted for scope discussion).

Testing (15.1-RELEASE-p5 amd64 client, Linux nfsd 7.2.9 server):

Before: continuous full ExchangeID+CreateSession cycles ~1/s under
load for days (clientid cl_id +22 per 30 s; OP_SEQUENCE
NFS4ERR_BADSESSION ~1/4 s, OP_READ NFS4ERR_BADSTATEID ~2/s).

After: 25 min quiet window with zero recovery prints under a 2 GiB
write+sync; clientid frozen; zero non-zero-status compounds in a 30 s
tracepoint window; a 45 s full NFS outage (iptables DROP on the
server) survived with the clientid unchanged and no recovery fired.

Residual single events: during two hours of active testing (server
restarts, outages, boot-time recheck storm of ~700 torrents) six
isolated recoveries occurred, each completing and going quiet --
bounded events, not a loop. Pre-patch, the same conditions produce
continuous churn.

Known issue this patch surfaces (verified in-tree on both sides):
FreeBSD nfsd's confirmed-reuse branch of exchange_id returns
eir_sequenceid = 0 with a comment saying the client ignores it
(sys/fs/nfs/nfs_nfsdstate.c), but nfscl stores eir_sequenceid and the
fallback sends it as csa_sequenceid, while the server's confirm check
expects lc_confirm + 1 and returns NFSERR_SEQMISORDERED -- which is
not in nfscl_recover()'s retry set. Pre-patch this was unreachable
because the verifier-change branch pre-syncs lc_confirm. Options:
server returns lc_confirm + 1 on the reuse branch; or the client bumps
the sequenceid when EXCHGID4_FLAG_CONFIRMED_R is set; or add
SEQMISORDERED to the retry set with a one-time bump. Happy to prepare
any of these as a follow-up.

Also of note: pNFS/DS paths do not call nfsrpc_setclient (they do
ExchangeID/CreateSession directly on the same clp), so this patch
stabilizes the DS identity as well.

Open question: the guard relies on the convention that odsp is only
populated when retokp != NULL (nfscl_recover() is the only
identity-preserving caller). A future caller passing retokp would
silently lose the bump. If preferred, the bump can be hoisted to the
callers or put behind an explicit identity_reset parameter.

Patch (against 15.1-RELEASE-p5; applies to stable/15 and main at the
same site):

--- a/sys/fs/nfsclient/nfs_clrpcops.c
+++ b/sys/fs/nfsclient/nfs_clrpcops.c
@@ -1109,7 +1109,15 @@ nfsrpc_setclient(struct nfsmount *nmp, struct
nfsclclient *clp, int reclaim,
                        /*
                         * Either there was no previous session or the
                         * CreateSession attempt failed, so...
                         * do an ExchangeID followed by the CreateSession.
+                        * Only bump the verifier when there was no previous
+                        * session (identity reset).  When a session existed,
+                        * keep the verifier so that a server following
+                        * RFC 5661 s18.35.4 case 2 reuses the confirmed
+                        * client record instead of replacing it, destroying
+                        * all sessions/stateids and re-triggering recovery
+                        * for every racing operation (PR 300130).
                         */
-                       clp->nfsc_rev = rev++;
+                       if (odsp == NULL)
+                               clp->nfsc_rev = rev++;
                        error = nfsrpc_exchangeid(nmp, clp, &nmp->nm_sockreq,
0,
                            NFSV4EXCH_USEPNFSMDS | NFSV4EXCH_USENONPNFS, &dsp,
                            cred, p);

Developed with LLM assistance; the analysis, verification, and testing
are the reporter's.

-- 
You are receiving this mail because:
You are the assignee for the bug.