Re: [PATCH v2] ext2: fix datatype of block number in ext2_xattr_set2()

[Georg Ottinger]

I missed the proper format string for the debug message.

answering Andreas question - I did check the remaining calls to
ext2_new_block(), ext2_new_blocks() and ext2_free_blocks() within the
ext2 directory - here the block argument is either unsigned long or
ext2_fsblk_t (which is a typedef to unsigend long) - However I want to
mention that the use of unsigned long / ext2_fsblk_t is inconsistent. I
guess that ext2_fsblk_t should be the prefered data type.

Concerning the fs corruption - thanks Jan for your input.

The server is an old Centos6 / RHEL6 Machine, and as a workaround - i
resized the partition to 8180GB - Unfortunately I am unsure if I find
time to investigate this issue further - I just did a quick look at the
kernel source and the Bug was kind of obvious ...


On 16.08.23 07:21, Georg Ottinger wrote:
> I run a small server that uses external hard drives for backups. The
> backup software I use uses ext2 filesystems with 4KiB block size and
> the server is running SELinux and therefore relies on xattr. I recently
> upgraded the hard drives from 4TB to 12TB models. I noticed that after
> transferring some TBs I got a filesystem error "Freeing blocks not in
> datazone - block = 18446744071529317386, count = 1" and the backup
> process stopped. Trying to fix the fs with e2fsck resulted in a
> completely corrupted fs. The error probably came from ext2_free_blocks(),
> and because of the large number 18e19 this problem immediately looked
> like some kind of integer overflow. Whereas the 4TB fs was about 1e9
> blocks, the new 12TB is about 3e9 blocks. So, searching the ext2 code,
> I came across the line in fs/ext2/xattr.c:745 where ext2_new_block()
> is called and the resulting block number is stored in the variable block
> as an int datatype. If a block with a block number greater than
> INT32_MAX is returned, this variable overflows and the call to
> sb_getblk() at line fs/ext2/xattr.c:750 fails, then the call to
> ext2_free_blocks() produces the error.
>
> Signed-off-by: Georg Ottinger <g.ottinger@xxxxxx>
> ---
>   fs/ext2/xattr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 8906ba479..89517937d 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -742,10 +742,10 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
>   			/* We need to allocate a new block */
>   			ext2_fsblk_t goal = ext2_group_first_block_no(sb,
>   						EXT2_I(inode)->i_block_group);
> -			int block = ext2_new_block(inode, goal, &error);
> +			ext2_fsblk_t block = ext2_new_block(inode, goal, &error);
>   			if (error)
>   				goto cleanup;
> -			ea_idebug(inode, "creating block %d", block);
> +			ea_idebug(inode, "creating block %lu", block);
>
>   			new_bh = sb_getblk(sb, block);
>   			if (unlikely(!new_bh)) {
> --
> 2.17.1