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

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

[Baolin Liu]

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

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

[Markus Elfring]

…
> Fix this by checking folio before using it or calling
> folio_end_dropbehind().

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17#n145


…
> ---
>  fs/nfs/write.c | 11 ++++++-----
…

Can a summary phrase like “Prevent null pointer dereference in nfs_inode_remove_request()”
be nicer?


Will development interests grow for the application of scope-based resource management?
https://elixir.bootlin.com/linux/v6.17.1/source/include/linux/spinlock.h#L565-L567

Regards,
Markus

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

[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

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

[liubaolin]

> 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.



在 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?
> 
> 

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

[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

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

[liubaolin]

> 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.



在 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?
>>>
>>>
>>
> 

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

[Trond Myklebust]

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

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

[Trond Myklebust]

On Tue, 2025-10-21 at 23:15 -0400, Trond Myklebust wrote:
> 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.

Sorry. I meant to say there is no need to add a "Fixes" or a "Cc:
stable" tag...

> 
> > 
> > 在 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?
> > > > > 
> > > > > 
> > > > 
> > > 

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

[Dan Carpenter]

On Tue, Oct 21, 2025 at 11:15:21PM -0400, Trond Myklebust wrote:
> 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...

Yes.  That's the gist of it.

However Smatch can tell that the check is not needed then the warning
won't be printed.  In this case, Smatch breaks the return values from
nfs_page_to_folio() down like this, and it thinks folio can be NULL.

fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|        INTERNAL | -1 |                  175 | struct folio*(*)(struct nfs_page*) |
fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|     PARAM_LIMIT |  0 |                    $ |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|   PARAM_COMPARE | -1 |                    $ |      == $0->wb_folio |
fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|        STMT_CNT | -1 |                      |                   22 |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|        INTERNAL | -1 |                  175 | struct folio*(*)(struct nfs_page*) |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|     PARAM_LIMIT |  0 |                    $ |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|     PARAM_LIMIT |  0 |          $->wb_folio |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|   PARAM_COMPARE | -1 |                    $ |      == $0->wb_folio |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|        STMT_CNT | -1 |                      |                   22 |
fs/nfs/write.c | nfs_page_to_folio | 342 |             0|        INTERNAL | -1 |                  176 | struct folio*(*)(struct nfs_page*) |
fs/nfs/write.c | nfs_page_to_folio | 342 |             0|     PARAM_LIMIT |  0 |                    $ |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 342 |             0|        STMT_CNT | -1 |                      |                   22 |

But Smatch is taking short cuts in its analysis and it doesn't track
bit tests so it's going to be wrong sometimes.

> 
> 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.

Removing the check is probably the correct response to these warnings
more often than not.

regards,
dan carpenter

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

[Dan Carpenter]

On Wed, Oct 22, 2025 at 10:34:56AM +0300, Dan Carpenter wrote:
> On Tue, Oct 21, 2025 at 11:15:21PM -0400, Trond Myklebust wrote:
> > 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...
> 
> Yes.  That's the gist of it.
> 
> However Smatch can tell that the check is not needed then the warning
> won't be printed.

However IF Smatch...

Gar.

regards,
dan carpenter