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

[Chuck Lever <cel@kernel.org>]

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