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

[Shijie Luo <luoshijie1@huawei.com>]

On 2020/9/19 22:56, Matthew Wilcox wrote:
> 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.
> .

Thanks for your review，the underlying filesystem is ext4, 
ext4_alloc_inode doesn't

allocate a new vfs inode from slab, and I found the "new inode" was used 
by another

thread in vmcore, in other words, the new inode should be a new one , 
but not.

Maybe it's not a filesystem problem, and fixing this problem in 
iget_locked is not

a good way, I 'll try to find the root cause and fix it.