[U-Boot] [PATCH 0/2] fs:ext4: Fixes and code cleanup
This code fixes problem with storing file with the same name multiple times on the same ext4 fs (with different sizes).
Code reorganization and cleanup is also included.
Lukasz Majewski (2): fs:ext4:cleanup: Remove superfluous code fs:ext4:write:fix: Reinitialize global variables after updating a file
fs/ext4/ext4_common.c | 29 +++++++++++++++------------- fs/ext4/ext4_write.c | 51 +++++++++++++++++-------------------------------- include/ext4fs.h | 1 + 3 files changed, 34 insertions(+), 47 deletions(-)
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com --- fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 50 ++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -904,10 +904,8 @@ long int ext4fs_get_new_blk_no(void) restart: fs->curr_blkno++; /* get the blockbitmap index respective to blockno */ - if (fs->blksz != 1024) { - bg_idx = fs->curr_blkno / blk_per_grp; - } else { - bg_idx = fs->curr_blkno / blk_per_grp; + bg_idx = fs->curr_blkno / blk_per_grp; + if (fs->blksz == 1024) { remainder = fs->curr_blkno % blk_per_grp; if (!remainder) bg_idx--; diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c index b674b6f..46c573b 100644 --- a/fs/ext4/ext4_write.c +++ b/fs/ext4/ext4_write.c @@ -116,10 +116,8 @@ static void delete_single_indirect_block(struct ext2_inode *inode) if (inode->b.blocks.indir_block != 0) { debug("SIPB releasing %u\n", inode->b.blocks.indir_block); blknr = inode->b.blocks.indir_block; - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -181,10 +179,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer); - if (fs->blksz != 1024) { - bg_idx = (*di_buffer) / blk_per_grp; - } else { - bg_idx = (*di_buffer) / blk_per_grp; + bg_idx = (*di_buffer) / blk_per_grp; + if (fs->blksz == 1024) { remainder = (*di_buffer) % blk_per_grp; if (!remainder) bg_idx--; @@ -213,10 +209,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
/* removing the parent double indirect block */ blknr = inode->b.blocks.double_indir_block; - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -293,11 +287,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) for (j = 0; j < fs->blksz / sizeof(int); j++) { if (*tip_buffer == 0) break; - if (fs->blksz != 1024) { - bg_idx = (*tip_buffer) / blk_per_grp; - } else { - bg_idx = (*tip_buffer) / blk_per_grp; - + bg_idx = (*tip_buffer) / blk_per_grp; + if (fs->blksz == 1024) { remainder = (*tip_buffer) % blk_per_grp; if (!remainder) bg_idx--; @@ -336,11 +327,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) * removing the grand parent blocks * which is connected to inode */ - if (fs->blksz != 1024) { - bg_idx = (*tigp_buffer) / blk_per_grp; - } else { - bg_idx = (*tigp_buffer) / blk_per_grp; - + bg_idx = (*tigp_buffer) / blk_per_grp; + if (fs->blksz == 1024) { remainder = (*tigp_buffer) % blk_per_grp; if (!remainder) bg_idx--; @@ -371,10 +359,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
/* removing the grand parent triple indirect block */ blknr = inode->b.blocks.triple_indir_block; - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -452,10 +438,8 @@ static int ext4fs_delete_file(int inodeno)
for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&(node_inode->inode), i); - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -499,10 +483,8 @@ static int ext4fs_delete_file(int inodeno) no_blocks++; for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&inode, i); - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;
On 30 April 2014 03:39, Lukasz Majewski l.majewski@samsung.com wrote:
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Reviewed-by: Simon Glass sjg@chromium.org
fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 50 ++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c
[snip]
@@ -181,10 +179,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer);
if (fs->blksz != 1024) {bg_idx = (*di_buffer) / blk_per_grp;} else {bg_idx = (*di_buffer) / blk_per_grp;
bg_idx = (*di_buffer) / blk_per_grp;
You don't need the brackets here (or below).
if (fs->blksz == 1024) { remainder = (*di_buffer) % blk_per_grp; if (!remainder) bg_idx--;@@ -213,10 +209,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
/* removing the parent double indirect block */ blknr = inode->b.blocks.double_indir_block;
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;@@ -293,11 +287,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) for (j = 0; j < fs->blksz / sizeof(int); j++) { if (*tip_buffer == 0) break;
if (fs->blksz != 1024) {bg_idx = (*tip_buffer) / blk_per_grp;} else {bg_idx = (*tip_buffer) / blk_per_grp;
bg_idx = (*tip_buffer) / blk_per_grp;if (fs->blksz == 1024) { remainder = (*tip_buffer) % blk_per_grp; if (!remainder) bg_idx--;@@ -336,11 +327,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) * removing the grand parent blocks * which is connected to inode */
if (fs->blksz != 1024) {bg_idx = (*tigp_buffer) / blk_per_grp;} else {bg_idx = (*tigp_buffer) / blk_per_grp;
bg_idx = (*tigp_buffer) / blk_per_grp;if (fs->blksz == 1024) { remainder = (*tigp_buffer) % blk_per_grp; if (!remainder) bg_idx--;@@ -371,10 +359,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
/* removing the grand parent triple indirect block */ blknr = inode->b.blocks.triple_indir_block;
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;@@ -452,10 +438,8 @@ static int ext4fs_delete_file(int inodeno)
for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&(node_inode->inode), i);
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;@@ -499,10 +483,8 @@ static int ext4fs_delete_file(int inodeno) no_blocks++; for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&inode, i);
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;-- 1.7.10.4
Hi Simon,
On 30 April 2014 03:39, Lukasz Majewski l.majewski@samsung.com wrote:
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Reviewed-by: Simon Glass sjg@chromium.org
fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 50 ++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c
[snip]
@@ -181,10 +179,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer);
if (fs->blksz != 1024) {bg_idx = (*di_buffer) / blk_per_grp;} else {bg_idx = (*di_buffer) / blk_per_grp;
bg_idx = (*di_buffer) / blk_per_grp;You don't need the brackets here (or below).
Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.
if (fs->blksz == 1024) { remainder = (*di_buffer) %blk_per_grp; if (!remainder) bg_idx--; @@ -213,10 +209,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
/* removing the parent double indirect block */ blknr = inode->b.blocks.double_indir_block;
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;@@ -293,11 +287,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) for (j = 0; j < fs->blksz / sizeof(int); j++) { if (*tip_buffer == 0) break;
if (fs->blksz != 1024) {bg_idx = (*tip_buffer) /blk_per_grp;
} else {bg_idx = (*tip_buffer) /blk_per_grp; -
bg_idx = (*tip_buffer) /blk_per_grp;
if (fs->blksz == 1024) { remainder = (*tip_buffer) %blk_per_grp; if (!remainder) bg_idx--; @@ -336,11 +327,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) * removing the grand parent blocks * which is connected to inode */
if (fs->blksz != 1024) {bg_idx = (*tigp_buffer) /blk_per_grp;
} else {bg_idx = (*tigp_buffer) /blk_per_grp; -
bg_idx = (*tigp_buffer) / blk_per_grp;if (fs->blksz == 1024) { remainder = (*tigp_buffer) %blk_per_grp; if (!remainder) bg_idx--; @@ -371,10 +359,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
/* removing the grand parent triple indirect block*/ blknr = inode->b.blocks.triple_indir_block;
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;@@ -452,10 +438,8 @@ static int ext4fs_delete_file(int inodeno)
for (i = 0; i < no_blocks; i++) { blknr =read_allocated_block(&(node_inode->inode), i);
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;@@ -499,10 +483,8 @@ static int ext4fs_delete_file(int inodeno) no_blocks++; for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&inode, i);
if (fs->blksz != 1024) {bg_idx = blknr / blk_per_grp;} else {bg_idx = blknr / blk_per_grp;
bg_idx = blknr / blk_per_grp;if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;-- 1.7.10.4
Hi Lukasz,
On 4 May 2014 23:20, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Simon,
On 30 April 2014 03:39, Lukasz Majewski l.majewski@samsung.com wrote:
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Reviewed-by: Simon Glass sjg@chromium.org
fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 50 ++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c
[snip]
@@ -181,10 +179,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer);
if (fs->blksz != 1024) {bg_idx = (*di_buffer) / blk_per_grp;} else {bg_idx = (*di_buffer) / blk_per_grp;
bg_idx = (*di_buffer) / blk_per_grp;You don't need the brackets here (or below).
Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.
OK. What is di_buffer such that (*di_buffer) works but *di_buffer doesn't?
Regards, Simon
On Mon, 5 May 2014 08:24:22 -0600 Simon Glass sjg@chromium.org wrote:
Hi Lukasz,
On 4 May 2014 23:20, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Simon,
On 30 April 2014 03:39, Lukasz Majewski l.majewski@samsung.com wrote:
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Reviewed-by: Simon Glass sjg@chromium.org
fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 50 ++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c
[snip]
@@ -181,10 +179,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer);
if (fs->blksz != 1024) {bg_idx = (*di_buffer) /blk_per_grp;
} else {bg_idx = (*di_buffer) /blk_per_grp;
bg_idx = (*di_buffer) / blk_per_grp;You don't need the brackets here (or below).
Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.
OK. What is di_buffer such that (*di_buffer) works but *di_buffer doesn't?
It is hard to admit :-), but I've misunderstood you. I thought that you are talking about the {} parentheses.
I will check the code tomorrow and prepare proper patch.
Regards, Simon
Best regards, Lukasz Majewski
Hi Lukasz,
On 5 May 2014 15:10, Lukasz Majewski l.majewski@majess.pl wrote:
On Mon, 5 May 2014 08:24:22 -0600 Simon Glass sjg@chromium.org wrote:
Hi Lukasz,
On 4 May 2014 23:20, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Simon,
On 30 April 2014 03:39, Lukasz Majewski l.majewski@samsung.com wrote:
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Reviewed-by: Simon Glass sjg@chromium.org
fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 50 ++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c
[snip]
@@ -181,10 +179,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer);
if (fs->blksz != 1024) {bg_idx = (*di_buffer) /blk_per_grp;
} else {bg_idx = (*di_buffer) /blk_per_grp;
bg_idx = (*di_buffer) / blk_per_grp;You don't need the brackets here (or below).
Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.
OK. What is di_buffer such that (*di_buffer) works but *di_buffer doesn't?
It is hard to admit :-), but I've misunderstood you. I thought that you are talking about the {} parentheses.
I will check the code tomorrow and prepare proper patch.
Ah, sorry I wasn't at all clear on that.
Regards, Simon
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(-)
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);
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)
- 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.
- 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:
- 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
This code fixes problem with storing file with the same name multiple times on the same ext4 fs (with different sizes).
Code reorganization and cleanup is also included.
Lukasz Majewski (2): fs:ext4:cleanup: Remove superfluous code fs:ext4:write:fix: Reinitialize global variables after updating a file
fs/ext4/ext4_common.c | 41 ++++++++++++++++++++++++----------- fs/ext4/ext4_write.c | 57 +++++++++++++++++-------------------------------- include/ext4fs.h | 1 + 3 files changed, 49 insertions(+), 50 deletions(-)
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com --- Changes for v2: - Remove () parenthesis around pointers --- fs/ext4/ext4_common.c | 6 ++---- fs/ext4/ext4_write.c | 56 +++++++++++++++++-------------------------------- 2 files changed, 21 insertions(+), 41 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 02da75c..62e2e80 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -904,10 +904,8 @@ long int ext4fs_get_new_blk_no(void) restart: fs->curr_blkno++; /* get the blockbitmap index respective to blockno */ - if (fs->blksz != 1024) { - bg_idx = fs->curr_blkno / blk_per_grp; - } else { - bg_idx = fs->curr_blkno / blk_per_grp; + bg_idx = fs->curr_blkno / blk_per_grp; + if (fs->blksz == 1024) { remainder = fs->curr_blkno % blk_per_grp; if (!remainder) bg_idx--; diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c index b674b6f..3db22f8 100644 --- a/fs/ext4/ext4_write.c +++ b/fs/ext4/ext4_write.c @@ -116,10 +116,8 @@ static void delete_single_indirect_block(struct ext2_inode *inode) if (inode->b.blocks.indir_block != 0) { debug("SIPB releasing %u\n", inode->b.blocks.indir_block); blknr = inode->b.blocks.indir_block; - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -181,11 +179,9 @@ static void delete_double_indirect_block(struct ext2_inode *inode) break;
debug("DICB releasing %u\n", *di_buffer); - if (fs->blksz != 1024) { - bg_idx = (*di_buffer) / blk_per_grp; - } else { - bg_idx = (*di_buffer) / blk_per_grp; - remainder = (*di_buffer) % blk_per_grp; + bg_idx = *di_buffer / blk_per_grp; + if (fs->blksz == 1024) { + remainder = *di_buffer % blk_per_grp; if (!remainder) bg_idx--; } @@ -213,10 +209,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
/* removing the parent double indirect block */ blknr = inode->b.blocks.double_indir_block; - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -293,12 +287,9 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) for (j = 0; j < fs->blksz / sizeof(int); j++) { if (*tip_buffer == 0) break; - if (fs->blksz != 1024) { - bg_idx = (*tip_buffer) / blk_per_grp; - } else { - bg_idx = (*tip_buffer) / blk_per_grp; - - remainder = (*tip_buffer) % blk_per_grp; + bg_idx = *tip_buffer / blk_per_grp; + if (fs->blksz == 1024) { + remainder = *tip_buffer % blk_per_grp; if (!remainder) bg_idx--; } @@ -336,12 +327,9 @@ static void delete_triple_indirect_block(struct ext2_inode *inode) * removing the grand parent blocks * which is connected to inode */ - if (fs->blksz != 1024) { - bg_idx = (*tigp_buffer) / blk_per_grp; - } else { - bg_idx = (*tigp_buffer) / blk_per_grp; - - remainder = (*tigp_buffer) % blk_per_grp; + bg_idx = *tigp_buffer / blk_per_grp; + if (fs->blksz == 1024) { + remainder = *tigp_buffer % blk_per_grp; if (!remainder) bg_idx--; } @@ -371,10 +359,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
/* removing the grand parent triple indirect block */ blknr = inode->b.blocks.triple_indir_block; - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -452,10 +438,8 @@ static int ext4fs_delete_file(int inodeno)
for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&(node_inode->inode), i); - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--; @@ -499,10 +483,8 @@ static int ext4fs_delete_file(int inodeno) no_blocks++; for (i = 0; i < no_blocks; i++) { blknr = read_allocated_block(&inode, i); - if (fs->blksz != 1024) { - bg_idx = blknr / blk_per_grp; - } else { - bg_idx = blknr / blk_per_grp; + bg_idx = blknr / blk_per_grp; + if (fs->blksz == 1024) { remainder = blknr % blk_per_grp; if (!remainder) bg_idx--;
On Tue, May 06, 2014 at 09:36:04AM +0200, Łukasz Majewski wrote:
Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.
This patch does that for ext4 common and write files.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com
Applied to u-boot/master, thanks!
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 LTHOR/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 --- Changes for v2: - Add comment describing the purpose of ext4fs_reinit_global() function --- fs/ext4/ext4_common.c | 35 ++++++++++++++++++++++++++--------- fs/ext4/ext4_write.c | 1 + include/ext4fs.h | 1 + 3 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 62e2e80..1c11721 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -1841,16 +1841,20 @@ long int read_allocated_block(struct ext2_inode *inode, int fileblock) return blknr; }
-void ext4fs_close(void) +/** + * ext4fs_reinit_global() - Reinitialize values of ext4 write implementation's + * global pointers + * + * This function assures that for a file with the same name but different size + * the sequential store on the ext4 filesystem will be correct. + * + * In this function the global data, responsible for internal representation + * of the ext4 data are initialized to the reset state. Without this, during + * replacement of the smaller file with the bigger truncation of new file was + * performed. + */ +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 +1874,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 3db22f8..c42add9 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);
On Tue, May 06, 2014 at 09:36:05AM +0200, Łukasz Majewski 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)
- Developer wants to upload their custom uImage [**]
correctly - we are able to store the whole smaller file with corrupted ext4fs_indir2_block pointer
- When new uImage [**] is smaller than the [*] - everything works
since truncation on data stored at eMMC was done.
- When new uImage [**] is larger than the [*] - theCRC is corrupted,
- When uImage CRC error appears, then reboot and LTHOR/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
Applied to u-boot/master, thanks!
participants (4)
-
Lukasz Majewski -
Lukasz Majewski -
Simon Glass -
Tom Rini