Re: [PATCH] xfs: eliminate lockdep false positives in xfs_attr_shortform_list

[Long Li <leo.lilong@huawei.com>]

On Mon, Jun 24, 2024 at 09:03:42AM -0700, 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@google.com/
> > Reported-by: syzbot+4248e91deb3db78358a2@syzkaller.appspotmail.com
> > Signed-off-by: Long Li <leo.lilong@huawei.com>
> > ---
> >  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?
> 
> --D
> 
> 

I believe using memalloc_nofs_save could solve the problem, sometimes it may be 
more effective than using the __GFP_NOLOCKDEP flag. However, looking at similar
functions, for example xfs_btree_alloc_cursor, it uses __GFP_NOLOCKDEP to prevent
ABBA deadlock false positive warnings.

xfs_attr_list_ilocked
  xfs_iread_extents
    xfs_bmbt_init_cursor
      xfs_btree_alloc_cursor
        kmem_cache_zalloc(cache, GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL)

After thinking a little more, I found out that just using __GFP_NOLOCKDEP may
not be enough, AA deadlock false positive warnings [1] still exist in the
mainline kernel if my understanding is correct.

[1] https://lore.kernel.org/linux-xfs/20240622094411.GA830005@ceph-admin/T/#m6f7ab8438bf82f0dc44c6d42d183ae08c07dcd5f

thanks,
Long Li