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.
Cheers,
- Ted
