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

[Hongling Zeng <zhongling0719@126.com>]

在 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. 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);