On 6/24/24 11:03 AM, Darrick J. Wong wrote:
> On Sat, Jun 22, 2024 at 04:26:31PM +0800, Long Li wrote:
>> xfs_attr_shortform_list() only called from a non-transactional context, it
>> hold ilock before alloc memory and maybe trapped in memory reclaim. Since
>> commit 204fae32d5f7("xfs: clean up remaining GFP_NOFS users") removed
>> GFP_NOFS flag, lockdep warning will be report as [1]. Eliminate lockdep
>> false positives by use __GFP_NOLOCKDEP to alloc memory
>> in xfs_attr_shortform_list().
>>
>> [1] https://lore.kernel.org/linux-xfs/000000000000e33add0616358204@xxxxxxxxxx/
>> Reported-by: syzbot+4248e91deb3db78358a2@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: Long Li <leo.lilong@xxxxxxxxxx>
>> ---
>> fs/xfs/xfs_attr_list.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
>> index 5c947e5ce8b8..8cd6088e6190 100644
>> --- a/fs/xfs/xfs_attr_list.c
>> +++ b/fs/xfs/xfs_attr_list.c
>> @@ -114,7 +114,8 @@ xfs_attr_shortform_list(
>> * It didn't all fit, so we have to sort everything on hashval.
>> */
>> sbsize = sf->count * sizeof(*sbuf);
>> - sbp = sbuf = kmalloc(sbsize, GFP_KERNEL | __GFP_NOFAIL);
>> + sbp = sbuf = kmalloc(sbsize,
>> + GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
>
> Why wouldn't we memalloc_nofs_save any time we take an ILOCK when we're
> not in transaction context? Surely you'd want to NOFS /any/ allocation
> when the ILOCK is held, right?
I'm not sure I understand this. AFAICT, this is indeed a false positive, and can
be fixed by applying exactly the same pattern used elsewhere in
94a69db2367e ("xfs: use __GFP_NOLOCKDEP instead of GFP_NOFS")
Using memalloc_nofs_save implies that this really /would/ deadlock without
GFP_NOFS, right? Is that the case?
I was under the impression that this was simply a missed callsite in 94a69db2367e
and as Long Li points out, other allocations under xfs_attr_list_ilocked()
use the exact same (GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL) pattern
proposed in this change.
Thanks,
-Eric
