[PATCH v7 00/14] nfsd: assorted cleanups involving v4 special stateids.

[PATCH v7 00/14] nfsd: assorted cleanups involving v4 special stateids.

[NeilBrown]

I've done some proper testing this time and found that nfsd4_open() does
something a bit unusualy with current_fh so that my change to how
current-stateid is saved didn't work.  I've added two patches to fix that (08 and 09).

I've also split the non-RFC-7862 change to foreign-filehandle handling
out to its own patch (04).

This series compiles at each commit and passes all nfsv4.1 pynfs tests.
A few nfsv4.0 pynfs tests fail because they try a "stale" stateid which
isn't one that the server ever would generate, so those tests get a
BAD_STATEID error.  Hopefully I'l post a patch for pynfs.

Thanks,
NeilBrown

 [PATCH v6 01/14] nfsd: rename ALLOWED_WITHOUT_FH to
 [PATCH v6 02/14] nfsd: discard NFSD4_FH_FOREIGN
 [PATCH v6 03/14] nfsd: simplify foreign-filehandle handling to better
 [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign
 [PATCH v6 05/14] nfsd: report correct error for attempt to use
 [PATCH v6 06/14] nfsd: drop explicit tests for special stateids which
 [PATCH v6 07/14] nfsd: revise names of special stateid, and predicate
 [PATCH v6 08/14] nfsd: pass parent_fh explicitly to
 [PATCH v6 09/14] nfsd: revert nfsd4: delay setting current_fh in open
 [PATCH v6 10/14] nfsd: simplify clearing of current-state-id
 [PATCH v6 11/14] nfsd: simplify use of the current stateid.
 [PATCH v6 12/14] nfsd: simplify saving of the current stateid
 [PATCH v6 13/14] nfsd: discard current_stateid.h
 [PATCH v6 14/14] nfsd: conditionally clear seqid when current_stateid

[PATCH v6 01/14] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsdv4 OPs which do not have ALLOWED_WITHOUT_FH can assume that a PUTFH
has been called and may actually assume that current_fh->fh_dentry is
non-NULL.  nfsd4_setattr(), for example, assumes fh_dentry != NULL.

However the possibility of foreign filehandles (needed for v4.2 COPY)
means that there may be a filehandle present while fh_dentry is NULL.

Sending a COMPOUND containing:
   SEQUENCE
   PUTFH - foreign filehandle
   SETATTR - new mode
   SAVEFH
   COPY - with non-empty server list

to an NFS server with inter-server copy enabled will cause a NULL
pointer dereference when nfsd4_setattr() calls fh_want_write().

Most NFSv4 OPs actually want a "local" filehandle, not just any
filehandle.  So this patch renames ALLOWED_WITHOUT_FH to
ALLOWED_WITHOUT_LOCAL_FH and sets it on all the other which DON'T
require a local filehandle.  That is: all that don't require any
filehandle together with SAVEFH, which is the only OP which needs to
handle a foreign current_fh.  (COPY must handle a foreign save_fh, but
all OPs which access save_fh already do any required validity tests
themselves).

nfsd4_savefh() is changed to validate the filehandle itself as the
caller no longer validates it.

nfsd4_proc_compound no longer allows ops without
ALLOWED_WITHOUT_LOCAL_FH to be called with a foreign fh - current_fh
must be local and ->fh_dentry must be non-NULL.  This protects
nfsd4_setattr() and any others that might use ->fh_dentry without
checking.

The
       current_fh->fh_export &&
test is removed from an "else if" because that condition is now only
tested when current_fh->fh_dentry is not NULL, and in that case
current_fh->fh_export is also guaranteed to not be NULL.

Fixes: b9e8638e3d9e ("NFSD: allow inter server COPY to have a STALE source server fh")
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>

---
changes since v5
 - fix compile failure: need cstate->current_fh
 - comment for ALLOWED_WITHOUT_LOCAL_FH improved
---
 fs/nfsd/nfs4proc.c | 59 ++++++++++++++++++++++++++--------------------
 fs/nfsd/xdr4.h     |  2 +-
 2 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index dcad50846a97..947b1b6d2282 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -729,6 +729,16 @@ static __be32
 nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	     union nfsd4_op_u *u)
 {
+	/*
+	 * SAVEFH is "ALLOWED_WITHOUT_LOCAL_FH" in that
+	 * current_fh.fh_dentry is not required, those some fh_handle
+	 * *is* required so that a foreign fh can be saved as needed for
+	 * inter-server COPY.
+	 */
+	if (!cstate->current_fh.fh_dentry &&
+	    !HAS_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN))
+		return nfserr_nofilehandle;
+
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
 	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
 		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
@@ -2919,14 +2929,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 				op->status = nfsd4_open_omfg(rqstp, cstate, op);
 			goto encode_op;
 		}
-		if (!current_fh->fh_dentry &&
-				!HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
-			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
+		if (!current_fh->fh_dentry) {
+			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
 				op->status = nfserr_nofilehandle;
 				goto encode_op;
 			}
-		} else if (current_fh->fh_export &&
-			   current_fh->fh_export->ex_fslocs.migrated &&
+		} else if (current_fh->fh_export->ex_fslocs.migrated &&
 			  !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
 			op->status = nfserr_moved;
 			goto encode_op;
@@ -3507,21 +3515,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_PUTFH] = {
 		.op_func = nfsd4_putfh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
 		.op_name = "OP_PUTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTPUBFH] = {
 		.op_func = nfsd4_putrootfh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
 		.op_name = "OP_PUTPUBFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTROOTFH] = {
 		.op_func = nfsd4_putrootfh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
 		.op_name = "OP_PUTROOTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3557,7 +3565,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_RENEW] = {
 		.op_func = nfsd4_renew,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RENEW",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3565,14 +3573,15 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_RESTOREFH] = {
 		.op_func = nfsd4_restorefh,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_IS_PUTFH_LIKE | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RESTOREFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_SAVEFH] = {
 		.op_func = nfsd4_savefh,
-		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH
+				| OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_SAVEFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
@@ -3593,7 +3602,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_SETCLIENTID] = {
 		.op_func = nfsd4_setclientid,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING | OP_CACHEME
 				| OP_NONTRIVIAL_ERROR_ENCODE,
 		.op_name = "OP_SETCLIENTID",
@@ -3601,7 +3610,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_SETCLIENTID_CONFIRM] = {
 		.op_func = nfsd4_setclientid_confirm,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING | OP_CACHEME,
 		.op_name = "OP_SETCLIENTID_CONFIRM",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3620,7 +3629,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_RELEASE_LOCKOWNER] = {
 		.op_func = nfsd4_release_lockowner,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RELEASE_LOCKOWNER",
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3630,54 +3639,54 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	[OP_EXCHANGE_ID] = {
 		.op_func = nfsd4_exchange_id,
 		.op_release = nfsd4_exchange_id_release,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_EXCHANGE_ID",
 		.op_rsize_bop = nfsd4_exchange_id_rsize,
 	},
 	[OP_BACKCHANNEL_CTL] = {
 		.op_func = nfsd4_backchannel_ctl,
-		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_BACKCHANNEL_CTL",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_BIND_CONN_TO_SESSION] = {
 		.op_func = nfsd4_bind_conn_to_session,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_BIND_CONN_TO_SESSION",
 		.op_rsize_bop = nfsd4_bind_conn_to_session_rsize,
 	},
 	[OP_CREATE_SESSION] = {
 		.op_func = nfsd4_create_session,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_CREATE_SESSION",
 		.op_rsize_bop = nfsd4_create_session_rsize,
 	},
 	[OP_DESTROY_SESSION] = {
 		.op_func = nfsd4_destroy_session,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_DESTROY_SESSION",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_SEQUENCE] = {
 		.op_func = nfsd4_sequence,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP,
 		.op_name = "OP_SEQUENCE",
 		.op_rsize_bop = nfsd4_sequence_rsize,
 	},
 	[OP_DESTROY_CLIENTID] = {
 		.op_func = nfsd4_destroy_clientid,
-		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
 				| OP_MODIFIES_SOMETHING,
 		.op_name = "OP_DESTROY_CLIENTID",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_RECLAIM_COMPLETE] = {
 		.op_func = nfsd4_reclaim_complete,
-		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_RECLAIM_COMPLETE",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
@@ -3690,13 +3699,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_TEST_STATEID] = {
 		.op_func = nfsd4_test_stateid,
-		.op_flags = ALLOWED_WITHOUT_FH,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
 		.op_name = "OP_TEST_STATEID",
 		.op_rsize_bop = nfsd4_test_stateid_rsize,
 	},
 	[OP_FREE_STATEID] = {
 		.op_func = nfsd4_free_stateid,
-		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_FREE_STATEID",
 		.op_get_currentstateid = nfsd4_get_freestateid,
 		.op_rsize_bop = nfsd4_only_status_rsize,
@@ -3711,7 +3720,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	[OP_GETDEVICEINFO] = {
 		.op_func = nfsd4_getdeviceinfo,
 		.op_release = nfsd4_getdeviceinfo_release,
-		.op_flags = ALLOWED_WITHOUT_FH,
+		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
 		.op_name = "OP_GETDEVICEINFO",
 		.op_rsize_bop = nfsd4_getdeviceinfo_rsize,
 	},
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ae75846b3cd7..e27258e694a9 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -1006,7 +1006,7 @@ extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
 extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
 
 enum nfsd4_op_flags {
-	ALLOWED_WITHOUT_FH = 1 << 0,    /* No current filehandle required */
+	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No fh_dentry needed in current filehandle */
 	ALLOWED_ON_ABSENT_FS = 1 << 1,  /* ops processed on absent fs */
 	ALLOWED_AS_FIRST_OP = 1 << 2,   /* ops reqired first in compound */
 	/* For rfc 5661 section 2.6.3.1.1: */
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 02/14] nfsd: discard NFSD4_FH_FOREIGN

[NeilBrown]

From: NeilBrown <neil@brown.name>

NFSD4_FH_FOREIGN is not needed as the same information is elsewhere.

If fh_handle.fh_size is 0 then there is no filehandle
else if fh_dentry is NULL then the filehandle is foreign
else the filehandle is local.

So we can discard NFSD4_FH_FOREIGN and the related struct field,
and code.

In NFSv4.1, SECINFO and similar OPs can consume current_fh, but don't
currently set fh_size to zero.  So the above rule for filehandle type
determination isn't currently accurate.  This is fixed by enhancing
fh_put() to clear fhp->fh_handle.fh_size.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>

---
changes since v5
- clear fh_handle.fh_size in fh_put()
---
 fs/nfsd/nfs4proc.c | 7 ++-----
 fs/nfsd/nfsfh.c    | 1 +
 fs/nfsd/nfsfh.h    | 4 ----
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 947b1b6d2282..cbf66091b0e4 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -693,10 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	       putfh->pf_fhlen);
 	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
-	if (ret == nfserr_stale && putfh->no_verify) {
-		SET_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN);
+	if (ret == nfserr_stale && putfh->no_verify)
 		ret = 0;
-	}
 #endif
 	return ret;
 }
@@ -735,8 +733,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * *is* required so that a foreign fh can be saved as needed for
 	 * inter-server COPY.
 	 */
-	if (!cstate->current_fh.fh_dentry &&
-	    !HAS_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN))
+	if (cstate->current_fh.fh_handle.fh_size == 0)
 		return nfserr_nofilehandle;
 
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index ed85dd43da18..3c449e5e2459 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -806,6 +806,7 @@ fh_put(struct svc_fh *fhp)
 		fhp->fh_export = NULL;
 	}
 	fhp->fh_no_wcc = false;
+	fhp->fh_handle.fh_size = 0;
 	return;
 }
 
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 5ef7191f8ad8..43fcc1dcf69a 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -93,7 +93,6 @@ typedef struct svc_fh {
 						 */
 	bool			fh_use_wgather;	/* NFSv2 wgather option */
 	bool			fh_64bit_cookies;/* readdir cookie size */
-	int			fh_flags;	/* FH flags */
 	bool			fh_post_saved;	/* post-op attrs saved */
 	bool			fh_pre_saved;	/* pre-op attrs saved */
 
@@ -111,9 +110,6 @@ typedef struct svc_fh {
 	struct kstat		fh_post_attr;	/* full attrs after operation */
 	u64			fh_post_change; /* nfsv4 change; see above */
 } svc_fh;
-#define NFSD4_FH_FOREIGN (1<<0)
-#define SET_FH_FLAG(c, f) ((c)->fh_flags |= (f))
-#define HAS_FH_FLAG(c, f) ((c)->fh_flags & (f))
 
 enum nfsd_fsid {
 	FSID_DEV = 0,
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 03/14] nfsd: simplify foreign-filehandle handling to better match RFC-7862

[NeilBrown]

From: NeilBrown <neil@brown.name>

When the COPY v4.2 op is used in the inter-server copy mode, the file
handle of the source file (presented as the saved filehandle to COPY) is
for a different ("foreign") file server which would not be expected to
be understood by this server.  fh_verify() is likely to return
nfserr_stale.

In order of this filehandle to still be available to COPY, both PUTFH
and SAVEFH must not trigger an error.

RFC 7862 section 15.2.3 says:

   If the request is for an inter-server copy, the source-fh is a
   filehandle from the source server and the COMPOUND procedure is being
   executed on the destination server.  In this case, the source-fh is a
   foreign filehandle on the server receiving the COPY request.  If
   either PUTFH or SAVEFH checked the validity of the filehandle, the
   operation would likely fail and return NFS4ERR_STALE.

   If a server supports the inter-server copy feature, a PUTFH followed
   by a SAVEFH MUST NOT return NFS4ERR_STALE for either operation.
   These restrictions do not pose substantial difficulties for servers.
   CURRENT_FH and SAVED_FH may be validated in the context of the
   operation referencing them and an NFS4ERR_STALE error returned for an
   invalid filehandle at that point.

Linux nfsd currently takes a different approach.  Rather than just
checking for "a PUTFH followed by a SAVEFH", it goes further to consider
only that case when this filehandle is then used for COPY, and allows
that it might have been subject of RESTOREFH and SAVEFH in between.

This is not a problem in itself except for the extra code with little
benefit.  This analysis of the COMPOUND to detect PUTFH ops which need
care is performed on every COMPOUND request, which is an unwanted cost.

It is sufficient to check if the relevant conditions are met only when a
PUTFH op actually receives an error from fh_verify().

This patch removes the checking code from common paths and places it in
nfsd4_putfh() only when fh_verify() returns a relevant error.

Rather than scanning ahead for a COPY, this patch notes (in
nfsd4_compoundargs) when an inter-server COPY is decoded, and in that
case only checks the next op to see if it is SAVEFH as this is what the
RFC requires.

A test on "inter_copy_offload_enable" is also added to be completely
consistent with the "If a server supports the inter-server copy feature"
precondition.

As we do this test in nfsd4_putfh() there is no now need to mark the
putfh op as "no_verify".

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>

---
Changes since v5:
- drop nextop variable - only used once.
- add comment clarifying role of resp->opcnt and args->opcnt
- fix comparison between these two
- Move handling of BADHANDLE to a separate patch
---
 fs/nfsd/nfs4proc.c | 67 +++++++++++++++++-----------------------------
 fs/nfsd/nfs4xdr.c  |  2 +-
 fs/nfsd/xdr4.h     |  2 +-
 3 files changed, 27 insertions(+), 44 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index cbf66091b0e4..112e62b6b9c6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -693,8 +693,31 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	       putfh->pf_fhlen);
 	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
-	if (ret == nfserr_stale && putfh->no_verify)
-		ret = 0;
+	if (ret == nfserr_stale && inter_copy_offload_enable) {
+		struct nfsd4_compoundargs *args = rqstp->rq_argp;
+		struct nfsd4_compoundres *resp = rqstp->rq_resp;
+
+		/*
+		 * args->opcnt is the number of ops in the request.
+		 * resp->opcnt is the number of ops, including this
+		 * one, that have been processed, so it points
+		 * to the next op.
+		 */
+		if (resp->opcnt < args->opcnt &&
+		    args->ops[resp->opcnt].opnum == OP_SAVEFH &&
+		    args->is_inter_server_copy) {
+			/*
+			 * RFC 7862 section 15.2.3 says:
+			 *  If a server supports the inter-server copy
+			 *  feature, a PUTFH followed by a SAVEFH MUST
+			 *  NOT return NFS4ERR_STALE for either
+			 *  operation.
+			 * We limit this to when there is a COPY
+			 * in the COMPOUND.
+			 */
+			ret = 0;
+		}
+	}
 #endif
 	return ret;
 }
@@ -2810,45 +2833,6 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
 	return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
 }
 
-#ifdef CONFIG_NFSD_V4_2_INTER_SSC
-static void
-check_if_stalefh_allowed(struct nfsd4_compoundargs *args)
-{
-	struct nfsd4_op	*op, *current_op = NULL, *saved_op = NULL;
-	struct nfsd4_copy *copy;
-	struct nfsd4_putfh *putfh;
-	int i;
-
-	/* traverse all operation and if it's a COPY compound, mark the
-	 * source filehandle to skip verification
-	 */
-	for (i = 0; i < args->opcnt; i++) {
-		op = &args->ops[i];
-		if (op->opnum == OP_PUTFH)
-			current_op = op;
-		else if (op->opnum == OP_SAVEFH)
-			saved_op = current_op;
-		else if (op->opnum == OP_RESTOREFH)
-			current_op = saved_op;
-		else if (op->opnum == OP_COPY) {
-			copy = (struct nfsd4_copy *)&op->u;
-			if (!saved_op) {
-				op->status = nfserr_nofilehandle;
-				return;
-			}
-			putfh = (struct nfsd4_putfh *)&saved_op->u;
-			if (nfsd4_ssc_is_inter(copy))
-				putfh->no_verify = true;
-		}
-	}
-}
-#else
-static void
-check_if_stalefh_allowed(struct nfsd4_compoundargs *args)
-{
-}
-#endif
-
 /*
  * COMPOUND call.
  */
@@ -2898,7 +2882,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 		resp->opcnt = 1;
 		goto encode_op;
 	}
-	check_if_stalefh_allowed(args);
 
 	rqstp->rq_lease_breaker = (void **)&cstate->clp;
 
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 51ef97c25456..7e44af3d10b9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1250,7 +1250,6 @@ nfsd4_decode_putfh(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
 	if (!putfh->pf_fhval)
 		return nfserr_jukebox;
 
-	putfh->no_verify = false;
 	return nfs_ok;
 }
 
@@ -2047,6 +2046,7 @@ nfsd4_decode_copy(struct nfsd4_compoundargs *argp, union nfsd4_op_u *u)
 	if (status)
 		return status;
 
+	argp->is_inter_server_copy = true;
 	ns_dummy = kmalloc(sizeof(struct nl4_server), GFP_KERNEL);
 	if (ns_dummy == NULL)
 		return nfserr_jukebox;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index e27258e694a9..cf2355ceb1ff 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -335,7 +335,6 @@ struct nfsd4_lookup {
 struct nfsd4_putfh {
 	u32		pf_fhlen;           /* request */
 	char		*pf_fhval;          /* request */
-	bool		no_verify;	    /* represents foreigh fh */
 };
 
 struct nfsd4_getxattr {
@@ -907,6 +906,7 @@ struct nfsd4_compoundargs {
 	u32				client_opcnt;
 	u32				opcnt;
 	bool				splice_ok;
+	bool				is_inter_server_copy;
 	struct nfsd4_op			*ops;
 	struct nfsd4_op			iops[8];
 };
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY

[NeilBrown]

From: NeilBrown <neil@brown.name>

RFC-7862 acknowledges that a filehandle provided as the source of an
inter-server copy might result in NFS4ERR_STALE when given to PUTFH, and
gives guidance on how this error can be ignored (section 15.2.3).

NFS4ERR_BADHANDLE is also a possible error in this circumstance if the
foreign server is running a different implementation of NFS than the
current one.  This appears to be a simple omission in the RFC.

There can be no harm in delaying a BADHANDLE error in the same situation
where we already delay STALE errors, and no harm in sending a locally
"bad" handle to a foreign server to request a COPY.

So extend the test in nfsd4_putfh to also check for nfserr_badhandle.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 112e62b6b9c6..ae34b816371c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -693,7 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	       putfh->pf_fhlen);
 	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
 #ifdef CONFIG_NFSD_V4_2_INTER_SSC
-	if (ret == nfserr_stale && inter_copy_offload_enable) {
+	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
+	    inter_copy_offload_enable) {
 		struct nfsd4_compoundargs *args = rqstp->rq_argp;
 		struct nfsd4_compoundres *resp = rqstp->rq_resp;
 
@@ -713,7 +714,11 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			 *  NOT return NFS4ERR_STALE for either
 			 *  operation.
 			 * We limit this to when there is a COPY
-			 * in the COMPOUND.
+			 * in the COMPOUND, and extend it to
+			 * also ignore NFS4ERR_BADHANDLE despite the
+			 * RFC not requiring this.  If the remote
+			 * server is running a different NFS implementation,
+			 * NFS4ERR_BADHANDLE is a likely error.
 			 */
 			ret = 0;
 		}
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 05/14] nfsd: report correct error for attempt to use foreign filehandle

[NeilBrown]

From: NeilBrown <neil@brown.name>

A COMPOUND with
   SEQUENCE
   PUTFH foreign filehandle
   SAVFH
   RESTOREFH
   some-op-that-uses-current_fh
   COPY

will report NFS4ERR_NOHANDLE for that 5th op as ALLOWED_WITHOUT_LOCAL_FH
is not set and ->fh_dentry is NULL.  However that error is not correct.
NFS4ERR_STALE or NFS4ERR_BADHANDLE would be the correct error.

This patch saves the correct error and reports it when appropriate.

It is highly unlikely that a client would ever notice this difference as
the COMPOUND that would produce it is bizarre.  Maybe we don't need this
patch.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 4 +++-
 fs/nfsd/xdr4.h     | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ae34b816371c..bd7ba5df07a7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -720,6 +720,7 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			 * server is running a different NFS implementation,
 			 * NFS4ERR_BADHANDLE is a likely error.
 			 */
+			cstate->saved_status = ret;
 			ret = 0;
 		}
 	}
@@ -2865,6 +2866,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 	resp->tag = args->tag;
 	resp->rqstp = rqstp;
 	cstate->minorversion = args->minorversion;
+	cstate->saved_status = nfserr_nofilehandle;
 	fh_init(current_fh, NFS4_FHSIZE);
 	fh_init(save_fh, NFS4_FHSIZE);
 	/*
@@ -2916,7 +2918,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 		}
 		if (!current_fh->fh_dentry) {
 			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
-				op->status = nfserr_nofilehandle;
+				op->status = cstate->saved_status;
 				goto encode_op;
 			}
 		} else if (current_fh->fh_export->ex_fslocs.migrated &&
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index cf2355ceb1ff..b5ec2cdd61a0 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -187,10 +187,11 @@ struct nfsd4_compound_state {
 	struct nfsd4_session	*session;
 	struct nfsd4_slot	*slot;
 	int			data_offset;
-	bool                    spo_must_allowed;
+	bool			spo_must_allowed;
 	size_t			iovlen;
 	u32			minorversion;
 	__be32			status;
+	__be32			saved_status;
 	stateid_t	current_stateid;
 	stateid_t	save_stateid;
 	/* to indicate current and saved state id presents */
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 06/14] nfsd: drop explicit tests for special stateids which would be invalid.

[NeilBrown]

From: NeilBrown <neil@brown.name>

In two places nfsd has code to test for special stateids and to report
nfserr_bad_stateid if they are found.
One is for handling TEST_STATEID ops which always forbid these stateids,
and one is for all other places that a stateid is used, and the code is
*after* any checks for special stateids which might be permitted.

These tests add no value.  In each case there is a subsequent lookup for
the stateid which will return the same error code if the stateid is not
found, and special stateids never will be found.

Special stateids have a si.opaque.so_id which is either 0 or UINT_MAX.
Stateids stored in the idr only have so_id ranging from 1 or INT_MAX.
So there is no possibility of a special stateid being found.

Having the explicit test optimises the unexpected case where a special
stateid is incorrectly given, and adds unnecessary comparisons to the
common case of a non-special stateid being given.

In nfsd4_lookup_stateid(), simply removing the test would mean that
a special stateid could result in the incorrect nfserr_stale_stateid
error, as the validity of so_clid is checked before so_id.  So we
add extra checks to only return nfserr_stale_stateid if the stateid
looks like it might have been locally generated - so_id not
all zeroes or all ones.

Note that current pynfs synthesises a "stale" stateid for testing which
is actually invalid - it has a form that Linux NFSD will never generate.
So the nfsv4.0 tests in pynfs incorrectly report several errors with
this patch.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>

---
changes since v5:
- minor improvements to stateid_well_formed() comment.
---
 fs/nfsd/nfs4state.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 35004568d43e..aae0b301f38f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -74,6 +74,23 @@ static const stateid_t close_stateid = {
 	.si_generation = 0xffffffffU,
 };
 
+/*
+ * In NFSv4.0 there is a case where nfsd should return NFS4ERR_STALE_STATEID
+ * if the stateid looks like one it might have created previously, and
+ * NFS4ERR_BAD_STATEID if the stateid looks like it was never valid.
+ * There is not a lot of redundancy in the stateid that can be used to make
+ * this distinction, but it would be useful to differentiate special
+ * stateids from locally generated stateid.
+ * Special stateids have si.opaque.so_id being either all zeros or all 1s,
+ * so 0 or (u32)-1. Locally generated stateids have si.opaque.so_id as
+ * a number from 1 to INT_MAX (as generated by idr_alloc_cyclic()).
+ * We can test for the later range with some simple arithmetic.
+ */
+static inline bool stateid_well_formed(stateid_t *stid)
+{
+	return (stid->si_opaque.so_id - 1) < INT_MAX;
+}
+
 static u64 current_sessionid = 1;
 
 #define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
@@ -7129,9 +7146,6 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 	struct nfs4_stid *s;
 	__be32 status = nfserr_bad_stateid;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
-		CLOSE_STATEID(stateid))
-		return status;
 	spin_lock(&cl->cl_lock);
 	s = find_stateid_locked(cl, stateid);
 	if (!s)
@@ -7186,14 +7200,15 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 
 	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
-		CLOSE_STATEID(stateid))
-		return nfserr_bad_stateid;
 	status = set_client(&stateid->si_opaque.so_clid, cstate, nn);
 	if (status == nfserr_stale_clientid) {
-		if (cstate->session)
-			return nfserr_bad_stateid;
-		return nfserr_stale_stateid;
+		if (!cstate->session && stateid_well_formed(stateid))
+			/*
+			 * Might be from earlier instance - v4.0 likes
+			 * to know
+			 */
+			return nfserr_stale_stateid;
+		return nfserr_bad_stateid;
 	}
 	if (status)
 		return status;
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 07/14] nfsd: revise names of special stateid, and predicate functions.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When I see "CURRENT_STATEID(foo)" in the code it is not clear that this
is testing the stateid to see if it is a special stateid.  I find that
IS_CURRENT_STATEID(foo) is clearer.  But an inline function is even
better, so is_current_stateid().

There are other special stateids which are described in RFC 8881 Section
8.2.3 as "anonymous", "READ bypass", and "invalid".  The nfsd code
currently names them "zero", "one" and "close" which doesn't help with
comparing the code to the RFC.

So this patch changes the names of those special stateids and adds
"is_" to the front of the predicates.

As CLOSE_STATEID() was not needed, it is discarded rather than replacing
with is_invalid_stateid().

I felt that is_read_bypass_stateid() was a little too verbose, so I made
it is_bypass_stateid().

For consistency, invalid_stateid is changed to use ~0 rather than
0xffffffffU for the generation number.  (RFC 8881 say to use
"NFS4_UINT32_MAX" for the generation number here, and "all ones" for the
generation and opaque of anon_stateid).

Signed-off-by: NeilBrown <neil@brown.name>

---
Changes since v7
- fix is_XXX_stateid() which all inverted the test
- use sizeof(*stateid) instead of sizeof(stateid_t)
---
 fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index aae0b301f38f..f10c22d02735 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -60,18 +60,18 @@
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
 #define all_ones {{ ~0, ~0}, ~0}
-static const stateid_t one_stateid = {
+static const stateid_t read_bypass_stateid = {
 	.si_generation = ~0,
 	.si_opaque = all_ones,
 };
-static const stateid_t zero_stateid = {
+static const stateid_t anon_stateid = {
 	/* all fields zero */
 };
-static const stateid_t currentstateid = {
+static const stateid_t current_stateid = {
 	.si_generation = 1,
 };
-static const stateid_t close_stateid = {
-	.si_generation = 0xffffffffU,
+static const stateid_t invalid_stateid = {
+	.si_generation = ~0,
 };
 
 /*
@@ -93,10 +93,16 @@ static inline bool stateid_well_formed(stateid_t *stid)
 
 static u64 current_sessionid = 1;
 
-#define ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
-#define ONE_STATEID(stateid)  (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
-#define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
-#define CLOSE_STATEID(stateid)  (!memcmp((stateid), &close_stateid, sizeof(stateid_t)))
+/* These special stateid are defined in RFC 8881 Section 8.2.3 */
+static inline bool is_anon_stateid(stateid_t *stateid) {
+	return memcmp(stateid, &anon_stateid, sizeof(*stateid)) == 0;
+}
+static inline bool is_bypass_stateid(stateid_t *stateid) {
+	return memcmp(stateid, &read_bypass_stateid, sizeof(*stateid)) == 0;
+}
+static inline bool is_current_stateid(stateid_t *stateid) {
+	return memcmp(stateid, &current_stateid, sizeof(*stateid)) == 0;
+}
 
 /* forward declarations */
 static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
@@ -388,7 +394,7 @@ nfsd4_cb_notify_lock_prepare(struct nfsd4_callback *cb)
 static int
 nfsd4_cb_notify_lock_done(struct nfsd4_callback *cb, struct rpc_task *task)
 {
-	trace_nfsd_cb_notify_lock_done(&zero_stateid, task);
+	trace_nfsd_cb_notify_lock_done(&anon_stateid, task);
 
 	/*
 	 * Since this is just an optimization, we don't try very hard if it
@@ -6512,7 +6518,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * open stateid would have to be created.
 	 */
 	if (new_stp && open_xor_delegation(open)) {
-		memcpy(&open->op_stateid, &zero_stateid, sizeof(open->op_stateid));
+		memcpy(&open->op_stateid, &anon_stateid, sizeof(open->op_stateid));
 		open->op_rflags |= OPEN4_RESULT_NO_OPEN_STATEID;
 		release_open_stateid(stp);
 	}
@@ -7076,7 +7082,7 @@ __be32 nfs4_check_openmode(struct nfs4_ol_stateid *stp, int flags)
 static inline __be32
 check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid, int flags)
 {
-	if (ONE_STATEID(stateid) && (flags & RD_STATE))
+	if (is_bypass_stateid(stateid) && (flags & RD_STATE))
 		return nfs_ok;
 	else if (opens_in_grace(net)) {
 		/* Answer in remaining cases depends on existence of
@@ -7085,7 +7091,7 @@ check_special_stateids(struct net *net, svc_fh *current_fh, stateid_t *stateid,
 	} else if (flags & WR_STATE)
 		return nfs4_share_conflict(current_fh,
 				NFS4_SHARE_DENY_WRITE);
-	else /* (flags & RD_STATE) && ZERO_STATEID(stateid) */
+	else /* (flags & RD_STATE) && is_anon_stateid(stateid) */
 		return nfs4_share_conflict(current_fh,
 				NFS4_SHARE_DENY_READ);
 }
@@ -7401,7 +7407,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 	if (nfp)
 		*nfp = NULL;
 
-	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
+	if (is_anon_stateid(stateid) || is_bypass_stateid(stateid)) {
 		status = check_special_stateids(net, fhp, stateid, flags);
 		goto done;
 	}
@@ -7823,12 +7829,12 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	/* v4.1+ suggests that we send a special stateid in here, since the
 	 * clients should just ignore this anyway. Since this is not useful
-	 * for v4.0 clients either, we set it to the special close_stateid
+	 * for v4.0 clients either, we set it to the special invalid_stateid
 	 * universally.
 	 *
 	 * See RFC5661 section 18.2.4, and RFC7530 section 16.2.5
 	 */
-	memcpy(&close->cl_stateid, &close_stateid, sizeof(close->cl_stateid));
+	memcpy(&close->cl_stateid, &invalid_stateid, sizeof(close->cl_stateid));
 
 	/* put reference from nfs4_preprocess_seqid_op */
 	nfs4_put_stid(&stp->st_stid);
@@ -9101,7 +9107,7 @@ static void
 get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
 	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
-	    CURRENT_STATEID(stateid))
+	    is_current_stateid(stateid))
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
 }
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 08/14] nfsd: pass parent_fh explicitly to nfsd4_process_open2()

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_process_open2() sometimes needs the filehandle of the parent of
the created file.  This is passed to nfs4_set_delegation() to verify
that the parent is unchanged after the delegation is obtained.

Currently nfsd4_process_open2() knows that the parent is
cstate->current_fh but that will change in the next patch.

So with this patch we take a copy of current_fh earlier in the one case
(NFS4_OPEN_CLAIM_NULL) where it is needed and before any lookup happens.

nfs4_open_delegation() currently uses "currentfh" (which is the parent)
to pass to nfs4_delegation_stat().  As nfs4_delegation_stat() only wants
the vfsmount, it can equally well use "fh" which is the filehandle for the
newly created file - it must be on the same filesystem.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c  |  8 ++++++--
 fs/nfsd/nfs4state.c | 25 ++++++++++++-------------
 fs/nfsd/xdr4.h      |  4 +++-
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index bd7ba5df07a7..0c13c75e4092 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -538,6 +538,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfsd4_open *open = &u->open;
 	__be32 status;
 	struct svc_fh *resfh = NULL;
+	struct svc_fh parent_fh = {};
 	struct net *net = SVC_NET(rqstp);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	bool reclaim = false;
@@ -601,8 +602,10 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 
 	switch (open->op_claim_type) {
-	case NFS4_OPEN_CLAIM_DELEGATE_CUR:
 	case NFS4_OPEN_CLAIM_NULL:
+		fh_dup2(&parent_fh, &cstate->current_fh);
+		fallthrough;
+	case NFS4_OPEN_CLAIM_DELEGATE_CUR:
 		status = do_open_lookup(rqstp, cstate, open, &resfh);
 		if (status)
 			goto out;
@@ -630,7 +633,8 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	status = nfsd4_process_open2(rqstp, resfh, open);
+	status = nfsd4_process_open2(rqstp, resfh, &parent_fh, open);
+	fh_put(&parent_fh);
 	if (status && open->op_created)
 		pr_warn("nfsd4_process_open2 failed to open newly-created file: status=%u\n",
 			be32_to_cpu(status));
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f10c22d02735..3aebe90a12ec 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6046,7 +6046,7 @@ static bool nfsd4_want_deleg_timestamps(const struct nfsd4_open *open)
 
 static struct nfs4_delegation *
 nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
-		    struct svc_fh *parent)
+		    struct svc_fh *parent_fh)
 {
 	bool deleg_ts = nfsd4_want_deleg_timestamps(open);
 	struct nfs4_client *clp = stp->st_stid.sc_client;
@@ -6146,8 +6146,8 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	if (status)
 		goto out_clnt_odstate;
 
-	if (parent) {
-		status = nfsd4_verify_deleg_dentry(open, fp, parent);
+	if (parent_fh && parent_fh->fh_dentry) {
+		status = nfsd4_verify_deleg_dentry(open, fp, parent_fh);
 		if (status)
 			goto out_unlock;
 	}
@@ -6288,13 +6288,12 @@ nfsd4_add_rdaccess_to_wrdeleg(struct svc_rqst *rqstp, struct nfsd4_open *open,
  */
 static void
 nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
-		     struct nfs4_ol_stateid *stp, struct svc_fh *currentfh,
+		     struct nfs4_ol_stateid *stp, struct svc_fh *parent_fh,
 		     struct svc_fh *fh)
 {
 	struct nfs4_openowner *oo = openowner(stp->st_stateowner);
 	bool deleg_ts = nfsd4_want_deleg_timestamps(open);
 	struct nfs4_client *clp = stp->st_stid.sc_client;
-	struct svc_fh *parent = NULL;
 	struct nfs4_delegation *dp;
 	struct kstat stat;
 	int status = 0;
@@ -6308,8 +6307,6 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 				open->op_recall = true;
 			break;
 		case NFS4_OPEN_CLAIM_NULL:
-			parent = currentfh;
-			fallthrough;
 		case NFS4_OPEN_CLAIM_FH:
 			/*
 			 * Let's not give out any delegations till everyone's
@@ -6327,7 +6324,7 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 		default:
 			goto out_no_deleg;
 	}
-	dp = nfs4_set_delegation(open, stp, parent);
+	dp = nfs4_set_delegation(open, stp, parent_fh);
 	if (IS_ERR(dp))
 		goto out_no_deleg;
 
@@ -6337,7 +6334,7 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 		struct file *f = dp->dl_stid.sc_file->fi_deleg_file->nf_file;
 
 		if (!nfsd4_add_rdaccess_to_wrdeleg(rqstp, open, fh, stp) ||
-				!nfs4_delegation_stat(dp, currentfh, &stat)) {
+				!nfs4_delegation_stat(dp, fh, &stat)) {
 			nfs4_put_stid(&dp->dl_stid);
 			destroy_delegation(dp);
 			goto out_no_deleg;
@@ -6354,7 +6351,7 @@ nfs4_open_delegation(struct svc_rqst *rqstp, struct nfsd4_open *open,
 		spin_unlock(&f->f_lock);
 		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
 	} else {
-		open->op_delegate_type = deleg_ts && nfs4_delegation_stat(dp, currentfh, &stat) ?
+		open->op_delegate_type = deleg_ts && nfs4_delegation_stat(dp, fh, &stat) ?
 					 OPEN_DELEGATE_READ_ATTRS_DELEG : OPEN_DELEGATE_READ;
 		dp->dl_atime = stat.atime;
 		trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
@@ -6403,6 +6400,7 @@ static bool open_xor_delegation(struct nfsd4_open *open)
  * nfsd4_process_open2 - finish open processing
  * @rqstp: the RPC transaction being executed
  * @current_fh: NFSv4 COMPOUND's current filehandle
+ * @parent_fh: filehandle of parent when CLAIM_NULL
  * @open: OPEN arguments
  *
  * If successful, (1) truncate the file if open->op_truncate was
@@ -6412,7 +6410,9 @@ static bool open_xor_delegation(struct nfsd4_open *open)
  * network byte order is returned.
  */
 __be32
-nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open *open)
+nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh,
+		    struct svc_fh *parent_fh,
+		    struct nfsd4_open *open)
 {
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
 	struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
@@ -6509,8 +6509,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	* Attempt to hand out a delegation. No error return, because the
 	* OPEN succeeds even if we fail.
 	*/
-	nfs4_open_delegation(rqstp, open, stp,
-		&resp->cstate.current_fh, current_fh);
+	nfs4_open_delegation(rqstp, open, stp, parent_fh, current_fh);
 
 	/*
 	 * If there is an existing open stateid, it must be updated and
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index b5ec2cdd61a0..ea61e1a4c263 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -974,7 +974,9 @@ __be32 nfsd4_reclaim_complete(struct svc_rqst *, struct nfsd4_compound_state *,
 extern __be32 nfsd4_process_open1(struct nfsd4_compound_state *,
 		struct nfsd4_open *open, struct nfsd_net *nn);
 extern __be32 nfsd4_process_open2(struct svc_rqst *rqstp,
-		struct svc_fh *current_fh, struct nfsd4_open *open);
+				  struct svc_fh *current_fh,
+				  struct svc_fh *parent_fh,
+				  struct nfsd4_open *open);
 extern void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate);
 extern void nfsd4_cleanup_open_state(struct nfsd4_compound_state *cstate,
 		struct nfsd4_open *open);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 09/14] nfsd: revert nfsd4: delay setting current_fh in open

[NeilBrown]

From: NeilBrown <neil@brown.name>

This reverts
 Commit: c0e6bee48059 ("nfsd4: delay setting current_fh in open")

That patch was a precusor to

Commit: 4335723e8e9f ("nfsd4: fix delegation-unlink/rename race")

which attempted to fix a race by holding the parent directory locked for
longer.  That race was subsequently fixed a better way
Commit: 876c553cb410 ("NFSD: verify the opened dentry after setting a delegation")

which repeated the lookup after the delegation was obtained.

So delaying the setting of current_fh is no longer needed.

A cost of setting current_fh later is that when
nfs4_inc_and_copy_stateid() is called, current_fh has already been set
to the correct filehandle everywhere *except* in nfsd4_process_open2().
With this change, the current_fh will be stable from when
nfs4_inc_and_copy_stateid() which will allow a future patch which
simplifies the handling of current_stateid.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0c13c75e4092..0f490f2524fa 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -424,16 +424,17 @@ set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
 }
 
 static __be32
-do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh)
+do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
 {
 	struct svc_fh *current_fh = &cstate->current_fh;
+	struct svc_fh *resfh;
 	int accmode;
 	__be32 status;
 
-	*resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
-	if (!*resfh)
+	resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
+	if (!resfh)
 		return nfserr_jukebox;
-	fh_init(*resfh, NFS4_FHSIZE);
+	fh_init(resfh, NFS4_FHSIZE);
 	open->op_truncate = false;
 
 	if (open->op_create) {
@@ -453,7 +454,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		 */
 
 		current->fs->umask = open->op_umask;
-		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
+		status = nfsd4_create_file(rqstp, current_fh, resfh, open);
 		current->fs->umask = 0;
 
 		/*
@@ -466,7 +467,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 						FATTR4_WORD1_TIME_MODIFY);
 	} else {
 		status = nfsd_lookup(rqstp, current_fh,
-				     open->op_fname, open->op_fnamelen, *resfh);
+				     open->op_fname, open->op_fnamelen, resfh);
 		if (status == nfs_ok)
 			/* NFSv4 protocol requires change attributes even though
 			 * no change happened.
@@ -475,18 +476,21 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	}
 	if (status)
 		goto out;
-	status = nfsd_check_obj_isreg(*resfh, cstate->minorversion);
+	status = nfsd_check_obj_isreg(resfh, cstate->minorversion);
 	if (status)
 		goto out;
 
-	nfsd4_set_open_owner_reply_cache(cstate, open, *resfh);
+	nfsd4_set_open_owner_reply_cache(cstate, open, resfh);
 	accmode = NFSD_MAY_NOP;
 	if (open->op_created ||
 			open->op_claim_type == NFS4_OPEN_CLAIM_DELEGATE_CUR)
 		accmode |= NFSD_MAY_OWNER_OVERRIDE;
-	status = do_open_permission(rqstp, *resfh, open, accmode);
+	status = do_open_permission(rqstp, resfh, open, accmode);
 	set_change_info(&open->op_cinfo, current_fh);
+	fh_dup2(current_fh, resfh);
 out:
+	fh_put(resfh);
+	kfree(resfh);
 	return status;
 }
 
@@ -537,7 +541,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 {
 	struct nfsd4_open *open = &u->open;
 	__be32 status;
-	struct svc_fh *resfh = NULL;
 	struct svc_fh parent_fh = {};
 	struct net *net = SVC_NET(rqstp);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
@@ -606,7 +609,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		fh_dup2(&parent_fh, &cstate->current_fh);
 		fallthrough;
 	case NFS4_OPEN_CLAIM_DELEGATE_CUR:
-		status = do_open_lookup(rqstp, cstate, open, &resfh);
+		status = do_open_lookup(rqstp, cstate, open);
 		if (status)
 			goto out;
 		break;
@@ -622,7 +625,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		status = do_open_fhandle(rqstp, cstate, open);
 		if (status)
 			goto out;
-		resfh = &cstate->current_fh;
 		break;
 	case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
 	case NFS4_OPEN_CLAIM_DELEGATE_PREV:
@@ -633,7 +635,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	status = nfsd4_process_open2(rqstp, resfh, &parent_fh, open);
+	status = nfsd4_process_open2(rqstp, &cstate->current_fh, &parent_fh, open);
 	fh_put(&parent_fh);
 	if (status && open->op_created)
 		pr_warn("nfsd4_process_open2 failed to open newly-created file: status=%u\n",
@@ -645,11 +647,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		fput(open->op_filp);
 		open->op_filp = NULL;
 	}
-	if (resfh && resfh != &cstate->current_fh) {
-		fh_dup2(&cstate->current_fh, resfh);
-		fh_put(resfh);
-		kfree(resfh);
-	}
 	nfsd4_cleanup_open_state(cstate, open);
 	nfsd4_bump_seqid(cstate, status);
 	return status;
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 10/14] nfsd: simplify clearing of current-state-id

[NeilBrown]

From: NeilBrown <neil@brown.name>

In NFSv4.1 the current and saved filehandle each have a corresponding
stateid.  RFC 8881 requires that in certain circumstances - particularly
when the current filehandle is changed - the current stateid should be
set to the anonymous stateid (all zeros).  It also requires that when a
request tries to use the current stateid, if that stateid is "special"
(which includes the anonymous stateid), then a BAD_STATEID error must
be produced.

Linux nfsd current implements these requirements using a flag to record
if the stateid (current or saved) is valid rather than actually storing
the anon stateid when invalid.  This has the same net effect.

The aim of this patch is to simplify this code and particularly to
remove the per-op OP_CLEAR_STATEID flag which is unnecessary.

The "is valid" flag is moved from 'struct nfsd4_compound_state' to
'struct svc_fh' which already has a number of flags related to the
filehandle.  This flag will only ever be set for the v4 current_fh and
saved_fh and records if the corresponding stateid is valid.

fh_put() is changed to clear this flag.  As this is called on current_fh
in each op that currently has OP_CLEAR_STATEID set, this automatically
clears the stored stateid when required so OP_CLEAR_STATEID is no longer
needed.

As fh_dup2() already copies all of the svc_fh, it now copies the new
fh_have_stateid flag as well, so savefh and restorefh are simplified.

Signed-off-by: NeilBrown <neil@brown.name>

---
changes since v5
- fh_put() is ACTUALLY changed to clear fh_have_stateid.
---
 fs/nfsd/current_stateid.h |  1 -
 fs/nfsd/nfs4proc.c        | 25 ++++++++-----------------
 fs/nfsd/nfs4state.c       | 10 ++--------
 fs/nfsd/nfsfh.c           |  1 +
 fs/nfsd/nfsfh.h           |  1 +
 fs/nfsd/xdr4.h            | 13 -------------
 6 files changed, 12 insertions(+), 39 deletions(-)

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
index c28540d86742..24d769043207 100644
--- a/fs/nfsd/current_stateid.h
+++ b/fs/nfsd/current_stateid.h
@@ -5,7 +5,6 @@
 #include "state.h"
 #include "xdr4.h"
 
-extern void clear_current_stateid(struct nfsd4_compound_state *cstate);
 /*
  * functions to set current state id
  */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0f490f2524fa..ae9427ac7e5e 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -746,10 +746,7 @@ nfsd4_restorefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		return nfserr_restorefh;
 
 	fh_dup2(&cstate->current_fh, &cstate->save_fh);
-	if (HAS_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG)) {
-		memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
-		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
-	}
+	memcpy(&cstate->current_stateid, &cstate->save_stateid, sizeof(stateid_t));
 	return nfs_ok;
 }
 
@@ -767,10 +764,7 @@ nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		return nfserr_nofilehandle;
 
 	fh_dup2(&cstate->save_fh, &cstate->current_fh);
-	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
-		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
-		SET_CSTATE_FLAG(cstate, SAVED_STATE_ID_FLAG);
-	}
+	memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
 	return nfs_ok;
 }
 
@@ -2964,9 +2958,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 			if (op->opdesc->op_set_currentstateid)
 				op->opdesc->op_set_currentstateid(cstate, &op->u);
 
-			if (op->opdesc->op_flags & OP_CLEAR_STATEID)
-				clear_current_stateid(cstate);
-
 			if (current_fh->fh_export &&
 					need_wrongsec_check(rqstp))
 				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
@@ -3411,7 +3402,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_CREATE] = {
 		.op_func = nfsd4_create,
-		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME | OP_CLEAR_STATEID,
+		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
 		.op_name = "OP_CREATE",
 		.op_rsize_bop = nfsd4_create_rsize,
 	},
@@ -3465,13 +3456,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_LOOKUP] = {
 		.op_func = nfsd4_lookup,
-		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
+		.op_flags = OP_HANDLES_WRONGSEC,
 		.op_name = "OP_LOOKUP",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_LOOKUPP] = {
 		.op_func = nfsd4_lookupp,
-		.op_flags = OP_HANDLES_WRONGSEC | OP_CLEAR_STATEID,
+		.op_flags = OP_HANDLES_WRONGSEC,
 		.op_name = "OP_LOOKUPP",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
@@ -3504,21 +3495,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	[OP_PUTFH] = {
 		.op_func = nfsd4_putfh,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
-				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
+				| OP_IS_PUTFH_LIKE,
 		.op_name = "OP_PUTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTPUBFH] = {
 		.op_func = nfsd4_putrootfh,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
-				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
+				| OP_IS_PUTFH_LIKE,
 		.op_name = "OP_PUTPUBFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_PUTROOTFH] = {
 		.op_func = nfsd4_putrootfh,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
-				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
+				| OP_IS_PUTFH_LIKE,
 		.op_name = "OP_PUTROOTFH",
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3aebe90a12ec..03f29a6d7c14 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -9105,7 +9105,7 @@ nfs4_state_shutdown(void)
 static void
 get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
-	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG) &&
+	if (cstate->current_fh.fh_have_stateid &&
 	    is_current_stateid(stateid))
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
 }
@@ -9115,16 +9115,10 @@ put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
 	if (cstate->minorversion) {
 		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-		SET_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
+		cstate->current_fh.fh_have_stateid = true;
 	}
 }
 
-void
-clear_current_stateid(struct nfsd4_compound_state *cstate)
-{
-	CLEAR_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG);
-}
-
 /*
  * functions to set current state id
  */
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 3c449e5e2459..b8e6abe830e3 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -807,6 +807,7 @@ fh_put(struct svc_fh *fhp)
 	}
 	fhp->fh_no_wcc = false;
 	fhp->fh_handle.fh_size = 0;
+	fhp->fh_have_stateid = false;
 	return;
 }
 
diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
index 43fcc1dcf69a..0142227c90e6 100644
--- a/fs/nfsd/nfsfh.h
+++ b/fs/nfsd/nfsfh.h
@@ -93,6 +93,7 @@ typedef struct svc_fh {
 						 */
 	bool			fh_use_wgather;	/* NFSv2 wgather option */
 	bool			fh_64bit_cookies;/* readdir cookie size */
+	bool			fh_have_stateid; /* associated v4.1 stateid is not special */
 	bool			fh_post_saved;	/* post-op attrs saved */
 	bool			fh_pre_saved;	/* pre-op attrs saved */
 
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ea61e1a4c263..87986f1d5506 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -43,13 +43,6 @@
 #define NFSD4_MAX_TAGLEN	128
 #define XDR_LEN(n)                     (((n) + 3) & ~3)
 
-#define CURRENT_STATE_ID_FLAG (1<<0)
-#define SAVED_STATE_ID_FLAG (1<<1)
-
-#define SET_CSTATE_FLAG(c, f) ((c)->sid_flags |= (f))
-#define HAS_CSTATE_FLAG(c, f) ((c)->sid_flags & (f))
-#define CLEAR_CSTATE_FLAG(c, f) ((c)->sid_flags &= ~(f))
-
 /**
  * nfsd4_encode_bool - Encode an XDR bool type result
  * @xdr: target XDR stream
@@ -194,8 +187,6 @@ struct nfsd4_compound_state {
 	__be32			saved_status;
 	stateid_t	current_stateid;
 	stateid_t	save_stateid;
-	/* to indicate current and saved state id presents */
-	u32		sid_flags;
 };
 
 static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
@@ -1032,10 +1023,6 @@ enum nfsd4_op_flags {
 	 * the v4.0 case).
 	 */
 	OP_CACHEME = 1 << 6,
-	/*
-	 * These are ops which clear current state id.
-	 */
-	OP_CLEAR_STATEID = 1 << 7,
 	/* Most ops return only an error on failure; some may do more: */
 	OP_NONTRIVIAL_ERROR_ENCODE = 1 << 8,
 };
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 11/14] nfsd: simplify use of the current stateid.

[NeilBrown]

From: NeilBrown <neil@brown.name>

In NFSv4.1 any use of a stateid in an OP should check if it is
the special stateid that
   has "other" set to zero and "seqid" set to one

If it is, then the stateid saved from a previous operation should be
used unless that stateid as "special" stateid, in which case BAD_STATEID
must be returned.

Most ops that include a stateid pass it (indirectly) to
nfsd4_lookup_stateid().  The one exception is FREE_STATEID which uses
find_stateid_locked() directly.  (TEST_STATEID is not considered.  It is
given a list of stateids, among which any special stateid is explicitly
forbidden).

So rather than providing dedicated op_get_currentstateid functions for
each relevant op, we can include the required logic directly in
nfsd4_lookup_stateid() and nfsd4_free_stateid().  That is what this
patch does.

We also include an explicit return of nfserr_bad_stateid when the
special stateid is used, but no valid stateid is available.  This does
not change behaviour as the error would have been returned eventually.

This patch also removes the "minorversion" test in put_stateid() and
instead checks that sessions are in use when the stateid is used.  This
seems more natural.  Note that nfsd4_free_stateid() doesn't need this
explicit test as the function cannot be called for v4.0.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h | 21 ----------
 fs/nfsd/nfs4proc.c        | 11 -----
 fs/nfsd/nfs4state.c       | 85 ++++++---------------------------------
 fs/nfsd/xdr4.h            |  2 -
 4 files changed, 13 insertions(+), 106 deletions(-)

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
index 24d769043207..eaa4b8e40dc8 100644
--- a/fs/nfsd/current_stateid.h
+++ b/fs/nfsd/current_stateid.h
@@ -16,25 +16,4 @@ extern void nfsd4_set_lockstateid(struct nfsd4_compound_state *,
 		union nfsd4_op_u *);
 extern void nfsd4_set_closestateid(struct nfsd4_compound_state *,
 		union nfsd4_op_u *);
-
-/*
- * functions to consume current state id
- */
-extern void nfsd4_get_opendowngradestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_delegreturnstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_freestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_setattrstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_closestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_lockustateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_readstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_get_writestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-
 #endif   /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index ae9427ac7e5e..784f241c7119 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2943,8 +2943,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 		if (op->status)
 			goto encode_op;
 
-		if (op->opdesc->op_get_currentstateid)
-			op->opdesc->op_get_currentstateid(cstate, &op->u);
 		op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
 		trace_nfsd_compound_op_err(rqstp, op->opnum, op->status);
 
@@ -3391,7 +3389,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_CLOSE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_get_currentstateid = nfsd4_get_closestateid,
 		.op_set_currentstateid = nfsd4_set_closestateid,
 	},
 	[OP_COMMIT] = {
@@ -3411,7 +3408,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_DELEGRETURN",
 		.op_rsize_bop = nfsd4_only_status_rsize,
-		.op_get_currentstateid = nfsd4_get_delegreturnstateid,
 	},
 	[OP_GETATTR] = {
 		.op_func = nfsd4_getattr,
@@ -3452,7 +3448,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_LOCKU",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_get_currentstateid = nfsd4_get_lockustateid,
 	},
 	[OP_LOOKUP] = {
 		.op_func = nfsd4_lookup,
@@ -3489,7 +3484,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN_DOWNGRADE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_get_currentstateid = nfsd4_get_opendowngradestateid,
 		.op_set_currentstateid = nfsd4_set_opendowngradestateid,
 	},
 	[OP_PUTFH] = {
@@ -3518,7 +3512,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_release = nfsd4_read_release,
 		.op_name = "OP_READ",
 		.op_rsize_bop = nfsd4_read_rsize,
-		.op_get_currentstateid = nfsd4_get_readstateid,
 	},
 	[OP_READDIR] = {
 		.op_func = nfsd4_readdir,
@@ -3577,7 +3570,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME
 				| OP_NONTRIVIAL_ERROR_ENCODE,
 		.op_rsize_bop = nfsd4_setattr_rsize,
-		.op_get_currentstateid = nfsd4_get_setattrstateid,
 	},
 	[OP_SETCLIENTID] = {
 		.op_func = nfsd4_setclientid,
@@ -3604,7 +3596,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING | OP_CACHEME,
 		.op_name = "OP_WRITE",
 		.op_rsize_bop = nfsd4_write_rsize,
-		.op_get_currentstateid = nfsd4_get_writestateid,
 	},
 	[OP_RELEASE_LOCKOWNER] = {
 		.op_func = nfsd4_release_lockowner,
@@ -3686,7 +3677,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_func = nfsd4_free_stateid,
 		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_FREE_STATEID",
-		.op_get_currentstateid = nfsd4_get_freestateid,
 		.op_rsize_bop = nfsd4_only_status_rsize,
 	},
 	[OP_GET_DIR_DELEGATION] = {
@@ -3754,7 +3744,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_release = nfsd4_read_release,
 		.op_name = "OP_READ_PLUS",
 		.op_rsize_bop = nfsd4_read_plus_rsize,
-		.op_get_currentstateid = nfsd4_get_readstateid,
 	},
 	[OP_SEEK] = {
 		.op_func = nfsd4_seek,
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 03f29a6d7c14..de66b2971cd3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7191,6 +7191,11 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 	struct nfs4_stid *stid;
 	bool return_revoked = false;
 
+	if (nfsd4_has_session(cstate) && is_current_stateid(stateid)) {
+		if (!cstate->current_fh.fh_have_stateid)
+			return nfserr_bad_stateid;
+		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+	}
 	/*
 	 *  only return revoked delegations if explicitly asked.
 	 *  otherwise we report revoked or bad_stateid status.
@@ -7507,6 +7512,12 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfs4_client *cl = cstate->clp;
 	__be32 ret = nfserr_bad_stateid;
 
+	if (is_current_stateid(stateid)) {
+		if (!cstate->current_fh.fh_have_stateid)
+			return nfserr_bad_stateid;
+		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+	}
+
 	spin_lock(&cl->cl_lock);
 	s = find_stateid_locked(cl, stateid);
 	if (!s || s->sc_status & SC_STATUS_CLOSED)
@@ -9102,21 +9113,11 @@ nfs4_state_shutdown(void)
 	shrinker_free(nfsd_slot_shrinker);
 }
 
-static void
-get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
-{
-	if (cstate->current_fh.fh_have_stateid &&
-	    is_current_stateid(stateid))
-		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
-}
-
 static void
 put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
 {
-	if (cstate->minorversion) {
-		memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-		cstate->current_fh.fh_have_stateid = true;
-	}
+	memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
+	cstate->current_fh.fh_have_stateid = true;
 }
 
 /*
@@ -9150,66 +9151,6 @@ nfsd4_set_lockstateid(struct nfsd4_compound_state *cstate,
 	put_stateid(cstate, &u->lock.lk_resp_stateid);
 }
 
-/*
- * functions to consume current state id
- */
-
-void
-nfsd4_get_opendowngradestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->open_downgrade.od_stateid);
-}
-
-void
-nfsd4_get_delegreturnstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->delegreturn.dr_stateid);
-}
-
-void
-nfsd4_get_freestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->free_stateid.fr_stateid);
-}
-
-void
-nfsd4_get_setattrstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->setattr.sa_stateid);
-}
-
-void
-nfsd4_get_closestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->close.cl_stateid);
-}
-
-void
-nfsd4_get_lockustateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->locku.lu_stateid);
-}
-
-void
-nfsd4_get_readstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->read.rd_stateid);
-}
-
-void
-nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	get_stateid(cstate, &u->write.wr_stateid);
-}
-
 /**
  * nfsd4_vet_deleg_time - vet and set the timespec for a delegated timestamp update
  * @req: timestamp from the client
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 87986f1d5506..4f54108f24f9 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -1036,8 +1036,6 @@ struct nfsd4_operation {
 	/* Try to get response size before operation */
 	u32 (*op_rsize_bop)(const struct svc_rqst *rqstp,
 			const struct nfsd4_op *op);
-	void (*op_get_currentstateid)(struct nfsd4_compound_state *,
-			union nfsd4_op_u *);
 	void (*op_set_currentstateid)(struct nfsd4_compound_state *,
 			union nfsd4_op_u *);
 };
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 12/14] nfsd: simplify saving of the current stateid

[NeilBrown]

From: NeilBrown <neil@brown.name>

OPs that return a stateid must update the current stateid stored in the
COMPOUND state so it can be used in later OPs.

Every OP that returns a stateid uses nfs4_inc_and_copy_stateid() to
provide the stateid to return.

This patch updates that function to store the resulting stateid in
cstate->current_stateid, and passes cstate (the COMPOUND state) through
to the function in all cases except when used in a callback.

This removes the need for op_set_currentstateid.

This also sets the current stateid in more cases, such as LOCKU and
LAYOUTGET.

One case that this does not cover is CLOSE.  CLOSE always reports a
special (invalid) stateid, so it is changed to simply mark the current
stateid as not valid.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h | 11 -------
 fs/nfsd/nfs4layouts.c     | 10 ++++---
 fs/nfsd/nfs4proc.c        | 11 ++-----
 fs/nfsd/nfs4state.c       | 62 ++++++++++-----------------------------
 fs/nfsd/pnfs.h            |  5 ++--
 fs/nfsd/state.h           |  3 +-
 fs/nfsd/xdr4.h            |  1 +
 7 files changed, 30 insertions(+), 73 deletions(-)

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
index eaa4b8e40dc8..9dce3004b846 100644
--- a/fs/nfsd/current_stateid.h
+++ b/fs/nfsd/current_stateid.h
@@ -5,15 +5,4 @@
 #include "state.h"
 #include "xdr4.h"
 
-/*
- * functions to set current state id
- */
-extern void nfsd4_set_opendowngradestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_set_openstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_set_lockstateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
-extern void nfsd4_set_closestateid(struct nfsd4_compound_state *,
-		union nfsd4_op_u *);
 #endif   /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 683bd1130afe..4519a01bb354 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -414,7 +414,8 @@ nfsd4_recall_conflict(struct nfs4_layout_stateid *ls)
 }
 
 __be32
-nfsd4_insert_layout(struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
+nfsd4_insert_layout(struct nfsd4_compound_state *cstate,
+		    struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
 {
 	struct nfsd4_layout_seg *seg = &lgp->lg_seg;
 	struct nfs4_file *fp = ls->ls_stid.sc_file;
@@ -453,7 +454,7 @@ nfsd4_insert_layout(struct nfsd4_layoutget *lgp, struct nfs4_layout_stateid *ls)
 	list_add_tail(&new->lo_perstate, &ls->ls_layouts);
 	new = NULL;
 done:
-	nfs4_inc_and_copy_stateid(&lgp->lg_sid, &ls->ls_stid);
+	nfs4_inc_and_copy_stateid(cstate, &lgp->lg_sid, &ls->ls_stid);
 	spin_unlock(&ls->ls_lock);
 out:
 	spin_unlock(&fp->fi_lock);
@@ -528,7 +529,8 @@ nfsd4_return_file_layouts(struct svc_rqst *rqstp,
 	}
 	if (!list_empty(&ls->ls_layouts)) {
 		if (found)
-			nfs4_inc_and_copy_stateid(&lrp->lr_sid, &ls->ls_stid);
+			nfs4_inc_and_copy_stateid(cstate, &lrp->lr_sid,
+						  &ls->ls_stid);
 		lrp->lrs_present = true;
 	} else {
 		trace_nfsd_layoutstate_unhash(&ls->ls_stid.sc_stateid);
@@ -659,7 +661,7 @@ nfsd4_cb_layout_prepare(struct nfsd4_callback *cb)
 		container_of(cb, struct nfs4_layout_stateid, ls_recall);
 
 	mutex_lock(&ls->ls_mutex);
-	nfs4_inc_and_copy_stateid(&ls->ls_recall_sid, &ls->ls_stid);
+	nfs4_inc_and_copy_stateid(NULL, &ls->ls_recall_sid, &ls->ls_stid);
 	mutex_unlock(&ls->ls_mutex);
 }
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 784f241c7119..806306f48043 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -635,7 +635,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	status = nfsd4_process_open2(rqstp, &cstate->current_fh, &parent_fh, open);
+	status = nfsd4_process_open2(rqstp, cstate, &cstate->current_fh, &parent_fh, open);
 	fh_put(&parent_fh);
 	if (status && open->op_created)
 		pr_warn("nfsd4_process_open2 failed to open newly-created file: status=%u\n",
@@ -2536,7 +2536,7 @@ nfsd4_layoutget(struct svc_rqst *rqstp,
 	if (nfserr)
 		goto out_put_stid;
 
-	nfserr = nfsd4_insert_layout(lgp, ls);
+	nfserr = nfsd4_insert_layout(cstate, lgp, ls);
 
 out_put_stid:
 	mutex_unlock(&ls->ls_mutex);
@@ -2953,9 +2953,6 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
 			goto out;
 		}
 		if (!op->status) {
-			if (op->opdesc->op_set_currentstateid)
-				op->opdesc->op_set_currentstateid(cstate, &op->u);
-
 			if (current_fh->fh_export &&
 					need_wrongsec_check(rqstp))
 				op->status = check_nfsd_access(current_fh->fh_export, rqstp, false);
@@ -3389,7 +3386,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_CLOSE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_set_currentstateid = nfsd4_set_closestateid,
 	},
 	[OP_COMMIT] = {
 		.op_func = nfsd4_commit,
@@ -3434,7 +3430,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 				OP_NONTRIVIAL_ERROR_ENCODE,
 		.op_name = "OP_LOCK",
 		.op_rsize_bop = nfsd4_lock_rsize,
-		.op_set_currentstateid = nfsd4_set_lockstateid,
 	},
 	[OP_LOCKT] = {
 		.op_func = nfsd4_lockt,
@@ -3471,7 +3466,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN",
 		.op_rsize_bop = nfsd4_open_rsize,
-		.op_set_currentstateid = nfsd4_set_openstateid,
 	},
 	[OP_OPEN_CONFIRM] = {
 		.op_func = nfsd4_open_confirm,
@@ -3484,7 +3478,6 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 		.op_flags = OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN_DOWNGRADE",
 		.op_rsize_bop = nfsd4_status_stateid_rsize,
-		.op_set_currentstateid = nfsd4_set_opendowngradestateid,
 	},
 	[OP_PUTFH] = {
 		.op_func = nfsd4_putfh,
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index de66b2971cd3..e3b38245ba45 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1216,7 +1216,8 @@ nfs4_put_stid(struct nfs4_stid *s)
 }
 
 void
-nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid)
+nfs4_inc_and_copy_stateid(struct nfsd4_compound_state *cstate,
+			  stateid_t *dst, struct nfs4_stid *stid)
 {
 	stateid_t *src = &stid->sc_stateid;
 
@@ -1225,6 +1226,10 @@ nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid)
 		src->si_generation = 1;
 	memcpy(dst, src, sizeof(*dst));
 	spin_unlock(&stid->sc_lock);
+	if (cstate) {
+		memcpy(&cstate->current_stateid, dst, sizeof(stateid_t));
+		cstate->current_fh.fh_have_stateid = true;
+	}
 }
 
 static void put_deleg_file(struct nfs4_file *fp)
@@ -6399,6 +6404,7 @@ static bool open_xor_delegation(struct nfsd4_open *open)
 /**
  * nfsd4_process_open2 - finish open processing
  * @rqstp: the RPC transaction being executed
+ * @cstate: the nfsd4_compound_state for the current COMPOUND.
  * @current_fh: NFSv4 COMPOUND's current filehandle
  * @parent_fh: filehandle of parent when CLAIM_NULL
  * @open: OPEN arguments
@@ -6410,7 +6416,8 @@ static bool open_xor_delegation(struct nfsd4_open *open)
  * network byte order is returned.
  */
 __be32
-nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh,
+nfsd4_process_open2(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
+		    struct svc_fh *current_fh,
 		    struct svc_fh *parent_fh,
 		    struct nfsd4_open *open)
 {
@@ -6494,7 +6501,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh,
 			open->op_odstate = NULL;
 	}
 
-	nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &open->op_stateid, &stp->st_stid);
 	mutex_unlock(&stp->st_mutex);
 
 	if (nfsd4_has_session(&resp->cstate)) {
@@ -7688,7 +7695,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto put_stateid;
 	}
 	oo->oo_flags |= NFS4_OO_CONFIRMED;
-	nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &oc->oc_resp_stateid, &stp->st_stid);
 	mutex_unlock(&stp->st_mutex);
 	trace_nfsd_open_confirm(oc->oc_seqid, &stp->st_stid.sc_stateid);
 	nfsd4_client_record_create(oo->oo_owner.so_client);
@@ -7760,7 +7767,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
 	}
 	nfs4_stateid_downgrade(stp, od->od_share_access);
 	reset_union_bmap_deny(od->od_share_deny, stp);
-	nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &od->od_stateid, &stp->st_stid);
 	status = nfs_ok;
 put_stateid:
 	mutex_unlock(&stp->st_mutex);
@@ -7830,7 +7837,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * copied value below, but we continue to do so here just to ensure
 	 * that racing ops see that there was a state change.
 	 */
-	nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &close->cl_stateid, &stp->st_stid);
 
 	need_move_to_close_list = nfsd4_close_open_stateid(stp);
 	mutex_unlock(&stp->st_mutex);
@@ -7845,6 +7852,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * See RFC5661 section 18.2.4, and RFC7530 section 16.2.5
 	 */
 	memcpy(&close->cl_stateid, &invalid_stateid, sizeof(close->cl_stateid));
+	cstate->current_fh.fh_have_stateid = false;
 
 	/* put reference from nfs4_preprocess_seqid_op */
 	nfs4_put_stid(&stp->st_stid);
@@ -8419,7 +8427,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	err = vfs_lock_file(nf->nf_file, F_SETLK, file_lock, conflock);
 	switch (err) {
 	case 0: /* success! */
-		nfs4_inc_and_copy_stateid(&lock->lk_resp_stateid, &lock_stp->st_stid);
+		nfs4_inc_and_copy_stateid(cstate, &lock->lk_resp_stateid, &lock_stp->st_stid);
 		status = 0;
 		if (lock->lk_reclaim)
 			nn->somebody_reclaimed = true;
@@ -8664,7 +8672,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		dprintk("NFSD: nfs4_locku: vfs_lock_file failed!\n");
 		goto out_nfserr;
 	}
-	nfs4_inc_and_copy_stateid(&locku->lu_stateid, &stp->st_stid);
+	nfs4_inc_and_copy_stateid(cstate, &locku->lu_stateid, &stp->st_stid);
 put_file:
 	nfsd_file_put(nf);
 put_stateid:
@@ -9113,44 +9121,6 @@ nfs4_state_shutdown(void)
 	shrinker_free(nfsd_slot_shrinker);
 }
 
-static void
-put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
-{
-	memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
-	cstate->current_fh.fh_have_stateid = true;
-}
-
-/*
- * functions to set current state id
- */
-void
-nfsd4_set_opendowngradestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->open_downgrade.od_stateid);
-}
-
-void
-nfsd4_set_openstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->open.op_stateid);
-}
-
-void
-nfsd4_set_closestateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->close.cl_stateid);
-}
-
-void
-nfsd4_set_lockstateid(struct nfsd4_compound_state *cstate,
-		union nfsd4_op_u *u)
-{
-	put_stateid(cstate, &u->lock.lk_resp_stateid);
-}
-
 /**
  * nfsd4_vet_deleg_time - vet and set the timespec for a delegated timestamp update
  * @req: timestamp from the client
diff --git a/fs/nfsd/pnfs.h b/fs/nfsd/pnfs.h
index db9af780438b..1714471d173d 100644
--- a/fs/nfsd/pnfs.h
+++ b/fs/nfsd/pnfs.h
@@ -56,8 +56,9 @@ extern const struct nfsd4_layout_ops ff_layout_ops;
 __be32 nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate, stateid_t *stateid,
 		bool create, u32 layout_type, struct nfs4_layout_stateid **lsp);
-__be32 nfsd4_insert_layout(struct nfsd4_layoutget *lgp,
-		struct nfs4_layout_stateid *ls);
+__be32 nfsd4_insert_layout(struct nfsd4_compound_state *cstate,
+			   struct nfsd4_layoutget *lgp,
+			   struct nfs4_layout_stateid *ls);
 __be32 nfsd4_return_file_layouts(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *cstate,
 		struct nfsd4_layoutreturn *lrp);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1e736f402426..c6e97d6daa5f 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -797,7 +797,8 @@ void nfs4_free_copy_state(struct nfsd4_copy *copy);
 struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
 			struct nfs4_stid *p_stid);
 void nfs4_put_stid(struct nfs4_stid *s);
-void nfs4_inc_and_copy_stateid(stateid_t *dst, struct nfs4_stid *stid);
+void nfs4_inc_and_copy_stateid(struct nfsd4_compound_state *cstate,
+			       stateid_t *dst, struct nfs4_stid *stid);
 void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *, struct nfsd_net *);
 extern void nfs4_release_reclaim(struct nfsd_net *);
 extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct xdr_netobj name,
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4f54108f24f9..ad1b96f50131 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -965,6 +965,7 @@ __be32 nfsd4_reclaim_complete(struct svc_rqst *, struct nfsd4_compound_state *,
 extern __be32 nfsd4_process_open1(struct nfsd4_compound_state *,
 		struct nfsd4_open *open, struct nfsd_net *nn);
 extern __be32 nfsd4_process_open2(struct svc_rqst *rqstp,
+				  struct nfsd4_compound_state *cstate,
 				  struct svc_fh *current_fh,
 				  struct svc_fh *parent_fh,
 				  struct nfsd4_open *open);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 13/14] nfsd: discard current_stateid.h

[NeilBrown]

From: NeilBrown <neil@brown.name>

current_stateid.h no longer contains anything useful.  So remove it.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/current_stateid.h | 8 --------
 fs/nfsd/nfs4proc.c        | 1 -
 fs/nfsd/nfs4state.c       | 1 -
 3 files changed, 10 deletions(-)
 delete mode 100644 fs/nfsd/current_stateid.h

diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
deleted file mode 100644
index 9dce3004b846..000000000000
--- a/fs/nfsd/current_stateid.h
+++ /dev/null
@@ -1,8 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _NFSD4_CURRENT_STATE_H
-#define _NFSD4_CURRENT_STATE_H
-
-#include "state.h"
-#include "xdr4.h"
-
-#endif   /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 806306f48043..49700e85761f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -46,7 +46,6 @@
 #include "cache.h"
 #include "xdr4.h"
 #include "vfs.h"
-#include "current_stateid.h"
 #include "netns.h"
 #include "acl.h"
 #include "pnfs.h"
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e3b38245ba45..67afa253f408 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -50,7 +50,6 @@
 #include "xdr4.h"
 #include "xdr4cb.h"
 #include "vfs.h"
-#include "current_stateid.h"
 
 #include "netns.h"
 #include "pnfs.h"
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v6 14/14] nfsd: conditionally clear seqid when current_stateid is used.

[NeilBrown]

From: NeilBrown <neil@brown.name>

As described in RFC 8881 scions 8.2.3 on Special Stateids:

    The stateid passed to the operation in place of the special value
    has its "seqid" value set to zero, except when the current stateid
    is used by the operation CLOSE or OPEN_DOWNGRADE.

Linux NFSD does not currently follow this guidance.  The seqid (known as
si_generation) is left unchanged.

This patch introduces a new status flag SC_STATUS_KEEP_SEQID which is
only used for lookup requests and sets it for the two exceptions: CLOSE
and OPEN_DOWNGRADE.  When this flag is not present, the value copied
from the current stateid has the si_generation (aka seqid) cleared.

A situation where this might produce a different result is if an
  OPEN WRITE
request which makes use for current_stateid for the WRITE races with
another request which OPENs the same file (possibly via different name)
and that OPEN happens between the OPEN and WRITE in the first request.
The second OPEN would update the seqid on the stateid of that file and
prior to this patch, the WRITE would fail.  After this patch the WRITE
will succeed.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4state.c | 24 +++++++++++++++---------
 fs/nfsd/state.h     |  6 ++++++
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 67afa253f408..c1dcc332b22c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7201,6 +7201,8 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		if (!cstate->current_fh.fh_have_stateid)
 			return nfserr_bad_stateid;
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+		if (!(statusmask & SC_STATUS_KEEP_SEQID))
+			stateid->si_generation = 0;
 	}
 	/*
 	 *  only return revoked delegations if explicitly asked.
@@ -7522,6 +7524,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		if (!cstate->current_fh.fh_have_stateid)
 			return nfserr_bad_stateid;
 		memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
+		stateid->si_generation = 0;
 	}
 
 	spin_lock(&cl->cl_lock);
@@ -7644,15 +7647,17 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
 	return status;
 }
 
-static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
-						 stateid_t *stateid, struct nfs4_ol_stateid **stpp, struct nfsd_net *nn)
+static __be32
+nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
+				   stateid_t *stateid, struct nfs4_ol_stateid **stpp,
+				   struct nfsd_net *nn, unsigned short statusmask)
 {
 	__be32 status;
 	struct nfs4_openowner *oo;
 	struct nfs4_ol_stateid *stp;
 
 	status = nfs4_preprocess_seqid_op(cstate, seqid, stateid,
-					  SC_TYPE_OPEN, 0, &stp, nn);
+					  SC_TYPE_OPEN, statusmask, &stp, nn);
 	if (status)
 		return status;
 	oo = openowner(stp->st_stateowner);
@@ -7750,7 +7755,8 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
 			od->od_deleg_want);
 
 	status = nfs4_preprocess_confirmed_seqid_op(cstate, od->od_seqid,
-					&od->od_stateid, &stp, nn);
+						    &od->od_stateid, &stp, nn,
+						    SC_STATUS_KEEP_SEQID);
 	if (status)
 		goto out; 
 	status = nfserr_inval;
@@ -7820,7 +7826,8 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	status = nfs4_preprocess_seqid_op(cstate, close->cl_seqid,
 					  &close->cl_stateid,
-					  SC_TYPE_OPEN, SC_STATUS_CLOSED,
+					  SC_TYPE_OPEN,
+					  SC_STATUS_CLOSED | SC_STATUS_KEEP_SEQID,
 					  &stp, nn);
 	nfsd4_bump_seqid(cstate, status);
 	if (status)
@@ -8312,10 +8319,9 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 				sizeof(clientid_t));
 
 		/* validate and update open stateid and open seqid */
-		status = nfs4_preprocess_confirmed_seqid_op(cstate,
-				        lock->lk_new_open_seqid,
-		                        &lock->lk_new_open_stateid,
-					&open_stp, nn);
+		status = nfs4_preprocess_confirmed_seqid_op(
+			cstate,	lock->lk_new_open_seqid,
+			&lock->lk_new_open_stateid, &open_stp, nn, 0);
 		if (status)
 			goto out;
 		mutex_unlock(&open_stp->st_mutex);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index c6e97d6daa5f..6043baf10ceb 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -138,6 +138,12 @@ struct nfs4_stid {
 #define SC_STATUS_ADMIN_REVOKED	BIT(2)
 #define SC_STATUS_FREEABLE	BIT(3)
 #define SC_STATUS_FREED		BIT(4)
+/*
+ * Ops other than CLOSE and OPEN_DOWNGRADE which use the "current stateid"
+ * must clear the seqid (aka si_generation). Following flag is never stored
+ * in states but is passed through to request the seq not be cleared.
+ */
+#define SC_STATUS_KEEP_SEQID	BIT(5)
 	unsigned short		sc_status;
 
 	struct list_head	sc_cp_list;
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v6 01/14] nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and revise use

[Chuck Lever]

On Sat, Nov 22, 2025 at 11:46:59AM +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsdv4 OPs which do not have ALLOWED_WITHOUT_FH can assume that a PUTFH
> has been called and may actually assume that current_fh->fh_dentry is
> non-NULL.  nfsd4_setattr(), for example, assumes fh_dentry != NULL.
> 
> However the possibility of foreign filehandles (needed for v4.2 COPY)
> means that there may be a filehandle present while fh_dentry is NULL.
> 
> Sending a COMPOUND containing:
>    SEQUENCE
>    PUTFH - foreign filehandle
>    SETATTR - new mode
>    SAVEFH
>    COPY - with non-empty server list
> 
> to an NFS server with inter-server copy enabled will cause a NULL
> pointer dereference when nfsd4_setattr() calls fh_want_write().

The short description for 1/14 is:

  nfsd: rename ALLOWED_WITHOUT_FH to ALLOWED_WITHOUT_LOCAL_FH and
    revise use

But here the description claims to be fixing a NULL pointer
dereference. Let's help out our sustaining engineers and make
this "fixes" statement the main subject. That also makes it
clear why there is a Fixes: tag on this patch.


> Most NFSv4 OPs actually want a "local" filehandle, not just any
> filehandle.  So this patch renames ALLOWED_WITHOUT_FH to
> ALLOWED_WITHOUT_LOCAL_FH and sets it on all the other which DON'T

s/the other/operations/


> require a local filehandle.  That is: all that don't require any
> filehandle together with SAVEFH, which is the only OP which needs to
> handle a foreign current_fh.  (COPY must handle a foreign save_fh, but
> all OPs which access save_fh already do any required validity tests
> themselves).

This arrangement strikes me as a little confusing. Why not leave
ALLOWED_WITHOUT_FH but add ALLOWED_WITH_ANY_FH just for SAVE_FH?


> nfsd4_savefh() is changed to validate the filehandle itself as the
> caller no longer validates it.

"the caller" is... nfsd4_proc_compound ? Since this lone caller uses
an indirect function call, it would be friendlier (grep-wise) for
the patch description to refer to nfsd4_proc_compound by name.


> nfsd4_proc_compound no longer allows ops without
> ALLOWED_WITHOUT_LOCAL_FH to be called with a foreign fh - current_fh
> must be local and ->fh_dentry must be non-NULL.  This protects
> nfsd4_setattr() and any others that might use ->fh_dentry without
> checking.
> 
> The
>        current_fh->fh_export &&
> test is removed from an "else if" because that condition is now only
> tested when current_fh->fh_dentry is not NULL, and in that case
> current_fh->fh_export is also guaranteed to not be NULL.
> 
> Fixes: b9e8638e3d9e ("NFSD: allow inter server COPY to have a STALE source server fh")

Since this patch addresses an NPD, we want to consider it for LTS
backport (ie, add "Cc: stable").

However, looking at 2/14:

> In NFSv4.1, SECINFO and similar OPs can consume current_fh, but don't
> currently set fh_size to zero.  So the above rule for filehandle type
> determination isn't currently accurate.  This is fixed by enhancing
> fh_put() to clear fhp->fh_handle.fh_size.

I'm getting tripped up by "the above rule ... isn't currently
accurate." Is this an existing behavior upstream, or is it a
behavior that is introduced by 1/14 ? If the latter, then maybe
hunks 1, 2, and 3 of 2/14 need to be squashed into 1/14.


> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neil@brown.name>
> 
> ---
> changes since v5
>  - fix compile failure: need cstate->current_fh
>  - comment for ALLOWED_WITHOUT_LOCAL_FH improved
> ---
>  fs/nfsd/nfs4proc.c | 59 ++++++++++++++++++++++++++--------------------
>  fs/nfsd/xdr4.h     |  2 +-
>  2 files changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index dcad50846a97..947b1b6d2282 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -729,6 +729,16 @@ static __be32
>  nfsd4_savefh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	     union nfsd4_op_u *u)
>  {
> +	/*
> +	 * SAVEFH is "ALLOWED_WITHOUT_LOCAL_FH" in that
> +	 * current_fh.fh_dentry is not required, those some fh_handle

s/those/though


> +	 * *is* required so that a foreign fh can be saved as needed for
> +	 * inter-server COPY.
> +	 */
> +	if (!cstate->current_fh.fh_dentry &&
> +	    !HAS_FH_FLAG(&cstate->current_fh, NFSD4_FH_FOREIGN))
> +		return nfserr_nofilehandle;
> +
>  	fh_dup2(&cstate->save_fh, &cstate->current_fh);
>  	if (HAS_CSTATE_FLAG(cstate, CURRENT_STATE_ID_FLAG)) {
>  		memcpy(&cstate->save_stateid, &cstate->current_stateid, sizeof(stateid_t));
> @@ -2919,14 +2929,12 @@ nfsd4_proc_compound(struct svc_rqst *rqstp)
>  				op->status = nfsd4_open_omfg(rqstp, cstate, op);
>  			goto encode_op;
>  		}
> -		if (!current_fh->fh_dentry &&
> -				!HAS_FH_FLAG(current_fh, NFSD4_FH_FOREIGN)) {
> -			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
> +		if (!current_fh->fh_dentry) {
> +			if (!(op->opdesc->op_flags & ALLOWED_WITHOUT_LOCAL_FH)) {
>  				op->status = nfserr_nofilehandle;
>  				goto encode_op;
>  			}
> -		} else if (current_fh->fh_export &&
> -			   current_fh->fh_export->ex_fslocs.migrated &&
> +		} else if (current_fh->fh_export->ex_fslocs.migrated &&
>  			  !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
>  			op->status = nfserr_moved;
>  			goto encode_op;
> @@ -3507,21 +3515,21 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_PUTFH] = {
>  		.op_func = nfsd4_putfh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
>  		.op_name = "OP_PUTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTPUBFH] = {
>  		.op_func = nfsd4_putrootfh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
>  		.op_name = "OP_PUTPUBFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_PUTROOTFH] = {
>  		.op_func = nfsd4_putrootfh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_CLEAR_STATEID,
>  		.op_name = "OP_PUTROOTFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3557,7 +3565,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_RENEW] = {
>  		.op_func = nfsd4_renew,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RENEW",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3565,14 +3573,15 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_RESTOREFH] = {
>  		.op_func = nfsd4_restorefh,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_IS_PUTFH_LIKE | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RESTOREFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_SAVEFH] = {
>  		.op_func = nfsd4_savefh,
> -		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH
> +				| OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_SAVEFH",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> @@ -3593,7 +3602,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_SETCLIENTID] = {
>  		.op_func = nfsd4_setclientid,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING | OP_CACHEME
>  				| OP_NONTRIVIAL_ERROR_ENCODE,
>  		.op_name = "OP_SETCLIENTID",
> @@ -3601,7 +3610,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_SETCLIENTID_CONFIRM] = {
>  		.op_func = nfsd4_setclientid_confirm,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING | OP_CACHEME,
>  		.op_name = "OP_SETCLIENTID_CONFIRM",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3620,7 +3629,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_RELEASE_LOCKOWNER] = {
>  		.op_func = nfsd4_release_lockowner,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_ON_ABSENT_FS
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_ON_ABSENT_FS
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RELEASE_LOCKOWNER",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3630,54 +3639,54 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	[OP_EXCHANGE_ID] = {
>  		.op_func = nfsd4_exchange_id,
>  		.op_release = nfsd4_exchange_id_release,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_EXCHANGE_ID",
>  		.op_rsize_bop = nfsd4_exchange_id_rsize,
>  	},
>  	[OP_BACKCHANNEL_CTL] = {
>  		.op_func = nfsd4_backchannel_ctl,
> -		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_BACKCHANNEL_CTL",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_BIND_CONN_TO_SESSION] = {
>  		.op_func = nfsd4_bind_conn_to_session,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_BIND_CONN_TO_SESSION",
>  		.op_rsize_bop = nfsd4_bind_conn_to_session_rsize,
>  	},
>  	[OP_CREATE_SESSION] = {
>  		.op_func = nfsd4_create_session,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_CREATE_SESSION",
>  		.op_rsize_bop = nfsd4_create_session_rsize,
>  	},
>  	[OP_DESTROY_SESSION] = {
>  		.op_func = nfsd4_destroy_session,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_DESTROY_SESSION",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_SEQUENCE] = {
>  		.op_func = nfsd4_sequence,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP,
>  		.op_name = "OP_SEQUENCE",
>  		.op_rsize_bop = nfsd4_sequence_rsize,
>  	},
>  	[OP_DESTROY_CLIENTID] = {
>  		.op_func = nfsd4_destroy_clientid,
> -		.op_flags = ALLOWED_WITHOUT_FH | ALLOWED_AS_FIRST_OP
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | ALLOWED_AS_FIRST_OP
>  				| OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_DESTROY_CLIENTID",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
>  	[OP_RECLAIM_COMPLETE] = {
>  		.op_func = nfsd4_reclaim_complete,
> -		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_RECLAIM_COMPLETE",
>  		.op_rsize_bop = nfsd4_only_status_rsize,
>  	},
> @@ -3690,13 +3699,13 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	},
>  	[OP_TEST_STATEID] = {
>  		.op_func = nfsd4_test_stateid,
> -		.op_flags = ALLOWED_WITHOUT_FH,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
>  		.op_name = "OP_TEST_STATEID",
>  		.op_rsize_bop = nfsd4_test_stateid_rsize,
>  	},
>  	[OP_FREE_STATEID] = {
>  		.op_func = nfsd4_free_stateid,
> -		.op_flags = ALLOWED_WITHOUT_FH | OP_MODIFIES_SOMETHING,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH | OP_MODIFIES_SOMETHING,
>  		.op_name = "OP_FREE_STATEID",
>  		.op_get_currentstateid = nfsd4_get_freestateid,
>  		.op_rsize_bop = nfsd4_only_status_rsize,
> @@ -3711,7 +3720,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
>  	[OP_GETDEVICEINFO] = {
>  		.op_func = nfsd4_getdeviceinfo,
>  		.op_release = nfsd4_getdeviceinfo_release,
> -		.op_flags = ALLOWED_WITHOUT_FH,
> +		.op_flags = ALLOWED_WITHOUT_LOCAL_FH,
>  		.op_name = "OP_GETDEVICEINFO",
>  		.op_rsize_bop = nfsd4_getdeviceinfo_rsize,
>  	},
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ae75846b3cd7..e27258e694a9 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -1006,7 +1006,7 @@ extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
>  extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
>  
>  enum nfsd4_op_flags {
> -	ALLOWED_WITHOUT_FH = 1 << 0,    /* No current filehandle required */
> +	ALLOWED_WITHOUT_LOCAL_FH = 1 << 0,    /* No fh_dentry needed in current filehandle */
>  	ALLOWED_ON_ABSENT_FS = 1 << 1,  /* ops processed on absent fs */
>  	ALLOWED_AS_FIRST_OP = 1 << 2,   /* ops reqired first in compound */
>  	/* For rfc 5661 section 2.6.3.1.1: */
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 
> 

-- 
Chuck Lever

Re: [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY

[Chuck Lever]

On Sat, Nov 22, 2025 at 11:47:02AM +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> RFC-7862 acknowledges that a filehandle provided as the source of an
> inter-server copy might result in NFS4ERR_STALE when given to PUTFH, and
> gives guidance on how this error can be ignored (section 15.2.3).
> 
> NFS4ERR_BADHANDLE is also a possible error in this circumstance if the
> foreign server is running a different implementation of NFS than the
> current one.

The RFC uses the terms "source" and "destination" server, fwiw.

It would be interesting to see if nfserr_badhandle can be triggered
during an NFSD <-> NFSD copy.


> This appears to be a simple omission in the RFC.

Perhaps. It might also be the result of the RFC authors giving
implementers flexibility to innovate.

I would like to consult with the WG and possibly file an errata, or
add this observation to the "NFSv4.2 COPY implementation experience"
document I'm helping Olga with:

https://datatracker.ietf.org/doc/draft-cel-nfsv4-copy-implementation-experience/

They might want to consider NFS4ERR_MOVED as well.


> There can be no harm in delaying a BADHANDLE error in the same situation
> where we already delay STALE errors, and no harm in sending a locally
> "bad" handle to a foreign server to request a COPY.

These are plausible claims. But IMO, they need firmer rationale.


> So extend the test in nfsd4_putfh to also check for nfserr_badhandle.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 112e62b6b9c6..ae34b816371c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -693,7 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	       putfh->pf_fhlen);
>  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> -	if (ret == nfserr_stale && inter_copy_offload_enable) {
> +	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
> +	    inter_copy_offload_enable) {
>  		struct nfsd4_compoundargs *args = rqstp->rq_argp;
>  		struct nfsd4_compoundres *resp = rqstp->rq_resp;
>  
> @@ -713,7 +714,11 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  			 *  NOT return NFS4ERR_STALE for either
>  			 *  operation.
>  			 * We limit this to when there is a COPY
> -			 * in the COMPOUND.
> +			 * in the COMPOUND, and extend it to
> +			 * also ignore NFS4ERR_BADHANDLE despite the
> +			 * RFC not requiring this.  If the remote
> +			 * server is running a different NFS implementation,
> +			 * NFS4ERR_BADHANDLE is a likely error.
>  			 */
>  			ret = 0;
>  		}
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 
> 

-- 
Chuck Lever

Re: [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY

[Chuck Lever]

On Sat, Nov 22, 2025 at 04:16:02PM -0500, Chuck Lever wrote:
> On Sat, Nov 22, 2025 at 11:47:02AM +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > RFC-7862 acknowledges that a filehandle provided as the source of an
> > inter-server copy might result in NFS4ERR_STALE when given to PUTFH, and
> > gives guidance on how this error can be ignored (section 15.2.3).
> > 
> > NFS4ERR_BADHANDLE is also a possible error in this circumstance if the
> > foreign server is running a different implementation of NFS than the
> > current one.
> 
> The RFC uses the terms "source" and "destination" server, fwiw.
> 
> It would be interesting to see if nfserr_badhandle can be triggered
> during an NFSD <-> NFSD copy.
> 
> 
> > This appears to be a simple omission in the RFC.
> 
> Perhaps. It might also be the result of the RFC authors giving
> implementers flexibility to innovate.
> 
> I would like to consult with the WG and possibly file an errata, or
> add this observation to the "NFSv4.2 COPY implementation experience"
> document I'm helping Olga with:
> 
> https://datatracker.ietf.org/doc/draft-cel-nfsv4-copy-implementation-experience/
> 
> They might want to consider NFS4ERR_MOVED as well.
> 
> 
> > There can be no harm in delaying a BADHANDLE error in the same situation
> > where we already delay STALE errors, and no harm in sending a locally
> > "bad" handle to a foreign server to request a COPY.
> 
> These are plausible claims. But IMO, they need firmer rationale.

My comments aren't terribly clear about this, but I agree that PUTFH
handling NFS4ERR_BADHANDLE is a concern when there is a subsequent
COPY operation in the COMPOUND.

I think we can't rely on the letter of RFC 7862, as you pointed out.
However, the spirit of RFC 7862 is that PUTFH mustn't fail if a
client presents a foreign file handle via a PUTFH that precedes a
COPY operation. The lack of BCP14 language specific to other status
codes shouldn't stop NFSD from handling other status codes as it
needs to (in fact, the absence of such language affords us the
opportunity of doing exactly that).

The reason for this potential PUTFH failure is an artifact of NFSD's
PUTFH implementation, which unconditionally invokes fh_verify.

I'm wondering if PUTFH needs to check for /specific/ status codes
from fh_verify before looking ahead in the incoming COMPOUND, or if
it ought to look ahead on /any/ failure of fh_verify.

And, is this patch a fix that needs to be backported to LTS kernels?
If so, then perhaps it needs to go before the previous patch.

(It seems like NFSv4.2 could have added a PUT_FOREIGN_FH operation
to avoid all this nonsense).

(I'll also note that now that our MAX_OPS_PER_COMPOUND has increased
and might increase again, moving the COPY look ahead checking out of
nfsd4_proc_compound is a huge bonus for handling large COMPOUNDs).


> > So extend the test in nfsd4_putfh to also check for nfserr_badhandle.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4proc.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 112e62b6b9c6..ae34b816371c 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -693,7 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	       putfh->pf_fhlen);
> >  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > -	if (ret == nfserr_stale && inter_copy_offload_enable) {
> > +	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
> > +	    inter_copy_offload_enable) {
> >  		struct nfsd4_compoundargs *args = rqstp->rq_argp;
> >  		struct nfsd4_compoundres *resp = rqstp->rq_resp;
> >  
> > @@ -713,7 +714,11 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  			 *  NOT return NFS4ERR_STALE for either
> >  			 *  operation.
> >  			 * We limit this to when there is a COPY
> > -			 * in the COMPOUND.
> > +			 * in the COMPOUND, and extend it to
> > +			 * also ignore NFS4ERR_BADHANDLE despite the
> > +			 * RFC not requiring this.  If the remote
> > +			 * server is running a different NFS implementation,
> > +			 * NFS4ERR_BADHANDLE is a likely error.
> >  			 */
> >  			ret = 0;
> >  		}
> > -- 
> > 2.50.0.107.gf914562f5916.dirty
> > 
> > 
> 
> -- 
> Chuck Lever
> 

-- 
Chuck Lever

Re: [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY

[NeilBrown]

On Mon, 24 Nov 2025, Chuck Lever wrote:
> On Sat, Nov 22, 2025 at 04:16:02PM -0500, Chuck Lever wrote:
> > On Sat, Nov 22, 2025 at 11:47:02AM +1100, NeilBrown wrote:
> > > From: NeilBrown <neil@brown.name>
> > > 
> > > RFC-7862 acknowledges that a filehandle provided as the source of an
> > > inter-server copy might result in NFS4ERR_STALE when given to PUTFH, and
> > > gives guidance on how this error can be ignored (section 15.2.3).
> > > 
> > > NFS4ERR_BADHANDLE is also a possible error in this circumstance if the
> > > foreign server is running a different implementation of NFS than the
> > > current one.
> > 
> > The RFC uses the terms "source" and "destination" server, fwiw.
> > 
> > It would be interesting to see if nfserr_badhandle can be triggered
> > during an NFSD <-> NFSD copy.
> > 
> > 
> > > This appears to be a simple omission in the RFC.
> > 
> > Perhaps. It might also be the result of the RFC authors giving
> > implementers flexibility to innovate.
> > 
> > I would like to consult with the WG and possibly file an errata, or
> > add this observation to the "NFSv4.2 COPY implementation experience"
> > document I'm helping Olga with:
> > 
> > https://datatracker.ietf.org/doc/draft-cel-nfsv4-copy-implementation-experience/
> > 
> > They might want to consider NFS4ERR_MOVED as well.
> > 
> > 
> > > There can be no harm in delaying a BADHANDLE error in the same situation
> > > where we already delay STALE errors, and no harm in sending a locally
> > > "bad" handle to a foreign server to request a COPY.
> > 
> > These are plausible claims. But IMO, they need firmer rationale.
> 
> My comments aren't terribly clear about this, but I agree that PUTFH
> handling NFS4ERR_BADHANDLE is a concern when there is a subsequent
> COPY operation in the COMPOUND.
> 
> I think we can't rely on the letter of RFC 7862, as you pointed out.
> However, the spirit of RFC 7862 is that PUTFH mustn't fail if a
> client presents a foreign file handle via a PUTFH that precedes a
> COPY operation. The lack of BCP14 language specific to other status
> codes shouldn't stop NFSD from handling other status codes as it
> needs to (in fact, the absence of such language affords us the
> opportunity of doing exactly that).
> 
> The reason for this potential PUTFH failure is an artifact of NFSD's
> PUTFH implementation, which unconditionally invokes fh_verify.

RFC8881 says very little in the description of PUTFH.
However the section on error codes say:

 15.1.2.1. NFS4ERR_BADHANDLE (Error Code 10001)

    Illegal NFS filehandle for the current server.  The current
    filehandle failed internal consistency checks.  Once accepted as
    valid (by PUTFH), no subsequent status change can cause the
    filehandle to generate this error. 

This implies to me that if PUTFH accepts a filehandle, then it is
"accepted as valid" and that cannot change.  So no other op can claim
BADHANDLE if PUTFH didn't.  But other OPs do have BADHANDLE as a valid
error.

This text also says the "current filehandle" failed, so that implies
something that has already been accepted by PUTFH (or generated by
LOOKUP???).

So I think we are in agreement that the RFC isn't crystal clear but
doesn't put any barriers in the way of us making this change.

> 
> I'm wondering if PUTFH needs to check for /specific/ status codes
> from fh_verify before looking ahead in the incoming COMPOUND, or if
> it ought to look ahead on /any/ failure of fh_verify.

I don't have a strong opinion.
fh_verify() can also generate
  nfserr_nofilehandle
  nfserr_jukebox
  nfserr_perm

nferr_perm could arguably be handled the same way that nfserr_stale is
handled.

> 
> And, is this patch a fix that needs to be backported to LTS kernels?
> If so, then perhaps it needs to go before the previous patch.

It doesn't fix a regression, a security hole, and data corruption, a
kernel panic.  So I don't think it needs to be backported (others might
think otherwise).

I think the failure mode is that the client would fallback to handling
the copy like it would for v4.1.

> 
> (It seems like NFSv4.2 could have added a PUT_FOREIGN_FH operation
> to avoid all this nonsense).

That would make a lot of sense!

> 
> (I'll also note that now that our MAX_OPS_PER_COMPOUND has increased
> and might increase again, moving the COPY look ahead checking out of
> nfsd4_proc_compound is a huge bonus for handling large COMPOUNDs).

true

NeilBrown

> 
> 
> > > So extend the test in nfsd4_putfh to also check for nfserr_badhandle.
> > > 
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  fs/nfsd/nfs4proc.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 112e62b6b9c6..ae34b816371c 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -693,7 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  	       putfh->pf_fhlen);
> > >  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
> > >  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
> > > -	if (ret == nfserr_stale && inter_copy_offload_enable) {
> > > +	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
> > > +	    inter_copy_offload_enable) {
> > >  		struct nfsd4_compoundargs *args = rqstp->rq_argp;
> > >  		struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > >  
> > > @@ -713,7 +714,11 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >  			 *  NOT return NFS4ERR_STALE for either
> > >  			 *  operation.
> > >  			 * We limit this to when there is a COPY
> > > -			 * in the COMPOUND.
> > > +			 * in the COMPOUND, and extend it to
> > > +			 * also ignore NFS4ERR_BADHANDLE despite the
> > > +			 * RFC not requiring this.  If the remote
> > > +			 * server is running a different NFS implementation,
> > > +			 * NFS4ERR_BADHANDLE is a likely error.
> > >  			 */
> > >  			ret = 0;
> > >  		}
> > > -- 
> > > 2.50.0.107.gf914562f5916.dirty
> > > 
> > > 
> > 
> > -- 
> > Chuck Lever
> > 
> 
> -- 
> Chuck Lever
> 
> 

Re: [PATCH v6 04/14] nfsd: allow unrecognisable filehandle for foreign servers in COPY

[Chuck Lever]

On 11/26/25 7:55 PM, NeilBrown wrote:
> On Mon, 24 Nov 2025, Chuck Lever wrote:
>> On Sat, Nov 22, 2025 at 04:16:02PM -0500, Chuck Lever wrote:
>>> On Sat, Nov 22, 2025 at 11:47:02AM +1100, NeilBrown wrote:
>>>> From: NeilBrown <neil@brown.name>
>>>>
>>>> RFC-7862 acknowledges that a filehandle provided as the source of an
>>>> inter-server copy might result in NFS4ERR_STALE when given to PUTFH, and
>>>> gives guidance on how this error can be ignored (section 15.2.3).
>>>>
>>>> NFS4ERR_BADHANDLE is also a possible error in this circumstance if the
>>>> foreign server is running a different implementation of NFS than the
>>>> current one.
>>>
>>> The RFC uses the terms "source" and "destination" server, fwiw.
>>>
>>> It would be interesting to see if nfserr_badhandle can be triggered
>>> during an NFSD <-> NFSD copy.
>>>
>>>
>>>> This appears to be a simple omission in the RFC.
>>>
>>> Perhaps. It might also be the result of the RFC authors giving
>>> implementers flexibility to innovate.
>>>
>>> I would like to consult with the WG and possibly file an errata, or
>>> add this observation to the "NFSv4.2 COPY implementation experience"
>>> document I'm helping Olga with:
>>>
>>> https://datatracker.ietf.org/doc/draft-cel-nfsv4-copy-implementation-experience/
>>>
>>> They might want to consider NFS4ERR_MOVED as well.
>>>
>>>
>>>> There can be no harm in delaying a BADHANDLE error in the same situation
>>>> where we already delay STALE errors, and no harm in sending a locally
>>>> "bad" handle to a foreign server to request a COPY.
>>>
>>> These are plausible claims. But IMO, they need firmer rationale.
>>
>> My comments aren't terribly clear about this, but I agree that PUTFH
>> handling NFS4ERR_BADHANDLE is a concern when there is a subsequent
>> COPY operation in the COMPOUND.
>>
>> I think we can't rely on the letter of RFC 7862, as you pointed out.
>> However, the spirit of RFC 7862 is that PUTFH mustn't fail if a
>> client presents a foreign file handle via a PUTFH that precedes a
>> COPY operation. The lack of BCP14 language specific to other status
>> codes shouldn't stop NFSD from handling other status codes as it
>> needs to (in fact, the absence of such language affords us the
>> opportunity of doing exactly that).
>>
>> The reason for this potential PUTFH failure is an artifact of NFSD's
>> PUTFH implementation, which unconditionally invokes fh_verify.
> 
> RFC8881 says very little in the description of PUTFH.
> However the section on error codes say:
> 
>  15.1.2.1. NFS4ERR_BADHANDLE (Error Code 10001)
> 
>     Illegal NFS filehandle for the current server.  The current
>     filehandle failed internal consistency checks.  Once accepted as
>     valid (by PUTFH), no subsequent status change can cause the
>     filehandle to generate this error. 
> 
> This implies to me that if PUTFH accepts a filehandle, then it is
> "accepted as valid" and that cannot change.  So no other op can claim
> BADHANDLE if PUTFH didn't.  But other OPs do have BADHANDLE as a valid
> error.
> 
> This text also says the "current filehandle" failed, so that implies
> something that has already been accepted by PUTFH (or generated by
> LOOKUP???).
> 
> So I think we are in agreement that the RFC isn't crystal clear but
> doesn't put any barriers in the way of us making this change.
> 
>>
>> I'm wondering if PUTFH needs to check for /specific/ status codes
>> from fh_verify before looking ahead in the incoming COMPOUND, or if
>> it ought to look ahead on /any/ failure of fh_verify.
> 
> I don't have a strong opinion.
> fh_verify() can also generate
>   nfserr_nofilehandle
>   nfserr_jukebox
>   nfserr_perm
> 
> nferr_perm could arguably be handled the same way that nfserr_stale is
> handled.
> 
>>
>> And, is this patch a fix that needs to be backported to LTS kernels?
>> If so, then perhaps it needs to go before the previous patch.
> 
> It doesn't fix a regression, a security hole, and data corruption, a
> kernel panic.  So I don't think it needs to be backported (others might
> think otherwise).
> 
> I think the failure mode is that the client would fallback to handling
> the copy like it would for v4.1.
> 
>>
>> (It seems like NFSv4.2 could have added a PUT_FOREIGN_FH operation
>> to avoid all this nonsense).
> 
> That would make a lot of sense!
> 
>>
>> (I'll also note that now that our MAX_OPS_PER_COMPOUND has increased
>> and might increase again, moving the COPY look ahead checking out of
>> nfsd4_proc_compound is a huge bonus for handling large COMPOUNDs).
> 
> true
> 
> NeilBrown
> 
>>
>>
>>>> So extend the test in nfsd4_putfh to also check for nfserr_badhandle.
>>>>
>>>> Signed-off-by: NeilBrown <neil@brown.name>
>>>> ---
>>>>  fs/nfsd/nfs4proc.c | 9 +++++++--
>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index 112e62b6b9c6..ae34b816371c 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -693,7 +693,8 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>  	       putfh->pf_fhlen);
>>>>  	ret = fh_verify(rqstp, &cstate->current_fh, 0, NFSD_MAY_BYPASS_GSS);
>>>>  #ifdef CONFIG_NFSD_V4_2_INTER_SSC
>>>> -	if (ret == nfserr_stale && inter_copy_offload_enable) {
>>>> +	if ((ret == nfserr_badhandle || ret == nfserr_stale) &&
>>>> +	    inter_copy_offload_enable) {
>>>>  		struct nfsd4_compoundargs *args = rqstp->rq_argp;
>>>>  		struct nfsd4_compoundres *resp = rqstp->rq_resp;
>>>>  
>>>> @@ -713,7 +714,11 @@ nfsd4_putfh(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>  			 *  NOT return NFS4ERR_STALE for either
>>>>  			 *  operation.
>>>>  			 * We limit this to when there is a COPY
>>>> -			 * in the COMPOUND.
>>>> +			 * in the COMPOUND, and extend it to
>>>> +			 * also ignore NFS4ERR_BADHANDLE despite the
>>>> +			 * RFC not requiring this.  If the remote
>>>> +			 * server is running a different NFS implementation,
>>>> +			 * NFS4ERR_BADHANDLE is a likely error.
>>>>  			 */
>>>>  			ret = 0;
>>>>  		}

Hi Neil -

I continue to be interested in improving NFSD's handling of foreign
file handles (and it might be complicated by the addition of file
handle signing). Do you have plans to revise this series, or would
you like me to take it across the finish line?



-- 
Chuck Lever