[PATCH 0/2] nfsd: fix up some sparse warnings

[PATCH 0/2] nfsd: fix up some sparse warnings

[Jeff Layton]

The first patch should fix the sparse warning that the KTR noticed last
week. I also took the opportunity to add a comment over the weird xdr
stream (ab)use we're doing there.

The second patch just fixes up a sparse warning that I noticed.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (2):
      nfsd: fix type mismatch and explain host-endian xdr buffer usage
      nfsd: fix nfserr type in nfsd_lookup_dentry()

 fs/nfsd/nfs4xdr.c | 11 ++++++++---
 fs/nfsd/vfs.c     |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)
---
base-commit: 106c31f3d72c56fce98bb2f0e8b3de1a0024228d
change-id: 20260803-dir-deleg-1774f5da07e0

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

[PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Jeff Layton]

sparse flagged this type mismatch. Also the xdr buffer usage is a bit
non-standard, so explain what we're doing and why.

Fixes: 4b64b811f368 ("nfsd: add notification handlers for dir events")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202607310455.AT0GoC5j-lkp@intel.com/
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4xdr.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index a47eb544b99f..5cbb4a415384 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4383,13 +4383,18 @@ nfsd4_setup_notify_entry4(struct notify_entry4 *ne, struct xdr_stream *xdr,
 	struct path path = nf->nf_file->f_path;
 	struct nfsd4_fattr_args args = { };
 	const u32 *reqmask;
-	uint32_t *attrmask;
+	u32 *attrmask;
 	__be32 status;
 	bool parent;
 	int ret;
 
-	/* Reserve space for attrmask */
-	attrmask = xdr_reserve_space(xdr, 3 * sizeof(uint32_t));
+	/*
+	 * attrmask is the host-order bmval[3] that nfsd4_encode_attr_vals()
+	 * consumes and that ne_attrs.attrmask.element points at. It must
+	 * outlive this call, so steal a few words from the xdr stream to hold
+	 * it.
+	 */
+	attrmask = (u32 *)xdr_reserve_space(xdr, 3 * sizeof(*attrmask));
 	if (!attrmask)
 		return false;
 

-- 
2.55.0

Re: [PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Chuck Lever]

On Mon, Aug 3, 2026, at 10:49 AM, Jeff Layton wrote:
> sparse flagged this type mismatch. Also the xdr buffer usage is a bit
> non-standard, so explain what we're doing and why.
>
> Fixes: 4b64b811f368 ("nfsd: add notification handlers for dir events")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202607310455.AT0GoC5j-lkp@intel.com/
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4xdr.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index a47eb544b99f..5cbb4a415384 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4383,13 +4383,18 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
> *ne, struct xdr_stream *xdr,
>  	struct path path = nf->nf_file->f_path;
>  	struct nfsd4_fattr_args args = { };
>  	const u32 *reqmask;
> -	uint32_t *attrmask;
> +	u32 *attrmask;
>  	__be32 status;
>  	bool parent;
>  	int ret;
> 
> -	/* Reserve space for attrmask */
> -	attrmask = xdr_reserve_space(xdr, 3 * sizeof(uint32_t));
> +	/*
> +	 * attrmask is the host-order bmval[3] that nfsd4_encode_attr_vals()
> +	 * consumes and that ne_attrs.attrmask.element points at. It must
> +	 * outlive this call, so steal a few words from the xdr stream to hold
> +	 * it.
> +	 */
> +	attrmask = (u32 *)xdr_reserve_space(xdr, 3 * sizeof(*attrmask));
>  	if (!attrmask)
>  		return false;

I still don't understand what's going on. "Steal a few words to hold it"
sounds like you're trying to avoid a kmalloc call. Is this code reserving
space in the XDR stream, or isn't it? If it is, then accessing those bytes
has to be done with a big-endian pointer, the same way it is done at every
other xdr_reserve_space() call site. Storing host-order bytes that are not
part of an opaque into an XDR stream is just... wrong.

If you're doing the usual "reserve and backfill the encoded data later"
dance, then spell it the way everyone else does so us poor human readers
can recognize that's what's going on.


-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Jeff Layton]

On Mon, 2026-08-03 at 11:32 -0400, Chuck Lever wrote:
> 
> On Mon, Aug 3, 2026, at 10:49 AM, Jeff Layton wrote:
> > sparse flagged this type mismatch. Also the xdr buffer usage is a bit
> > non-standard, so explain what we're doing and why.
> > 
> > Fixes: 4b64b811f368 ("nfsd: add notification handlers for dir events")
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: 
> > https://lore.kernel.org/oe-kbuild-all/202607310455.AT0GoC5j-lkp@intel.com/
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4xdr.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index a47eb544b99f..5cbb4a415384 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -4383,13 +4383,18 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
> > *ne, struct xdr_stream *xdr,
> >  	struct path path = nf->nf_file->f_path;
> >  	struct nfsd4_fattr_args args = { };
> >  	const u32 *reqmask;
> > -	uint32_t *attrmask;
> > +	u32 *attrmask;
> >  	__be32 status;
> >  	bool parent;
> >  	int ret;
> > 
> > -	/* Reserve space for attrmask */
> > -	attrmask = xdr_reserve_space(xdr, 3 * sizeof(uint32_t));
> > +	/*
> > +	 * attrmask is the host-order bmval[3] that nfsd4_encode_attr_vals()
> > +	 * consumes and that ne_attrs.attrmask.element points at. It must
> > +	 * outlive this call, so steal a few words from the xdr stream to hold
> > +	 * it.
> > +	 */
> > +	attrmask = (u32 *)xdr_reserve_space(xdr, 3 * sizeof(*attrmask));
> >  	if (!attrmask)
> >  		return false;
> 
> I still don't understand what's going on. "Steal a few words to hold it"
> sounds like you're trying to avoid a kmalloc call. Is this code reserving
> space in the XDR stream, or isn't it? If it is, then accessing those bytes
> has to be done with a big-endian pointer, the same way it is done at every
> other xdr_reserve_space() call site. Storing host-order bytes that are not
> part of an opaque into an XDR stream is just... wrong.
> 
> If you're doing the usual "reserve and backfill the encoded data later"
> dance, then spell it the way everyone else does so us poor human readers
> can recognize that's what's going on.
> 

It's using a small piece of the xdr buffer (3 32-bit words) to hold the
host-endian bitmap fields that will later be encoded into the stream
when the encoding is actually done.

This piece is not accessed by anything else and there is plenty of room
in the buffer, so I don't see the issue here. Can you elaborate on why
you feel that this is a problem?
 
Certainly we could do this with yet another set of separate
allocations, but I'm not clear on how we'd track them to free them.
Each notify_entry4 gets a separate bitmap, so we'd need more than one.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Chuck Lever]

On Mon, Aug 3, 2026, at 11:43 AM, Jeff Layton wrote:
> On Mon, 2026-08-03 at 11:32 -0400, Chuck Lever wrote:
>> 
>> On Mon, Aug 3, 2026, at 10:49 AM, Jeff Layton wrote:
>> > sparse flagged this type mismatch. Also the xdr buffer usage is a bit
>> > non-standard, so explain what we're doing and why.
>> > 
>> > Fixes: 4b64b811f368 ("nfsd: add notification handlers for dir events")
>> > Reported-by: kernel test robot <lkp@intel.com>
>> > Closes: 
>> > https://lore.kernel.org/oe-kbuild-all/202607310455.AT0GoC5j-lkp@intel.com/
>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> > ---
>> >  fs/nfsd/nfs4xdr.c | 11 ++++++++---
>> >  1 file changed, 8 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> > index a47eb544b99f..5cbb4a415384 100644
>> > --- a/fs/nfsd/nfs4xdr.c
>> > +++ b/fs/nfsd/nfs4xdr.c
>> > @@ -4383,13 +4383,18 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
>> > *ne, struct xdr_stream *xdr,
>> >  	struct path path = nf->nf_file->f_path;
>> >  	struct nfsd4_fattr_args args = { };
>> >  	const u32 *reqmask;
>> > -	uint32_t *attrmask;
>> > +	u32 *attrmask;
>> >  	__be32 status;
>> >  	bool parent;
>> >  	int ret;
>> > 
>> > -	/* Reserve space for attrmask */
>> > -	attrmask = xdr_reserve_space(xdr, 3 * sizeof(uint32_t));
>> > +	/*
>> > +	 * attrmask is the host-order bmval[3] that nfsd4_encode_attr_vals()
>> > +	 * consumes and that ne_attrs.attrmask.element points at. It must
>> > +	 * outlive this call, so steal a few words from the xdr stream to hold
>> > +	 * it.
>> > +	 */
>> > +	attrmask = (u32 *)xdr_reserve_space(xdr, 3 * sizeof(*attrmask));
>> >  	if (!attrmask)
>> >  		return false;
>> 
>> I still don't understand what's going on. "Steal a few words to hold it"
>> sounds like you're trying to avoid a kmalloc call. Is this code reserving
>> space in the XDR stream, or isn't it? If it is, then accessing those bytes
>> has to be done with a big-endian pointer, the same way it is done at every
>> other xdr_reserve_space() call site. Storing host-order bytes that are not
>> part of an opaque into an XDR stream is just... wrong.
>> 
>> If you're doing the usual "reserve and backfill the encoded data later"
>> dance, then spell it the way everyone else does so us poor human readers
>> can recognize that's what's going on.
>
> It's using a small piece of the xdr buffer (3 32-bit words) to hold the
> host-endian bitmap fields that will later be encoded into the stream
> when the encoding is actually done.
>
> This piece is not accessed by anything else and there is plenty of room
> in the buffer, so I don't see the issue here. Can you elaborate on why
> you feel that this is a problem?

Well A-number-1 is that it's a hack and technical debt. It just
isn't obvious what's going on, it needs a four-line comment to
explain it, and it opens the door to people copy-pasting it to
other encoders.

Number 2 is that the XDR stream buffer is ephemeral. There are
conditions that callers must stick to to keep that pointer valid
for the lifetime of the xdr_stream.

Number 3: Yes, we have a sordid history of grabbing a piece of
the xdr_buf's tail iov for temporary storage. That doesn't make
it wise to do, and assumes behavior about that buffer that is
not guaranteed by sunrpc's API contracts. That makes life hard
when future changes need to change that buffer to, say, a page
rather than kmalloc'd memory. Or when I want to convert this
code to use xdrgen instead of hand-rolled encoding.

So what you've done is fine for operational prototype code, but
not something we can carry forward as production code. The sparse
warning is a canary, it's not the actual structural problem.


> Certainly we could do this with yet another set of separate
> allocations, but I'm not clear on how we'd track them to free them.
> Each notify_entry4 gets a separate bitmap, so we'd need more than one.

The server has a chain of temporary allocations that are freed
after the COMPOUND is released. Could do something similar for
the server's callback client.


-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Jeff Layton]

On Mon, 2026-08-03 at 12:04 -0400, Chuck Lever wrote:
> 
> On Mon, Aug 3, 2026, at 11:43 AM, Jeff Layton wrote:
> > On Mon, 2026-08-03 at 11:32 -0400, Chuck Lever wrote:
> > > 
> > > On Mon, Aug 3, 2026, at 10:49 AM, Jeff Layton wrote:
> > > > sparse flagged this type mismatch. Also the xdr buffer usage is a bit
> > > > non-standard, so explain what we're doing and why.
> > > > 
> > > > Fixes: 4b64b811f368 ("nfsd: add notification handlers for dir events")
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Closes: 
> > > > https://lore.kernel.org/oe-kbuild-all/202607310455.AT0GoC5j-lkp@intel.com/
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/nfsd/nfs4xdr.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > > index a47eb544b99f..5cbb4a415384 100644
> > > > --- a/fs/nfsd/nfs4xdr.c
> > > > +++ b/fs/nfsd/nfs4xdr.c
> > > > @@ -4383,13 +4383,18 @@ nfsd4_setup_notify_entry4(struct notify_entry4 
> > > > *ne, struct xdr_stream *xdr,
> > > >  	struct path path = nf->nf_file->f_path;
> > > >  	struct nfsd4_fattr_args args = { };
> > > >  	const u32 *reqmask;
> > > > -	uint32_t *attrmask;
> > > > +	u32 *attrmask;
> > > >  	__be32 status;
> > > >  	bool parent;
> > > >  	int ret;
> > > > 
> > > > -	/* Reserve space for attrmask */
> > > > -	attrmask = xdr_reserve_space(xdr, 3 * sizeof(uint32_t));
> > > > +	/*
> > > > +	 * attrmask is the host-order bmval[3] that nfsd4_encode_attr_vals()
> > > > +	 * consumes and that ne_attrs.attrmask.element points at. It must
> > > > +	 * outlive this call, so steal a few words from the xdr stream to hold
> > > > +	 * it.
> > > > +	 */
> > > > +	attrmask = (u32 *)xdr_reserve_space(xdr, 3 * sizeof(*attrmask));
> > > >  	if (!attrmask)
> > > >  		return false;
> > > 
> > > I still don't understand what's going on. "Steal a few words to hold it"
> > > sounds like you're trying to avoid a kmalloc call. Is this code reserving
> > > space in the XDR stream, or isn't it? If it is, then accessing those bytes
> > > has to be done with a big-endian pointer, the same way it is done at every
> > > other xdr_reserve_space() call site. Storing host-order bytes that are not
> > > part of an opaque into an XDR stream is just... wrong.
> > > 
> > > If you're doing the usual "reserve and backfill the encoded data later"
> > > dance, then spell it the way everyone else does so us poor human readers
> > > can recognize that's what's going on.
> > 
> > It's using a small piece of the xdr buffer (3 32-bit words) to hold the
> > host-endian bitmap fields that will later be encoded into the stream
> > when the encoding is actually done.
> > 
> > This piece is not accessed by anything else and there is plenty of room
> > in the buffer, so I don't see the issue here. Can you elaborate on why
> > you feel that this is a problem?
> 
> Well A-number-1 is that it's a hack and technical debt. It just
> isn't obvious what's going on, it needs a four-line comment to
> explain it, and it opens the door to people copy-pasting it to
> other encoders.
> 

Ok.

> Number 2 is that the XDR stream buffer is ephemeral. There are
> conditions that callers must stick to to keep that pointer valid
> for the lifetime of the xdr_stream.
> 

We only need these allocations to stick around until the encoding is
done. They have almost the same lifetime as the xdr buffer itself which
is why allocating from it made sense.

> Number 3: Yes, we have a sordid history of grabbing a piece of
> the xdr_buf's tail iov for temporary storage. That doesn't make
> it wise to do, and assumes behavior about that buffer that is
> not guaranteed by sunrpc's API contracts. That makes life hard
> when future changes need to change that buffer to, say, a page
> rather than kmalloc'd memory. Or when I want to convert this
> code to use xdrgen instead of hand-rolled encoding.
> 

I had no idea this practice was suddenly forbidden. That has certainly
not been communicated to me in any of the previous review rounds over
the last couple of years.


> So what you've done is fine for operational prototype code, but
> not something we can carry forward as production code. The sparse
> warning is a canary, it's not the actual structural problem.
> 

Does this mean you intend to drop dir delegation series again?

Allocating from the xdr stream in order to hold host-endian fields is
done in several places in this code. Reworking that will be a
substantial effort.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Chuck Lever]

On Mon, Aug 3, 2026, at 12:23 PM, Jeff Layton wrote:
> On Mon, 2026-08-03 at 12:04 -0400, Chuck Lever wrote:

>> Number 3: Yes, we have a sordid history of grabbing a piece of
>> the xdr_buf's tail iov for temporary storage. That doesn't make
>> it wise to do, and assumes behavior about that buffer that is
>> not guaranteed by sunrpc's API contracts. That makes life hard
>> when future changes need to change that buffer to, say, a page
>> rather than kmalloc'd memory. Or when I want to convert this
>> code to use xdrgen instead of hand-rolled encoding.
>
> I had no idea this practice was suddenly forbidden. That has certainly
> not been communicated to me in any of the previous review rounds over
> the last couple of years.

I've been removing instances of this anti-pattern for some time. I
haven't seen new code (until now) that tries to do this. It simply
hasn't come up recently and I typically don't look closely for it.


>> So what you've done is fine for operational prototype code, but
>> not something we can carry forward as production code. The sparse
>> warning is a canary, it's not the actual structural problem.
>
> Does this mean you intend to drop dir delegation series again?

No. It's deep in nfsd-next now, and this is a long-term issue, not
an immediate on-the-wire behavioral problem or a vulnerability.


-- 
Chuck Lever

Re: [PATCH 1/2] nfsd: fix type mismatch and explain host-endian xdr buffer usage

[Jeff Layton]

On Mon, 2026-08-03 at 14:33 -0400, Chuck Lever wrote:
> 
> On Mon, Aug 3, 2026, at 12:23 PM, Jeff Layton wrote:
> > On Mon, 2026-08-03 at 12:04 -0400, Chuck Lever wrote:
> 
> > > Number 3: Yes, we have a sordid history of grabbing a piece of
> > > the xdr_buf's tail iov for temporary storage. That doesn't make
> > > it wise to do, and assumes behavior about that buffer that is
> > > not guaranteed by sunrpc's API contracts. That makes life hard
> > > when future changes need to change that buffer to, say, a page
> > > rather than kmalloc'd memory. Or when I want to convert this
> > > code to use xdrgen instead of hand-rolled encoding.
> > 
> > I had no idea this practice was suddenly forbidden. That has certainly
> > not been communicated to me in any of the previous review rounds over
> > the last couple of years.
> 
> I've been removing instances of this anti-pattern for some time. I
> haven't seen new code (until now) that tries to do this. It simply
> hasn't come up recently and I typically don't look closely for it.
> 
> 
> > > So what you've done is fine for operational prototype code, but
> > > not something we can carry forward as production code. The sparse
> > > warning is a canary, it's not the actual structural problem.
> > 
> > Does this mean you intend to drop dir delegation series again?
> 
> No. It's deep in nfsd-next now, and this is a long-term issue, not
> an immediate on-the-wire behavioral problem or a vulnerability.
> 

Ok. I had an LLM draft up a propsed fix for this, but it's a bit
invasive and needs more testing. I'll see if I can put something
together in a follow-on series to the CB_NOTIFY patches.
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 2/2] nfsd: fix nfserr type in nfsd_lookup_dentry()

[Jeff Layton]

Both nfsd_lookup_dentry() and nfsd_cross_mnt() return __be32.

Fixes: cf78c5a7d742 ("nfsd: move check_nfsd_access() call into nfsd_cross_mnt()")
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/vfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index edb08c006a23..807e09521e0c 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -287,7 +287,7 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		if (IS_ERR(dentry))
 			goto out_nfserr;
 		if (nfsd_mountpoint(dentry, exp)) {
-			int nfserr = nfsd_cross_mnt(rqstp, &dentry, &exp);
+			__be32 nfserr = nfsd_cross_mnt(rqstp, &dentry, &exp);
 
 			if (nfserr) {
 				dput(dentry);

-- 
2.55.0

Re: [PATCH 2/2] nfsd: fix nfserr type in nfsd_lookup_dentry()

[Chuck Lever]

On Mon, Aug 3, 2026, at 10:49 AM, Jeff Layton wrote:
> Both nfsd_lookup_dentry() and nfsd_cross_mnt() return __be32.
>
> Fixes: cf78c5a7d742 ("nfsd: move check_nfsd_access() call into 
> nfsd_cross_mnt()")
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/vfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index edb08c006a23..807e09521e0c 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -287,7 +287,7 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct 
> svc_fh *fhp,
>  		if (IS_ERR(dentry))
>  			goto out_nfserr;
>  		if (nfsd_mountpoint(dentry, exp)) {
> -			int nfserr = nfsd_cross_mnt(rqstp, &dentry, &exp);
> +			__be32 nfserr = nfsd_cross_mnt(rqstp, &dentry, &exp);
> 
>  			if (nfserr) {
>  				dput(dentry);
>
> -- 
> 2.55.0

Folded into "nfsd: move check_nfsd_access() call into nfsd_cross_mnt()".

-- 
Chuck Lever