[PATCH v3] 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() failed to marshal
the change into the buffer. 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.9.4 requires the server to recall the delegation when the
directory changes in a way that is not covered by the 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.

The granted notification mask and the requested dir attribute mask come
from independent fields of the GET_DIR_DELEGATION request, so a client
can be granted NOTIFY4_CHANGE_DIR_ATTRS while requesting no specific dir
attributes. nfsd4_encode_dir_attr_change() now distinguishes a genuine
encode failure (which forces the recall) from that benign case (which
simply omits the event).

For that distinction to hold, nfsd4_setup_notify_entry4() must not mask
genuine failures as an empty attribute set. It previously routed three
different conditions through its noattrs label and returned true with a
zero-length attribute set:

  - a NULL or negative dentry (no attributes to report),
  - a vfs_getattr() failure, and
  - an nfsd4_encode_attr_vals() failure (XDR buffer exhaustion).

Only the first is benign; the latter two are genuine failures that were
indistinguishable from the "no attributes requested" case, so a
requested change could still be silently dropped (the per-event entries
encoded by nfsd4_encode_notify_event() were affected the same way).
Return false on a vfs_getattr() or nfsd4_encode_attr_vals() failure so
the error propagates to the caller and the delegation is recalled, and
reserve the noattrs path for the NULL or negative dentry case.

While we're in there, also remove an unneeded check for
NOTIFY4_CHANGE_DIR_ATTRS from nfsd4_encode_dir_attr_change().

Fixes: 757a16cd93d5 ("nfsd: add support to CB_NOTIFY for dir attribute changes")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
---
This version should address Chuck's comments about the NULL pointer
handling.
---
Changes in v3:
- Recall delegations on vfs_getattr() failure instead of sending empty attr set
- Link to v2: https://lore.kernel.org/r/20260619-dir-deleg-fixes-v2-1-ad22fe2c7d7d@xxxxxxxxxx

Changes in v2:
- Allow nfsd4_encode_dir_attr_change() to return ERR_PTR when there are errors
- Drop unneeded NOTIFY4_CHANGE_DIR_ATTRS check
- Link to v1: https://lore.kernel.org/r/20260617-dir-deleg-fixes-v1-0-32b85b366c29@xxxxxxxxxx
---
 fs/nfsd/nfs4state.c | 43 ++++++++++++++++++++++++--------------
 fs/nfsd/nfs4xdr.c   | 60 ++++++++++++++++++++++++++++++-----------------------
 2 files changed, 62 insertions(+), 41 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b830aed7ae39..4ae5d65c056a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3598,23 +3598,36 @@ 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;
 
-			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);
+		else
+			p = ERR_PTR(-ENOBUFS);
+
+		if (IS_ERR(p)) {
+			/*
+			 * The client asked to be told about dir attr changes
+			 * but the change could not be encoded. RFC 8881
+			 * s10.9.4 requires the server to recall the delegation
+			 * rather than drop a requested notification, so fall
+			 * through to recall. A NULL return instead means there
+			 * were no attributes to report, so omit the event in
+			 * that case.
+			 */
+			error = true;
+		} else 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 (!error) {
 		ncn->ncn_nf_cnt = count;
 		nfsd_file_put(nf);
 		return true;
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 5282e00805af..b37290b723f5 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4291,9 +4291,21 @@ nfsd4_setup_notify_entry4(struct notify_entry4 *ne, struct xdr_stream *xdr,
 	parent = (dentry == path.dentry);
 	path.dentry = dentry;
 
-	/* FIXME: d_find_alias for inode ? */
-	if (!path.dentry || !d_inode(path.dentry))
-		goto noattrs;
+	/*
+	 * A NULL or negative dentry has no attributes to report. This is
+	 * expected, e.g. for the old entry of a rename or for an entry that
+	 * has already been removed, so encode an empty attribute set rather
+	 * than failing.
+	 */
+	if (!path.dentry || !d_inode(path.dentry)) {
+		attrmask[0] = 0;
+		attrmask[1] = 0;
+		attrmask[2] = 0;
+		ne->ne_attrs.attr_vals.data = NULL;
+		ne->ne_attrs.attr_vals.len = 0;
+		ne->ne_attrs.attrmask.count = 1;
+		return true;
+	}
 
 	/*
 	 * It is possible that the client was granted a delegation when a file
@@ -4302,7 +4314,7 @@ nfsd4_setup_notify_entry4(struct notify_entry4 *ne, struct xdr_stream *xdr,
 	 */
 	ret = vfs_getattr(&path, &args.stat, CB_NOTIFY_STATX_REQUEST_MASK, AT_STATX_SYNC_AS_STAT);
 	if (ret)
-		goto noattrs;
+		return false;
 
 	args.change_attr = nfsd4_change_attribute(&args.stat);
 
@@ -4326,18 +4338,10 @@ nfsd4_setup_notify_entry4(struct notify_entry4 *ne, struct xdr_stream *xdr,
 
 	status = nfsd4_encode_attr_vals(xdr, attrmask, &args);
 	if (status != nfs_ok)
-		goto noattrs;
+		return false;
 
 	ne->ne_attrs.attr_vals.len = (u8 *)xdr->p - ne->ne_attrs.attr_vals.data;
 	return true;
-noattrs:
-	attrmask[0] = 0;
-	attrmask[1] = 0;
-	attrmask[2] = 0;
-	ne->ne_attrs.attr_vals.data = NULL;
-	ne->ne_attrs.attr_vals.len = 0;
-	ne->ne_attrs.attrmask.count = 1;
-	return true;
 }
 
 /**
@@ -4439,28 +4443,32 @@ u8 *nfsd4_encode_notify_event(struct xdr_stream *xdr, struct nfsd_notify_event *
  * @nf: nfsd_file opened on the directory
  *
  * Encode a dir attr change event.
+ *
+ * Return: a pointer to the start of the encoded event on success; NULL
+ * if there were no requested attributes to report, in which case the
+ * caller should omit the event; or an ERR_PTR if the event was requested
+ * but could not be marshalled into @xdr, in which case the caller should
+ * recall the delegation.
  */
 u8 *nfsd4_encode_dir_attr_change(struct xdr_stream *xdr, struct nfs4_delegation *dp,
 				 struct nfsd_file *nf)
 {
 	struct dentry *dentry = nf->nf_file->f_path.dentry;
 	struct notify_attr4 na = { };
-	bool ret;
-	u8 *p = NULL;
-
-	if (!(dp->dl_notify_mask & BIT(NOTIFY4_CHANGE_DIR_ATTRS)))
-		return NULL;
+	u8 *p;
 
 	/* RFC 8881 s10.4.3: ne_file must be a zero-length string for dir attrs */
-	ret = nfsd4_setup_notify_entry4(&na.na_changed_entry, xdr,
-					dentry, dp, nf, "", 0);
+	if (!nfsd4_setup_notify_entry4(&na.na_changed_entry, xdr,
+				       dentry, dp, nf, "", 0))
+		return ERR_PTR(-ENOBUFS);
 
-	/* Don't bother with the event if we're not encoding attrs */
-	if (ret && na.na_changed_entry.ne_attrs.attr_vals.len) {
-		p = (u8 *)xdr->p;
-		if (!xdrgen_encode_notify_attr4(xdr, &na))
-			p = NULL;
-	}
+	/* No requested attributes to report; omit the event */
+	if (!na.na_changed_entry.ne_attrs.attr_vals.len)
+		return NULL;
+
+	p = (u8 *)xdr->p;
+	if (!xdrgen_encode_notify_attr4(xdr, &na))
+		return ERR_PTR(-ENOBUFS);
 	return p;
 }
 

---
base-commit: 6f90c7618528b5ca5887f8c6057f26dcc7a27a99
change-id: 20260617-dir-deleg-fixes-ecac84095b84

Best regards,
-- 
Jeff Layton <jlayton@xxxxxxxxxx>