Re: [syzbot] memory leak in xas_create

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jul 12, 2022 at 01:57:59PM +0100, Matthew Wilcox wrote:
> > So I assumed the nodes stored in the xas object, which is local to the
> > collapse_file() function.
> 
> Yes, that's a reasonable thing to think, but it's actually not how
> it works.  When we allocate a node in xas_create(), we put it straight
> into the tree without storing it in xas->xa_alloc.  We may then end
> up not using it, but the node isn't leaked because it's in the tree.
> 
> If the GFP_NOWAIT allocation fails (it didn't in these stack traces),
> we call xas_nomem(), which sees an -ENOMEM, allocates a node and stores
> it in xas->xa_alloc; then we go round the loop again where xas_create()
> will take the node from xas->xa_alloc.  But the backtraces here don't
> implicate xas_nomem().

There is actually a leak here, but it's not the one that's been found.

        do {
                xas_lock_irq(&xas);
                xas_create_range(&xas);
                if (!xas_error(&xas))
                        break;
                xas_unlock_irq(&xas);
                if (!xas_nomem(&xas, GFP_KERNEL)) {
                        result = SCAN_FAIL;
                        goto out;
                }
        } while (1);

If xas_create() fails, it sets xas error to -ENOMEM.  So we unlock the
xarray lock and call xas_nomem().  xas_nomem() sees the error is -ENOMEM
and allocates a node with GFP_KERNEL, putting the node in xas->xa_alloc.
We re-take the spinlock and call xas_create() again.  If we still need
a node, xas_alloc() will take the node stored in xas->xa_alloc.  If we
raced and don't need a node, xas_create_range() succeeds and we break out,
failing to free the xas->xa_alloc node.

We should call xas_destroy() at the out: label.  However, this does not
explain what is going on, and will not fix what is going on, since any
nodes allocated during xas_create_range() will be stored safely in the
tree and will not be freed by xas_destroy().