Re: [PATCH] fs/ext2.c: fix handling of missing sparse extent leafs

[Daniel Kiper]

On Fri, Sep 10, 2021 at 09:54:31AM +0200, Krzysztof Nowicki wrote:
> When a file on ext4 is stored as sparse the data belonging to
> zero-filled blocks is not written to storage and the extent map is
> missing entries for these blocks. Such case can happen both for depth
> 0 extents (leafs) as well as higher-level tables.
>
> Consider a scenario of a file which has a zero-filled
> beginning (e.g. ISO image). In such case real data starts at block
> 8. If such a file is stored using 2-level extent structure the extent
> list in the inode will be depth 1 and will have an entry to a depth
> 0 (leaf) extent header for blocks 8-n.
>
> Unfortunately existing GRUB2 ext2 driver is only able to handle
> missing entries in leaf extent tables, for which the
> grub_ext2_read_block() function returns 0. In case the whole leaf
> extent list is missing for a block the function fails with "invalid
> extent" error.
>
> The fix for this problem relies on the grub_ext4_find_leaf() helper
> function to distinguish two error cases: missing extent and error
> walking through the extent tree. The existing error message is raised
> only for the latter case, while for the missing leaf extent zero is
> returned from grub_ext2_read_block() indicating a sparse block.
>
> Signed-off-by: Krzysztof Nowicki <krzysztof.nowicki@nokia.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> except two nitpicks
below which I fix before committing...

> ---
>  grub-core/fs/ext2.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/fs/ext2.c b/grub-core/fs/ext2.c
> index ac33bcd68..884ffaa97 100644
> --- a/grub-core/fs/ext2.c
> +++ b/grub-core/fs/ext2.c
> @@ -407,13 +407,15 @@ grub_ext2_blockgroup (struct grub_ext2_data *data, 
> grub_uint64_t group,
>                        sizeof (struct grub_ext2_block_group), blkgrp);
>  }
>
> -static struct grub_ext4_extent_header *
> +static grub_err_t
>  grub_ext4_find_leaf (struct grub_ext2_data *data,
>                       struct grub_ext4_extent_header *ext_block,
> -                     grub_uint32_t fileblock)
> +                     grub_uint32_t fileblock,
> +                     struct grub_ext4_extent_header **leaf)
>  {
>    struct grub_ext4_extent_idx *index;
>    void *buf = NULL;
> +  *leaf = NULL;
>
>    while (1)
>      {
> @@ -426,7 +428,10 @@ grub_ext4_find_leaf (struct grub_ext2_data *data,
>       goto fail;
>
>        if (ext_block->depth == 0)
> -        return ext_block;
> +        {
> +          *leaf = ext_block;
> +          return GRUB_ERR_NONE;
> +        }
>
>        for (i = 0; i < grub_le_to_cpu16 (ext_block->entries); i++)
>          {
> @@ -435,7 +440,10 @@ grub_ext4_find_leaf (struct grub_ext2_data *data,
>          }
>
>        if (--i < 0)
> -     goto fail;
> +        {
> +          grub_free (buf);
> +          return GRUB_ERR_NONE;
> +        }
>
>        block = grub_le_to_cpu16 (index[i].leaf_hi);
>        block = (block << 32) | grub_le_to_cpu32 (index[i].leaf);
> @@ -452,7 +460,7 @@ grub_ext4_find_leaf (struct grub_ext2_data *data,
>      }
>   fail:
>    grub_free (buf);
> -  return 0;
> +  return GRUB_ERR_BAD_FS;
>  }
>
>  static grub_disk_addr_t
> @@ -474,13 +482,16 @@ grub_ext2_read_block (grub_fshelp_node_t node, 
> grub_disk_addr_t fileblock)
>        int i;
>        grub_disk_addr_t ret;
>
> -      leaf = grub_ext4_find_leaf (data, (struct grub_ext4_extent_header *) 
> inode->blocks.dir_blocks, fileblock);
> -      if (! leaf)
> +      if (grub_ext4_find_leaf (data, (struct grub_ext4_extent_header *) 
> inode->blocks.dir_blocks, fileblock, &leaf))

if (grub_ext4_find_leaf (...) != GRUB_ERR_NONE)

>          {
>            grub_error (GRUB_ERR_BAD_FS, "invalid extent");
>            return -1;
>          }
>
> +      if (!leaf)

if (leaf == NULL)

> +        /* Leaf for the given block is absent (i.e. sparse) */
> +        return 0;
> +
>        ext = (struct grub_ext4_extent *) (leaf + 1);
>        for (i = 0; i < grub_le_to_cpu16 (leaf->entries); i++)
>          {

Daniel