Re: [PATCH v6 20/20] nfsd: add support to CB_NOTIFY for dir attribute changes

["Chuck Lever" <cel@kernel.org>]

On Thu, Jun 11, 2026, at 1:50 PM, Jeff Layton wrote:
> If the client requested dir attribute change notifications, send those
> alongside any set of add/remove/rename events. Note that the server will
> still recall the delegation on a SETATTR, so these are only sent for
> changes to child dirents.
>
> The child filehandle returned in these notifications is composed by
> setup_notify_fhandle() without going through fh_compose(), so it does
> not get a MAC appended. On exports configured with NFSEXP_SIGN_FH the
> client would then get back an unsigned filehandle that fh_verify()
> rejects as stale. Pass the delegation's export down to
> setup_notify_fhandle() and append the MAC with fh_append_mac() when the
> export requires signed filehandles; if signing fails, drop the
> filehandle attribute rather than handing out an unusable one.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4state.c | 25 ++++++++++++++++--
>  fs/nfsd/nfs4xdr.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++--------
>  fs/nfsd/xdr4.h      |  2 ++
>  3 files changed, 88 insertions(+), 12 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 12627afb604f..e394278fb92e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3503,10 +3503,15 @@ nfsd4_cb_notify_prepare(struct nfsd4_callback *cb)
>  	struct nfsd_notify_event *events[NOTIFY4_EVENT_QUEUE_SIZE];
>  	struct xdr_buf xdr = { .buflen = PAGE_SIZE * NOTIFY4_PAGE_ARRAY_SIZE,
>  			       .pages  = ncn->ncn_pages };
> +	int limit = NOTIFY4_EVENT_QUEUE_SIZE;

When a client requests NOTIFY4_CHANGE_DIR_ATTRS, the CB_NOTIFY event
queue can fill to NOTIFY4_EVENT_QUEUE_SIZE (3) events while the consumer
only accepts 2 (it reserves a slot for the dir-attr-change entry). The
resulting overflow path in nfsd4_cb_notify_prepare() recalls the
delegation without draining the queue, and nfsd4_cb_notify_release()
then requeues the same callback indefinitely.


>  	struct xdr_stream stream;
>  	struct nfsd_file *nf;
> -	int count, i;
>  	bool error = false;
> +	int count, i;
> +
> +	/* Save a slot for dir attr update if requested */
> +	if (dp->dl_notify_mask & BIT(NOTIFY4_CHANGE_DIR_ATTRS))
> +		--limit;
> 
>  	xdr_init_encode_pages(&stream, &xdr);
> 
> @@ -3520,7 +3525,7 @@ nfsd4_cb_notify_prepare(struct nfsd4_callback *cb)
>  	}
> 
>  	/* we can't keep up! */
> -	if (count > NOTIFY4_EVENT_QUEUE_SIZE) {
> +	if (count > limit) {
>  		spin_unlock(&ncn->ncn_lock);
>  		goto out_recall;
>  	}
> @@ -3567,6 +3572,22 @@ nfsd4_cb_notify_prepare(struct nfsd4_callback 
> *cb)
>  		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 (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;
> +				}
> +			}
> +		}

Nit:

When xdr_reserve_space() for maskp succeeds but nfsd4_encode_dir_attr_change()
returns NULL, the 4-byte reservation is never rolled back and *maskp is never
written, yet the function still takes the success path (return true). Unlike
the child-event loop, this branch does not escalate to error = true.

This is probably benign only because nfs4_xdr_enc_cb_notify re-encodes from
the ncn_nf[] array (and count was not incremented), so the garbage hole is
never transmitted.


>  		ncn->ncn_nf_cnt = count;
>  		nfsd_file_put(nf);
>  		return true;
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 1e3c360c06cd..7dd8476028d6 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4199,7 +4199,8 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, 
> struct xdr_stream *xdr,
> 
>  static bool
>  setup_notify_fhandle(struct dentry *dentry, struct nfs4_file *fi,
> -		     struct nfsd_file *nf, struct nfsd4_fattr_args *args)
> +		     struct nfsd_file *nf, struct svc_export *exp,
> +		     struct nfsd4_fattr_args *args)
>  {
>  	int fileid_type, fsid_len, maxsize, flags = 0;
>  	struct knfsd_fh *fhp = &args->fhandle;

The function dereferences the new exp parameter unconditionally.

The argument is dp->dl_stid.sc_export, read unlocked at
nfs4xdr.c:4297 and handed down. An in-flight CB_NOTIFY callback holds a
sc_count reference but NOT an export reference. drop_stid_export() can
run concurrently (admin revoke / unexport), NULL sc_export, and drop what
may be the last export reference, freeing the svc_export while the
callback dereferences it.


> @@ -4227,6 +4228,17 @@ setup_notify_fhandle(struct dentry *dentry, 
> struct nfs4_file *fi,
> 
>  	fhp->fh_fileid_type = fileid_type;
>  	fhp->fh_size += maxsize * 4;
> +
> +	/*
> +	 * fh_compose() appends a MAC to filehandles on signed exports; this
> +	 * hand-rolled filehandle must do the same or the client will get back
> +	 * an unsigned filehandle that fh_verify() later rejects as stale.
> +	 * If we can't sign it, don't hand it out at all.
> +	 */
> +	if (exp && (exp->ex_flags & NFSEXP_SIGN_FH))
> +		if (!fh_append_mac(fhp, NFS4_FHSIZE, exp->cd->net))
> +			return false;
> +
>  	return true;
>  }
> 
> @@ -4240,11 +4252,11 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
> *ne, struct xdr_stream *xdr,
>  			  struct nfsd_file *nf, char *name, u32 namelen)
>  {
>  	struct nfs4_file *fi = dp->dl_stid.sc_file;
> -	struct path path =  { .mnt = nf->nf_file->f_path.mnt,
> -			      .dentry = dentry };
> +	struct path path = nf->nf_file->f_path;
>  	struct nfsd4_fattr_args args = { };
>  	uint32_t *attrmask;
>  	__be32 status;
> +	bool parent;
>  	int ret;
> 
>  	/* Reserve space for attrmask */
> @@ -4256,6 +4268,9 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
> *ne, struct xdr_stream *xdr,
>  	ne->ne_file.len = namelen;
>  	ne->ne_attrs.attrmask.element = attrmask;
> 
> +	parent = (dentry == path.dentry);
> +	path.dentry = dentry;
> +
>  	/* FIXME: d_find_alias for inode ? */
>  	if (!path.dentry || !d_inode(path.dentry))
>  		goto noattrs;
> @@ -4271,15 +4286,21 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
> *ne, struct xdr_stream *xdr,
> 
>  	args.change_attr = nfsd4_change_attribute(&args.stat);
> 
> -	attrmask[0] = dp->dl_child_attrs[0];
> -	attrmask[1] = dp->dl_child_attrs[1];
> -	attrmask[2] = 0;
> +	if (parent) {
> +		attrmask[0] = dp->dl_dir_attrs[0];
> +		attrmask[1] = dp->dl_dir_attrs[1];
> +	} else {
> +		attrmask[0] = dp->dl_child_attrs[0];
> +		attrmask[1] = dp->dl_child_attrs[1];
> 
> -	if (!setup_notify_fhandle(dentry, fi, nf, &args))
> -		attrmask[0] &= ~FATTR4_WORD0_FILEHANDLE;
> +		if (!setup_notify_fhandle(dentry, fi, nf,
> +					  dp->dl_stid.sc_export, &args))
> +			attrmask[0] &= ~FATTR4_WORD0_FILEHANDLE;
> 
> -	if (!(args.stat.result_mask & STATX_BTIME))
> -		attrmask[1] &= ~FATTR4_WORD1_TIME_CREATE;
> +		if (!(args.stat.result_mask & STATX_BTIME))
> +			attrmask[1] &= ~FATTR4_WORD1_TIME_CREATE;
> +	}
> +	attrmask[2] = 0;
> 
>  	ne->ne_attrs.attrmask.count = 2;
>  	ne->ne_attrs.attr_vals.data = (u8 *)xdr->p;
> @@ -4392,6 +4413,38 @@ u8 *nfsd4_encode_notify_event(struct xdr_stream 
> *xdr, struct nfsd_notify_event *
>  	return NULL;
>  }
> 
> +/**
> + * nfsd4_encode_dir_attr_change
> + * @xdr: stream to which to encode the fattr4
> + * @dp: delegation where the event occurred
> + * @nf: nfsd_file opened on the directory
> + *
> + * Encode a dir attr change event.
> + */
> +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;

It looks like this if() re-checks dl_notify_mask even though its
sole caller already gated on the identical check.

nfsd4_encode_notify_event() does not repeat its caller's check.
The guard is unreachable from current callers.


> +
> +	/* 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);
> +
> +	/* 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;
> +	}
> +	return p;
> +}
> +
>  static void svcxdr_init_encode_from_buffer(struct xdr_stream *xdr,
>  				struct xdr_buf *buf, __be32 *p, int bytes)
>  {
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 62ac790428be..805c7122eb93 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -973,6 +973,8 @@ __be32 nfsd4_encode_fattr_to_buf(__be32 **p, int 
> words,
>  u8 *nfsd4_encode_notify_event(struct xdr_stream *xdr, struct 
> nfsd_notify_event *nne,
>  			      struct nfs4_delegation *dd, struct nfsd_file *nf,
>  			      u32 *notify_mask);
> +u8 *nfsd4_encode_dir_attr_change(struct xdr_stream *xdr, struct 
> nfs4_delegation *dp,
> +				 struct nfsd_file *nf);
>  extern __be32 nfsd4_setclientid(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *, union nfsd4_op_u *u);
>  extern __be32 nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
>
> -- 
> 2.54.0

-- 
Chuck Lever