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