Re: [PATCH] btrfs: handle highmem folios in read_key_bytes

[Hongling Zeng <zhongling0719@126.com>]

在 2026年08月17日 14:02, Qu Wenruo 写道:
>
>
> 在 2026/8/17 13:44, Hongling Zeng 写道:
>>
>> 在 2026年08月17日 10:55, Qu Wenruo 写道:
>>>
>>>
>>> 在 2026/8/17 11:50, Hongling Zeng 写道:
>>>> On 32-bit systems with highmem, folio_address() can return NULL for
>>>> unmapped highmem folios. When this NULL is passed as the dest 
>>>> parameter
>>>> to read_key_bytes() with a non-NULL dest_folio, it violates the 
>>>> function's
>>>> contract (which requires dest to be non-NULL when dest_folio is 
>>>> provided).
>>>
>>> But metadata folios are not allocated by page cache, but by btrfs 
>>> itself, which always use GFP_NOFS then attach the folio to page 
>>> cache, thus should not get highmem memory in the first place.
>>>
>>> And there is no way to trigger read from userspace on btree inode, 
>>> so there should be no highmem folios from the beginning.
>>    Thanks for the review.
>>
>>    I was not able to reproduce this on a real system. This was 
>> identified through code analysis with assistance from glm 5.5, not 
>> from a
>>    real-world encounter.
>>
>>    Your analysis about btrfs using ~__GFP_FS for folio allocation and 
>> the lack
>>    of userspace trigger paths makes sense.
>
> Sorry, it doesn't.
>
> I mis-read the context and considered the folio to be from btree 
> inode, but it's not.
>
> So it can still be from page cache, which can be highmem.
>
> But on the other hand, I do not think your fix is doing any good to 
> the readability either.
>
> I'll rework the involved function and caller to remove the folio 
> parameter, and instead always pass a vaddr and length to 
> read_key_bytes(), and let the caller to handle the kmap instead.
   Thanks for the feedback.

   I understand your rework plan. Will it also address the folio_unlock
   issue in the error path? The current code leaves the folio permanently
   locked after filemap_add_folio() + read_key_bytes() failure.

   If your rework doesn't cover this, could you help to review the 
folio_unlock
   fix patch?

>
>> The proposed changes are based on the
>>    generic folio_address() behavior and the theoretical dest/dest_folio
>>    invariant, but I do not currently have evidence that a highmem 
>> folio can
>>    reach read_key_bytes() in this state.
>>
>>    I will withdraw this patch rather than claim a reachable highmem bug
>>    without a reproducer.
>>
>>>
>>>>
>>>> The original bug had two symptoms:
>>>> 1. Unsigned len underflow when len -= copy_bytes executes (infinite 
>>>> loop)
>>>> 2. The folio remains uninitialized because the copy block is skipped
>>>
>>> So did you really hit the problem in the real world?
>>>
>>>>
>>>> Fix requires two changes:
>>>>
>>>> 1. Change "if (!dest)" to "if (!dest && !dest_folio)"
>>>>     - Prevents the "counting-only" mode when dest_folio is provided
>>>>     - Fixes the underflow/infinite loop
>>>>
>>>> 2. Change "if (dest)" to "if (dest || dest_folio)"
>>>>     - Ensures the copy block executes when dest_folio is provided
>>>>     - Allows kmap_local_folio() to properly map the highmem folio
>>>>     - Actually writes data to the folio
>>>>
>>>> Without the second change, the highmem folio is not populated even
>>>> though the read succeeds, causing subsequent fs-verity verification to
>>>> operate on stale or uninitialized data.
>>>>
>>>> Fixes: 884937793db5 ("btrfs: convert read_key_bytes() to take a 
>>>> folio")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>>>
>>> Missing LLM disclosure.
>>>> ---
>>>>   fs/btrfs/verity.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/verity.c b/fs/btrfs/verity.c
>>>> index 983365a73541..80bc945c7dcb 100644
>>>> --- a/fs/btrfs/verity.c
>>>> +++ b/fs/btrfs/verity.c
>>>> @@ -351,7 +351,7 @@ static int read_key_bytes(struct btrfs_inode 
>>>> *inode, u8 key_type, u64 offset,
>>>>           }
>>>>             /* desc = NULL to just sum all the item lengths */
>>>> -        if (!dest)
>>>> +        if (!dest && !dest_folio)
>>>>               copy_end = item_end;
>>>>           else
>>>>               copy_end = min(offset + len, item_end);
>>>> @@ -362,7 +362,7 @@ static int read_key_bytes(struct btrfs_inode 
>>>> *inode, u8 key_type, u64 offset,
>>>>           /* Offset from the start of item for copying */
>>>>           copy_offset = offset - key.offset;
>>>>   -        if (dest) {
>>>> +        if (dest || dest_folio) {
>>>>               if (dest_folio)
>>>>                   kaddr = kmap_local_folio(dest_folio, 0);
>>