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

[Lukasz Majewski]

Hi Simon,

> 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.

If my time budget allows, I will try to add test to sandbox.

> 
> > 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?

I can add comment to the source code. No problem.

> 
> >  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



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group