Re: [PATCH v4] lib: xarray: free unused spare node in xas_create_range()

[Shardul Bankar]

On Fri, 2025-12-05 at 08:22 +0100, David Hildenbrand (Red Hat) wrote:
> > Link:
> > https://syzkaller.appspot.com/bug?id=a274d65fc733448ed518ad15481ed575669dd98c
> ...
> Reviewed-by: David Hildenbrand (Red Hat) <david@xxxxxxxxxx>
> 
> 
> BTW, do we have a way to test this in a test case?
Hi David,

Thanks for the review and the Reviewed-by.

Regarding a test case: I don’t have a focused selftest or fault-
injection setup yet that reliably hits this xas_nomem() +
xas_create_range() corner case. 
I noticed this spare-node leak while analyzing the Syzbot report I
referenced in the Link: tag, but the reproducer I see there doesn’t
isolate this path and reports other kmemleaks.

For now I’d prefer to treat this as a small correctness fix in xarray
itself. If I manage to come up with a robust way to exercise this path
in a selftest (e.g. via targeted fault injection in lib/test_xarray.c),
I can follow up with a separate patch, but I don’t have anything solid
to propose today.

> 
> 
> A follow-up cleanup that avoids labels could be something like
> (untested):
> 
> 
> diff --git a/lib/xarray.c b/lib/xarray.c
> index 9a8b4916540cf..325f264530fb2 100644
> --- a/lib/xarray.c
> +++ b/lib/xarray.c
> @@ -714,6 +714,7 @@ void xas_create_range(struct xa_state *xas)
>          unsigned long index = xas->xa_index;
>          unsigned char shift = xas->xa_shift;
>          unsigned char sibs = xas->xa_sibs;
> +       bool success = false;
>   
>          xas->xa_index |= ((sibs + 1UL) << shift) - 1;
>          if (xas_is_node(xas) && xas->xa_node->shift == xas-
> >xa_shift)
> @@ -724,9 +725,11 @@ void xas_create_range(struct xa_state *xas)
>          for (;;) {
>                  xas_create(xas, true);
>                  if (xas_error(xas))
> -                       goto restore;
> -               if (xas->xa_index <= (index | XA_CHUNK_MASK))
> -                       goto success;
> +                       break
> +               if (xas->xa_index <= (index | XA_CHUNK_MASK)) {
> +                       succeess = true;
> +                       break;
> +               }
>                  xas->xa_index -= XA_CHUNK_SIZE;
>   
>                  for (;;) {
> @@ -740,15 +743,17 @@ void xas_create_range(struct xa_state *xas)
>                  }
>          }
>   
> -restore:
> -       xas->xa_shift = shift;
> -       xas->xa_sibs = sibs;
> -       xas->xa_index = index;
> -       return;
> -success:
> -       xas->xa_index = index;
> -       if (xas->xa_node)
> -               xas_set_offset(xas);
> +       if (success) {
> +               xas->xa_index = index;
> +               if (xas->xa_node)
> +                       xas_set_offset(xas);
> +       } else {
> +               xas->xa_shift = shift;
> +               xas->xa_sibs = sibs;
> +               xas->xa_index = index;
> +       }
> +       /* Free any unused spare node from xas_nomem() */
> +       xas_destroy(xas);
>   }
>   EXPORT_SYMBOL_GPL(xas_create_range);
> 
> 
Your bool-based version reads nicer; I’m happy to follow up with a
small cleanup patch on top that switches xas_create_range() over to
that style (with a Suggested-by tag).

Thanks and Regards,
Shardul