Patch "btrfs: set start on clone before calling copy_extent_buffer_full" has been added to the 6.9-stable tree

[Sasha Levin <sashal@xxxxxxxxxx>]

This is a note to let you know that I've just added the patch titled

    btrfs: set start on clone before calling copy_extent_buffer_full

to the 6.9-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     btrfs-set-start-on-clone-before-calling-copy_extent_.patch
and it can be found in the queue-6.9 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.



commit e8abefb34d3792f1e7b38bc1b76b6a58d766a080
Author: Josef Bacik <josef@xxxxxxxxxxxxxx>
Date:   Sun Apr 14 05:42:43 2024 +0000

    btrfs: set start on clone before calling copy_extent_buffer_full
    
    [ Upstream commit 53e24158684b527d013b5b2204ccb34d1f94c248 ]
    
    Our subpage testing started hanging on generic/560 and I bisected it
    down to 1cab1375ba6d ("btrfs: reuse cloned extent buffer during
    fiemap to avoid re-allocations").  This is subtle because we use
    eb->start to figure out where in the folio we're copying to when we're
    subpage, as our ->start may refer to an area inside of the folio.
    
    For example, assume a 16K page size machine with a 4K node size, and
    assume that we already have a cloned extent buffer when we cloned the
    previous search.
    
    copy_extent_buffer_full() will do the following when copying the extent
    buffer path->nodes[0] (src) into cloned (dest):
    
      src->start = 8k; // this is the new leaf we're cloning
      cloned->start = 4k; // this is left over from the previous clone
    
      src_addr = folio_address(src->folios[0]);
      dest_addr = folio_address(dest->folios[0]);
    
      memcpy(dest_addr + get_eb_offset_in_folio(dst, 0),
             src_addr + get_eb_offset_in_folio(src, 0), src->len);
    
    Now get_eb_offset_in_folio() is where the problems occur, because for
    sub-pagesize blocksize we can have multiple eb's per folio, the code for
    this is as follows
    
      size_t get_eb_offset_in_folio(eb, offset) {
              return (eb->start + offset & (folio_size(eb->folio[0]) - 1));
      }
    
    So in the above example we are copying into offset 4K inside the folio.
    However once we update cloned->start to 8K to match the src the math for
    get_eb_offset_in_folio() changes, and any subsequent reads (i.e.
    btrfs_item_key_to_cpu()) will start reading from the offset 8K instead
    of 4K where we copied to, giving us garbage.
    
    Fix this by setting start before we co copy_extent_buffer_full() to make
    sure that we're copying into the same offset inside of the folio that we
    will read from later.
    
    All other sites of copy_extent_buffer_full() are correct because we
    either set ->start beforehand or we simply don't change it in the case
    of the tree-log usage.
    
    With this fix we now pass generic/560 on our subpage tests.
    
    Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during fiemap to avoid re-allocations")
    Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
    Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
    Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
    Signed-off-by: David Sterba <dsterba@xxxxxxxx>
    Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2776112dbdf8d..87f487b116577 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2773,13 +2773,19 @@ static int fiemap_next_leaf_item(struct btrfs_inode *inode, struct btrfs_path *p
 		goto out;
 	}
 
-	/* See the comment at fiemap_search_slot() about why we clone. */
-	copy_extent_buffer_full(clone, path->nodes[0]);
 	/*
 	 * Important to preserve the start field, for the optimizations when
 	 * checking if extents are shared (see extent_fiemap()).
+	 *
+	 * We must set ->start before calling copy_extent_buffer_full().  If we
+	 * are on sub-pagesize blocksize, we use ->start to determine the offset
+	 * into the folio where our eb exists, and if we update ->start after
+	 * the fact then any subsequent reads of the eb may read from a
+	 * different offset in the folio than where we originally copied into.
 	 */
 	clone->start = path->nodes[0]->start;
+	/* See the comment at fiemap_search_slot() about why we clone. */
+	copy_extent_buffer_full(clone, path->nodes[0]);
 
 	slot = path->slots[0];
 	btrfs_release_path(path);