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

[Trond Myklebust <trondmy@kernel.org>]

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