2022-06-29 02:17:51

by Zhang Yi

Subject: [PATCH v2 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 ASSERT to check and
detect this issue in advance.

Suggested-by: Jan Kara <[email protected]>
Signed-off-by: Zhang Yi <[email protected]>
---
fs/ext4/inode.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 702cc208689a..2ba74412aa89 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.
+ */
+ ASSERT(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 10:29:40

by Jan Kara

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

On Wed 29-06-22 10:29:40, 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 ASSERT to check and
> detect this issue in advance.
>
> Suggested-by: Jan Kara <[email protected]>
> Signed-off-by: Zhang Yi <[email protected]>
> ---
> fs/ext4/inode.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 702cc208689a..2ba74412aa89 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.
> + */
> + ASSERT(list_empty_careful(&inode->i_io_list));

ASSERT() is harsh as it will take the kernel down. Just use WARN_ON_ONCE()
to notify us of the coming problems :).

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR