[PATCH] NFSD: Prevent a NULL pointer dereference in fh_getattr()

[PATCH] NFSD: Prevent a NULL pointer dereference in fh_getattr()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

In general, fh_getattr() can be called after the target dentry has
gone negative. For a negative dentry, d_inode(p.dentry) will return
NULL. S_ISREG() will dereference that pointer.

Avoid this potential regression by using the d_is_reg() helper
instead.

Suggested-by: NeilBrown <neil@brown.name>
Fixes: d11f6cd1bb4a ("NFSD: filecache: add STATX_DIOALIGN and STATX_DIO_READ_ALIGN support")
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Mike Snitzer <snitzer@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfsfh.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Hi Anna -

nfsd-fixes is still based on v6.17-rc, so this patch does not apply
to it. Can you take it for v6.18-rc ?

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index ed85dd43da18..16182936828f 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -696,10 +696,9 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
 		.mnt		= fhp->fh_export->ex_path.mnt,
 		.dentry		= fhp->fh_dentry,
 	};
-	struct inode *inode = d_inode(p.dentry);
 	u32 request_mask = STATX_BASIC_STATS;
 
-	if (S_ISREG(inode->i_mode))
+	if (d_is_reg(p.dentry))
 		request_mask |= (STATX_DIOALIGN | STATX_DIO_READ_ALIGN);
 
 	if (fhp->fh_maxsize == NFS4_FHSIZE)
-- 
2.51.0

Re: [PATCH] NFSD: Prevent a NULL pointer dereference in fh_getattr()

[NeilBrown]

On Wed, 05 Nov 2025, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> In general, fh_getattr() can be called after the target dentry has
> gone negative. For a negative dentry, d_inode(p.dentry) will return
> NULL. S_ISREG() will dereference that pointer.

That isn't correct.  While a reference to a dentry is held the inode
cannot become NULL asynchronously.
It can change from NULL to non-NULL if another thread "creates".
It can become NULL if *this* thread calls unlink and no other thread has
a reference.
But it cannot suddenly become NULL.

I like the patch as it avoids a dereference and so puts less pressure on
the dcache, but it does not change correctness.

Sorry if I implied otherwise when I suggested it.

NeilBrown


> 
> Avoid this potential regression by using the d_is_reg() helper
> instead.
> 
> Suggested-by: NeilBrown <neil@brown.name>
> Fixes: d11f6cd1bb4a ("NFSD: filecache: add STATX_DIOALIGN and STATX_DIO_READ_ALIGN support")
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Reviewed-by: Mike Snitzer <snitzer@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfsfh.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> Hi Anna -
> 
> nfsd-fixes is still based on v6.17-rc, so this patch does not apply
> to it. Can you take it for v6.18-rc ?
> 
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index ed85dd43da18..16182936828f 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -696,10 +696,9 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
>  		.mnt		= fhp->fh_export->ex_path.mnt,
>  		.dentry		= fhp->fh_dentry,
>  	};
> -	struct inode *inode = d_inode(p.dentry);
>  	u32 request_mask = STATX_BASIC_STATS;
>  
> -	if (S_ISREG(inode->i_mode))
> +	if (d_is_reg(p.dentry))
>  		request_mask |= (STATX_DIOALIGN | STATX_DIO_READ_ALIGN);
>  
>  	if (fhp->fh_maxsize == NFS4_FHSIZE)
> -- 
> 2.51.0
> 
> 

Re: [PATCH] NFSD: Prevent a NULL pointer dereference in fh_getattr()

[Chuck Lever]

On 11/4/25 4:17 PM, NeilBrown wrote:
> On Wed, 05 Nov 2025, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> In general, fh_getattr() can be called after the target dentry has
>> gone negative. For a negative dentry, d_inode(p.dentry) will return
>> NULL. S_ISREG() will dereference that pointer.
> 
> That isn't correct.  While a reference to a dentry is held the inode
> cannot become NULL asynchronously.
> It can change from NULL to non-NULL if another thread "creates".
> It can become NULL if *this* thread calls unlink and no other thread has
> a reference.
> But it cannot suddenly become NULL.
> 
> I like the patch as it avoids a dereference and so puts less pressure on
> the dcache, but it does not change correctness.

I think the steps I'm worried about is if NFSD unlinks the file, and
then something subsequently invokes fh_getattr() assuming that is
safe to do.

How should I update the patch description?


> Sorry if I implied otherwise when I suggested it.
> 
> NeilBrown
> 
> 
>>
>> Avoid this potential regression by using the d_is_reg() helper
>> instead.
>>
>> Suggested-by: NeilBrown <neil@brown.name>
>> Fixes: d11f6cd1bb4a ("NFSD: filecache: add STATX_DIOALIGN and STATX_DIO_READ_ALIGN support")
>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>> Reviewed-by: Mike Snitzer <snitzer@kernel.org>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  fs/nfsd/nfsfh.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> Hi Anna -
>>
>> nfsd-fixes is still based on v6.17-rc, so this patch does not apply
>> to it. Can you take it for v6.18-rc ?
>>
>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>> index ed85dd43da18..16182936828f 100644
>> --- a/fs/nfsd/nfsfh.c
>> +++ b/fs/nfsd/nfsfh.c
>> @@ -696,10 +696,9 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
>>  		.mnt		= fhp->fh_export->ex_path.mnt,
>>  		.dentry		= fhp->fh_dentry,
>>  	};
>> -	struct inode *inode = d_inode(p.dentry);
>>  	u32 request_mask = STATX_BASIC_STATS;
>>  
>> -	if (S_ISREG(inode->i_mode))
>> +	if (d_is_reg(p.dentry))
>>  		request_mask |= (STATX_DIOALIGN | STATX_DIO_READ_ALIGN);
>>  
>>  	if (fhp->fh_maxsize == NFS4_FHSIZE)
>> -- 
>> 2.51.0
>>
>>
> 
> 


-- 
Chuck Lever

Re: [PATCH] NFSD: Prevent a NULL pointer dereference in fh_getattr()

[NeilBrown]

On Wed, 05 Nov 2025, Chuck Lever wrote:
> On 11/4/25 4:17 PM, NeilBrown wrote:
> > On Wed, 05 Nov 2025, Chuck Lever wrote:
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >>
> >> In general, fh_getattr() can be called after the target dentry has
> >> gone negative. For a negative dentry, d_inode(p.dentry) will return
> >> NULL. S_ISREG() will dereference that pointer.
> > 
> > That isn't correct.  While a reference to a dentry is held the inode
> > cannot become NULL asynchronously.
> > It can change from NULL to non-NULL if another thread "creates".
> > It can become NULL if *this* thread calls unlink and no other thread has
> > a reference.
> > But it cannot suddenly become NULL.
> > 
> > I like the patch as it avoids a dereference and so puts less pressure on
> > the dcache, but it does not change correctness.
> 
> I think the steps I'm worried about is if NFSD unlinks the file, and
> then something subsequently invokes fh_getattr() assuming that is
> safe to do.

nfsd never creates a svc_fh for the dentry is it about the unlink (or
rmdir).
A delete involves an fh for the 
I'm guessing 
Commit: 1519fbc8832b ("minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()")

is the problem.
parent, and a name.  No fh for the child.

> 
> How should I update the patch description?

Maybe just drop the patch for now.  There is no regression and nothing
to fix.  Maybe we can make the change latter as part of a cleanup.

NeilBrown


> 
> 
> > Sorry if I implied otherwise when I suggested it.
> > 
> > NeilBrown
> > 
> > 
> >>
> >> Avoid this potential regression by using the d_is_reg() helper
> >> instead.
> >>
> >> Suggested-by: NeilBrown <neil@brown.name>
> >> Fixes: d11f6cd1bb4a ("NFSD: filecache: add STATX_DIOALIGN and STATX_DIO_READ_ALIGN support")
> >> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >> Reviewed-by: Mike Snitzer <snitzer@kernel.org>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >>  fs/nfsd/nfsfh.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> Hi Anna -
> >>
> >> nfsd-fixes is still based on v6.17-rc, so this patch does not apply
> >> to it. Can you take it for v6.18-rc ?
> >>
> >> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> >> index ed85dd43da18..16182936828f 100644
> >> --- a/fs/nfsd/nfsfh.c
> >> +++ b/fs/nfsd/nfsfh.c
> >> @@ -696,10 +696,9 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat)
> >>  		.mnt		= fhp->fh_export->ex_path.mnt,
> >>  		.dentry		= fhp->fh_dentry,
> >>  	};
> >> -	struct inode *inode = d_inode(p.dentry);
> >>  	u32 request_mask = STATX_BASIC_STATS;
> >>  
> >> -	if (S_ISREG(inode->i_mode))
> >> +	if (d_is_reg(p.dentry))
> >>  		request_mask |= (STATX_DIOALIGN | STATX_DIO_READ_ALIGN);
> >>  
> >>  	if (fhp->fh_maxsize == NFS4_FHSIZE)
> >> -- 
> >> 2.51.0
> >>
> >>
> > 
> > 
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH] NFSD: Prevent a NULL pointer dereference in fh_getattr()

[Chuck Lever]

On 11/4/25 5:26 PM, NeilBrown wrote:
> On Wed, 05 Nov 2025, Chuck Lever wrote:
>> On 11/4/25 4:17 PM, NeilBrown wrote:
>>> On Wed, 05 Nov 2025, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>
>>>> In general, fh_getattr() can be called after the target dentry has
>>>> gone negative. For a negative dentry, d_inode(p.dentry) will return
>>>> NULL. S_ISREG() will dereference that pointer.
>>>
>>> That isn't correct.  While a reference to a dentry is held the inode
>>> cannot become NULL asynchronously.
>>> It can change from NULL to non-NULL if another thread "creates".
>>> It can become NULL if *this* thread calls unlink and no other thread has
>>> a reference.
>>> But it cannot suddenly become NULL.
>>>
>>> I like the patch as it avoids a dereference and so puts less pressure on
>>> the dcache, but it does not change correctness.
>>
>> I think the steps I'm worried about is if NFSD unlinks the file, and
>> then something subsequently invokes fh_getattr() assuming that is
>> safe to do.
> 
> nfsd never creates a svc_fh for the dentry is it about the unlink (or
> rmdir).
> A delete involves an fh for the 
> I'm guessing 
> Commit: 1519fbc8832b ("minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()")
> 
> is the problem.
> parent, and a name.  No fh for the child.
> 
>>
>> How should I update the patch description?
> 
> Maybe just drop the patch for now.  There is no regression and nothing
> to fix.  Maybe we can make the change latter as part of a cleanup.

OK, then. Anna, you can ignore this one and I will drop it from
nfsd-testing.


-- 
Chuck Lever