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

[Jeff Layton]

On Mon, 2026-06-08 at 16:52 -0400, Chuck Lever wrote:
> 
> 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.
> 

That sounds legit. We probably need to snapshot the name sooner, when
we create the event. I'll spin something up. As far as memory
exhaustion goes: if that happens we'll just recall the delegation.
That's always the remedy when there are problems here.

> 
> > +
> > +		/* 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)
> >  {
> 

-- 
Jeff Layton <jlayton@xxxxxxxxxx>