Re: [PATCH v1] NFS: Fix possible NULL pointer dereference in nfs_inode_remove_request()

[Trond Myklebust <trondmy@kernel.org>]

On Wed, 2025-10-22 at 10:44 +0800, liubaolin wrote:
> > Sorry, I didn’t actually see any case where req->wb_head == NULL. 
> > I found this through a smatch warning that pointed out a potential
> > null pointer dereference. 
> > Instead of removing the NULL folio check, I prefer to keep it to
> > prevent this potential issue. Checking pointer validity before use
> > is a good practice. 
> > From a maintenance perspective, we can’t rule out the possibility
> > that future changes might introduce a req->wb_head == NULL case, so
> > I suggest keeping the NULL folio check.
> 

I think you need to look at how smatch works in these situations. It is
not looking at the call chain, but is rather looking at how the
function is structured.
Specifically, as I understand it, smatch looks at whether a test for a
NULL pointer exists, and whether it is placed before or after the
pointer is dereferenced. So it has nothing to say about whether the
check is needed; all it says is that *if* the check is needed, then it
should be placed differently.
Dan Carpenter, please correct me if my information above is outdated...

So in this case, since we've never seen a case where the NULL check is
violated, and an analysis of the call chain doesn't show up any
(remaining) cases where that NULL pointer test is needed, my
recommendation is that we just remove the test going forward.

We should not need to add a "Tested" or "stable" tag, since this test
is harmless, and so the change is just an optimisation.

> 
> 在 2025/10/17 23:02, Trond Myklebust 写道:
> > On Fri, 2025-10-17 at 14:57 +0800, liubaolin wrote:
> > > [You don't often get email from liubaolin12138@163.com. Learn why
> > > this is important at
> > > https://aka.ms/LearnAboutSenderIdentification ]
> > > 
> > > > This modification addresses a potential issue detected by
> > > > Smatch
> > > > during a scan of the NFS code. After reviewing the relevant
> > > > code, I
> > > > confirmed that the change is required to remove the potential
> > > > risk.
> > > 
> > > 
> > 
> > I'm sorry, but I'm still not seeing why we can't just remove the
> > check
> > for a NULL folio.
> > 
> > Under what circumstances do you see us calling
> > nfs_inode_remove_request() with a request that has req->wb_head ==
> > NULL? I'm asking for a concrete example.
> > 
> > > 
> > > 在 2025/10/13 12:47, Trond Myklebust 写道:
> > > > On Sun, 2025-10-12 at 16:39 +0800, Baolin Liu wrote:
> > > > > [You don't often get email from liubaolin12138@163.com. Learn
> > > > > why
> > > > > this is important at
> > > > > https://aka.ms/LearnAboutSenderIdentification ]
> > > > > 
> > > > > From: Baolin Liu <liubaolin@kylinos.cn>
> > > > > 
> > > > > nfs_page_to_folio(req->wb_head) may return NULL in certain
> > > > > conditions,
> > > > > but the function dereferences folio->mapping and calls
> > > > > folio_end_dropbehind(folio) unconditionally. This may cause a
> > > > > NULL
> > > > > pointer dereference crash.
> > > > > 
> > > > > Fix this by checking folio before using it or calling
> > > > > folio_end_dropbehind().
> > > > > 
> > > > > Signed-off-by: Baolin Liu <liubaolin@kylinos.cn>
> > > > > ---
> > > > >    fs/nfs/write.c | 11 ++++++-----
> > > > >    1 file changed, 6 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > > > > index 0fb6905736d5..e148308c1923 100644
> > > > > --- a/fs/nfs/write.c
> > > > > +++ b/fs/nfs/write.c
> > > > > @@ -739,17 +739,18 @@ static void
> > > > > nfs_inode_remove_request(struct
> > > > > nfs_page *req)
> > > > >           nfs_page_group_lock(req);
> > > > >           if (nfs_page_group_sync_on_bit_locked(req,
> > > > > PG_REMOVE)) {
> > > > >                   struct folio *folio =
> > > > > nfs_page_to_folio(req-
> > > > > > wb_head);
> > > > > -               struct address_space *mapping = folio-
> > > > > >mapping;
> > > > > 
> > > > > -               spin_lock(&mapping->i_private_lock);
> > > > >                   if (likely(folio)) {
> > > > > +                       struct address_space *mapping =
> > > > > folio-
> > > > > > mapping;
> > > > > +
> > > > > +                       spin_lock(&mapping->i_private_lock);
> > > > >                           folio->private = NULL;
> > > > >                           folio_clear_private(folio);
> > > > >                           clear_bit(PG_MAPPED, &req->wb_head-
> > > > > > wb_flags);
> > > > > -               }
> > > > > -               spin_unlock(&mapping->i_private_lock);
> > > > > +                       spin_unlock(&mapping-
> > > > > >i_private_lock);
> > > > > 
> > > > > -               folio_end_dropbehind(folio);
> > > > > +                       folio_end_dropbehind(folio);
> > > > > +               }
> > > > >           }
> > > > >           nfs_page_group_unlock(req);
> > > > > 
> > > > > --
> > > > > 2.39.2
> > > > > 
> > > > 
> > > > What reason is there to believe that we can ever call
> > > > nfs_inode_remove_request() with a NULL value for req->wb_head-
> > > > > wb_folio, or even with a NULL value for req->wb_head-
> > > > > >wb_folio-
> > > > > mapping?
> > > > 
> > > > 
> > > 
> > 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com