Re: [PATCH v2 2/6] btrfs-progs: fix a bug in btrfs_find_item()

[David Sterba <dsterba@suse.cz>]

On Thu, May 15, 2025 at 05:30:17PM +0930, Qu Wenruo wrote:
> [BUG]
> There is a seldomly utilized function, btrfs_find_item(), which has no
> document and is not behaving correctly.
> 
> Inside backref.c, iterate_inode_refs() and btrfs_ref_to_path() both
> utilize this function, to find the parent inode.
> 
> However btrfs_find_item() will never return 0 if @ioff is passed as 0
> for such usage, result early failure for all kinds of inode iteration
> functions.
> 
> [CAUSE]
> Both functions pass 0 as the @ioff parameter initially, e.g:
> 
>  We have the following fs tree root:
> 
>   	item 0 key (256 INODE_ITEM 0) itemoff 16123 itemsize 160
> 		generation 3 transid 9 size 6 nbytes 16384
> 		block group 0 mode 40755 links 1 uid 0 gid 0 rdev 0
> 		sequence 1 flags 0x0(none)
> 	item 1 key (256 INODE_REF 256) itemoff 16111 itemsize 12
> 		index 0 namelen 2 name: ..
> 	item 2 key (256 DIR_ITEM 2507850652) itemoff 16078 itemsize 33
> 		location key (257 INODE_ITEM 0) type FILE
> 		transid 9 data_len 0 name_len 3
> 		name: foo
> 	item 3 key (256 DIR_INDEX 2) itemoff 16045 itemsize 33
> 		location key (257 INODE_ITEM 0) type FILE
> 		transid 9 data_len 0 name_len 3
> 		name: foo
> 	item 4 key (257 INODE_ITEM 0) itemoff 15885 itemsize 160
> 		generation 9 transid 9 size 16384 nbytes 16384
> 		block group 0 mode 100600 links 1 uid 0 gid 0 rdev 0
> 		sequence 4 flags 0x0(none)
> 	item 5 key (257 INODE_REF 256) itemoff 15872 itemsize 13
> 		index 2 namelen 3 name: foo
> 	item 6 key (257 EXTENT_DATA 0) itemoff 15819 itemsize 53
> 		generation 9 type 1 (regular)
> 		extent data disk byte 13631488 nr 16384
> 		extent data offset 0 nr 16384 ram 16384
> 		extent compression 0 (none)
> 
>   Then we call paths_from_inode() with:
>   - @inum = 257
>   - ipath = {.fs_root = 5}
> 
>   Then we got the following sequence:
> 
>   iterate_irefs(257, fs_root, inode_to_path)
>   |- iterate_inode_refs()
>      |- inode_ref_info()
>         |- btrfs_find_item(257, 0, fs_root)
> 	|  Returned 1, with @found_key updated to
> 	|  (257, INODE_REF, 256).
> 
>   This makes iterate_irefs() exit immediately, but obviously that
>   btrfs_find_item() call is to find any INODE_REF, not to find the
>   exact match.
> 
> [FIX]
> If btrfs_find_item() found an item matching the objectid and type, then
> it should return 0 other than 1.
> 
> Fix it and keep the behavior the same across btrfs-progs and the kernel.
> 
> Since we're here, also add some comments explaining the function.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  kernel-shared/ctree.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel-shared/ctree.c b/kernel-shared/ctree.c
> index 3184c916175e..f90de606e7b1 100644
> --- a/kernel-shared/ctree.c
> +++ b/kernel-shared/ctree.c
> @@ -1246,6 +1246,17 @@ static void reada_for_search(struct btrfs_fs_info *fs_info,
>  	}
>  }
>  
> +/*
> + * Find the first key in @fs_root that matches all the following conditions:
> + *
> + * - key.obojectid == @iobjectid
> + * - key.type == @key_type
> + * - key.offset >= ioff
> + *
> + * Return 0 if such key can be found, and @found_key is updated.
> + * Return >0 if no such key can be found.
> + * Return <0 for critical errors.
> + */
>  int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *found_path,
>  		u64 iobjectid, u64 ioff, u8 key_type,
>  		struct btrfs_key *found_key)
> @@ -1280,10 +1291,10 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *found_path,
>  
>  	btrfs_item_key_to_cpu(eb, found_key, path->slots[0]);
>  	if (found_key->type != key.type ||
> -			found_key->objectid != key.objectid) {
> +	    found_key->objectid != key.objectid)

This fits one line. We don't have perfect 1:1 line matching with kernel
so the style can be fixed.

>  		ret = 1;
> -		goto out;
> -	}
> +	else
> +		ret = 0;
>  
>  out:
>  	if (path != found_path)
> -- 
> 2.49.0
>