Re: [PATCH v11 1/2] fs/erofs: Add support for EROFS - Daniel Kiper via Grub-devel

From: Daniel Kiper via Grub-devel <grub-devel@gnu.org>
To: Gao Xiang <hsiangkao@linux.alibaba.com>
Cc: Daniel Kiper <daniel.kiper@oracle.com>,
	Glenn Washburn <development@efficientek.com>,
	Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>,
	Daniel Axtens <dja@axtens.net>,
	grub-devel@gnu.org, Yifan Zhao <zhaoyifan@sjtu.edu.cn>
Subject: Re: [PATCH v11 1/2] fs/erofs: Add support for EROFS
Date: Thu, 16 May 2024 23:33:57 +0200	[thread overview]
Message-ID: <ZkZ7xW56AwyBFZYF@tomti.i.net-space.pl> (raw)
In-Reply-To: <20240510005256.1192695-2-hsiangkao@linux.alibaba.com>

On Fri, May 10, 2024 at 08:52:55AM +0800, Gao Xiang wrote:
> From: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
>
> EROFS [1] is a lightweight read-only filesystem designed for performance
> which has already been shipped in most Linux distributions as well as widely
> used in several scenarios, such as Android system partitions, container
> images, and rootfs for embedded devices.
>
> This patch brings EROFS uncompressed support. Now, it's possible to boot
> directly through GRUB with an EROFS rootfs.
>
> EROFS compressed files will be supported later since it has more work to
> polish.
>
> [1] https://erofs.docs.kernel.org
>
> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> Tested-by: Daniel Axtens <dja@axtens.net> # fuzz testing only
> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>

In general patch LGTM except some nits...

> ---
> Tested-by Link: https://lists.gnu.org/archive/html/grub-devel/2024-05/msg00001.html
>
>  INSTALL                     |    8 +-
>  Makefile.util.def           |    1 +
>  docs/grub.texi              |    3 +-
>  grub-core/Makefile.core.def |    5 +
>  grub-core/fs/erofs.c        | 1008 +++++++++++++++++++++++++++++++++++
>  5 files changed, 1020 insertions(+), 5 deletions(-)
>  create mode 100644 grub-core/fs/erofs.c
>
> diff --git a/INSTALL b/INSTALL
> index 8d9207c84..84030c9f4 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -77,15 +77,15 @@ Prerequisites for make-check:
>
>  * If running a Linux kernel the following modules must be loaded:
>    - fuse, loop
> -  - btrfs, ext4, f2fs, fat, hfs, hfsplus, jfs, mac-roman, minix, nilfs2,
> +  - btrfs, erofs, ext4, f2fs, fat, hfs, hfsplus, jfs, mac-roman, minix, nilfs2,
>      reiserfs, udf, xfs
>    - On newer kernels, the exfat kernel modules may be used instead of the
>      exfat FUSE filesystem
>  * The following are Debian named packages required mostly for the full
>    suite of filesystem testing (but some are needed by other tests as well):
> -  - btrfs-progs, dosfstools, e2fsprogs, exfat-utils, f2fs-tools, genromfs,
> -    hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs, squashfs-tools,
> -    reiserfsprogs, udftools, xfsprogs, zfs-fuse
> +  - btrfs-progs, dosfstools, e2fsprogs, erofs-utils, exfat-utils, f2fs-tools,
> +    genromfs, hfsprogs, jfsutils, nilfs-tools, ntfs-3g, reiserfsprogs,
> +    squashfs-tools, reiserfsprogs, udftools, xfsprogs, zfs-fuse
>    - exfat-fuse, if not using the exfat kernel module
>    - gzip, lzop, xz-utils
>    - attr, cpio, g++, gawk, parted, recode, tar, util-linux
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 9432365a9..8d3bc107f 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -98,6 +98,7 @@ library = {
>    common = grub-core/fs/cpio_be.c;
>    common = grub-core/fs/odc.c;
>    common = grub-core/fs/newc.c;
> +  common = grub-core/fs/erofs.c;
>    common = grub-core/fs/ext2.c;
>    common = grub-core/fs/fat.c;
>    common = grub-core/fs/exfat.c;
> diff --git a/docs/grub.texi b/docs/grub.texi
> index d32266f69..b198d963d 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -353,6 +353,7 @@ blocklist notation. The currently supported filesystem types are @dfn{Amiga
>  Fast FileSystem (AFFS)}, @dfn{AtheOS fs}, @dfn{BeFS},
>  @dfn{BtrFS} (including raid0, raid1, raid10, gzip and lzo),
>  @dfn{cpio} (little- and big-endian bin, odc and newc variants),
> +@dfn{EROFS} (only uncompressed support for now),
>  @dfn{Linux ext2/ext3/ext4}, @dfn{DOS FAT12/FAT16/FAT32},
>  @dfn{exFAT}, @dfn{F2FS}, @dfn{HFS}, @dfn{HFS+},
>  @dfn{ISO9660} (including Joliet, Rock-ridge and multi-chunk files),
> @@ -6276,7 +6277,7 @@ assumed to be encoded in UTF-8.
>  NTFS, JFS, UDF, HFS+, exFAT, long filenames in FAT, Joliet part of
>  ISO9660 are treated as UTF-16 as per specification. AFS and BFS are read
>  as UTF-8, again according to specification. BtrFS, cpio, tar, squash4, minix,
> -minix2, minix3, ROMFS, ReiserFS, XFS, ext2, ext3, ext4, FAT (short names),
> +minix2, minix3, ROMFS, ReiserFS, XFS, EROFS, ext2, ext3, ext4, FAT (short names),
>  F2FS, RockRidge part of ISO9660, nilfs2, UFS1, UFS2 and ZFS are assumed
>  to be UTF-8. This might be false on systems configured with legacy charset
>  but as long as the charset used is superset of ASCII you should be able to
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8e1b1d9f3..7fa9446bd 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1442,6 +1442,11 @@ module = {
>    common = fs/odc.c;
>  };
>
> +module = {
> +  name = erofs;
> +  common = fs/erofs.c;
> +};
> +
>  module = {
>    name = ext2;
>    common = fs/ext2.c;
> diff --git a/grub-core/fs/erofs.c b/grub-core/fs/erofs.c
> new file mode 100644
> index 000000000..14c86f435
> --- /dev/null
> +++ b/grub-core/fs/erofs.c
> @@ -0,0 +1,1008 @@
> +/* erofs.c - Enhanced Read-Only File System */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2024 Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/disk.h>
> +#include <grub/dl.h>
> +#include <grub/err.h>
> +#include <grub/file.h>
> +#include <grub/fs.h>
> +#include <grub/fshelp.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/safemath.h>
> +#include <grub/types.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define EROFS_SUPER_OFFSET	1024
> +#define EROFS_MAGIC		0xE0F5E1E2
> +#define EROFS_ISLOTBITS		5
> +
> +#define EROFS_FEATURE_INCOMPAT_CHUNKED_FILE	0x00000004
> +#define EROFS_ALL_FEATURE_INCOMPAT		EROFS_FEATURE_INCOMPAT_CHUNKED_FILE
> +
> +struct grub_erofs_super
> +{
> +  grub_uint32_t magic;
> +  grub_uint32_t checksum;
> +  grub_uint32_t feature_compat;
> +  grub_uint8_t log2_blksz;
> +  grub_uint8_t sb_extslots;

May I ask you to align struct/union members in the following way?

  grub_uint32_t		 magic;
  grub_uint32_t		 checksum;
  grub_uint32_t		 feature_compat;
  grub_uint8_t		 log2_blksz;
  grub_uint8_t		 sb_extslots;

  grub_uint16_t		 root_nid;
  grub_uint64_t		 inos;

  grub_uint64_t		 build_time;
  grub_uint32_t		 build_time_nsec;

  ...

Please do this for all/most structs and unions definitions below.

> +
> +  grub_uint16_t root_nid;
> +  grub_uint64_t inos;
> +
> +  grub_uint64_t build_time;
> +  grub_uint32_t build_time_nsec;
> +  grub_uint32_t blocks;
> +  grub_uint32_t meta_blkaddr;
> +  grub_uint32_t xattr_blkaddr;
> +  grub_packed_guid_t uuid;
> +  grub_uint8_t volume_name[16];
> +  grub_uint32_t feature_incompat;
> +
> +  union
> +  {
> +    grub_uint16_t available_compr_algs;
> +    grub_uint16_t lz4_max_distance;
> +  } GRUB_PACKED u1;
> +
> +  grub_uint16_t extra_devices;
> +  grub_uint16_t devt_slotoff;
> +  grub_uint8_t log2_dirblksz;
> +  grub_uint8_t xattr_prefix_count;
> +  grub_uint32_t xattr_prefix_start;
> +  grub_uint64_t packed_nid;
> +  grub_uint8_t reserved2[24];
> +} GRUB_PACKED;
> +
> +#define EROFS_INODE_LAYOUT_COMPACT	0
> +#define EROFS_INODE_LAYOUT_EXTENDED	1
> +
> +#define EROFS_INODE_FLAT_PLAIN		0
> +#define EROFS_INODE_COMPRESSED_FULL	1
> +#define EROFS_INODE_FLAT_INLINE		2
> +#define EROFS_INODE_COMPRESSED_COMPACT	3
> +#define EROFS_INODE_CHUNK_BASED		4
> +
> +#define EROFS_I_VERSION_MASKS		0x01
> +#define EROFS_I_DATALAYOUT_MASKS	0x07
> +
> +#define EROFS_I_VERSION_BIT	0
> +#define EROFS_I_DATALAYOUT_BIT	1
> +
> +struct grub_erofs_inode_chunk_info
> +{
> +  grub_uint16_t format;
> +  grub_uint16_t reserved;
> +} GRUB_PACKED;
> +
> +#define EROFS_CHUNK_FORMAT_BLKBITS_MASK	0x001F
> +#define EROFS_CHUNK_FORMAT_INDEXES	0x0020
> +
> +#define EROFS_BLOCK_MAP_ENTRY_SIZE	4
> +#define EROFS_MAP_MAPPED		0x02
> +
> +#define EROFS_NULL_ADDR			1
> +#define EROFS_NAME_LEN			255
> +#define EROFS_PATH_LEN			4096
> +#define EROFS_MIN_LOG2_BLOCK_SIZE	9
> +#define EROFS_MAX_LOG2_BLOCK_SIZE	16
> +
> +struct grub_erofs_inode_chunk_index
> +{
> +  grub_uint16_t advise;
> +  grub_uint16_t device_id;
> +  grub_uint32_t blkaddr;
> +};
> +
> +union grub_erofs_inode_i_u
> +{
> +  grub_uint32_t compressed_blocks;
> +  grub_uint32_t raw_blkaddr;
> +
> +  grub_uint32_t rdev;
> +
> +  struct grub_erofs_inode_chunk_info c;

Things like this one can be an exception...

> +};
> +
> +struct grub_erofs_inode_compact
> +{
> +  grub_uint16_t i_format;
> +
> +  grub_uint16_t i_xattr_icount;
> +  grub_uint16_t i_mode;
> +  grub_uint16_t i_nlink;
> +  grub_uint32_t i_size;
> +  grub_uint32_t i_reserved;
> +
> +  union grub_erofs_inode_i_u i_u;
> +
> +  grub_uint32_t i_ino;
> +  grub_uint16_t i_uid;
> +  grub_uint16_t i_gid;
> +  grub_uint32_t i_reserved2;
> +} GRUB_PACKED;
> +
> +struct grub_erofs_inode_extended
> +{
> +  grub_uint16_t i_format;
> +
> +  grub_uint16_t i_xattr_icount;
> +  grub_uint16_t i_mode;
> +  grub_uint16_t i_reserved;
> +  grub_uint64_t i_size;
> +
> +  union grub_erofs_inode_i_u i_u;
> +
> +  grub_uint32_t i_ino;
> +
> +  grub_uint32_t i_uid;
> +  grub_uint32_t i_gid;
> +  grub_uint64_t i_mtime;
> +  grub_uint32_t i_mtime_nsec;
> +  grub_uint32_t i_nlink;
> +  grub_uint8_t i_reserved2[16];
> +} GRUB_PACKED;
> +
> +union grub_erofs_inode
> +{
> +  struct grub_erofs_inode_compact c;
> +  struct grub_erofs_inode_extended e;
> +} GRUB_PACKED;
> +
> +#define EROFS_FT_UNKNOWN	0
> +#define EROFS_FT_REG_FILE	1
> +#define EROFS_FT_DIR		2
> +#define EROFS_FT_CHRDEV		3
> +#define EROFS_FT_BLKDEV		4
> +#define EROFS_FT_FIFO		5
> +#define EROFS_FT_SOCK		6
> +#define EROFS_FT_SYMLINK	7
> +
> +struct grub_erofs_dirent
> +{
> +  grub_uint64_t nid;
> +  grub_uint16_t nameoff;
> +  grub_uint8_t file_type;
> +  grub_uint8_t reserved;
> +} GRUB_PACKED;
> +
> +struct grub_erofs_map_blocks
> +{
> +  grub_uint64_t m_pa;    /* physical address */
> +  grub_uint64_t m_la;    /* logical address */
> +  grub_uint64_t m_plen;  /* physical length */
> +  grub_uint64_t m_llen;  /* logical length */
> +  grub_uint32_t m_flags;
> +};
> +
> +struct grub_erofs_xattr_ibody_header
> +{
> +  grub_uint32_t h_reserved;
> +  grub_uint8_t h_shared_count;
> +  grub_uint8_t h_reserved2[7];
> +  grub_uint32_t h_shared_xattrs[0];
> +};
> +
> +struct grub_fshelp_node
> +{
> +  struct grub_erofs_data *data;
> +  union grub_erofs_inode inode;
> +
> +  grub_uint64_t ino;
> +  grub_uint8_t inode_type;
> +  grub_uint8_t inode_datalayout;
> +
> +  /* if the inode has been read into memory? */
> +  bool inode_loaded;
> +};
> +
> +struct grub_erofs_data
> +{
> +  grub_disk_t disk;
> +  struct grub_erofs_super sb;
> +
> +  struct grub_fshelp_node inode;
> +};
> +
> +#define erofs_blocksz(data) (((grub_uint32_t) 1) << data->sb.log2_blksz)
> +
> +static grub_size_t
> +grub_erofs_strnlen (const char *s, grub_size_t n)
> +{
> +  const char *p = s;
> +
> +  if (n == 0)
> +    return 0;
> +
> +  while (n-- && *p)
> +    p++;
> +
> +  return p - s;
> +}
> +
> +static grub_uint64_t
> +erofs_iloc (grub_fshelp_node_t node)
> +{
> +  struct grub_erofs_super *sb = &node->data->sb;
> +
> +  return ((grub_uint64_t) grub_le_to_cpu32 (sb->meta_blkaddr) << sb->log2_blksz) +
> +	 (node->ino << EROFS_ISLOTBITS);
> +}
> +
> +static grub_err_t
> +erofs_read_inode (struct grub_erofs_data *data, grub_fshelp_node_t node)
> +{
> +  union grub_erofs_inode *di;
> +  grub_err_t err;
> +  grub_uint16_t i_format;
> +  grub_uint64_t addr = erofs_iloc (node);
> +
> +  di = (union grub_erofs_inode *) &node->inode;
> +
> +  err = grub_disk_read (data->disk, addr >> GRUB_DISK_SECTOR_BITS,
> +			addr & (GRUB_DISK_SECTOR_SIZE - 1),
> +			sizeof (struct grub_erofs_inode_compact), &di->c);
> +  if (err != GRUB_ERR_NONE)
> +    return err;
> +
> +  i_format = grub_le_to_cpu16 (di->c.i_format);
> +  node->inode_type = (i_format >> EROFS_I_VERSION_BIT) & EROFS_I_VERSION_MASKS;
> +  node->inode_datalayout = (i_format >> EROFS_I_DATALAYOUT_BIT) & EROFS_I_DATALAYOUT_MASKS;
> +
> +  switch (node->inode_type)
> +    {
> +    case EROFS_INODE_LAYOUT_EXTENDED:
> +      addr += sizeof (struct grub_erofs_inode_compact);
> +      err = grub_disk_read (
> +	  data->disk, addr >> GRUB_DISK_SECTOR_BITS,
> +	  addr & (GRUB_DISK_SECTOR_SIZE - 1),
> +	  sizeof (struct grub_erofs_inode_extended) - sizeof (struct grub_erofs_inode_compact),
> +	  (grub_uint8_t *) di + sizeof (struct grub_erofs_inode_compact));

This function call is unreadable. Please fix it. I am OK with lines
longer than 80 chars. So, first argument for the grub_disk_read() should
be immediately behind "(". Then other arguments may follow below
starting from the same column with the first argument.

  grub_disk_read (data->disk, addr >> GRUB_DISK_SECTOR_BITS,
                  addr & (GRUB_DISK_SECTOR_SIZE - 1),
		  ...

> +      if (err != GRUB_ERR_NONE)
> +	return err;
> +      break;
> +    case EROFS_INODE_LAYOUT_COMPACT:
> +      break;
> +    default:
> +      return grub_error (GRUB_ERR_BAD_FS, "invalid type %u @ inode %" PRIuGRUB_UINT64_T,
> +			 node->inode_type, node->ino);
> +    }
> +
> +  node->inode_loaded = true;
> +
> +  return 0;
> +}
> +
> +static grub_uint64_t
> +erofs_inode_size (grub_fshelp_node_t node)
> +{
> +  return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
> +	     ? sizeof (struct grub_erofs_inode_compact)
> +	     : sizeof (struct grub_erofs_inode_extended);
> +}
> +
> +static grub_uint64_t
> +erofs_inode_file_size (grub_fshelp_node_t node)
> +{
> +  union grub_erofs_inode *di = (union grub_erofs_inode *) &node->inode;
> +
> +  return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
> +	     ? grub_le_to_cpu32 (di->c.i_size)
> +	     : grub_le_to_cpu64 (di->e.i_size);
> +}
> +
> +static grub_uint32_t
> +erofs_inode_xattr_ibody_size (grub_fshelp_node_t node)
> +{
> +  grub_uint16_t cnt = grub_le_to_cpu16 (node->inode.e.i_xattr_icount);
> +
> +  if (cnt == 0)
> +    return 0;
> +
> +  return sizeof (struct grub_erofs_xattr_ibody_header) + ((cnt - 1) * sizeof (grub_uint32_t));
> +}
> +
> +static grub_uint64_t
> +erofs_inode_mtime (grub_fshelp_node_t node)
> +{
> +  return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
> +	     ? grub_le_to_cpu64 (node->data->sb.build_time)
> +	     : grub_le_to_cpu64 (node->inode.e.i_mtime);
> +}
> +
> +static grub_err_t
> +erofs_map_blocks_flatmode (grub_fshelp_node_t node,
> +			   struct grub_erofs_map_blocks *map)
> +{
> +  grub_uint64_t nblocks, lastblk, file_size;
> +  bool tailendpacking = (node->inode_datalayout == EROFS_INODE_FLAT_INLINE);
> +  grub_uint64_t blocksz = erofs_blocksz (node->data);
> +
> +  /* file_size is checked by caller and cannot be zero, hence nblocks > 0 */
> +  file_size = erofs_inode_file_size (node);
> +  if (grub_add (file_size, blocksz - 1, &nblocks))
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");

You use "overflow is detected" message everywhere. It is not helpful.
I think (almost) every message should be different, e.g. "nblocks overflow"
or "invalid argument: %d..." or ...

> +  nblocks >>= node->data->sb.log2_blksz;
> +  lastblk = nblocks - tailendpacking;
> +
> +  map->m_flags = EROFS_MAP_MAPPED;
> +
> +  /* no overflow as (lastblk <= nblocks) && (nblocks * blocksz <= UINT64_MAX - blocksz + 1) */
> +  if (map->m_la < (lastblk * blocksz))
> +    {
> +      if (grub_mul ((grub_uint64_t) grub_le_to_cpu32 (node->inode.e.i_u.raw_blkaddr), blocksz,
> +		    &map->m_pa) ||

Please move this to the line with grub_mul()...

> +	  grub_add (map->m_pa, map->m_la, &map->m_pa))
> +	return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");

Again, this message does not help...

> +      if (grub_sub (lastblk * blocksz, map->m_la, &map->m_plen))
> +	return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");

To be precise, this is an underflow...

> +    }
> +  else if (tailendpacking)
> +    {
> +      if (grub_add (erofs_iloc (node), erofs_inode_size (node), &map->m_pa) ||
> +	  grub_add (map->m_pa, erofs_inode_xattr_ibody_size (node), &map->m_pa) ||
> +	  grub_add (map->m_pa, map->m_la % blocksz, &map->m_pa))
> +	return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");

Please fix all these messages...

> +      if (grub_sub (file_size, map->m_la, &map->m_plen))
> +	return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +
> +      /* no overflow as map->m_plen <= UINT64_MAX - blocksz + 1 */
> +      if (((map->m_pa % blocksz) + map->m_plen) > blocksz)
> +	return grub_error (
> +	    GRUB_ERR_BAD_FS,
> +	    "inline data cross block boundary @ inode %" PRIuGRUB_UINT64_T,
> +	    node->ino);

Wrong wrapping as above. Please fix it.

> +    }
> +  else
> +    return grub_error (GRUB_ERR_BAD_FS,
> +		       "invalid map->m_la=%" PRIuGRUB_UINT64_T
> +		       " @ inode %" PRIuGRUB_UINT64_T,
> +		       map->m_la, node->ino);

Ditto... Please fix similar wrapping everywhere.

> +  map->m_llen = map->m_plen;
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
> +			    struct grub_erofs_map_blocks *map)
> +{
> +  grub_uint16_t chunk_format = grub_le_to_cpu16 (node->inode.e.i_u.c.format);
> +  grub_uint64_t unit, pos, chunknr, blkaddr;
> +  grub_uint8_t chunkbits;
> +  grub_err_t err;
> +
> +  if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
> +    unit = sizeof (struct grub_erofs_inode_chunk_index);
> +  else
> +    unit = EROFS_BLOCK_MAP_ENTRY_SIZE;
> +
> +  chunkbits = node->data->sb.log2_blksz + (chunk_format & EROFS_CHUNK_FORMAT_BLKBITS_MASK);
> +  if (chunkbits > 63)
> +    return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode %" PRIuGRUB_UINT64_T,
> +		       chunkbits, node->ino);
> +
> +  chunknr = map->m_la >> chunkbits;
> +
> +  if (grub_add (erofs_iloc (node), erofs_inode_size (node), &pos) ||
> +      grub_add (pos, erofs_inode_xattr_ibody_size (node), &pos))

Maybe in some cases it is worth checking overflow for every operation
separately and then print relevant messages.

> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +
> +  /* pos = ALIGN_UP(pos, unit) */
> +  if (grub_add (pos, unit - 1, &pos))
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +  pos &= ~(unit - 1);

May I ask you to add a macro which allows overflow detection, e.g. ALIGN_UP_OVF().
This should go to a separate patch.

> +  /* no overflow for multiplication as chunkbits >= 9 and sizeof(unit) <= 8 */

Please start all comments with upper case and end with full stop.

> +  if (grub_add (pos, chunknr * unit, &pos))
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +
> +  map->m_la = chunknr << chunkbits;
> +
> +  if (grub_sub (erofs_inode_file_size (node), map->m_la, &map->m_plen))
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +  map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
> +			  ALIGN_UP (map->m_plen, erofs_blocksz (node->data)));
> +
> +  if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
> +    {
> +      struct grub_erofs_inode_chunk_index idx;
> +
> +      err = grub_disk_read (node->data->disk, pos >> GRUB_DISK_SECTOR_BITS,
> +			    pos & (GRUB_DISK_SECTOR_SIZE - 1), unit, &idx);
> +      if (err != GRUB_ERR_NONE)
> +	return err;
> +
> +      blkaddr = grub_le_to_cpu32 (idx.blkaddr);
> +    }
> +  else
> +    {
> +      grub_uint32_t blkaddr_le;
> +
> +      err = grub_disk_read (node->data->disk, pos >> GRUB_DISK_SECTOR_BITS,
> +			    pos & (GRUB_DISK_SECTOR_SIZE - 1), unit, &blkaddr_le);
> +      if (err != GRUB_ERR_NONE)
> +	return err;
> +
> +      blkaddr = grub_le_to_cpu32 (blkaddr_le);
> +    }
> +
> +  if (blkaddr == EROFS_NULL_ADDR)
> +    {
> +      map->m_pa = 0;
> +      map->m_flags = 0;
> +    }
> +  else
> +    {
> +      map->m_pa = blkaddr << node->data->sb.log2_blksz;
> +      map->m_flags = EROFS_MAP_MAPPED;
> +    }
> +
> +  map->m_llen = map->m_plen;
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +erofs_map_blocks (grub_fshelp_node_t node, struct grub_erofs_map_blocks *map)
> +{
> +  if (map->m_la >= erofs_inode_file_size (node))
> +    {
> +      map->m_llen = map->m_plen = 0;
> +      map->m_pa = 0;
> +      map->m_flags = 0;
> +      return GRUB_ERR_NONE;
> +    }
> +
> +  if (node->inode_datalayout != EROFS_INODE_CHUNK_BASED)
> +    return erofs_map_blocks_flatmode (node, map);
> +  else
> +    return erofs_map_blocks_chunkmode (node, map);
> +}
> +
> +static grub_err_t
> +erofs_read_raw_data (grub_fshelp_node_t node, grub_uint8_t *buf, grub_uint64_t size,
> +		     grub_uint64_t offset, grub_uint64_t *bytes)
> +{
> +  struct grub_erofs_map_blocks map = {0};
> +  grub_uint64_t cur;
> +  grub_err_t err;
> +
> +  if (bytes)
> +    *bytes = 0;
> +
> +  if (!node->inode_loaded)

if (node->inode_loaded == false)

... and please fix similar things everywhere...

> +    {
> +      err = erofs_read_inode (node->data, node);
> +      if (err != GRUB_ERR_NONE)
> +	return err;
> +    }
> +
> +  cur = offset;
> +  while (cur < offset + size)
> +    {
> +      grub_uint8_t *const estart = buf + cur - offset;
> +      grub_uint64_t eend, moff = 0;
> +
> +      map.m_la = cur;
> +      err = erofs_map_blocks (node, &map);
> +      if (err != GRUB_ERR_NONE)
> +	return err;
> +
> +      if (grub_add(map.m_la, map.m_llen, &eend))
> +	return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +
> +      eend = grub_min (eend, offset + size);
> +      if (!(map.m_flags & EROFS_MAP_MAPPED))
> +	{
> +	  if (!map.m_llen)
> +	    {
> +	      /* reached EOF */
> +	      grub_memset (estart, 0, offset + size - cur);
> +	      cur = offset + size;
> +	      continue;
> +	    }
> +
> +	  /* Hole */
> +	  grub_memset (estart, 0, eend - cur);
> +	  if (bytes)
> +	    *bytes += eend - cur;
> +	  cur = eend;
> +	  continue;
> +	}
> +
> +      if (cur > map.m_la)
> +	{
> +	  moff = cur - map.m_la;
> +	  map.m_la = cur;
> +	}
> +
> +      err = grub_disk_read (node->data->disk,
> +			    (map.m_pa + moff) >> GRUB_DISK_SECTOR_BITS,
> +			    (map.m_pa + moff) & (GRUB_DISK_SECTOR_SIZE - 1),
> +			    eend - map.m_la, estart);
> +      if (err != GRUB_ERR_NONE)
> +	return err;
> +
> +      if (bytes)
> +	*bytes += eend - map.m_la;
> +
> +      cur = eend;
> +    }
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static int
> +erofs_iterate_dir (grub_fshelp_node_t dir, grub_fshelp_iterate_dir_hook_t hook,
> +		   void *hook_data)
> +{
> +  grub_uint64_t offset = 0, file_size;
> +  grub_uint32_t blocksz = erofs_blocksz (dir->data);
> +  grub_uint8_t *buf;
> +  grub_err_t err;
> +
> +  if (!dir->inode_loaded)

Ditto.

> +    {
> +      err = erofs_read_inode (dir->data, dir);
> +      if (err != GRUB_ERR_NONE)
> +	return 0;
> +    }
> +
> +  file_size = erofs_inode_file_size (dir);
> +  buf = grub_malloc (blocksz);
> +  if (!buf)

if (buf == NULL)

I know your version works but it is less readable. Please fix similar
checks everywhere...

> +    return 0;
> +
> +  while (offset < file_size)
> +    {
> +      grub_uint64_t maxsize = grub_min (blocksz, file_size - offset);
> +      struct grub_erofs_dirent *de = (void *) buf, *end;
> +      grub_uint16_t nameoff;
> +
> +      err = erofs_read_raw_data (dir, buf, maxsize, offset, NULL);
> +      if (err != GRUB_ERR_NONE)
> +	goto not_found;
> +
> +      nameoff = grub_le_to_cpu16 (de->nameoff);
> +      if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff >= maxsize)
> +	{
> +	  grub_error (GRUB_ERR_BAD_FS,
> +		      "invalid nameoff %u @ inode %" PRIuGRUB_UINT64_T,
> +		      nameoff, dir->ino);
> +	  goto not_found;
> +	}
> +
> +      end = (struct grub_erofs_dirent *) ((grub_uint8_t *) de + nameoff);
> +      while (de < end)
> +	{
> +	  struct grub_fshelp_node *fdiro;
> +	  enum grub_fshelp_filetype type;
> +	  char filename[EROFS_NAME_LEN + 1];
> +	  grub_size_t de_namelen;
> +	  const char *de_name;
> +
> +	  fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
> +	  if (!fdiro)

Ditto...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel