On Tue, Aug 05, 2025 at 02:00:30PM +0530, Ojaswin Mujoo wrote:
> With bigalloc enabled, the logic to report last extent has a bug since
> we try to use cluster units instead of block units. This can cause an issue
> where extra incorrect entries might be returned back to the user. This was
> flagged by generic/365 with 64k bs and -O bigalloc.
>
> ** Details of issue **
>
> The issue was noticed on 5G 64k blocksize FS with -O bigalloc which has
> only 1 bg.
>
> $ xfs_io -c "fsmap -d" /mnt/scratch
>
> 0: 253:48 [0..127]: static fs metadata 128 /* sb */
> 1: 253:48 [128..255]: special 102:1 128 /* gdt */
> 3: 253:48 [256..383]: special 102:3 128 /* block bitmap */
> 4: 253:48 [384..2303]: unknown 1920 /* flex bg empty space */
> 5: 253:48 [2304..2431]: special 102:4 128 /* inode bitmap */
> 6: 253:48 [2432..4351]: unknown 1920 /* flex bg empty space */
> 7: 253:48 [4352..6911]: inodes 2560
> 8: 253:48 [6912..538623]: unknown 531712
> 9: 253:48 [538624..10485759]: free space 9947136
>
> The issue can be seen with:
>
> $ xfs_io -c "fsmap -d 0 3" /mnt/scratch
>
> 0: 253:48 [0..127]: static fs metadata 128
> 1: 253:48 [384..2047]: unknown 1664
>
> Only the first entry was expected to be returned but we get 2. This is
> because:
>
> ext4_getfsmap_datadev()
> first_cluster, last_cluster = 0
> ...
> info->gfi_last = true;
> ext4_getfsmap_datadev_helper(sb, end_ag, last_cluster + 1, 0, info);
> fsb = C2B(1) = 16
> fslen = 0
> ...
> /* Merge in any relevant extents from the meta_list */
> list_for_each_entry_safe(p, tmp, &info->gfi_meta_list, fmr_list) {
> ...
> // since fsb = 16, considers all metadata which starts before 16 blockno
> iter 1: error = ext4_getfsmap_helper(sb, info, p); // p = sb (0,1), nop
> info->gfi_next_fsblk = 1
> iter 2: error = ext4_getfsmap_helper(sb, info, p); // p = gdt (1,2), nop
> info->gfi_next_fsblk = 2
> iter 3: error = ext4_getfsmap_helper(sb, info, p); // p = blk bitmap (2,3), nop
> info->gfi_next_fsblk = 3
> iter 4: error = ext4_getfsmap_helper(sb, info, p); // p = ino bitmap (18,19)
> if (rec_blk > info->gfi_next_fsblk) { // (18 > 3)
> // emits an extra entry ** BUG **
> }
> }
>
> Fix this by directly calling ext4_getfsmap_datadev() with a dummy record
> that has fmr_physical set to (end_fsb + 1) instead of last_cluster + 1. By
> using the block instead of cluster we get the correct behavior.
>
> Replacing ext4_getfsmap_datadev_helper() with ext4_getfsmap_helper() is
> okay since the gfi_lastfree and metadata checks in
> ext4_getfsmap_datadev_helper() are anyways redundant when we only want to
> emit the last allocated block of the range, as we have already taken care
> of emitting metadata and any last free blocks.
>
> Reported-by: Disha Goel <disgoel@xxxxxxxxxxxxx>
> Fixes: 4a622e4d477b ("ext4: fix FS_IOC_GETFSMAP handling")
> Signed-off-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
Looks fine to me
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--D
> ---
> fs/ext4/fsmap.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c
> index 383c6edea6dd..9d63c39f6077 100644
> --- a/fs/ext4/fsmap.c
> +++ b/fs/ext4/fsmap.c
> @@ -526,6 +526,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
> ext4_group_t end_ag;
> ext4_grpblk_t first_cluster;
> ext4_grpblk_t last_cluster;
> + struct ext4_fsmap irec;
> int error = 0;
>
> bofs = le32_to_cpu(sbi->s_es->s_first_data_block);
> @@ -609,10 +610,18 @@ static int ext4_getfsmap_datadev(struct super_block *sb,
> goto err;
> }
>
> - /* Report any gaps at the end of the bg */
> + /*
> + * The dummy record below will cause ext4_getfsmap_helper() to report
> + * any allocated blocks at the end of the range.
> + */
> + irec.fmr_device = 0;
> + irec.fmr_physical = end_fsb + 1;
> + irec.fmr_length = 0;
> + irec.fmr_owner = EXT4_FMR_OWN_FREE;
> + irec.fmr_flags = 0;
> +
> info->gfi_last = true;
> - error = ext4_getfsmap_datadev_helper(sb, end_ag, last_cluster + 1,
> - 0, info);
> + error = ext4_getfsmap_helper(sb, info, &irec);
> if (error)
> goto err;
>
> --
> 2.49.0
>
>
