[U-Boot] [PATCH 2/2] fs:ext4:write:fix: Reinitialize global variables after updating a file

[Simon Glass]

Hi Lukasz,

On 30 April 2014 03:39, Lukasz Majewski <l.majewski at samsung.com> wrote:
> This bug shows up when file stored on the ext4 file system is updated.
>
> The ext4fs_delete_file() is responsible for deleting file's (e.g. uImage)
> data.
> However some global data (especially ext4fs_indir2_block), which is used
> during file deletion are left unchanged.
>
> The ext4fs_indir2_block pointer stores reference to old ext4 double
> indirect allocated blocks. When it is unchanged, after file deletion,
> ext4fs_write_file() uses the same pointer (since it is already initialized
> - i.e. not NULL) to return number of blocks to write. This trunks larger
> file when previous one was smaller.
>
> Lets consider following scenario:
>
> 1. Flash target with ext4 formatted boot.img (which has uImage [*] on itself)
> 2. Developer wants to upload their custom uImage [**]
>         - When new uImage [**] is smaller than the [*] - everything works
>         correctly - we are able to store the whole smaller file with corrupted
>         ext4fs_indir2_block pointer
>         - When new uImage [**] is larger than the [*] - theCRC is corrupted,
>         since truncation on data stored at eMMC was done.
> 3. When uImage CRC error appears, then reboot and THOR/DFU reflashing causes
>         proper setting of ext4fs_indir2_block() and after that uImage[**]
>         is successfully stored (correct uImage [*] metadata is stored at an
>         eMMC on the first flashing).
>
> Due to above the bug was very difficult to reproduce.

I wonder if a sandbox test would be a good idea? You could fairly easy
mkfs in a loopback file and write a U-Boot script to operate on it.
See test/ for some examples.

> This patch sets default values for all ext4fs_indir* pointers/variables.
>
> Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> ---
>  fs/ext4/ext4_common.c |   23 ++++++++++++++---------
>  fs/ext4/ext4_write.c  |    1 +
>  include/ext4fs.h      |    1 +
>  3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 62e2e80..d0de285 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -1841,16 +1841,8 @@ long int read_allocated_block(struct ext2_inode *inode, int fileblock)
>         return blknr;
>  }
>
> -void ext4fs_close(void)
> +void ext4fs_reinit_global(void)
>  {
> -       if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
> -               ext4fs_free_node(ext4fs_file, &ext4fs_root->diropen);
> -               ext4fs_file = NULL;
> -       }
> -       if (ext4fs_root != NULL) {
> -               free(ext4fs_root);
> -               ext4fs_root = NULL;
> -       }
>         if (ext4fs_indir1_block != NULL) {
>                 free(ext4fs_indir1_block);
>                 ext4fs_indir1_block = NULL;
> @@ -1870,6 +1862,19 @@ void ext4fs_close(void)
>                 ext4fs_indir3_blkno = -1;
>         }
>  }
> +void ext4fs_close(void)
> +{
> +       if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
> +               ext4fs_free_node(ext4fs_file, &ext4fs_root->diropen);
> +               ext4fs_file = NULL;
> +       }
> +       if (ext4fs_root != NULL) {
> +               free(ext4fs_root);
> +               ext4fs_root = NULL;
> +       }
> +
> +       ext4fs_reinit_global();
> +}
>
>  int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,
>                                 struct ext2fs_node **fnode, int *ftype)
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index 46c573b..4a5f652 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -562,6 +562,7 @@ static int ext4fs_delete_file(int inodeno)
>
>         ext4fs_update();
>         ext4fs_deinit();
> +       ext4fs_reinit_global();
>
>         if (ext4fs_init() != 0) {
>                 printf("error in File System init\n");
> diff --git a/include/ext4fs.h b/include/ext4fs.h
> index aacb147..fbbb002 100644
> --- a/include/ext4fs.h
> +++ b/include/ext4fs.h
> @@ -133,6 +133,7 @@ int ext4fs_open(const char *filename);
>  int ext4fs_read(char *buf, unsigned len);
>  int ext4fs_mount(unsigned part_length);
>  void ext4fs_close(void);
> +void ext4fs_reinit_global(void);

Can we have a comment as to what this function does?

>  int ext4fs_ls(const char *dirname);
>  int ext4fs_exists(const char *filename);
>  void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot);
> --
> 1.7.10.4
>

Regards,
Simon