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@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> > >
> > > 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);