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

[liubaolin <liubaolin12138@163.com>]

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