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

[liubaolin <liubaolin12138@163.com>]

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