| Message ID | 20260508121539.4174601-11-libaokun@linux.alibaba.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ext4/lib-crc: LBS performance part 1 - incremental CRC32c for bitmap checksums | expand |
On Fri, May 08, 2026 at 11:33:05PM +0000, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [High] ext4_inode_bitmap_csum_set_fast() corrupts the inode bitmap checksum if EXT4_INODES_PER_GROUP(sb) is not a multiple of 8. > > > - ext4_inode_bitmap_csum_set(sb, gdp, bitmap_bh); > > + ext4_inode_bitmap_csum_set_fast(sb, gdp, bit); > > Can this corrupt the inode bitmap checksum if EXT4_INODES_PER_GROUP(sb) is not > a multiple of 8? > > In ext4_inode_bitmap_csum_set(), the CRC32c checksum is computed over exactly > EXT4_INODES_PER_GROUP(sb) >> 3 bytes. If s_inodes_per_group is not a multiple > of 8, any trailing fractional bits are strictly excluded from the checksum > calculation. > > Because ext4_fill_super() does not enforce that s_inodes_per_group is a > multiple of 8, a crafted filesystem can have an unaligned s_inodes_per_group. The reason why ext4_fill_super() doesn't enforce that s_inodes_per_group is a multiple of 8 was that a long time ago, back when Android was allergic to GPLv2 in userspace, they implemented their own version of mke2fs (and didn't run fsck on the file system, sigh). Their MIT licensed version of make_ext4fs would occasionally make file systems that were not a multiple of 8, and this ran afoul of e2fsck[1] if someone actually tried to repair a corrupted Android user data file system (as opposed to just wiping the flash and starting from scratch). [1] https://sourceforge.net/p/e2fsprogs/bugs/292/ This was fixed long ago (over a decade ago), and so at this point, I'm pretty sure any such mobile handsets are in the landfill, so we probably should fix this by adding a check in ext4_fill_super() and a corresponding check in e2fsck. - Ted
On 2026/6/3 22:17, Theodore Tso wrote: > On Fri, May 08, 2026 at 11:33:05PM +0000, sashiko-bot@kernel.org wrote: >> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: >> - [High] ext4_inode_bitmap_csum_set_fast() corrupts the inode bitmap checksum if EXT4_INODES_PER_GROUP(sb) is not a multiple of 8. >> >>> - ext4_inode_bitmap_csum_set(sb, gdp, bitmap_bh); >>> + ext4_inode_bitmap_csum_set_fast(sb, gdp, bit); >> Can this corrupt the inode bitmap checksum if EXT4_INODES_PER_GROUP(sb) is not >> a multiple of 8? >> >> In ext4_inode_bitmap_csum_set(), the CRC32c checksum is computed over exactly >> EXT4_INODES_PER_GROUP(sb) >> 3 bytes. If s_inodes_per_group is not a multiple >> of 8, any trailing fractional bits are strictly excluded from the checksum >> calculation. >> >> Because ext4_fill_super() does not enforce that s_inodes_per_group is a >> multiple of 8, a crafted filesystem can have an unaligned s_inodes_per_group. > The reason why ext4_fill_super() doesn't enforce that > s_inodes_per_group is a multiple of 8 was that a long time ago, back > when Android was allergic to GPLv2 in userspace, they implemented > their own version of mke2fs (and didn't run fsck on the file system, > sigh). Their MIT licensed version of make_ext4fs would occasionally > make file systems that were not a multiple of 8, and this ran afoul of > e2fsck[1] if someone actually tried to repair a corrupted Android user > data file system (as opposed to just wiping the flash and starting > from scratch). > > [1] https://sourceforge.net/p/e2fsprogs/bugs/292/ > > This was fixed long ago (over a decade ago), and so at this point, I'm > pretty sure any such mobile handsets are in the landfill, so we > probably should fix this by adding a check in ext4_fill_super() and a > corresponding check in e2fsck. > > - Ted Hi Ted, Thank you for your information and suggestions. I will send two fix patches to synchronize the checks in mke2fs with ext4_fill_super and e2fsck. Thanks, Baokun
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c index 3fd8f0099852..55eb69fbb4c9 100644 --- a/fs/ext4/ialloc.c +++ b/fs/ext4/ialloc.c @@ -327,7 +327,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode) if (percpu_counter_initialized(&sbi->s_dirs_counter)) percpu_counter_dec(&sbi->s_dirs_counter); } - ext4_inode_bitmap_csum_set(sb, gdp, bitmap_bh); + ext4_inode_bitmap_csum_set_fast(sb, gdp, bit); ext4_group_desc_csum_set(sb, block_group, gdp); ext4_unlock_group(sb, block_group);
Replace ext4_inode_bitmap_csum_set() with the newly added ext4_inode_bitmap_csum_set_fast() in ext4_free_inode() for incremental inode bitmap checksum update. This is safe because: - At inode free time, the inode bitmap checksum has already been initialized, so the old checksum is always valid. - The bitmap buffer modification and checksum update are protected by the same group lock, ensuring the old checksum is consistent with the bitmap content before the bit flip. Signed-off-by: Baokun Li <libaokun@linux.alibaba.com> --- fs/ext4/ialloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)