| Message ID | 1398854380-10778-3-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Lukasz, On 30 April 2014 03:39, Lukasz Majewski <l.majewski@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@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
Hi Simon, > Hi Lukasz, > > On 30 April 2014 03:39, Lukasz Majewski <l.majewski@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@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
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); int ext4fs_ls(const char *dirname); int ext4fs_exists(const char *filename); void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot);
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. This patch sets default values for all ext4fs_indir* pointers/variables. Signed-off-by: Lukasz Majewski <l.majewski@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(-)