[U-Boot] [PATCH 0/2] fs:ext4: Fixes and code cleanup

[U-Boot] [PATCH 0/2] fs:ext4: Fixes and code cleanup

[Lukasz Majewski]

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

-- 
1.7.10.4

[U-Boot] [PATCH 1/2] fs:ext4:cleanup: Remove superfluous code

[Lukasz Majewski]

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

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

[Lukasz Majewski]

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);
-- 
1.7.10.4

[U-Boot] [PATCH 1/2] fs:ext4:cleanup: Remove superfluous code

[Simon Glass]

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
>

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

[U-Boot] [PATCH 1/2] fs:ext4:cleanup: Remove superfluous code

[Lukasz Majewski]

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



-- 
Best regards,

Lukasz Majewski

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

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



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH 1/2] fs:ext4:cleanup: Remove superfluous code

[Simon Glass]

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

[U-Boot] [PATCH 1/2] fs:ext4:cleanup: Remove superfluous code

[Lukasz Majewski]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140505/417aecc9/attachment.pgp>

[U-Boot] [PATCH 1/2] fs:ext4:cleanup: Remove superfluous code

[Simon Glass]

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

[U-Boot] [PATCH v2 0/2] fs:ext4: Fixes and code cleanup

[Lukasz Majewski]

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

-- 
1.7.10.4

[U-Boot] [PATCH v2 1/2] fs:ext4:cleanup: Remove superfluous code

[Lukasz Majewski]

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

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

[Lukasz Majewski]

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);
-- 
1.7.10.4

[U-Boot] [U-Boot, v2, 1/2] fs:ext4:cleanup: Remove superfluous code

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140512/43945f1a/attachment.pgp>

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

[Tom Rini]

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

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140512/83b32b6e/attachment.pgp>