On Fri, Nov 14, 2014 at 05:15:37PM +0800, Xiaoguang Wang wrote:
> If we apply this patch 'e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size',
> we will trigger a segfault, this is because of the inode cache issues.
>
> Firstly we should notice that in expand_inode_table(), we have change the super block's s_inode_size
> to new inode size(for example, 256).
>
> Then we re-compute metadata checksums, see below code flow:
> |-->rewrite_metadata_checksums
> |----->rewrite_inodes
> |-------->ext2fs_write_inode_full
> In ext2fs_write_inode_full(), if an inode cache is hit, the below code will be executed:
> /* Check to see if the inode cache needs to be updated */
> if (fs->icache) {
> for (i=0; i < fs->icache->cache_size; i++) {
> if (fs->icache->cache[i].ino == ino) {
> memcpy(fs->icache->cache[i].inode, inode,
> (bufsize > length) ? length : bufsize);
> break;
> }
> }
> }
>
> Before executing rewrite_inodes(), actually the inode in inode cache is allocated by
> old inode size(for example, 128), but here the memcpy will obviously write overflow,
> '(bufsize > length) ? length : bufsize' here will return 256(new inode size), so this is
> wrong, we need to fix this.
> I think we should call ext2fs_free_inode_cache() in expand_inode_table(), to drop the
> inode cache, because inode size has changed, if necessary, we will re-create this inode cache.
>
> Steps to reproduce this bug(apply 'e2fsprogs/tune2fs: rewrite metadata checksums when resizing inode size' first).
> dd if=/dev/zero of=file.img bs=1M count=128
> device_name=$(/sbin/losetup -f)
> /sbin/losetup -f file.img
> mkfs.ext4 -I 128 -O ^flex_bg $device_name
> tune2fs -I 256 $device_name
>
> Signed-off-by: Xiaoguang Wang <wangxg.fnst@xxxxxxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> misc/tune2fs.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 91dc7c1..34d2cd9 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -2244,6 +2244,8 @@ static int expand_inode_table(ext2_filsys fs, unsigned long new_ino_size)
>
> /* Update the meta data */
> fs->inode_blocks_per_group = new_ino_blks_per_grp;
> + ext2fs_free_inode_cache(fs->icache);
> + fs->icache = 0;
By the way (and this is a general question, not one specific to just this
patch), why do some parts of e2fsprogs assign 0 (instead of NULL) to pointers?
--D
> fs->super->s_inode_size = new_ino_size;
>
> err_out:
> --
> 1.8.2.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
