From: sunjunchao <[email protected]>
There is a BUG_ON statement which will be triggered in the
following scenario, let's remove it.
thread0 thread1
ext4_write_begin(inode0)
->ext4_try_to_write_inline_data()
written some bits successfully
ext4_write_end(inode0)
->ext4_write_inline_data_end()
ext4_write_begin(inode0)
->ext4_try_to_write_inline_data()
->ext4_convert_inline_data_to_extent()
->ext4_write_lock_xattr()
->ext4_destroy_inline_data_nolock()
->ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
->ext4_write_unlock_xattr()
->ext4_write_lock_xattr()
->BUG_ON(!ext4_has_inline_data()) will be triggered
The problematic logic is that ext4_write_end() test ext4_has_inline_data()
without holding xattr_sem, and ext4_write_inline_data_end() test it again using
a BUG_ON() with holding xattr_sem.
Signed-off-by: JunChao Sun <[email protected]>
---
fs/ext4/inline.c | 6 +++++-
fs/ext4/inode.c | 5 ++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 1602d74..b5752975 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -753,7 +753,11 @@ int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
goto out;
}
ext4_write_lock_xattr(inode, &no_expand);
- BUG_ON(!ext4_has_inline_data(inode));
+ if (!ext4_has_inline_data(inode)) {
+ ext4_write_unlock_xattr(inode, &no_expand);
+ brelse(iloc.bh);
+ return -ENODATA;
+ }
/*
* ei->i_inline_off may have changed since
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index bf0b7de..7502657 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1317,7 +1317,10 @@ static int ext4_write_end(struct file *file,
if (ext4_has_inline_data(inode) &&
ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
- return ext4_write_inline_data_end(inode, pos, len, copied, page);
+ ret = ext4_write_inline_data_end(inode, pos, len, copied, page);
+
+ if (ret != -ENODATA)
+ return ret;
copied = block_write_end(file, mapping, pos, len, copied, page, fsdata);
/*
--
1.8.3.1
On Wed 12-04-23 00:47:37, JunChao Sun wrote:
> From: sunjunchao <[email protected]>
>
> There is a BUG_ON statement which will be triggered in the
> following scenario, let's remove it.
>
> thread0 thread1
> ext4_write_begin(inode0)
> ->ext4_try_to_write_inline_data()
> written some bits successfully
> ext4_write_end(inode0)
> ->ext4_write_inline_data_end()
> ext4_write_begin(inode0)
> ->ext4_try_to_write_inline_data()
> ->ext4_convert_inline_data_to_extent()
> ->ext4_write_lock_xattr()
> ->ext4_destroy_inline_data_nolock()
> ->ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> ->ext4_write_unlock_xattr()
> ->ext4_write_lock_xattr()
> ->BUG_ON(!ext4_has_inline_data()) will be triggered
>
> The problematic logic is that ext4_write_end() test ext4_has_inline_data()
> without holding xattr_sem, and ext4_write_inline_data_end() test it again using
> a BUG_ON() with holding xattr_sem.
Were you able to actually hit this? Because inode->i_rwsem should be
protecting us from races like this so I don't think the above described
scenario can happen.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Jan Kara <[email protected]> 于2023年4月12日周三 18:45写道：
>
> On Wed 12-04-23 00:47:37, JunChao Sun wrote:
> > From: sunjunchao <[email protected]>
> >
> > There is a BUG_ON statement which will be triggered in the
> > following scenario, let's remove it.
> >
> > thread0 thread1
> > ext4_write_begin(inode0)
> > ->ext4_try_to_write_inline_data()
> > written some bits successfully
> > ext4_write_end(inode0)
> > ->ext4_write_inline_data_end()
> > ext4_write_begin(inode0)
> > ->ext4_try_to_write_inline_data()
> > ->ext4_convert_inline_data_to_extent()
> > ->ext4_write_lock_xattr()
> > ->ext4_destroy_inline_data_nolock()
> > ->ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
> > ->ext4_write_unlock_xattr()
> > ->ext4_write_lock_xattr()
> > ->BUG_ON(!ext4_has_inline_data()) will be triggered
> >
> > The problematic logic is that ext4_write_end() test ext4_has_inline_data()
> > without holding xattr_sem, and ext4_write_inline_data_end() test it again using
> > a BUG_ON() with holding xattr_sem.
>
>
> > Were you able to actually hit this? Because inode->i_rwsem should be
> > protecting us from races like this so I don't think the above described
> > scenario can happen.
Yes, you are right. The write_begin() and write_end() were protected
by inode->i_rwsem. And then I checked the operations to
EXT4_INODE_INLINE_DATA, but didn't find any race. I didn't hit that, I
was reading code and recognized that there may be a race, but wasn't
aware of the inode->i_rwsem due to my carelessness. Sorry for that.
>
> Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR