Re: [PATCH] ext4: do not BUG when INLINE_DATA_FL lacks system.data xattr

Re: [PATCH] ext4: do not BUG when INLINE_DATA_FL lacks system.data xattr

[Theodore Ts'o]

On Thu, Jul 17, 2025 at 09:59:13AM -0700, Moon Hee Lee wrote:
> The current patch addresses ext4_update_inline_data() directly, but the
> same condition also leads to a BUG_ON in ext4_create_inline_data() [2],
> which the earlier approach intended to prevent as well.

Actually, the two conditions are opposite to each other.  The one in
ext4_update_inline_data() was:

         BUG_ON(is.s.not_found);

while te one in ext4_create_inline_data() was:

	BUG_ON(!is.s.not_found);

So your patch would not only cause an extra xattr lookup in
ext4_prepare_inline_data(), but it would actually cause problems by
causing spurious failures when first writing to an inline data file.
(Which makes me suspect that you hadn't run other test on your patich
other than just vaidating that the syzkaller reproduce was no longer
reproducing.)   

Also, having taking a closer look at te code paths, I became
suspicious that there is something about the syzkaller reproducer is
doing which might be a bit sus.  That's because whether we call
ext4_update_inline_data() or ext4_create_inline_data() is based on
whether i_inline off is set or not:

	if (ei->i_inline_off)
		ret = ext4_update_inline_data(handle, inode, len);
	else
		ret = ext4_create_inline_data(handle, inode, len);


But how is ei->i_inline_off set?  It's set from a former call to
ext4_xattr_ibody_find():

	error = ext4_xattr_ibody_find(inode, &i, &is);
	if (error)
		goto out;

	if (!is.s.not_found) {
		if (is.s.here->e_value_inum) {
			EXT4_ERROR_INODE(inode, "inline data xattr refers "
					 "to an external xattr inode");
			error = -EFSCORRUPTED;
			goto out;
		}
		EXT4_I(inode)->i_inline_off = (u16)((void *)is.s.here -
					(void *)ext4_raw_inode(&is.iloc));
		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +
				le32_to_cpu(is.s.here->e_value_size);
	}

So the whole *reason* why i_inline_off exists is because we're caching
the result of calling ext4_xattr_ibody_find().  So if i_inline_off is
non-zero, and then when we call ext4_ibody_find() later on, and we
find that xattr has suddenly disappeared, there is something weird
going on.   That's why the BUG_ON was added orginally.

When I took a look at the reproduer, I found that indeed, it is
calling LOOP_CLR_FD and LOOP_SET_STATUS64 to reconfigure the loop
device out from under the mounted file system.  This is smashing the
file system, and is therefore corrupting the block device.  As it
turns out, Jan Kara recently sent out a patch, and it has been
accepted in the block tree, to prevent a similar Syzkaller issue using
LOOP_SET_BLOCK_SIZE[1].

[1] https://lore.kernel.org/r/20250711163202.19623-2-jack@suse.cz

We need to do something similar for LOOP_CLR_FD, LOOP_SET_STATUS,
LOOP_SET_STATUS64, LOOP_CHANGE_FD, and LOOP_SET_CAPACITY ioctls.

Cheers,

						- Ted

Re: [PATCH] ext4: do not BUG when INLINE_DATA_FL lacks system.data xattr

[Jan Kara]

On Thu 17-07-25 21:05:21, Theodore Ts'o wrote:
> On Thu, Jul 17, 2025 at 09:59:13AM -0700, Moon Hee Lee wrote:
> > The current patch addresses ext4_update_inline_data() directly, but the
> > same condition also leads to a BUG_ON in ext4_create_inline_data() [2],
> > which the earlier approach intended to prevent as well.
> 
> Actually, the two conditions are opposite to each other.  The one in
> ext4_update_inline_data() was:
> 
>          BUG_ON(is.s.not_found);
> 
> while te one in ext4_create_inline_data() was:
> 
> 	BUG_ON(!is.s.not_found);
> 
> So your patch would not only cause an extra xattr lookup in
> ext4_prepare_inline_data(), but it would actually cause problems by
> causing spurious failures when first writing to an inline data file.
> (Which makes me suspect that you hadn't run other test on your patich
> other than just vaidating that the syzkaller reproduce was no longer
> reproducing.)   
> 
> Also, having taking a closer look at te code paths, I became
> suspicious that there is something about the syzkaller reproducer is
> doing which might be a bit sus.  That's because whether we call
> ext4_update_inline_data() or ext4_create_inline_data() is based on
> whether i_inline off is set or not:
> 
> 	if (ei->i_inline_off)
> 		ret = ext4_update_inline_data(handle, inode, len);
> 	else
> 		ret = ext4_create_inline_data(handle, inode, len);
> 
> 
> But how is ei->i_inline_off set?  It's set from a former call to
> ext4_xattr_ibody_find():
> 
> 	error = ext4_xattr_ibody_find(inode, &i, &is);
> 	if (error)
> 		goto out;
> 
> 	if (!is.s.not_found) {
> 		if (is.s.here->e_value_inum) {
> 			EXT4_ERROR_INODE(inode, "inline data xattr refers "
> 					 "to an external xattr inode");
> 			error = -EFSCORRUPTED;
> 			goto out;
> 		}
> 		EXT4_I(inode)->i_inline_off = (u16)((void *)is.s.here -
> 					(void *)ext4_raw_inode(&is.iloc));
> 		EXT4_I(inode)->i_inline_size = EXT4_MIN_INLINE_DATA_SIZE +
> 				le32_to_cpu(is.s.here->e_value_size);
> 	}
> 
> So the whole *reason* why i_inline_off exists is because we're caching
> the result of calling ext4_xattr_ibody_find().  So if i_inline_off is
> non-zero, and then when we call ext4_ibody_find() later on, and we
> find that xattr has suddenly disappeared, there is something weird
> going on.   That's why the BUG_ON was added orginally.
> 
> When I took a look at the reproduer, I found that indeed, it is
> calling LOOP_CLR_FD and LOOP_SET_STATUS64 to reconfigure the loop
> device out from under the mounted file system.  This is smashing the
> file system, and is therefore corrupting the block device.  As it
> turns out, Jan Kara recently sent out a patch, and it has been
> accepted in the block tree, to prevent a similar Syzkaller issue using
> LOOP_SET_BLOCK_SIZE[1].
> 
> [1] https://lore.kernel.org/r/20250711163202.19623-2-jack@suse.cz
> 
> We need to do something similar for LOOP_CLR_FD, LOOP_SET_STATUS,
> LOOP_SET_STATUS64, LOOP_CHANGE_FD, and LOOP_SET_CAPACITY ioctls.

Well, careful here. Changing loop device underneath mounted filesystem is a
valid usecase in active use (similarly as changing DM device underneath a
filesystem). So don't think we can play similar tricks as with
LOOP_SET_BLOCK_SIZE where changing block device block size just doesn't
make sense while the device is in use. Similarly LOOP_CLR_FD is an
equivalent of device going away. LOOP_CHANGE_FD is a legacy of the past but
it was *designed* to be used to swap backing file under a life filesystem
(old days of Wild West :)) during boot. We may get away with dropping that
these days but so far I'm not convinced it's worth the risk. So in this case
I don't see anything here that couldn't happen with say DM device and thus
I wouldn't really restrict the loop device functionality...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: do not BUG when INLINE_DATA_FL lacks system.data xattr

[Theodore Ts'o]

On Mon, Jul 21, 2025 at 02:49:57PM +0200, Jan Kara wrote:
> > We need to do something similar for LOOP_CLR_FD, LOOP_SET_STATUS,
> > LOOP_SET_STATUS64, LOOP_CHANGE_FD, and LOOP_SET_CAPACITY ioctls.
> 
> Well, careful here. Changing loop device underneath mounted filesystem is a
> valid usecase in active use (similarly as changing DM device underneath a
> filesystem). So don't think we can play similar tricks as with
> LOOP_SET_BLOCK_SIZE where changing block device block size just doesn't
> make sense while the device is in use. Similarly LOOP_CLR_FD is an
> equivalent of device going away. LOOP_CHANGE_FD is a legacy of the past but
> it was *designed* to be used to swap backing file under a life filesystem
> (old days of Wild West :)) during boot. We may get away with dropping that
> these days but so far I'm not convinced it's worth the risk. So in this case
> I don't see anything here that couldn't happen with say DM device and thus
> I wouldn't really restrict the loop device functionality...

Sure, and LOOP_SET_CAPACITY might be used to grow a file system image
which the file system could then grow into.  Fair.

This is related to BLK_DEV_WRITE_MOUNTED=n which the syzkaller folks
have agreed to use to prevent noisy syzkaller reports.  We're seeing a
bunch of syzkaller reports now that syzkaller has been taught how to
use these loop ioctls and so we're seeing loop device hijinks.  Which
is fine; I can just start filtering any syzkaller report that uses
loop device ioctls as false positive noise and call it a day.
Unfortunately, that won't help deal with researchers that are taking
the syzkaller code and then sending reports without any dashboards or
reproducers.  :-(

However, I do think that if the file system has advertised that they
don't support random underlying block device hijinks, such as XFS for
example, we should honor this and disallow those "wild west" loop
device operations.  And perhaps we should similarly disallow them if
BLK_DEV_WRITE_MOUNTED=n.

						- Ted