nfsd: medium-severity bugfixes

These are classified as medium-severity, remote-triggerable. Some memory leaks and refcounting bugs, and a potential buffer overrun in a tracepoint.

---
Chris Mason (3):
nfsd: size fh_verify server sockaddr slot by xpt_locallen
nfsd: release path refs on follow_down() error
nfsd: release OPEN-decoded posix ACLs via op_release

Jeff Layton (3):
nfsd: fix nfsd_file leak on inter-server COPY setup failure
nfsd: fix dentry ref leak on V4ROOT export filehandle lookup
nfsd: fix layout fence worker double-reference race

 fs/nfsd/nfs4layouts.c | 27 +++++++++++++++------------
 fs/nfsd/nfs4proc.c    | 20 ++++++++++++--------
 fs/nfsd/nfsfh.c       |  8 ++++++--
 fs/nfsd/state.h       |  1 +
 fs/nfsd/trace.h       |  4 ++--
 fs/nfsd/vfs.c         |  4 +++-
 6 files changed, 39 insertions(+), 25 deletions(-)

---
base-commit: d9f35aa8be6e05deded63f92b153292518bd60a4
change-id: 20260531-nfsd-testing-9122bf51ce95

Best regards,

Jeff Layton <[email protected]>

From: Chris Mason <[email protected]>

The nfsd_fh_verify and nfsd_fh_verify_err tracepoints declare the server sockaddr slot sized by xpt_remotelen but fill it from xpt_local using xpt_locallen:

    TP_STRUCT__entry(
            ...
            __sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
            ...
    )
    TP_fast_assign(
            ...
            __assign_sockaddr(server, &rqstp->rq_xprt->xpt_local,
                              rqstp->rq_xprt->xpt_locallen);
            ...
    )

When xpt_locallen exceeds xpt_remotelen, __assign_sockaddr's memcpy writes past the reserved ring-buffer slot. In the reverse direction (xpt_locallen < xpt_remotelen) the slot is oversized and the unwritten tail leaks prior ring-buffer contents to trace consumers.

The write-past-end case is reachable on NFS/UDP. svc_xprt_set_remote() is only called from svc_tcp_accept() (net/sunrpc/svcsock.c) and from the RDMA connect path; svc_create_socket() for UDP calls only svc_xprt_set_local(), so xpt_remotelen stays 0 for the xprt's lifetime. Every fh_verify trace for an NFSv2/v3-over-UDP request then copies 16 or 28 bytes from xpt_local into a zero-byte slot.

The other NFSD tracepoints that record the server address (NFSD_TRACE_PROC_CALL_FIELDS, NFSD_TRACE_PROC_RES_FIELDS, SVC_RQST_ENDPOINT_FIELDS) already size the server slot by xpt_locallen; nfsd_fh_verify and nfsd_fh_verify_err were the only exceptions.

Fix by sizing the server slot with xpt_locallen so the declared slot matches the copy length. The client slot and its assignment already agree on xpt_remotelen and are left untouched.

---
 fs/nfsd/trace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 9917c0440522..db0a0dc70660 100644
@@ -272,7 +272,7 @@ TRACE_EVENT_CONDITION(nfsd_fh_verify,
 	TP_CONDITION(rqstp != NULL),
 	TP_STRUCT__entry(
 		__field(unsigned int, netns_ino)
-		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
+		__sockaddr(server, rqstp->rq_xprt->xpt_locallen)
 		__sockaddr(client, rqstp->rq_xprt->xpt_remotelen)
 		__field(u32, xid)
 		__field(u32, fh_hash)

@@ -311,7 +311,7 @@ TRACE_EVENT_CONDITION(nfsd_fh_verify_err,
 	TP_CONDITION(rqstp != NULL && error),
 	TP_STRUCT__entry(
 		__field(unsigned int, netns_ino)
-		__sockaddr(server, rqstp->rq_xprt->xpt_remotelen)
+		__sockaddr(server, rqstp->rq_xprt->xpt_locallen)
 		__sockaddr(client, rqstp->rq_xprt->xpt_remotelen)
 		__field(u32, xid)
 		__field(u32, fh_hash)

2.54.0

From: Chris Mason <[email protected]>

nfsd_cross_mnt() initializes a local struct path with mntget() and dget() before calling follow_down(). On a negative return the error arm jumps to out without releasing those references:

    err = follow_down(&path, follow_flags);
    if (err < 0)
            goto out;

follow_down() never drops the caller's entry-time refs on any error sub-case; for example a pre-cross d_manage() failure leaves path untouched, so the mntget()/dget() taken on entry survive the call.

Every other early-exit arm in nfsd_cross_mnt() (other-namespace return, IS_ERR(exp2), and the success tail after the swap) already calls path_put(&path); the err < 0 arm is the lone omission. The leak inflates mnt_count and d_count on each failed cross-mount, blocking umount and pinning dentries against the shrinker, and is reachable by any authenticated NFS client through nfsd_lookup_dentry or the NFSv4 READDIR encode path.

Fix by calling path_put(&path) before the goto out in the err < 0 arm so the entry-time refs are released on all follow_down() error returns.

---
 fs/nfsd/vfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 62b56d73432a..95ce15440492 100644
@@ -137,8 +137,10 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		follow_flags = LOOKUP_AUTOMOUNT;
 
 	err = follow_down(&path, follow_flags);
-	if (err < 0)
+	if (err < 0) {
+		path_put(&path);
 		goto out;
+	}
 	if (path.mnt == exp->ex_path.mnt && path.dentry == dentry &&
 	    nfsd_mountpoint(dentry, exp) == 2) {
 		/* This is only a mountpoint in some other namespace */

2.54.0

Might be better to unify all those path_put()... Something like this, perhaps?

index eafdf7b7890f..bdcd78f49112 100644
@@ -141,13 +141,13 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 	if (path.mnt == exp->ex_path.mnt && path.dentry == dentry &&
 	    nfsd_mountpoint(dentry, exp) == 2) {
 		/* This is only a mountpoint in some other namespace */
-		path_put(&path);
 		goto out;
 	}
 
 	exp2 = rqst_exp_get_by_name(rqstp, &path);
 	if (IS_ERR(exp2)) {
 		err = PTR_ERR(exp2);
+		exp2 = NULL;
 		/*
 		 * We normally allow NFS clients to continue
 		 * "underneath" a mountpoint that is not exported.

@@ -157,10 +157,7 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		 */
 		if (err == -ENOENT && !(exp->ex_flags & NFSEXP_V4ROOT))
 			err = 0;
-		path_put(&path);
-		goto out;
-	}
-	if (nfsd_v4client(rqstp) ||
+	} else if (nfsd_v4client(rqstp) ||
 		(exp->ex_flags & NFSEXP_CROSSMOUNT) || EX_NOHIDE(exp2)) {
 		/* successfully crossed mount point */
 		/*

@@ -174,9 +171,10 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		*expp = exp2;
 		exp2 = exp;
 	}
-	path_put(&path);
-	exp_put(exp2);
 out:
+	path_put(&path);
+	if (exp2)
+		exp_put(exp2);
 	return err;
 }
 

Not a problem, but it's completely untested. It compiles, but...

unify cleanups in nfsd_cross_mnt() exits

Instead of having a separate path_put() on each failure exit, as well as on the normal path, let's move all of those past the point where these codepaths join. We want to keep the ordering between path_put() and exp_put(), so move that one as well.

---

index 95ce15440492..cfac0cc4207c 100644
@@ -137,20 +137,19 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		follow_flags = LOOKUP_AUTOMOUNT;
 
 	err = follow_down(&path, follow_flags);
-	if (err < 0) {
-		path_put(&path);
+	if (err < 0)
 		goto out;
-	}
+
 	if (path.mnt == exp->ex_path.mnt && path.dentry == dentry &&
 	    nfsd_mountpoint(dentry, exp) == 2) {
 		/* This is only a mountpoint in some other namespace */
-		path_put(&path);
 		goto out;
 	}
 
 	exp2 = rqst_exp_get_by_name(rqstp, &path);
 	if (IS_ERR(exp2)) {
 		err = PTR_ERR(exp2);
+		exp2 = NULL;
 		/*
 		 * We normally allow NFS clients to continue
 		 * "underneath" a mountpoint that is not exported.

@@ -160,10 +159,7 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		 */
 		if (err == -ENOENT && !(exp->ex_flags & NFSEXP_V4ROOT))
 			err = 0;
-		path_put(&path);
-		goto out;
-	}
-	if (nfsd_v4client(rqstp) ||
+	} else if (nfsd_v4client(rqstp) ||
 		(exp->ex_flags & NFSEXP_CROSSMOUNT) || EX_NOHIDE(exp2)) {
 		/* successfully crossed mount point */
 		/*

@@ -177,9 +173,10 @@ nfsd_cross_mnt(struct svc_rqst *rqstp, struct dentry **dpp,
 		*expp = exp2;
 		exp2 = exp;
 	}
-	path_put(&path);
-	exp_put(exp2);
 out:
+	path_put(&path);
+	if (exp2)
+		exp_put(exp2);
 	return err;
 }
 

Thanks Al. I'll toss it onto the pile that I'm testing.

Jeff Layton <[email protected]>

We can replace any of the patches already in nfsd-testing, it's a topic branch.

Chuck Lever

When nfsd4_setup_inter_ssc() fails, nfsd4_copy() returns nfserr_offload_denied directly, bypassing the out: label where release_copy_files() would drop the nf_dst reference taken by nfs4_preprocess_stateid_op(). Each failed inter-server COPY leaks one nfsd_file, pinning file/inode/dentry/vfsmount.

Fix by setting status and jumping to out: instead of returning directly.

---
 fs/nfsd/nfs4proc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 9473aeb53f72..017474cd63b5 100644
@@ -2159,16 +2159,14 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		}
 		status = nfsd4_setup_inter_ssc(rqstp, cstate, copy);
 		if (status) {
-			trace_nfsd_copy_done(copy, status);
-			return nfserr_offload_denied;
+			status = nfserr_offload_denied;
+			goto out;
 		}
 	} else {
 		trace_nfsd_copy_intra(copy);
 		status = nfsd4_setup_intra_ssc(rqstp, cstate, copy);
-		if (status) {
-			trace_nfsd_copy_done(copy, status);
-			return status;
-		}
+		if (status)
+			goto out;
 	}
 
 	memcpy(&copy->fh, &cstate->current_fh.fh_handle,

2.54.0

nfsd_set_fh_dentry() leaks the dentry reference from exportfs_decode_fh_raw() when the NFS3_FHSIZE or NFS_FHSIZE switch cases detect NFSEXP_V4ROOT and goto out. The out: label calls exp_put() but never dput(dentry), and fhp->fh_dentry was never assigned so fh_put() cannot compensate.

A crafted NFSv3 filehandle targeting a V4ROOT export's fsid triggers the leak on every request.

---
 fs/nfsd/nfsfh.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index 429ca5c6ec08..b36915401758 100644
@@ -344,15 +344,19 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,
 		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC)
 			fhp->fh_no_wcc = true;
 		fhp->fh_64bit_cookies = true;
-		if (exp->ex_flags & NFSEXP_V4ROOT)
+		if (exp->ex_flags & NFSEXP_V4ROOT) {
+			dput(dentry);
 			goto out;
+		}
 		break;
 	case NFS_FHSIZE:
 		fhp->fh_no_wcc = true;
 		if (EX_WGATHER(exp))
 			fhp->fh_use_wgather = true;
-		if (exp->ex_flags & NFSEXP_V4ROOT)
+		if (exp->ex_flags & NFSEXP_V4ROOT) {
+			dput(dentry);
 			goto out;
+		}
 	}
 
 	fhp->fh_dentry = dentry;

2.54.0

From: Chris Mason <[email protected]>

nfsd4_decode_createhow4() calls nfsd4_decode_fattr4(), which allocates refcounted struct posix_acl objects via posix_acl_alloc() and stores them in open->op_pacl and open->op_dpacl. These pointers must be released once the OPEN compound finishes.

When nfsd4_decode_open_claim4() returns a non-seqid-mutating error, the dispatcher short-circuits before op_func runs:

    nfsd4_proc_compound()
      opdesc->op_func == nfsd4_open_omfg
        if (!seqid_mutating_err(ntohl(op->status)))
            return op->status;   /* nfsd4_open() never runs */
      opdesc->op_release(&op->u)  /* must still release op_pacl/op_dpacl */

Before this change OP_OPEN had no .op_release in nfsd4_ops[], and the release pair lived inside nfsd4_open() at its out_err: label. On the short-circuit path nfsd4_open() is never invoked, so both posix_acl refs leak on every malformed OPEN compound that carries valid POSIX ACL createhow4 attributes.

Add nfsd4_open_release() and wire it as .op_release for OP_OPEN. posix_acl_release() is NULL-safe, so the single release site covers both the normal path and the nfsd4_open_omfg short-circuit. Remove the matching posix_acl_release() pair from nfsd4_open()'s out_err:
label: the compound dispatcher calls op_release unconditionally after every op, so leaving the in-function pair in place would double-release op_pacl and op_dpacl on every ACL-bearing OPEN that reaches nfsd4_open(), underflowing the refcount and freeing the posix_acl while it is still reachable through op->u.

---
 fs/nfsd/nfs4proc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 017474cd63b5..76de265bb9e1 100644
@@ -681,8 +681,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfsd4_cleanup_open_state(cstate, open);
 	nfsd4_bump_seqid(cstate, status);
 out_err:
-	posix_acl_release(open->op_dpacl);
-	posix_acl_release(open->op_pacl);
 	return status;
 }
 

@@ -704,6 +702,13 @@ static __be32 nfsd4_open_omfg(struct svc_rqst *rqstp, struct nfsd4_compound_stat
 	return nfsd4_open(rqstp, cstate, &op->u);
 }
 
+static void
+nfsd4_open_release(union nfsd4_op_u *u)
+{
+	posix_acl_release(u->open.op_dpacl);
+	posix_acl_release(u->open.op_pacl);
+}
+
 /*
  * filehandle-manipulating ops.
  */

@@ -3718,6 +3723,7 @@ static const struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_OPEN] = {
 		.op_func = nfsd4_open,
+		.op_release = nfsd4_open_release,
 		.op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
 		.op_name = "OP_OPEN",
 		.op_rsize_bop = nfsd4_open_rsize,

2.54.0

The double-free fix is right, but op_release is not called unconditionally after every op, and that gap leaks the ACLs on the v4.0 replay path.

op_release runs only at the release: label inside nfsd4_encode_operation(). The compound loop skips that encoder on a replay:

        if (op->status == nfserr_replay_me) {
                nfsd4_encode_replay(resp->xdr, op);     /* no op_release */
                ...
        } else {
                nfsd4_encode_operation(resp, op);       /* op_release here */
        }

So every ACL-bearing v4.0 OPEN retransmit leaks two posix_acl refs.

Please release op->u on the replay branch too:

        if (op->status == nfserr_replay_me) {
                op->replay = &cstate->replay_owner->so_replay;
                nfsd4_encode_replay(resp->xdr, op);
                status = op->status = op->replay->rp_status;
                if (op->opdesc->op_release)
                        op->opdesc->op_release(&op->u);
        }

Let's fix the "unconditionally after every op" wording too.

I've applied the other 5 in this series, so you can just resend this one.

Chuck Lever

Good catch, will fix.

I initially had hoped that we could rely on the fact that a v4.0 client couldn't issue an OPEN with a POSIX ACL attribute in it, but it turns out that knfsd doesn't gate attributes by minorversion.

Maybe it should? That's a separate changeset though if so.

Jeff Layton <[email protected]>

The workqueue core clears WORK_STRUCT_PENDING before the callback is invoked, so delayed_work_pending() in lm_breaker_timedout() can return false while the fence worker is already running. This lets the breaker take a duplicate sc_count reference and schedule a new worker that coalesces with the in-progress one. The extra reference is never put, leaking the layout stateid.

Replace the racy delayed_work_pending() check with an ls_fence_inflight boolean set atomically with refcount_inc_not_zero() under ls_lock, and cleared under ls_lock before nfs4_put_stid() on every exit path. Remove the self-rearm mod_delayed_work() at the top of the worker.

---
 fs/nfsd/nfs4layouts.c | 27 +++++++++++++++------------
 fs/nfsd/state.h       |  1 +
 2 files changed, 16 insertions(+), 12 deletions(-)

index 6c4e4fdd6c05..475246c0e20c 100644
@@ -260,6 +260,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
 	}
 
 	ls->ls_fenced = false;
+	ls->ls_fence_inflight = false;
 	ls->ls_fence_delay = 0;
 	INIT_DELAYED_WORK(&ls->ls_fence_work, nfsd4_layout_fence_worker);
 

@@ -798,15 +799,6 @@ nfsd4_layout_fence_worker(struct work_struct *work)
 	struct nfs4_client *clp;
 	struct nfsd_net *nn;
 
-	/*
-	 * The workqueue clears WORK_STRUCT_PENDING before invoking
-	 * this callback. Re-arm immediately so that
-	 * delayed_work_pending() returns true while the fence
-	 * operation is in progress, preventing
-	 * lm_breaker_timedout() from taking a duplicate reference.
-	 */
-	mod_delayed_work(system_dfl_wq, &ls->ls_fence_work, 0);
-
 	spin_lock(&ls->ls_lock);
 	if (list_empty(&ls->ls_layouts)) {
 		spin_unlock(&ls->ls_lock);

@@ -816,6 +808,9 @@ nfsd4_layout_fence_worker(struct work_struct *work)
 		nfsd4_close_layout(ls);
 
 		ls->ls_fenced = true;
+		spin_lock(&ls->ls_lock);
+		ls->ls_fence_inflight = false;
+		spin_unlock(&ls->ls_lock);
 		nfs4_put_stid(&ls->ls_stid);
 		return;
 	}

@@ -901,18 +896,26 @@ nfsd4_layout_lm_breaker_timedout(struct file_lease *fl)
 	if ((!nfsd4_layout_ops[ls->ls_layout_type]->fence_client) ||
 			ls->ls_fenced)
 		return true;
-	if (delayed_work_pending(&ls->ls_fence_work))
-		return false;
 	/*
 	 * Make sure layout has not been returned yet before
-	 * taking a reference count on the layout stateid.
+	 * taking a reference count on the layout stateid. The
+	 * ls_fence_inflight flag is set together with the sc_count
+	 * increment under ls_lock so that a fence worker invocation
+	 * already in progress (which has cleared WORK_STRUCT_PENDING
+	 * but not yet reached dispose:) cannot be coalesced with a
+	 * fresh schedule that takes an extra unmatched reference.
 	 */
 	spin_lock(&ls->ls_lock);
+	if (ls->ls_fence_inflight) {
+		spin_unlock(&ls->ls_lock);
+		return false;
+	}
 	if (list_empty(&ls->ls_layouts) ||
 			!refcount_inc_not_zero(&ls->ls_stid.sc_count)) {
 		spin_unlock(&ls->ls_lock);
 		return true;
 	}
+	ls->ls_fence_inflight = true;
 	spin_unlock(&ls->ls_lock);
 
 	mod_delayed_work(system_dfl_wq, &ls->ls_fence_work, 0);

index c26b2384d694..05b6f12040d8 100644
@@ -812,6 +812,7 @@ struct nfs4_layout_stateid {
 	struct delayed_work		ls_fence_work;
 	unsigned int			ls_fence_delay;
 	bool				ls_fenced;
+	bool				ls_fence_inflight;
 };
 
 static inline struct nfs4_layout_stateid *layoutstateid(struct nfs4_stid *s)

2.54.0

From: Chuck Lever <[email protected]>

Applied to nfsd-testing, thanks!

[1/6] nfsd: size fh_verify server sockaddr slot by xpt_locallen

      commit: bbbc993f3932be9428ebf81009d0d1a4aed26d00
[2/6] nfsd: release path refs on follow_down() error
      commit: 41c9073b511fe70d5e3b03f039c409d2060de06b
[3/6] nfsd: fix nfsd_file leak on inter-server COPY setup failure
      commit: b55429cfce784ffcbaed2960d66b7c5db90ceec3
[4/6] nfsd: fix dentry ref leak on V4ROOT export filehandle lookup
      commit: 657c7e3082589c35695f77821f1dfa31bf4c6a34
[6/6] nfsd: fix layout fence worker double-reference race
      commit: 4faa69d7158a4436d340b7324aa34fa672362b5c

--
Chuck Lever <[email protected]>