Re: [PATCH v2] F2FS support

[Andrei Borzenkov <arvidjaar@gmail.com>]

Sorry for delay.


В Fri, 3 Apr 2015 15:49:08 -0700
Jaegeuk Kim <jaegeuk@kernel.org> пишет:

> 
> This patch changes:
> 
>  * Makefile.util.def: Add f2fs.c.
>  * doc/grub.texi: Add f2fs description.
>  * grub-core/Makefile.core.def: Add f2fs module.
>  * grub-core/fs/f2fs.c: New file.
>  * tests/f2fs_test.in: New file.
>  * tests/util/grub-fs-tester.in: Add f2fs requirements.
> 

Drop file list, it is available from git.

...
> diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
> new file mode 100644
> index 0000000..e6b8386
> --- /dev/null
> +++ b/grub-core/fs/f2fs.c
> +
...
> +#define JENTRY_SIZE		13

sizeof (struct grub_f2fs_nat_jent), right?

...
> +
> +#define NAT_ENTRY_SIZE 		9

That's sizeof (struct grub_f2fs_nat_entry)?

...
> +#define ver_after (a, b)	((long long)((a) - (b)) > 0)

This macro is not used anywhere

> +#define F2FS_NAME_LEN		255
> +#define F2FS_SLOT_LEN		8
> +#define NR_DENTRY_IN_BLOCK	214
> +#define SIZE_OF_DIR_ENTRY	11	/* by byte */

sizeof (struct grub_f2fs_dir_entry)?

...
> +enum
> +  {
> +    FI_INLINE_XATTR = 9,
> +    FI_INLINE_DATA = 10,
> +    FI_INLINE_DENTRY = 11,
> +    FI_DATA_EXIST = 18,
> +  };
> +

This does not match subsequent usage; you use them with i_inline, not
i_flags.

...
> +
> +static inline int
> +grub_generic_test_bit (int nr, const grub_uint32_t *addr)
> +{
> +  return 1UL & (addr[nr / 32] >> (nr & 31));
> +}
> +

This is used only in grub_f2fs_check_dentries() with on-disk bitmap.
On-disk bitmap is little-endian; code is wrong on big-endian system.

Also dentry_bitmap is not multiple of 4 bytes as you replied earlier.
You should rather compute correct byte address instead and make all
parameters grub_uint8_t *. This will also avoid all those casts later.

That's really just

  grub_uint8_t *addr;

  byte = nr >> 3;
#ifdef WORDS_BIGENDIAN
  byte ^= 3;
#endif
  return addr[byte] & (1 << nr & 7);

...
> +
> +static inline int
> +__inode_inline_set (struct grub_f2fs_inode *inode, int flag)
> +{
> +  return inode->i_inline & flag;
> +}
> +

Function is completely redundant if you really want to check i_inline;
just use it directly. Then definition of flags is wrong.

If you mean inode->i_flags (as implied by passed flag values) this
should obviously be (1 << flag) as adjusted by system byte order.

Out of curiosity - it appears those flags are present in both i_inline
and i_flags; which one is authoritative?

> +static inline grub_uint32_t
> +__nat_bitmap_size (struct grub_f2fs_data *data)
> +{
> +  struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> +
> +  return grub_le_to_cpu32 (ckpt->nat_ver_bitmap_bytesize);
> +}

This function is not used anywhere.

...
> +
> +static inline grub_uint32_t
> +__get_node_id (struct grub_f2fs_node *rn, int off, int i)
> +{
> +  if (i)

Judging by usage something like "first" would probably be more
appropriate.

> +    return grub_le_to_cpu32 (rn->i.i_nid[off - NODE_DIR1_BLOCK]);
> +  return grub_le_to_cpu32 (rn->in.nid[off]);
> +}
> +
> +static inline grub_err_t
> +grub_f2fs_block_read (struct grub_f2fs_data *data, grub_uint32_t blkaddr, void *buf)
> +{
> +  return grub_disk_read (data->disk, blkaddr << F2FS_BLK_SEC_BITS,

I suspect coverity will complain about overflow here; cast of blkaddr
to grub_disk_addr_t should silence it.

> +					0, F2FS_BLKSIZE, buf);
> +}
> +
...
> +
> +static grub_err_t

Not sure if it is needed here; see comment below at caller.

> +grub_f2fs_read_sb (struct grub_f2fs_data *data, grub_uint64_t offset)
> +{
> +  grub_disk_t disk = data->disk;
> +  grub_err_t err;
> +
> +  /* Read first super block. */
> +  err = grub_disk_read (disk, offset >> GRUB_DISK_SECTOR_BITS, 0,

Make parameter grub_disk_addr_t and compute it in caller. It is
constant, there is no need to compute it at run time.

> +			sizeof (data->sblock), &data->sblock);
> +  if (err)
> +    return err;
> +
> +  if (grub_f2fs_sanity_check_sb (&data->sblock))
> +    err = GRUB_ERR_BAD_FS;
> +
> +  return err;
> +}
> +
> +static void *
> +validate_checkpoint (struct grub_f2fs_data *data, grub_uint32_t cp_addr,
> +						grub_uint64_t *version)
> +{
> +  void *cp_page_1, *cp_page_2;
> +  struct grub_f2fs_checkpoint *cp_block;
> +  grub_uint64_t cur_version = 0, pre_version = 0;
> +  grub_uint32_t crc = 0;
> +  grub_uint32_t crc_offset;
> +  grub_err_t err;
> +
> +  /* Read the 1st cp block in this CP pack */
> +  cp_page_1 = grub_malloc (F2FS_BLKSIZE);
> +  if (!cp_page_1)
> +    return NULL;
> +
> +  err = grub_f2fs_block_read (data, cp_addr, cp_page_1);
> +  if (err)
> +    goto invalid_cp1;
> +
> +  cp_block = (struct grub_f2fs_checkpoint *)cp_page_1;
> +  crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> +  if (crc_offset >= F2FS_BLKSIZE)
> +    goto invalid_cp1;
> +
> +  crc = grub_le_to_cpu32 (*(grub_uint32_t *)((char *)cp_block + crc_offset));

I understand that it /should/ be hardcoded to 4092, but then please
either check that crc_offset *is* 4092 before or use
grub_get_unaligned. Otherwise it crashes on archs that do not support
unaligned access.

> +  if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> +    goto invalid_cp1;
> +
> +  pre_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> +
> +  /* Read the 2nd cp block in this CP pack */
> +  cp_page_2 = grub_malloc (F2FS_BLKSIZE);
> +  if (!cp_page_2)
> +    goto invalid_cp1;
> +
> +  cp_addr += grub_le_to_cpu32 (cp_block->cp_pack_total_block_count) - 1;
> +
> +  err = grub_f2fs_block_read (data, cp_addr, cp_page_2);
> +  if (err)
> +    goto invalid_cp2;
> +
> +  cp_block = (struct grub_f2fs_checkpoint *)cp_page_2;
> +  crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> +  if (crc_offset >= F2FS_BLKSIZE)
> +    goto invalid_cp2;
> +
> +  crc = grub_le_to_cpu32 (*(grub_uint32_t *)((char *)cp_block + crc_offset));

Ditto.

> +  if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> +    goto invalid_cp2;
> +
> +  cur_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> +  if (cur_version == pre_version)
> +    {
> +      *version = cur_version;
> +      grub_free (cp_page_2);
> +      return cp_page_1;
> +    }
> +
> +invalid_cp2:
> +  grub_free (cp_page_2);
> +invalid_cp1:
> +  grub_free (cp_page_1);
> +  return NULL;
> +}
> +
> +static grub_err_t
> +grub_f2fs_read_cp (struct grub_f2fs_data *data)
> +{
> +  void *cp1, *cp2, *cur_page;
> +  grub_uint64_t cp1_version = 0, cp2_version = 0;
> +  grub_uint64_t cp_start_blk_no;
> +
> +  /*
> +   * Finding out valid cp block involves read both
> +   * sets (cp pack1 and cp pack 2)
> +   */
> +  cp_start_blk_no = data->cp_blkaddr;
> +  cp1 = validate_checkpoint (data, cp_start_blk_no, &cp1_version);
> +  if (!cp1 && grub_errno)
> +      return grub_errno;
> +
> +  /* The second checkpoint pack should start at the next segment */
> +  cp_start_blk_no += data->blocks_per_seg;
> +  cp2 = validate_checkpoint (data, cp_start_blk_no, &cp2_version);
> +  if (!cp2 && grub_errno)
> +    {
> +      grub_free (cp1);
> +      return grub_errno;
> +    }
> +
> +  if (cp1 && cp2)
> +    cur_page = (cp2_version > cp1_version) ? cp2 : cp1;
> +  else if (cp1)
> +    cur_page = cp1;
> +  else if (cp2)
> +    cur_page = cp2;
> +  else
> +    return grub_error (GRUB_ERR_BAD_FS, "no checkpoints\n");

Trailing "\n" is not needed.

> +
> +  grub_memcpy (&data->ckpt, cur_page, F2FS_BLKSIZE);
> +
> +  grub_free (cp1);
> +  grub_free (cp2);
> +  return 0;
> +}
> +
...
> +
> +static struct grub_f2fs_data *
> +grub_f2fs_mount (grub_disk_t disk)
> +{
> +  struct grub_f2fs_data *data;
> +  grub_err_t err;
> +
> +  data = grub_zalloc (sizeof (*data));

Is it needed to be zalloc? Structure is large and it runs every time
file is accessed. Most of it is immediately overwritten, may be
explicitly initialize what remains?

> +  if (!data)
> +    return NULL;
> +
> +  data->disk = disk;
> +
> +  err = grub_f2fs_read_sb (data, F2FS_SUPER_OFFSET);
> +  if (err)
> +    {
> +      err = grub_f2fs_read_sb (data, F2FS_BLKSIZE + F2FS_SUPER_OFFSET);

As mentioned just compute disk address here, it is static.

> +      if (err)
> +        {
> +          grub_error (GRUB_ERR_BAD_FS, "not a F2FS filesystem (no superblock)");
> +          goto fail;
> +	}
> +    }

You should check for err != GRUB_ERR_BAD_FS here, otherwise error from
grub_disk_read is lost. Alternatively just return 0/1 from read_sb, so
that

if (grub_f2fs_read_sb)
  if (grub_f2fs_read_sb)
    if (grub_errno == GRUB_ERR_NONE)
      grub_error (GRUB_ERR_BAD_FS, ...)
    goto fail

> +
> +  data->root_ino = grub_le_to_cpu32 (data->sblock.root_ino);
> +  data->cp_blkaddr = grub_le_to_cpu32 (data->sblock.cp_blkaddr);
> +  data->nat_blkaddr = grub_le_to_cpu32 (data->sblock.nat_blkaddr);
> +  data->blocks_per_seg = 1 <<
> +		grub_le_to_cpu32 (data->sblock.log_blocks_per_seg);
> +
> +  err = grub_f2fs_read_cp (data);
> +  if (err)
> +    goto fail;
> +
> +  data->nat_bitmap = __nat_bitmap_ptr (data);
> +
> +  err = get_nat_journal (data);
> +  if (err)
> +    goto fail;
> +
> +  data->diropen.data = data;
> +  data->diropen.ino = data->root_ino;
> +  data->diropen.inode_read = 1;
> +  data->inode = &data->diropen.inode;
> +
> +  err = grub_f2fs_read_node (data, data->root_ino, data->inode);
> +  if (err)
> +    goto fail;
> +
> +  return data;
> +
> +fail:
> +  grub_free (data);
> +  return NULL;
> +}
> +
...
> +
> +static grub_ssize_t
> +grub_f2fs_read_file (grub_fshelp_node_t node,
> +		       grub_disk_read_hook_t read_hook, void *read_hook_data,
> +		       grub_off_t pos, grub_size_t len, char *buf)
> +{
> +  struct grub_f2fs_inode *inode = &(node->inode.i);

Why extra parens?

> +  grub_off_t filesize = grub_f2fs_file_size (inode);
> +  char *inline_addr = __inline_addr (inode);
> +
> +  if (__inode_inline_set (&node->inode.i, FI_INLINE_DATA))
Just inode, you already have it.

> +    {
> +      if (pos > filesize || filesize > MAX_INLINE_DATA)
> +        {
> +          grub_error (GRUB_ERR_OUT_OF_RANGE,
> +		  N_("attempt to read past the end of file"));

If filesize > MAX_INLINE_DATA at this point we really deal with
corrupted filesystem so this should be GRUB_ERR_BAD_FS.

> +          return -1;
> +        }
> +      if (pos + len > filesize)

len > filesize - pos is probably more coverity-friendly.

> +        len = filesize - pos;
> +
> +      grub_memcpy (buf + pos, inline_addr + pos, len);
> +      return len;
> +    }
> +
> +  return grub_fshelp_read_file (node->data->disk, node,
> +				read_hook, read_hook_data,
> +				pos, len, buf, grub_f2fs_get_block,
> +				filesize,
> +				F2FS_BLK_SEC_BITS, 0);
> +}
> +
...
> +
> +static int
> +grub_f2fs_check_dentries (struct grub_f2fs_dir_iter_ctx *ctx)
> +{
> +  struct grub_fshelp_node *fdiro;
> +  int i;
> +
> +  for (i = 0; i < ctx->max;)
> +    {
> +      char *filename;
> +      enum grub_fshelp_filetype type = GRUB_FSHELP_UNKNOWN;
> +      enum FILE_TYPE ftype;
> +      int name_len;
> +      int ret;
> +
> +      if (grub_generic_test_bit (i, ctx->bitmap) == 0)
> +        {
> +          i++;
> +          continue;
> +        }
> +
> +      ftype = ctx->dentry[i].file_type;
> +      name_len = grub_le_to_cpu16 (ctx->dentry[i].name_len);
> +      filename = grub_zalloc (name_len + 1);
It is overwritten on next line, do you really need grub_zalloc only for
the trailing zero?

> +      if (!filename)
> +        return 0;
> +
> +      grub_memcpy (filename, ctx->filename[i], name_len);
> +
> +      fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
> +      if (!fdiro)
> +        {
> +          grub_free(filename);
> +          return 0;
> +        }
> +
> +      if (ftype == F2FS_FT_DIR)
> +        type = GRUB_FSHELP_DIR;
> +      else if (ftype == F2FS_FT_SYMLINK)
> +        type = GRUB_FSHELP_SYMLINK;
> +      else if (ftype == F2FS_FT_REG_FILE)
> +        type = GRUB_FSHELP_REG;
> +
> +      fdiro->data = ctx->data;
> +      fdiro->ino = grub_le_to_cpu32 (ctx->dentry[i].ino);
> +      fdiro->inode_read = 0;
> +
> +      ret = ctx->hook (filename, type, fdiro, ctx->hook_data);
> +      grub_free(filename);
> +      if (ret)
> +        return 1;
> +
> +      i += (name_len + F2FS_SLOT_LEN - 1) / F2FS_SLOT_LEN;
> +    }
> +    return 0;
> +}
> +
...
> +
> +static void
> +grub_f2fs_unicode_to_ascii (grub_uint8_t *out_buf, grub_uint16_t *in_buf)
> +{
> +  grub_uint16_t *pchTempPtr = in_buf;
> +  grub_uint8_t *pwTempPtr = out_buf;
> +
> +  while (*pchTempPtr != '\0')
> +  {
> +    *pwTempPtr = (grub_uint8_t) *pchTempPtr;

This is not byte order safe and it does not convert to ASCII as name
suggests. If you are going to convert to 8 bit encoding why not simply
use grub_utf16_to_utf8?

> +    pchTempPtr++;
> +    pwTempPtr++;
> +  }
> +  *pwTempPtr = '\0';
> +  return;
> +}
> +
> +static grub_err_t
> +grub_f2fs_label (grub_device_t device, char **label)
> +{
> +  struct grub_f2fs_data *data;
> +  grub_disk_t disk = device->disk;
> +
> +  grub_dl_ref (my_mod);
> +
> +  data = grub_f2fs_mount (disk);
> +  if (data)
> +    {
> +      *label = grub_zalloc (sizeof (data->sblock.volume_name));
> +      if (*label)
> +        grub_f2fs_unicode_to_ascii ((grub_uint8_t *) (*label),
> +				data->sblock.volume_name);

See above.

> +    }
> +  else
> +    *label = NULL;
> +
> +  grub_free (data);
> +  grub_dl_unref (my_mod);
> +  return grub_errno;
> +}
> +
...