2022-06-29 11:18:27

by Zhang Yi

Subject: [PATCH v3 2/2] ext4: check and assert if marking an no_delete evicting inode dirty

In ext4_evict_inode(), if we evicting an inode in the 'no_delete' path,
it cannot be raced by another mark_inode_dirty(). If it happens,
someone else may accidentally dirty it without holding inode refcount
and probably cause use-after-free issues in the writeback procedure.
It's indiscoverable and hard to debug, so add an WARN_ON_ONCE() to
check and detect this issue in advance.

Suggested-by: Jan Kara <[email protected]>
Signed-off-by: Zhang Yi <[email protected]>
---
v2->v3:
- Switch to use WARN_ON_ONCE instead of ASSERT.

fs/ext4/inode.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 702cc208689a..902393373152 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -333,6 +333,12 @@ void ext4_evict_inode(struct inode *inode)
ext4_xattr_inode_array_free(ea_inode_array);
return;
no_delete:
+ /*
+ * Check out some where else accidentally dirty the evicting inode,
+ * which may probably cause inode use-after-free issues later.
+ */
+ WARN_ON_ONCE(!list_empty_careful(&inode->i_io_list));
+
if (!list_empty(&EXT4_I(inode)->i_fc_list))
ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_NOMEM, NULL);
ext4_clear_inode(inode); /* We must guarantee clearing of inode... */
--
2.31.1

2022-06-29 12:54:07

by Jan Kara

Subject: Re: [PATCH v3 2/2] ext4: check and assert if marking an no_delete evicting inode dirty

On Wed 29-06-22 19:26:47, Zhang Yi wrote:
> In ext4_evict_inode(), if we evicting an inode in the 'no_delete' path,
> it cannot be raced by another mark_inode_dirty(). If it happens,
> someone else may accidentally dirty it without holding inode refcount
> and probably cause use-after-free issues in the writeback procedure.
> It's indiscoverable and hard to debug, so add an WARN_ON_ONCE() to
> check and detect this issue in advance.
>
> Suggested-by: Jan Kara <[email protected]>
> Signed-off-by: Zhang Yi <[email protected]>
> ---
> v2->v3:
> - Switch to use WARN_ON_ONCE instead of ASSERT.

Thanks! Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

Honza

>
> fs/ext4/inode.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 702cc208689a..902393373152 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -333,6 +333,12 @@ void ext4_evict_inode(struct inode *inode)
> ext4_xattr_inode_array_free(ea_inode_array);
> return;
> no_delete:
> + /*
> + * Check out some where else accidentally dirty the evicting inode,
> + * which may probably cause inode use-after-free issues later.
> + */
> + WARN_ON_ONCE(!list_empty_careful(&inode->i_io_list));
> +
> if (!list_empty(&EXT4_I(inode)->i_fc_list))
> ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_NOMEM, NULL);
> ext4_clear_inode(inode); /* We must guarantee clearing of inode... */
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR