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@xxxxxxx
>
> 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@xxxxxxxx>
SUSE Labs, CR
