[PATCH 0/3] nfsd: follow-on fixes for CB_NOTIFY support

[PATCH 0/3] nfsd: follow-on fixes for CB_NOTIFY support

[Jeff Layton]

These are just some problems that Sashiko pointed out in review that
seem to be valid concerns. The Fixes: tags have commit IDs from my tree
that you probably don't have, but they should indicate which patches
these need to be folded into.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (3):
      nfsd: fix CB_NOTIFY workqueue loop when queue overflows
      nfsd: fix NULL deref / UAF of sc_export in setup_notify_fhandle
      nfsd: recall deleg if a requested dir attr change can't be encoded

 fs/nfsd/nfs4state.c | 63 ++++++++++++++++++++++++++++++++++-------------------
 fs/nfsd/nfs4xdr.c   | 29 +++++++++++++++++++-----
 2 files changed, 64 insertions(+), 28 deletions(-)
---
base-commit: fd0d6dde2a57d47d976c936586f0be24669471ec
change-id: 20260617-dir-deleg-fixes-ecac84095b84

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/3] nfsd: fix CB_NOTIFY workqueue loop when queue overflows

[Jeff Layton]

When NOTIFY4_CHANGE_DIR_ATTRS is requested, nfsd4_cb_notify_prepare()
lowers its limit to NOTIFY4_EVENT_QUEUE_SIZE - 1 to reserve a slot for
the dir attribute update. The producer in nfsd_handle_dir_event() caps
ncn_evt_cnt at NOTIFY4_EVENT_QUEUE_SIZE, so the queue can fill to one
more than that limit.

In that case prepare() took the "we can't keep up!" branch and jumped to
out_recall without draining the queue, leaving ncn_evt_cnt nonzero.
Returning false from prepare causes nfsd41_destroy_cb() to run the
release op, and nfsd4_cb_notify_release() requeues the callback whenever
ncn_evt_cnt > 0. The requeued callback hits the same overflow check and
recalls again, spinning a workqueue thread forever. Because each cycle
holds an stid reference across the requeue, sc_count never reaches zero,
so the delegation is never freed even after the client returns it -
exhausting CPU and leaking the delegation.

Drain the queued events (dropping their references) and reset
ncn_evt_cnt before recalling, so the release op won't requeue. This also
fixes an event reference leak that previously existed on the recall path.

Also guard the release-side requeue against a revoked delegation: there
is no point notifying a client that no longer holds the delegation, and
skipping the requeue avoids pinning the stid and spinning the workqueue.

Fixes: fd0d6dde2a57 ("nfsd: add support to CB_NOTIFY for dir attribute changes")
Reported-by: Sashiko AI <https://sashiko.dev>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2f7210accdf1..b830aed7ae39 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3546,16 +3546,21 @@ nfsd4_cb_notify_prepare(struct nfsd4_callback *cb)
 		return false;
 	}
 
-	/* we can't keep up! */
-	if (count > limit) {
-		spin_unlock(&ncn->ncn_lock);
-		goto out_recall;
-	}
-
 	memcpy(events, ncn->ncn_evt, sizeof(*events) * count);
 	ncn->ncn_evt_cnt = 0;
 	spin_unlock(&ncn->ncn_lock);
 
+	/*
+	 * We can't keep up! Drop the queued events and recall. The queue must
+	 * be drained here: out_recall leaves ncn_evt_cnt at 0, so the release
+	 * op won't see leftover events and requeue this callback forever.
+	 */
+	if (count > limit) {
+		for (i = 0; i < count; ++i)
+			nfsd_notify_event_put(events[i]);
+		goto out_recall;
+	}
+
 	rcu_read_lock();
 	nf = nfsd_file_get(rcu_dereference(dp->dl_stid.sc_file->fi_deleg_file));
 	rcu_read_unlock();
@@ -3663,8 +3668,13 @@ nfsd4_cb_notify_release(struct nfsd4_callback *cb)
 	struct nfs4_delegation *dp =
 			container_of(ncn, struct nfs4_delegation, dl_cb_notify);
 
-	/* Drain events that arrived while this callback was in flight */
-	if (READ_ONCE(ncn->ncn_evt_cnt) > 0)
+	/*
+	 * Drain events that arrived while this callback was in flight, but
+	 * don't requeue against a revoked delegation: there's no point in
+	 * notifying a client that no longer holds it, and doing so can pin the
+	 * stid and spin the workqueue.
+	 */
+	if (!dp->dl_stid.sc_status && READ_ONCE(ncn->ncn_evt_cnt) > 0)
 		nfsd4_run_cb_notify(ncn);
 	nfs4_put_stid(&dp->dl_stid);
 }

-- 
2.54.0

[PATCH 2/3] nfsd: fix NULL deref / UAF of sc_export in setup_notify_fhandle

[Jeff Layton]

setup_notify_fhandle() read dp->dl_stid.sc_export locklessly and
dereferenced it unconditionally in the subtree-check branch:

	if (!(exp->ex_flags & NFSEXP_NOSUBTREECHECK) && ...

The later NFSEXP_SIGN_FH test already guards with "if (exp && ...)",
acknowledging that exp can be NULL here.

CB_NOTIFY callbacks run asynchronously, holding only an sc_count
reference on the delegation. That keeps the stid (and, on the normal
teardown path, its export) alive, because nfs4_put_stid() only releases
sc_export once sc_count reaches zero. drop_stid_export(), however, runs
on admin revocation (revoke_one_stid() for SC_TYPE_DELEG) and clears
sc_export and drops its reference independently of sc_count, under
cl_lock. A concurrent revocation can therefore make the lockless read
return NULL (NULL deref in the subtree-check branch) or a pointer that
exp_put() frees mid-encode (use-after-free).

Grab a reference to the export under cl_lock, mirroring the locking in
drop_stid_export(), and guard every use of exp with a NULL check. This
closes both the NULL deref and the use-after-free, and leaves the normal
(non-revoked) path unchanged.

Fixes: 121c372b2c55 ("nfsd: add the filehandle to returned attributes in CB_NOTIFY")
Reported-by: Sashiko AI <https://sashiko.dev>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4xdr.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index dfb2cf9239bf..c93eea36a5ea 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4203,12 +4203,26 @@ setup_notify_fhandle(struct dentry *dentry, struct nfs4_delegation *dp,
 		     struct nfsd_file *nf, struct nfsd4_fattr_args *args)
 {
 	struct nfs4_file *fi = dp->dl_stid.sc_file;
-	struct svc_export *exp = dp->dl_stid.sc_export;
+	struct nfs4_client *clp = dp->dl_stid.sc_client;
 	int fileid_type, fsid_len, maxsize, flags = 0;
 	struct knfsd_fh *fhp = &args->fhandle;
 	struct inode *inode = d_inode(dentry);
 	struct inode *parent = NULL;
+	struct svc_export *exp;
 	struct fid *fid;
+	bool ret = false;
+
+	/*
+	 * drop_stid_export() can clear sc_export and drop its reference
+	 * locklessly when the delegation is admin-revoked, concurrently with
+	 * this callback. Grab our own reference under cl_lock so the export
+	 * can be neither NULL-raced nor freed while we encode.
+	 */
+	spin_lock(&clp->cl_lock);
+	exp = dp->dl_stid.sc_export;
+	if (exp)
+		exp_get(exp);
+	spin_unlock(&clp->cl_lock);
 
 	fsid_len = key_len(fi->fi_fhandle.fh_fsid_type);
 	fhp->fh_size = 4 + fsid_len;
@@ -4225,23 +4239,28 @@ setup_notify_fhandle(struct dentry *dentry, struct nfs4_delegation *dp,
 	 * delegation's export rather than the shared nfs4_file, which may
 	 * have been initialized under a different export.
 	 */
-	if (!(exp->ex_flags & NFSEXP_NOSUBTREECHECK) && !S_ISDIR(inode->i_mode)) {
+	if (exp && !(exp->ex_flags & NFSEXP_NOSUBTREECHECK) &&
+	    !S_ISDIR(inode->i_mode)) {
 		parent = d_inode(nf->nf_file->f_path.dentry);
 		flags = EXPORT_FH_CONNECTABLE;
 	}
 
 	fileid_type = exportfs_encode_inode_fh(inode, fid, &maxsize, parent, flags);
 	if (fileid_type < 0 || fileid_type == FILEID_INVALID)
-		return false;
+		goto out;
 
 	fhp->fh_fileid_type = fileid_type;
 	fhp->fh_size += maxsize * 4;
 
 	if (exp && (exp->ex_flags & NFSEXP_SIGN_FH))
 		if (!fh_append_mac(fhp, NFS4_FHSIZE, exp->cd->net))
-			return false;
+			goto out;
 
-	return true;
+	ret = true;
+out:
+	if (exp)
+		exp_put(exp);
+	return ret;
 }
 
 #define CB_NOTIFY_STATX_REQUEST_MASK (STATX_BASIC_STATS   | \

-- 
2.54.0

[PATCH 3/3] nfsd: recall deleg if a requested dir attr change can't be encoded

[Jeff Layton]

When the client requested NOTIFY4_CHANGE_DIR_ATTRS,
nfsd4_cb_notify_prepare() tried to append the dir attribute change to the
CB_NOTIFY, but silently dropped it if the attrmask space reservation
failed or nfsd4_encode_dir_attr_change() returned NULL (out of buffer
space, encode error, or no attributes to send). It then returned true
and transmitted a CB_NOTIFY lacking the requested notification, leaving
the client's cached directory attributes stale with no indication.

RFC 8881 s10.4 requires the server to recall the delegation when it
cannot deliver a requested notification. Treat a failure to encode the
dir attribute change like the per-event encode failures already handled
in the loop above: set error and fall through to out_recall.

Fixes: fd0d6dde2a57 ("nfsd: add support to CB_NOTIFY for dir attribute changes")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b830aed7ae39..82125937dde2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3598,23 +3598,30 @@ nfsd4_cb_notify_prepare(struct nfsd4_callback *cb)
 put_event:
 		nfsd_notify_event_put(nne);
 	}
-	if (!error) {
-		if (dp->dl_notify_mask & BIT(NOTIFY4_CHANGE_DIR_ATTRS)) {
-			u32 *maskp = (u32 *)xdr_reserve_space(&stream, sizeof(*maskp));
+	if (!error && (dp->dl_notify_mask & BIT(NOTIFY4_CHANGE_DIR_ATTRS))) {
+		u32 *maskp = (u32 *)xdr_reserve_space(&stream, sizeof(*maskp));
+		u8 *p = NULL;
 
-			if (maskp) {
-				u8 *p = nfsd4_encode_dir_attr_change(&stream, dp, nf);
-
-				if (p) {
-					*maskp = BIT(NOTIFY4_CHANGE_DIR_ATTRS);
-					ncn->ncn_nf[count].notify_mask.count = 1;
-					ncn->ncn_nf[count].notify_mask.element = maskp;
-					ncn->ncn_nf[count].notify_vals.data = p;
-					ncn->ncn_nf[count].notify_vals.len = (u8 *)stream.p - p;
-					++count;
-				}
-			}
+		if (maskp)
+			p = nfsd4_encode_dir_attr_change(&stream, dp, nf);
+		if (!p) {
+			/*
+			 * The client asked to be told about dir attr changes
+			 * but we couldn't encode one. RFC 8881 s10.4 requires
+			 * recalling the delegation rather than dropping a
+			 * requested notification, so fall through to recall.
+			 */
+			error = true;
+		} else {
+			*maskp = BIT(NOTIFY4_CHANGE_DIR_ATTRS);
+			ncn->ncn_nf[count].notify_mask.count = 1;
+			ncn->ncn_nf[count].notify_mask.element = maskp;
+			ncn->ncn_nf[count].notify_vals.data = p;
+			ncn->ncn_nf[count].notify_vals.len = (u8 *)stream.p - p;
+			++count;
 		}
+	}
+	if (!error) {
 		ncn->ncn_nf_cnt = count;
 		nfsd_file_put(nf);
 		return true;

-- 
2.54.0