Re: [BUG report] security_inode_alloc return -ENOMEM let xfs shutdown

[Jackie Liu <liu.yun@linux.dev>]

在 2022/5/24 上午9:28, Dave Chinner 写道:
> On Tue, May 24, 2022 at 08:52:30AM +0800, Jackie Liu wrote:
>> 在 2022/5/24 上午7:20, Dave Chinner 写道:
>>> On Mon, May 23, 2022 at 04:51:50PM +0800, Jackie Liu wrote:
>>> Yup, that's a shutdown with a dirty transaction because memory
>>> allocation failed in the middle of a transaction. XFS can not
>>> tolerate memory allocation failure within the scope of a dirty
>>> transactions and, in practice, this almost never happens. Indeed,
>>> I've never seen this allocation from security_inode_alloc():
>>>
>>> int lsm_inode_alloc(struct inode *inode)
>>> {
>>>           if (!lsm_inode_cache) {
>>>                   inode->i_security = NULL;
>>>                   return 0;
>>>           }
>>>
>>>>>>>>     inode->i_security = kmem_cache_zalloc(lsm_inode_cache, GFP_NOFS);
>>>           if (inode->i_security == NULL)
>>>                   return -ENOMEM;
>>>           return 0;
>>> }
>>>
>>> fail in all my OOM testing. Hence, to me, this is a theoretical
>>> failure as I've never, ever seen this allocation fail in production
>>> or test systems, even when driving them hard into OOM with excessive
>>> inode allocation and triggering the OOM killer repeatedly until the
>>> system kills init....
>>>
>>> Hence I don't think there's anything we need to change here right
>>> now. If users start hitting this, then we're going to have add new
>>> memalloc_nofail_save/restore() functionality to XFS transaction
>>> contexts. But until then, I don't think we need to worry about
>>> syzkaller intentionally hitting this shutdown.
>>
>> Thanks Dave.
>>
>>    In the actual test, the x86 or arm64 device test will trigger this error
>> more easily when FAILSLAB is turned on. After our internal discussion, we
>> can try again through such a patch. Anyway, thank you for your reply.
> 
> What kernel is the patch against? It doesn't match a current TOT
> kernel...

It's linux-4.19.y with LSM security patch, but as long as the LSM 
framework is added, this problem can be repeated.

> 
>>
>> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
>> index ceee27b70384..360304409c0c 100644
>> --- a/fs/xfs/xfs_icache.c
>> +++ b/fs/xfs/xfs_icache.c
>> @@ -435,6 +435,7 @@ xfs_iget_cache_hit(
>>                                  wake_up_bit(&ip->i_flags, __XFS_INEW_BIT);
>>                          ASSERT(ip->i_flags & XFS_IRECLAIMABLE);
>>                          trace_xfs_iget_reclaim_fail(ip);
>> +                       error = -EAGAIN;
>>                          goto out_error;
>>                  }
> 
> Ok, I can see what you are suggesting here - it might work if we get
> it right. :)
> 
> We don't actually want (or need) an unconditional retry. This will
> turn persistent memory allocation failure into a CPU burning
> livelock rather than -ENOMEM being returned. It might work for a
> one-off memory failure, but it's not viable for long term failure as
> tends to happen when the system goes deep into OOM territory.

In my opinion, if it causes the filesystem to be shutdown, it's better 
to let it try again and again.

> 
> It also ignores the fact that we can return ENOMEM without
> consequences from this path if we are not in a transaction - any
> pathwalk lookup can have ENOMEM safely returned to it, and that will
> propagate the error to userspace. Same with bulkstat lookups, etc.
> So we still want them to fail with ENOMEM, not retry indefinitely.
> 
> Likely what we want to do is add conditions to the xfs_iget() lookup
> tail to detect ENOMEM when tp != NULL. IN that case, we can then run
> memalloc_retry_wait(GFP_NOFS) before retrying the lookup. That's in
> line with what we do in other places that cannot tolerate allocation
> failure (e.g. kmem_alloc(), xfs_buf_alloc_pages()) so it may make
> sense to do the same thing here....

Do you have any patch suggestions? I have a test environment here to verify.

--
BR, Jackie Liu

> 
> Cheers,
> 
> Dave.
>