Does have race between __mark_inode_dirty() and evict()

Does have race between __mark_inode_dirty() and evict()

[Xiaoguang Wang]

hi,

Recently we run into a NULL pointer dereference panic in our internal 4.9 kernel
it panics because inode->i_wb has become zero in wbc_attach_and_unlock_inode(),
and by crash tools analysis, inode's dirtied_when is zero, but dirtied_time_when
is not zero, seems that this inode has been used after free. Looking into both
4.9 and upstream codes, seems that there maybe a race:

__mark_inode_dirty(...)
{
     spin_lock(&inode->i_lock);
     ...
     if (inode->i_state & I_FREEING)
         goto out_unlock_inode;
     ...
     if (!was_dirty) {
         struct bdi_writeback *wb;
         struct list_head *dirty_list;
         bool wakeup_bdi = false;

         wb = locked_inode_to_wb_and_lock_list(inode);
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        this function will unlock inode->i_ilock firstly and then relock, but once the
inode->i_ilock is unlocked, evict() may run in, set I_FREEING flag, and free the inode,
and later locked_inode_to_wb_and_lock_list relocks inode->i_ilock again, but will not
check the I_FREEING flag again, so the use after free for this inode would happen.

I'm not familiar with vfs or cgroup writeback codes much, could you please confirm whether
this is an issue? Thanks.

Regards,
Xiaoguang Wang

Re: Does have race between __mark_inode_dirty() and evict()

[Jan Kara]

Hi!

On Mon 20-04-20 14:23:19, Xiaoguang Wang wrote:
> Recently we run into a NULL pointer dereference panic in our internal 4.9
> kernel it panics because inode->i_wb has become zero in
> wbc_attach_and_unlock_inode(), and by crash tools analysis, inode's
> dirtied_when is zero, but dirtied_time_when is not zero, seems that this
> inode has been used after free. Looking into both 4.9 and upstream codes,
> seems that there maybe a race:
> 
> __mark_inode_dirty(...)
> {
>     spin_lock(&inode->i_lock);
>     ...
>     if (inode->i_state & I_FREEING)
>         goto out_unlock_inode;
>     ...
>     if (!was_dirty) {
>         struct bdi_writeback *wb;
>         struct list_head *dirty_list;
>         bool wakeup_bdi = false;
> 
>         wb = locked_inode_to_wb_and_lock_list(inode);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>        this function will unlock inode->i_ilock firstly and then relock,
>        but once the inode->i_ilock is unlocked, evict() may run in, set
>        I_FREEING flag, and free the inode, and later
>        locked_inode_to_wb_and_lock_list relocks inode->i_ilock again, but
>        will not check the I_FREEING flag again, so the use after free for
>        this inode would happen.

If someone is calling __mark_inode_dirty(), he should be holding inode
reference (either directly or indirectly through having dentry reference,
file open, ...) which prevents the scenario you suggest.

But I've just recently tracked down a very similarly looking issue reported
to me. I've submitted patches to fix it here [1].

[1] https://lore.kernel.org/linux-ext4/20200421085445.5731-1-jack@suse.cz

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR