Re: [PATCH RESEND] fs: fix race condition oops between destroy_inode and writeback_sb_inodes

[Matthew Wilcox <willy@infradead.org>]

On Sat, Sep 19, 2020 at 05:39:23AM -0400, Shijie Luo wrote:
> There is a race condition between destroy_inode and writeback_sb_inodes,
> thread-1                                    thread-2
> wb_workfn
>   writeback_inodes_wb
>     __writeback_inodes_wb
>       writeback_sb_inodes
>         wbc_attach_and_unlock_inode
> 					iget_locked
>                                           destroy_inode
>                                             inode_detach_wb
>                                               inode->i_wb = NULL;
> 
>         inode_to_wb_and_lock_list
>           locked_inode_to_wb_and_lock_list
>             wb_get
>               oops
> 
> so destroy inode after adding I_FREEING to inode state and the I_SYNC state
>  being cleared.
> 
> Reported-by: Tianxiong Lu <lutianxiong@huawei.com>
> Signed-off-by: Shijie Luo <luoshijie1@huawei.com>
> Signed-off-by: Haotian Li <lihaotian9@huawei.com>
> ---
>  fs/inode.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 72c4c347afb7..b28a2a9e15d5 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1148,10 +1148,17 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
>  		struct inode *new = alloc_inode(sb);
>  
>  		if (new) {
> +			spin_lock(&new->i_lock);
>  			new->i_state = 0;
> +			spin_unlock(&new->i_lock);

This part is unnecessary.  We just allocated 'new' two lines above;
nobody else can see 'new' yet.  We make it visible with hlist_add_head_rcu()
which uses rcu_assign_pointer() whch contains a memory barrier, so it's
impossible for another CPU to see a stale i_state.

>  			inode = inode_insert5(new, hashval, test, set, data);
> -			if (unlikely(inode != new))
> +			if (unlikely(inode != new)) {
> +				spin_lock(&new->i_lock);
> +				new->i_state |= I_FREEING;
> +				spin_unlock(&new->i_lock);
> +				inode_wait_for_writeback(new);
>  				destroy_inode(new);

This doesn't make sense either.  If an inode is returned here which is not
'new', then adding 'new' to the hash failed, and new was never visible
to another CPU.

> @@ -1218,6 +1225,11 @@ struct inode *iget_locked(struct super_block *sb, unsigned long ino)
>  		 * allocated.
>  		 */
>  		spin_unlock(&inode_hash_lock);
> +
> +		spin_lock(&inode->i_lock);
> +		inode->i_state |= I_FREEING;
> +		spin_unlock(&inode->i_lock);
> +		inode_wait_for_writeback(inode);
>  		destroy_inode(inode);

Again, this doesn't make sense.  This is also a codepath which failed to
make 'inode' visible to any other thread.

I don't understand how this patch could fix anything.