Re: [PATCH v5 10/21] nfsd: add notification handlers for dir events

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

On Fri, May 22, 2026, at 3:42 PM, Jeff Layton wrote:
> Add the necessary parts to accept a fsnotify callback for directory
> change event and create a CB_NOTIFY request for it. When a dir nfsd_file
> is created set a handle_event callback to handle the notification.

> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e17488a911f7..31df04675713 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4172,6 +4172,127 @@ nfsd4_encode_fattr4(struct svc_rqst *rqstp, 
> struct xdr_stream *xdr,
>  	goto out;
>  }
> 
> +static bool
> +nfsd4_setup_notify_entry4(struct notify_entry4 *ne, struct xdr_stream 
> *xdr,
> +			  struct dentry *dentry, struct nfs4_delegation *dp,
> +			  struct nfsd_file *nf, char *name, u32 namelen)
> +{
> +	uint32_t *attrmask;
> +
> +	/* Reserve space for attrmask */
> +	attrmask = xdr_reserve_space(xdr, 3 * sizeof(uint32_t));
> +	if (!attrmask)
> +		return false;
> +
> +	ne->ne_file.data = name;
> +	ne->ne_file.len = namelen;
> +	ne->ne_attrs.attrmask.element = attrmask;
> +
> +	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;
> +}
> +
> +/**
> + * nfsd4_encode_notify_event - encode a notify
> + * @xdr: stream to which to encode the fattr4
> + * @nne: nfsd_notify_event to encode
> + * @dp: delegation where the event occurred
> + * @nf: nfsd_file on which event occurred
> + * @notify_mask: pointer to word where notification mask should be set
> + *
> + * Encode @nne into @xdr. Returns a pointer to the start of the event, 
> or NULL if
> + * the event couldn't be encoded. The appropriate bit in the 
> notify_mask will also
> + * be set on success.
> + */
> +u8 *nfsd4_encode_notify_event(struct xdr_stream *xdr, struct 
> nfsd_notify_event *nne,
> +			      struct nfs4_delegation *dp, struct nfsd_file *nf,
> +			      u32 *notify_mask)
> +{
> +	u8 *p = NULL;
> +
> +	*notify_mask = 0;
> +
> +	if (nne->ne_mask & FS_DELETE) {
> +		struct notify_remove4 nr = { };
> +
> +		if (!nfsd4_setup_notify_entry4(&nr.nrm_old_entry, xdr, 
> nne->ne_dentry, dp,
> +					       nf, nne->ne_name, nne->ne_namelen))
> +			goto out_err;
> +		p = (u8 *)xdr->p;
> +		if (!xdrgen_encode_notify_remove4(xdr, &nr))
> +			goto out_err;
> +		*notify_mask |= BIT(NOTIFY4_REMOVE_ENTRY);
> +	} else if (nne->ne_mask & FS_CREATE) {
> +		struct notify_add4 na = { };
> +		struct notify_remove4 old = { };
> +
> +		if (!nfsd4_setup_notify_entry4(&na.nad_new_entry, xdr, 
> nne->ne_dentry, dp,
> +					       nf, nne->ne_name, nne->ne_namelen))
> +			goto out_err;
> +
> +		/* If a file was overwritten, report it in nad_old_entry */
> +		if (nne->ne_target) {
> +			if (!nfsd4_setup_notify_entry4(&old.nrm_old_entry, xdr,
> +						       NULL, dp, nf,
> +						       nne->ne_name, nne->ne_namelen))
> +				goto out_err;
> +			na.nad_old_entry.count = 1;
> +			na.nad_old_entry.element = &old;
> +		}
> +
> +		p = (u8 *)xdr->p;
> +		if (!xdrgen_encode_notify_add4(xdr, &na))
> +			goto out_err;
> +
> +		*notify_mask |= BIT(NOTIFY4_ADD_ENTRY);
> +	} else if (nne->ne_mask & FS_RENAME) {
> +		struct notify_rename4 nr = { };
> +		struct notify_remove4 old = { };
> +		struct name_snapshot n;
> +		bool ret;
> +
> +		/* Don't send any attributes in the old_entry since they're the same 
> in new */
> +		if (!nfsd4_setup_notify_entry4(&nr.nrn_old_entry.nrm_old_entry, xdr,
> +					       NULL, dp, nf, nne->ne_name,
> +					       nne->ne_namelen))
> +			goto out_err;
> +
> +		take_dentry_name_snapshot(&n, nne->ne_dentry);
> +		ret = nfsd4_setup_notify_entry4(&nr.nrn_new_entry.nad_new_entry, xdr,
> +					       nne->ne_dentry, dp, nf, (char *)n.name.name,
> +					       n.name.len);

Now once I got all of the previous edits in place, all three LLM
reviewers identified an issue here that might require a significant
rewrite. This is why I stopped the minor editing here and decided
it was time for you to consider restructuring (or not). I haven't
looked at patches 11-21.

  I think the new name here has a time-of-use problem.
  
  nrn_old_entry uses nne->ne_name, which alloc_nfsd_notify_event() copied
  when fsnotify delivered the rename.  nrn_new_entry instead reads the
  live dentry via take_dentry_name_snapshot() at callback-prepare time,
  which can run long after the event was queued.

  CB_NOTIFY is asynchronous: nfsd_handle_dir_event() queues the event on
  ncn_evt[] and nothing holds ne_dentry stable until the work runs.
  d_move() reuses the same dentry and rewrites d_name in place, so a
  second rename of the entry before the queued callback encodes leaves
  the dget'd ne_dentry carrying the later name.  An A->B event then
  encodes as A->C, and a client holding the directory delegation applies
  the wrong old->new mapping to its cache.  The old name is immune
  because it was snapshotted up front; only the new name is read late.

  The new name is available at notification time -- fsnotify_move() passes
  &moved->d_name as new_name, and ne_dentry is that moved dentry -- so
  alloc_nfsd_notify_event() can snapshot it alongside the old name.

What I haven't assessed is whether the suggested restructuring is
now vulnerable to misbehavior during memory exhaustion.


> +
> +		/* If a file was overwritten, report it in nad_old_entry */
> +		if (ret && nne->ne_target) {
> +			ret = nfsd4_setup_notify_entry4(&old.nrm_old_entry, xdr,
> +							NULL, dp, nf,
> +							(char *)n.name.name, n.name.len);
> +			if (ret) {
> +				nr.nrn_new_entry.nad_old_entry.count = 1;
> +				nr.nrn_new_entry.nad_old_entry.element = &old;
> +			}
> +		}
> +
> +		if (ret) {
> +			p = (u8 *)xdr->p;
> +			ret = xdrgen_encode_notify_rename4(xdr, &nr);
> +		}
> +		release_dentry_name_snapshot(&n);
> +		if (!ret)
> +			goto out_err;
> +		*notify_mask |= BIT(NOTIFY4_RENAME_ENTRY);
> +	}
> +	return p;
> +out_err:
> +	pr_warn("nfsd: unable to marshal notify_rename4 to xdr stream\n");
> +	return NULL;
> +}
> +
>  static void svcxdr_init_encode_from_buffer(struct xdr_stream *xdr,
>  				struct xdr_buf *buf, __be32 *p, int bytes)
>  {


-- 
Chuck Lever