for example, 1708 inodes every group,3 block groups, bitmap bytes are 1708/8=213.5 when the inode bitmap has some errors, e2fsprogs cannot fix it
Signed-off-by: GaoMing <[email protected]>
---
e2fsck/pass5.c | 9 ++++-----
lib/ext2fs/imager.c | 35 +++++++++++++++++++++++++++--------
lib/ext2fs/rw_bitmaps.c | 39 ++++++++++++++++++++++++++++-----------
misc/dumpe2fs.c | 4 ++--
4 files changed, 61 insertions(+), 26 deletions(-)
diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index 7803e8b..4970dae 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -95,7 +95,7 @@ static void check_inode_bitmap_checksum(e2fsck_t ctx)
if (ext2fs_test_ib_dirty(ctx->fs))
return;
- nbytes = (size_t)(EXT2_INODES_PER_GROUP(ctx->fs->super) / 8);
+ nbytes = (size_t)((EXT2_INODES_PER_GROUP(ctx->fs->super)+7) / 8);
retval = ext2fs_get_mem(ctx->fs->blocksize, &buf);
if (retval) {
com_err(ctx->program_name, 0, "%s",
@@ -108,14 +108,13 @@ static void check_inode_bitmap_checksum(e2fsck_t ctx)
if (ext2fs_bg_flags_test(ctx->fs, i, EXT2_BG_INODE_UNINIT))
continue;
- ino_itr = 1 + (i * (nbytes << 3));
+ ino_itr = 1 + (i * EXT2_INODES_PER_GROUP(ctx->fs->super));
retval = ext2fs_get_inode_bitmap_range2(ctx->fs->inode_map,
- ino_itr, nbytes << 3,
+ ino_itr, EXT2_INODES_PER_GROUP(ctx->fs->super),
buf);
if (retval)
break;
-
- if (ext2fs_inode_bitmap_csum_verify(ctx->fs, i, buf, nbytes))
+ if (ext2fs_inode_bitmap_csum_verify(ctx->fs, i, buf, EXT2_INODES_PER_GROUP(ctx->fs->super) / 8))
continue;
pctx.group = i;
if (!fix_problem(ctx, PR_5_INODE_BITMAP_CSUM_INVALID, &pctx))
diff --git a/lib/ext2fs/imager.c b/lib/ext2fs/imager.c
index 7fd06f7..346ec70 100644
--- a/lib/ext2fs/imager.c
+++ b/lib/ext2fs/imager.c
@@ -341,7 +341,8 @@ errcode_t ext2fs_image_bitmap_write(ext2_filsys fs, int fd, int flags)
bmap = fs->inode_map;
itr = 1;
cnt = EXT2_INODES_PER_GROUP(fs->super) * fs->group_desc_count;
- size = (EXT2_INODES_PER_GROUP(fs->super) / 8);
+ size = ((EXT2_INODES_PER_GROUP(fs->super)+7) / 8);
+ total_size = (EXT2_INODES_PER_GROUP(fs->super) * fs->group_desc_count+7)/8;
} else {
if (!fs->block_map) {
retval = ext2fs_read_block_bitmap(fs);
@@ -352,13 +353,14 @@ errcode_t ext2fs_image_bitmap_write(ext2_filsys fs, int fd, int flags)
itr = fs->super->s_first_data_block;
cnt = EXT2_GROUPS_TO_CLUSTERS(fs->super, fs->group_desc_count);
size = EXT2_CLUSTERS_PER_GROUP(fs->super) / 8;
+ total_size = size * fs->group_desc_count;
}
- total_size = size * fs->group_desc_count;
+
while (cnt > 0) {
size = sizeof(buf);
- if (size > (cnt >> 3))
- size = (cnt >> 3);
+ if (size > ((cnt+7) >> 3))
+ size = ((cnt+7) >> 3);
retval = ext2fs_get_generic_bmap_range(bmap, itr,
size << 3, buf);
@@ -372,7 +374,10 @@ errcode_t ext2fs_image_bitmap_write(ext2_filsys fs, int fd, int flags)
return EXT2_ET_SHORT_READ;
itr += size << 3;
- cnt -= size << 3;
+ if (cnt < (size << 3))
+ cnt = 0;
+ else
+ cnt -= size << 3;
}
size = total_size % fs->blocksize;
@@ -406,6 +411,7 @@ errcode_t ext2fs_image_bitmap_read(ext2_filsys fs, int fd, int flags)
char buf[1024];
unsigned int size;
ssize_t actual;
+ __u64 count, pos;
if (flags & IMAGER_FLAG_INODEMAP) {
if (!fs->inode_map) {
@@ -431,8 +437,8 @@ errcode_t ext2fs_image_bitmap_read(ext2_filsys fs, int fd, int flags)
while (cnt > 0) {
size = sizeof(buf);
- if (size > (cnt >> 3))
- size = (cnt >> 3);
+ if (size > ((cnt+7) >> 3))
+ size = ((cnt+7) >> 3);
actual = read(fd, buf, size);
if (actual == -1)
@@ -440,13 +446,26 @@ errcode_t ext2fs_image_bitmap_read(ext2_filsys fs, int fd, int flags)
if (actual != (int) size)
return EXT2_ET_SHORT_READ;
+ if (cnt%8 != 0 && cnt < sizeof(buf)) {
+ pos = cnt;
+ count = (((cnt + 7)>>3)<<3)-cnt;
+ while (count > 0) {
+ ext2fs_fast_clear_bit(pos, buf);
+ pos++;
+ count--;
+ }
+ }
+
retval = ext2fs_set_generic_bmap_range(bmap, itr,
size << 3, buf);
if (retval)
return retval;
itr += size << 3;
- cnt -= size << 3;
+ if (cnt < (size << 3))
+ cnt = 0;
+ else
+ cnt -= size << 3;
}
return 0;
}
diff --git a/lib/ext2fs/rw_bitmaps.c b/lib/ext2fs/rw_bitmaps.c
index e86bacd..e2499ff 100644
--- a/lib/ext2fs/rw_bitmaps.c
+++ b/lib/ext2fs/rw_bitmaps.c
@@ -40,6 +40,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
blk64_t blk;
blk64_t blk_itr = EXT2FS_B2C(fs, fs->super->s_first_data_block);
ext2_ino_t ino_itr = 1;
+ __u64 count, pos;
EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
@@ -115,19 +116,25 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
if (csum_flag && ext2fs_bg_flags_test(fs, i, EXT2_BG_INODE_UNINIT)
)
goto skip_this_inode_bitmap;
-
retval = ext2fs_get_inode_bitmap_range2(fs->inode_map,
- ino_itr, inode_nbytes << 3, inode_buf);
+ ino_itr, EXT2_INODES_PER_GROUP(fs->super), inode_buf);
if (retval)
goto errout;
-
retval = ext2fs_inode_bitmap_csum_set(fs, i, inode_buf,
- inode_nbytes);
+ EXT2_INODES_PER_GROUP(fs->super) / 8);
if (retval)
goto errout;
+ if (EXT2_INODES_PER_GROUP(fs->super)%8 != 0) {
+ pos = EXT2_INODES_PER_GROUP(fs->super);
+ count = (((pos + 7) >> 3)<<3)-pos;
+ while (count > 0) {
+ ext2fs_fast_set_bit(pos, inode_buf);
+ pos++;
+ count--;
+ }
+ }
ext2fs_group_desc_csum_set(fs, i);
fs->flags |= EXT2_FLAG_DIRTY;
-
blk = ext2fs_inode_bitmap_loc(fs, i);
if (blk) {
retval = io_channel_write_blk64(fs->io, blk, 1,
@@ -138,7 +145,7 @@ static errcode_t write_bitmaps(ext2_filsys fs, int do_inode, int do_block)
}
}
skip_this_inode_bitmap:
- ino_itr += inode_nbytes << 3;
+ ino_itr += EXT2_INODES_PER_GROUP(fs->super);
}
if (do_block) {
@@ -202,7 +209,7 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
char *buf;
errcode_t retval;
int block_nbytes = EXT2_CLUSTERS_PER_GROUP(fs->super) / 8;
- int inode_nbytes = EXT2_INODES_PER_GROUP(fs->super) / 8;
+ int inode_nbytes = (EXT2_INODES_PER_GROUP(fs->super) + 7) / 8;
int csum_flag;
unsigned int cnt;
blk64_t blk;
@@ -210,6 +217,7 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
blk64_t blk_cnt;
ext2_ino_t ino_itr = 1;
ext2_ino_t ino_cnt;
+ __u64 count, pos;
EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
@@ -337,24 +345,33 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
retval = EXT2_ET_INODE_BITMAP_READ;
goto cleanup;
}
-
+ if (EXT2_INODES_PER_GROUP(fs->super)%8 != 0) {
+ pos = EXT2_INODES_PER_GROUP(fs->super);
+ count = (((pos + 7) >> 3)<<3)-pos;
+ while (count > 0) {
+ ext2fs_fast_clear_bit(pos, inode_bitmap);
+ pos++;
+ count--;
+ }
+ }
/* verify inode bitmap checksum */
if (!(fs->flags &
EXT2_FLAG_IGNORE_CSUM_ERRORS) &&
!ext2fs_inode_bitmap_csum_verify(fs, i,
- inode_bitmap, inode_nbytes)) {
+ inode_bitmap,
+ EXT2_INODES_PER_GROUP(fs->super) / 8)) {
retval =
EXT2_ET_INODE_BITMAP_CSUM_INVALID;
goto cleanup;
}
} else
memset(inode_bitmap, 0, inode_nbytes);
- cnt = inode_nbytes << 3;
+ cnt = EXT2_INODES_PER_GROUP(fs->super);
retval = ext2fs_set_inode_bitmap_range2(fs->inode_map,
ino_itr, cnt, inode_bitmap);
if (retval)
goto cleanup;
- ino_itr += inode_nbytes << 3;
+ ino_itr += EXT2_INODES_PER_GROUP(fs->super);
}
}
diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
index 395ea9e..5cf431a 100644
--- a/misc/dumpe2fs.c
+++ b/misc/dumpe2fs.c
@@ -168,7 +168,7 @@ static void list_desc(ext2_filsys fs, int grp_only)
units = _("clusters");
block_nbytes = EXT2_CLUSTERS_PER_GROUP(fs->super) / 8;
- inode_nbytes = EXT2_INODES_PER_GROUP(fs->super) / 8;
+ inode_nbytes = (EXT2_INODES_PER_GROUP(fs->super)+7) / 8;
if (fs->block_map)
block_bitmap = malloc(block_nbytes);
@@ -303,7 +303,7 @@ static void list_desc(ext2_filsys fs, int grp_only)
if (inode_bitmap) {
fputs(_(" Free inodes: "), stdout);
retval = ext2fs_get_inode_bitmap_range2(fs->inode_map,
- ino_itr, inode_nbytes << 3, inode_bitmap);
+ ino_itr, EXT2_INODES_PER_GROUP(fs->super), inode_bitmap);
if (retval)
com_err("list_desc", retval,
"while reading inode bitmap");
--
2.8.1
On Tue, Jun 26, 2018 at 07:54:06PM +0800, GaoMing wrote:
> for example, 1708 inodes every group,3 block groups, bitmap bytes are 1708/8=213.5 when the inode bitmap has some errors, e2fsprogs cannot fix it
>
> Signed-off-by: GaoMing <[email protected]>
File systems like this should not exist. Can you please please please
make sure that any use of make_ext2fs are ripped out by the roots?
What I *should* do is have e2fsck reject these file systems as
completely invalid in pass 0, when we check for superblock sanity. I
haven't because there are some older Android systems that use
make_ext2fs. But at this point, what I am **seriously** doing is to
make e2fsck reject these file systems by default, unless a setting in
e2fsck.conf is set to allow them as an exception.
My concern about applying this patch is that it will be taken as
acceptance of e2fsprogs to fully support these sorts of invalid file
systems. In particular, there is no guarantee that various tune2fs
options, resize2fs, etc., will work correctly even with your patch.
- Ted
Hi tytso,
I have checked that make_ext4fs code was deleted o Jun 21th 2018 on master branch of /system/extras repository.
e.g.
https://android-review.googlesource.com/c/platform/system/extras/+/708003
that means this tools has been widely used along serval years.
And it is probably that many many devices have been deliveryed in several year from android 3.0 to android 8.0, which used make_ext4fs.
And of course these devices cannot be recalled.
If this bug was induced in some android devices, when make_ext4fs tool produce ext4 filesystem with inodemap num not integer.
If some error occurs with this filesystem , the e2fsck try to fix it. Contrarily, it induced more errors.
I have checked tune2fs,resize2fs, these two progs not used something like " EXT2_INODES_PER_GROUP(fs->super) / 8" , or used lib which was modified by me.
Regards,
Ming
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??6??27?? 22:10
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A)
????: Re: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Tue, Jun 26, 2018 at 07:54:06PM +0800, GaoMing wrote:
> for example, 1708 inodes every group,3 block groups, bitmap bytes are 1708/8=213.5 when the inode bitmap has some errors, e2fsprogs cannot fix it
>
> Signed-off-by: GaoMing <[email protected]>
File systems like this should not exist. Can you please please please
make sure that any use of make_ext2fs are ripped out by the roots?
What I *should* do is have e2fsck reject these file systems as
completely invalid in pass 0, when we check for superblock sanity. I
haven't because there are some older Android systems that use
make_ext2fs. But at this point, what I am **seriously** doing is to
make e2fsck reject these file systems by default, unless a setting in
e2fsck.conf is set to allow them as an exception.
My concern about applying this patch is that it will be taken as
acceptance of e2fsprogs to fully support these sorts of invalid file
systems. In particular, there is no guarantee that various tune2fs
options, resize2fs, etc., will work correctly even with your patch.
- Ted
On Thu, Jun 28, 2018 at 01:40:30AM +0000, Gaoming (ming, consumer BG) wrote:
> Hi tytso,
>
> I have checked that make_ext4fs code was deleted o Jun 21th 2018 on master branch of /system/extras repository.
> e.g.
> https://android-review.googlesource.com/c/platform/system/extras/+/708003
Make_ext4fs was fixed not create invalid file systems a *long* time
ago. What I'm not sure about is why the patch hasn't gotten out to
more Android manufacters sooner.
It should have been fixed by sometime in 2012 --- I complained to the
Android team in early 2011. What I don't understand is why people are
coming out of the woodwork *now*.
- Ted
Hi,
I have cloned the repo /system/extras on master branch.
git clone https://android.googlesource.com/platform/system/extras && (cd extras && curl -Lo `git rev-parse --git-dir`/hooks/commit-msg https://gerrit-review.googlesource.com/tools/hooks/commit-msg ; chmod +x `git rev-parse --git-dir`/hooks/commit-msg)
and
git checkout git checkout 9c27970d142dc14b1e82b1d6f9fa827f994609b6 -b tmp
//this means rollback to the version before make_ext4fs code was deleted on Jun 21th 2018.
And copy the codes to android project, then compile, and produce the image. Like below,
./host/linux-x86/bin/make_ext4fs -s -l 20M -b 1024 gaoming.img
Creating filesystem with parameters:
Size: 20971520
Block size: 1024
Blocks per group: 8192
Inodes per group: 1708
Inode size: 256
Journal blocks: 1024
Label:
Blocks: 20480
Block groups: 3
Reserved block group size: 95
Created filesystem with 11/5124 inodes and 2509/20480 blocks
You see, Inodes per group is 1708,which is illegal as you said.
So, the problem exists a long time until Jun 21th 2018.
You complained the problem in 2011, they do not fix it till 2018.
Just as
I complained, fix it, and you do not accept it. ^_^
Regards,
Ming
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??6??28?? 10:29
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D)
????: Re: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Thu, Jun 28, 2018 at 01:40:30AM +0000, Gaoming (ming, consumer BG) wrote:
> Hi tytso,
>
> I have checked that make_ext4fs code was deleted o Jun 21th 2018 on master branch of /system/extras repository.
> e.g.
> https://android-review.googlesource.com/c/platform/system/extras/+/708003
Make_ext4fs was fixed not create invalid file systems a *long* time
ago. What I'm not sure about is why the patch hasn't gotten out to
more Android manufacters sooner.
It should have been fixed by sometime in 2012 --- I complained to the
Android team in early 2011. What I don't understand is why people are
coming out of the woodwork *now*.
- Ted
On Thu, Jun 28, 2018 at 07:56:59AM +0000, Gaoming (ming, consumer BG) wrote:
> You see, Inodes per group is 1708,which is illegal as you said.
>
> So, the problem exists a long time until Jun 21th 2018.
>
> You complained the problem in 2011, they do not fix it till 2018.
> Just as
> I complained, fix it, and you do not accept it. ^_^
Here's the commit. (Note the date.) Do you see it in your repository?
Or are you using an unusual inode size? (e.g., not 256 bytes, or 16
inodes per block).
- Ted
commit 06c35f935a7adadceb9ee326b3231f952f6ad203
Merge: 480a3b1 96cc54a
Author: Colin Cross <[email protected]>
Date: Sat Apr 30 19:08:09 2011 -0700
Merge "Make inodes per group a multiple of inodes per block"
diff --cc ext4_utils/make_ext4fs.c
index c9fd992,e37d617..22c9424
--- a/ext4_utils/make_ext4fs.c
+++ b/ext4_utils/make_ext4fs.c
@@@ -214,26 -215,10 +214,27 @@@ static u32 compute_inodes_per_group(
{
u32 blocks = DIV_ROUND_UP(info.len, info.block_size);
u32 block_groups = DIV_ROUND_UP(blocks, info.blocks_per_group);
- return DIV_ROUND_UP(info.inodes, block_groups);
+ u32 inodes = DIV_ROUND_UP(info.inodes, block_groups);
+ return ALIGN(inodes, (info.block_size / info.inode_size));
}
+static u32 compute_bg_desc_reserve_blocks()
+{
+ u32 blocks = DIV_ROUND_UP(info.len, info.block_size);
+ u32 block_groups = DIV_ROUND_UP(blocks, info.blocks_per_group);
+ u32 bg_desc_blocks = DIV_ROUND_UP(block_groups * sizeof(struct ext2_group_desc),
+ info.block_size);
+
+ u32 bg_desc_reserve_blocks =
+ DIV_ROUND_UP(block_groups * 1024 * sizeof(struct ext2_group_desc),
+ info.block_size) - bg_desc_blocks;
+
+ if (bg_desc_reserve_blocks > info.block_size / sizeof(u32))
+ bg_desc_reserve_blocks = info.block_size / sizeof(u32);
+
+ return bg_desc_reserve_blocks;
+}
+
void reset_ext4fs_info() {
// Reset all the global data structures used by make_ext4fs so it
// can be called again.
We use usual inode size, it is 256 bytes.
Yes, this commit is in my repository.
But there is a bug in this patch.
Let me show you,
Here is the bug: " return ALIGN(inodes, (info.block_size / info.inode_size));"
In my reproduce,
info.block_size = 1024, (it is legal)
info.inode_size =256,
the code only ALIGN BY 4.
But in real scenario, we should align by 8,( 8 bit per bytes in inode map).
So we got Inodes per group: 1708 , which is illegal.
Like below,
./host/linux-x86/bin/make_ext4fs -s -l 20M -b 1024 gaoming.img
Creating filesystem with parameters:
Size: 20971520
Block size: 1024
Blocks per group: 8192
Inodes per group: 1708
Inode size: 256
Journal blocks: 1024
Label:
Blocks: 20480
Block groups: 3
Reserved block group size: 95
Created filesystem with 11/5124 inodes and 2509/20480 blocks
This bug exists very very long time till now.
Regards,
Ming
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??6??28?? 23:30
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D); Renlipeng (OS driver)
????: Re: ????: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Thu, Jun 28, 2018 at 07:56:59AM +0000, Gaoming (ming, consumer BG) wrote:
> You see, Inodes per group is 1708,which is illegal as you said.
>
> So, the problem exists a long time until Jun 21th 2018.
>
> You complained the problem in 2011, they do not fix it till 2018.
> Just as
> I complained, fix it, and you do not accept it. ^_^
Here's the commit. (Note the date.) Do you see it in your repository?
Or are you using an unusual inode size? (e.g., not 256 bytes, or 16
inodes per block).
- Ted
commit 06c35f935a7adadceb9ee326b3231f952f6ad203
Merge: 480a3b1 96cc54a
Author: Colin Cross <[email protected]>
Date: Sat Apr 30 19:08:09 2011 -0700
Merge "Make inodes per group a multiple of inodes per block"
diff --cc ext4_utils/make_ext4fs.c
index c9fd992,e37d617..22c9424
--- a/ext4_utils/make_ext4fs.c
+++ b/ext4_utils/make_ext4fs.c
@@@ -214,26 -215,10 +214,27 @@@ static u32 compute_inodes_per_group(
{
u32 blocks = DIV_ROUND_UP(info.len, info.block_size);
u32 block_groups = DIV_ROUND_UP(blocks, info.blocks_per_group);
- return DIV_ROUND_UP(info.inodes, block_groups);
+ u32 inodes = DIV_ROUND_UP(info.inodes, block_groups);
+ return ALIGN(inodes, (info.block_size / info.inode_size));
}
+static u32 compute_bg_desc_reserve_blocks()
+{
+ u32 blocks = DIV_ROUND_UP(info.len, info.block_size);
+ u32 block_groups = DIV_ROUND_UP(blocks, info.blocks_per_group);
+ u32 bg_desc_blocks = DIV_ROUND_UP(block_groups * sizeof(struct ext2_group_desc),
+ info.block_size);
+
+ u32 bg_desc_reserve_blocks =
+ DIV_ROUND_UP(block_groups * 1024 * sizeof(struct ext2_group_desc),
+ info.block_size) - bg_desc_blocks;
+
+ if (bg_desc_reserve_blocks > info.block_size / sizeof(u32))
+ bg_desc_reserve_blocks = info.block_size / sizeof(u32);
+
+ return bg_desc_reserve_blocks;
+}
+
void reset_ext4fs_info() {
// Reset all the global data structures used by make_ext4fs so it
// can be called again.
On Fri, Jun 29, 2018 at 02:06:03AM +0000, Gaoming (ming, consumer BG) wrote:
> We use usual inode size, it is 256 bytes.
>
> Yes, this commit is in my repository.
> But there is a bug in this patch.
>
> Let me show you,
> Here is the bug: " return ALIGN(inodes, (info.block_size / info.inode_size));"
>
> In my reproduce,
> info.block_size = 1024, (it is legal)
> info.inode_size =256,
But why are you using a block size of 1024? It's legal, but in
general, it's a bad idea (often will hurt performance, will impose
additional scalability limitations, etc.).... It's certainly not
the default.
A block size of 1024 is also incompatible with file-based encryption
(FBE), which is required for a number of Android features. Support
for FBE is strongly required, and it wouldn't surprise me if it
becomes an mandatory requirement at some point in the future.
How long have you been using a block size of 1024, and why?
> But in real scenario, we should align by 8,( 8 bit per bytes in inode map).
Yes, and that's why mke2fs does. make_ext4fs was an Android specific
abomination, that I've been pushing hard for Android to rid itself of.
This is not the only thing it's gotten wrong in the past, and I've had
to help Android developers deal with data corrupting bugs caused by
the fact that way back when, Android had a GPL-in-userspace allergy.
- Ted
Yes, it is caused by using 1024 blocksize.
It is historical problem, and I have to admit that's not good idea. I don't know why somebody choose it some years before.
It has been corrected two years before or more early. But some ancient devices exist.
It is not user data, no need to do file-based encryption. It is a small partition for some use.
However, 1024 is legal though not good, somebody may use it.
And we should fix it.
Regards,
Ming
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??6??29?? 22:26
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D); Renlipeng (OS driver)
????: Re: ????: ????: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Fri, Jun 29, 2018 at 02:06:03AM +0000, Gaoming (ming, consumer BG) wrote:
> We use usual inode size, it is 256 bytes.
>
> Yes, this commit is in my repository.
> But there is a bug in this patch.
>
> Let me show you,
> Here is the bug: " return ALIGN(inodes, (info.block_size / info.inode_size));"
>
> In my reproduce,
> info.block_size = 1024, (it is legal)
> info.inode_size =256,
But why are you using a block size of 1024? It's legal, but in
general, it's a bad idea (often will hurt performance, will impose
additional scalability limitations, etc.).... It's certainly not
the default.
A block size of 1024 is also incompatible with file-based encryption
(FBE), which is required for a number of Android features. Support
for FBE is strongly required, and it wouldn't surprise me if it
becomes an mandatory requirement at some point in the future.
How long have you been using a block size of 1024, and why?
> But in real scenario, we should align by 8,( 8 bit per bytes in inode map).
Yes, and that's why mke2fs does. make_ext4fs was an Android specific
abomination, that I've been pushing hard for Android to rid itself of.
This is not the only thing it's gotten wrong in the past, and I've had
to help Android developers deal with data corrupting bugs caused by
the fact that way back when, Android had a GPL-in-userspace allergy.
- Ted
On Sat, Jun 30, 2018 at 01:26:43AM +0000, Gaoming (ming, consumer BG) wrote:
> Yes, it is caused by using 1024 blocksize.
> It is historical problem, and I have to admit that's not good idea. I don't know why somebody choose it some years before.
> It has been corrected two years before or more early. But some ancient devices exist.
> It is not user data, no need to do file-based encryption. It is a small partition for some use.
>
> However, 1024 is legal though not good, somebody may use it.
> And we should fix it.
So you understand my position --- the reason why I've been pushing so
hard is I'm trying to figure out how big of a problem this is.
Specifically speaking, is this a Huawei-specific problem, or something
across the entire Android ecosystem. I *thought* I had fixed most of
the disaster back in 2011. There have periodic headaches where
testers would discover problems where android handsets get bricked
after doing a factory reset that I had tracked down to make_ext4fs,
and the existence of make_ext4fs is not something I agreed to, and
have been fighting for years. So I've been cleaning up after
make_ext4fs for a while, even though it's not my responsiblity. (For
one thing my work responsibilities are for data center servers at
Google, *not* Android; for another, no one asked *me* before they came
up with the abomination which is make_ext4fs.)
So I don't feel particularly, or personally, responsible for bugs
caused by make_ext4fs, because if it had been up to me, it would have
never existed in the first place.
If it's only in ancient Huawei devices, I don't see a strong reason to
support his in upstream e2fsprogs. Are you really going to be
backporting the latest e2fsprogs into these ancient Huawei devices?
In my experience, the Android team has a hard enough time getting
their Android partners to backport kernel fixes for severe security
bugs into old Android devices --- never mind versions of e2fsprogs.
If not, what's the point?
Regards,
- Ted
I got it. You hate make_ext4fs, and me too.
You don't like to merge this patch in upstream e2fsprogs to resolve the bug of make_ext4fs.
Of course we will fix the bug on our ancient devices, we have to .
If this problem fixed or this patch merges in latest e2fsprogs, we will backport the latest e2fsprogs.
If not, we have no motivation to backport it.
I don't know whether other android devices suffer this problem.
If someone else come to complain this problem, you should consider to fix it.
Best wishes.
Ming
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??6??30?? 21:04
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D); Renlipeng (OS driver)
????: Re: ????: ????: ????: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Sat, Jun 30, 2018 at 01:26:43AM +0000, Gaoming (ming, consumer BG) wrote:
> Yes, it is caused by using 1024 blocksize.
> It is historical problem, and I have to admit that's not good idea. I don't know why somebody choose it some years before.
> It has been corrected two years before or more early. But some ancient devices exist.
> It is not user data, no need to do file-based encryption. It is a small partition for some use.
>
> However, 1024 is legal though not good, somebody may use it.
> And we should fix it.
So you understand my position --- the reason why I've been pushing so
hard is I'm trying to figure out how big of a problem this is.
Specifically speaking, is this a Huawei-specific problem, or something
across the entire Android ecosystem. I *thought* I had fixed most of
the disaster back in 2011. There have periodic headaches where
testers would discover problems where android handsets get bricked
after doing a factory reset that I had tracked down to make_ext4fs,
and the existence of make_ext4fs is not something I agreed to, and
have been fighting for years. So I've been cleaning up after
make_ext4fs for a while, even though it's not my responsiblity. (For
one thing my work responsibilities are for data center servers at
Google, *not* Android; for another, no one asked *me* before they came
up with the abomination which is make_ext4fs.)
So I don't feel particularly, or personally, responsible for bugs
caused by make_ext4fs, because if it had been up to me, it would have
never existed in the first place.
If it's only in ancient Huawei devices, I don't see a strong reason to
support his in upstream e2fsprogs. Are you really going to be
backporting the latest e2fsprogs into these ancient Huawei devices?
In my experience, the Android team has a hard enough time getting
their Android partners to backport kernel fixes for severe security
bugs into old Android devices --- never mind versions of e2fsprogs.
If not, what's the point?
Regards,
- Ted
On Mon, Jul 02, 2018 at 09:34:28AM +0000, Gaoming (ming, consumer BG) wrote:
> I got it. You hate make_ext4fs, and me too.
> You don't like to merge this patch in upstream e2fsprogs to resolve the bug of make_ext4fs.
>
> Of course we will fix the bug on our ancient devices, we have to .
> If this problem fixed or this patch merges in latest e2fsprogs, we will backport the latest e2fsprogs.
> If not, we have no motivation to backport it.
>
> I don't know whether other android devices suffer this problem.
Can you be explicit and tell me how many Huawei devices uses a block
size of 1024? And can you help me understand *why* such a choice was made?
- Ted
And can you help me understand *why* such a choice was made?
-----if there is such a problem in your devices, how will you do? Is there any other choice?
----- of course, you cannot format the partition.
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??7??2?? 20:17
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D); Renlipeng (OS driver)
????: Re: ????: ????: ????: ????: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Mon, Jul 02, 2018 at 09:34:28AM +0000, Gaoming (ming, consumer BG) wrote:
> I got it. You hate make_ext4fs, and me too.
> You don't like to merge this patch in upstream e2fsprogs to resolve the bug of make_ext4fs.
>
> Of course we will fix the bug on our ancient devices, we have to .
> If this problem fixed or this patch merges in latest e2fsprogs, we will backport the latest e2fsprogs.
> If not, we have no motivation to backport it.
>
> I don't know whether other android devices suffer this problem.
Can you be explicit and tell me how many Huawei devices uses a block
size of 1024? And can you help me understand *why* such a choice was made?
- Ted
On Tue, Jul 03, 2018 at 12:58:48AM +0000, Gaoming (ming, consumer BG) wrote:
> And can you help me understand *why* such a choice was made?
> -----if there is such a problem in your devices, how will you do? Is there any other choice?
> ----- of course, you cannot format the partition.
You misunderstand my question. Why was the choice of a blocksize of
1024 made?
I'm trying to understand how many devices, and why any other
manufacture would make, what seems to me, to be a completely insane
choice.
How long has Huawei been using a 1024 byte blocksize? And why? And
for how many devices? Essentially, I'm trying to figure out if this
was a Huawei-specific mistake.
- Ted
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??7??3?? 18:36
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D); Renlipeng (OS driver)
????: Re: ????: ????: ????: ????: ????: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Tue, Jul 03, 2018 at 12:58:48AM +0000, Gaoming (ming, consumer BG) wrote:
> And can you help me understand *why* such a choice was made?
> -----if there is such a problem in your devices, how will you do? Is there any other choice?
> ----- of course, you cannot format the partition.
You misunderstand my question. Why was the choice of a blocksize of
1024 made?
-----some one choose, not me . I guess they want get more inodes in 20M filesystem.
I'm trying to understand how many devices, and why any other
manufacture would make, what seems to me, to be a completely insane
choice.
How long has Huawei been using a 1024 byte blocksize? And why? And
for how many devices? Essentially, I'm trying to figure out if this
was a Huawei-specific mistake.
----- I cannot answer this question.
- Ted
On Tue, Jul 03, 2018 at 11:15:21AM +0000, Gaoming (ming, consumer BG) wrote:
>
> You misunderstand my question. Why was the choice of a blocksize of
> 1024 made?
>
> -----some one choose, not me . I guess they want get more inodes in 20M filesystem.
That can't be the explanation.
I just checked the sources for make_ext4fs; the blocksize was
hard-coded, as was the number of inodes. So in order to use a
block_size of 1024 bytes, someone must have hacked the sources
directly and modified compute_block_size(). And in fact, it would
have been *easier* to simply hack compute_inodes() which is just a few
lines below in make_ext4fs.c:
static u32 compute_block_size()
{
return 4096;
}
static u32 compute_inodes()
{
return DIV_ROUND_UP(info.len, info.block_size) / 4;
}
This also means commit 06c35f935a7a which fixed make_ext4fs.c wasn't
buggy; it was a valid fix given the complete-non-adjustability of the
blocksize in make_ext4fs. Yes, it's not a great fix, since it was
fragile --- but that's hardly the smallest problem in make_ext4fs,
there was plenty of other super-fragile bits in make_ext4fs. There's
a *reason* I was pushing hard to make make_ext4fs go away.
That being said, given the block size of make_ext4fs was hard-coded to
4096, it makes it clear that the change to make it create a blocksize
of 1024 must have beeen a Huawei-local change, and it was never sent
back to the the common AOSP tree. (If it had, I would have gotten an
e-mail, and I would have explained to whoever had made this
Huawei-local hack why it was such a incredibly bad idea.)
In any case, what I would recommend at this point if you need to
support Huawei devices that do this, is that you keep your e2fsprogs
as a Huawei-specific e2fsprogs repo.
> How long has Huawei been using a 1024 byte blocksize? And why? And
> for how many devices? Essentially, I'm trying to figure out if this
> was a Huawei-specific mistake.
> ----- I cannot answer this question.
Well, actually, it should be very easy for you to detremine the answer
this question --- it should just be a matter of checking git source
control history and see which product trees has the change to
platforms/system/extras/ext4_utils/make_ext4fs.c --- and you must have
platform-specific source trees in order to be compliant with the GNU
Public License (GPL).
Even if you didn't, it would just be a matter using dumpe2fs or
debugfs (they aren't built by default in e2fsprogs by the AOSP build
trees, but they should be buildable) to those Huawei devices that you
still support, and then run either dumpe2fs or debugfs via adb shell
and see what block size is reported.
Even if you aren't allowed to answer this question publically, I'd
strongly recommend you figure it out, so you know when your local
change to e2fsprogs can be dropped. I'd further recommend that you
make sure all new Huawei devices use a block size of 4096, preferably
using mke2fs and e2fsdroid, as is currently done in AOSP. Failing
that, at least please use the stock make_ext4fs which doesn't have the
Huawei-specific hack to support 1024 byte block sizes, or have that
Huawei-specific hack reverted.
Regards,
- Ted
Hi,
I do not think Huawei has hacked make_ext4fs.
For it could be reproduced in make_ext4fs master brach before Jun 21th 2018. Like below,
I think we just choose not recommended parameter "-b 1024" for small partition like below, but it's legal though not good.
./host/linux-x86/bin/make_ext4fs -s -l 20M -b 1024 gaoming.img
Creating filesystem with parameters:
Size: 20971520
Block size: 1024
Blocks per group: 8192
Inodes per group: 1708
Inode size: 256
Journal blocks: 1024
Label:
Blocks: 20480
Block groups: 3
Reserved block group size: 95
Created filesystem with 11/5124 inodes and 2509/20480 blocks
Other vendors may choose this para, or not, we don't know. maybe they are lucky or wise not choosing 1024.
Of course we will choose 4096, or mke2fs from now on.
We just want to fix historical problem. We will fix it by ourselves.
I fully understand your attitude.
Thanks a lot.
Regards,
Ming
-----?ʼ?ԭ??-----
??????: Theodore Y. Ts'o [mailto:[email protected]]
????ʱ??: 2018??7??4?? 0:03
?ռ???: Gaoming (ming, consumer BG)
????: [email protected]; [email protected]; Liqingchao (sorp); Shenchen (harry); miaoxie (A); yangfei (D); Renlipeng (OS driver)
????: Re: ????: ????: ????: ????: ????: ????: ????: [PATCH] ext4: e2fsprogs: fix inode bitmap num not integer,incompatible for ancient android devices
On Tue, Jul 03, 2018 at 11:15:21AM +0000, Gaoming (ming, consumer BG) wrote:
>
> You misunderstand my question. Why was the choice of a blocksize of
> 1024 made?
>
> -----some one choose, not me . I guess they want get more inodes in 20M filesystem.
That can't be the explanation.
I just checked the sources for make_ext4fs; the blocksize was
hard-coded, as was the number of inodes. So in order to use a
block_size of 1024 bytes, someone must have hacked the sources
directly and modified compute_block_size(). And in fact, it would
have been *easier* to simply hack compute_inodes() which is just a few
lines below in make_ext4fs.c:
static u32 compute_block_size()
{
return 4096;
}
static u32 compute_inodes()
{
return DIV_ROUND_UP(info.len, info.block_size) / 4;
}
This also means commit 06c35f935a7a which fixed make_ext4fs.c wasn't
buggy; it was a valid fix given the complete-non-adjustability of the
blocksize in make_ext4fs. Yes, it's not a great fix, since it was
fragile --- but that's hardly the smallest problem in make_ext4fs,
there was plenty of other super-fragile bits in make_ext4fs. There's
a *reason* I was pushing hard to make make_ext4fs go away.
That being said, given the block size of make_ext4fs was hard-coded to
4096, it makes it clear that the change to make it create a blocksize
of 1024 must have beeen a Huawei-local change, and it was never sent
back to the the common AOSP tree. (If it had, I would have gotten an
e-mail, and I would have explained to whoever had made this
Huawei-local hack why it was such a incredibly bad idea.)
In any case, what I would recommend at this point if you need to
support Huawei devices that do this, is that you keep your e2fsprogs
as a Huawei-specific e2fsprogs repo.
> How long has Huawei been using a 1024 byte blocksize? And why? And
> for how many devices? Essentially, I'm trying to figure out if this
> was a Huawei-specific mistake.
> ----- I cannot answer this question.
Well, actually, it should be very easy for you to detremine the answer
this question --- it should just be a matter of checking git source
control history and see which product trees has the change to
platforms/system/extras/ext4_utils/make_ext4fs.c --- and you must have
platform-specific source trees in order to be compliant with the GNU
Public License (GPL).
Even if you didn't, it would just be a matter using dumpe2fs or
debugfs (they aren't built by default in e2fsprogs by the AOSP build
trees, but they should be buildable) to those Huawei devices that you
still support, and then run either dumpe2fs or debugfs via adb shell
and see what block size is reported.
Even if you aren't allowed to answer this question publically, I'd
strongly recommend you figure it out, so you know when your local
change to e2fsprogs can be dropped. I'd further recommend that you
make sure all new Huawei devices use a block size of 4096, preferably
using mke2fs and e2fsdroid, as is currently done in AOSP. Failing
that, at least please use the stock make_ext4fs which doesn't have the
Huawei-specific hack to support 1024 byte block sizes, or have that
Huawei-specific hack reverted.
Regards,
- Ted