[PATCH] btrfs: handle highmem folios in read_key_bytes

[PATCH] btrfs: handle highmem folios in read_key_bytes

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

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

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>
---
 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);
 
-- 
2.25.1

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

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

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

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

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

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

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

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

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

[Hongling Zeng]

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