[PATCH] ext4: remove BUG_ON which will be triggered in race scenario

[PATCH] ext4: remove BUG_ON which will be triggered in race scenario

[JunChao Sun]

From: sunjunchao <sunjunchao@yanrongyun.com>

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 <sunjunchao2870@gmail.com>
---
 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

Re: [PATCH] ext4: remove BUG_ON which will be triggered in race scenario

[Jan Kara]

On Wed 12-04-23 00:47:37, JunChao Sun wrote:
> From: sunjunchao <sunjunchao@yanrongyun.com>
> 
> 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 <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: remove BUG_ON which will be triggered in race scenario

[JunChao Sun]

Jan Kara <jack@suse.cz> 于2023年4月12日周三 18:45写道：
>
> On Wed 12-04-23 00:47:37, JunChao Sun wrote:
> > From: sunjunchao <sunjunchao@yanrongyun.com>
> >
> > 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 <jack@suse.com>
> SUSE Labs, CR