Re: [PATCH net-next v2] net: convert netdev_nl_sock bindings list to xarray

[Shivaji Kant]

On Thu, Aug 13, 2026 at 5:58 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: convert netdev_nl_sock bindings list to xarray
>
> This converts netdev_nl_sock.bindings from a struct list_head to a struct
> xarray keyed by binding->id, replaces binding->list with a back pointer
> binding->sock_priv, and removes the per-socket priv->lock mutex from the
> bind-rx, bind-tx and sock priv init/destroy paths.
>
> Link (v1): https://lore.kernel.org/all/20260810065759.3735934-1-shivajikant@xxxxxxxxxx/
>
> A few questions below.
>
> > Since list modifications and iterations are not concurrency-safe,
> > priv->lock was introduced to serialize operations on priv->bindings.
> > However, xarray manages its own internal locking (xa_lock) for store,
> > erase, and iteration operations.
>
> [Severity: Low]
> Is the "iteration" part of this accurate?  xa_for_each() in the rewritten
> netdev_nl_sock_priv_destroy() expands to xa_find()/xa_find_after(), which
> take and drop only rcu_read_lock() internally and hold no lock at all
> across the caller's loop body:
>
> lib/xarray.c:xa_find() {
>         ...
>         rcu_read_lock();
>         do { ... } while (xas_retry(&xas, entry));
>         rcu_read_unlock();
>         ...
> }
>
> The loop body in netdev_nl_sock_priv_destroy() sleeps:
>
>         xa_for_each(&priv->bindings, xa_idx, binding) {
>                 mutex_lock(&binding->lock);
>                 ...
>                 netdev_lock(dev);
>                 net_devmem_unbind_dmabuf(binding);   /* synchronize_net() */
>
> so xa_lock does not cover any of that.
>
> Also, in the baseline priv->lock was taken before
> netdev_get_by_index_lock() and dropped only after the whole bind sequence
> (net_devmem_bind_dmabuf() plus the entire net_devmem_bind_dmabuf_to_queue()
> loop), i.e. it serialized the complete bind operation rather than just the
> list insertion.
>
> Could the changelog say that the serialization scope of the whole doit is
> being reduced, instead of describing this as a mechanical list-to-xarray
> conversion?
>

The observation that the loop body sleeps without holding the xarray
lock is exactly why this conversion provides a functional benefit. I
can reword the changelog in v3 to make it clear that the intentional
goal is to reduce the serialization scope of the entire doit sequence.

> > diff --git a/net/core/devmem.c b/net/core/devmem.c
> > index f4d60654ce7f..c620d986c418 100644
> > --- a/net/core/devmem.c
> > +++ b/net/core/devmem.c
> > @@ -136,8 +136,8 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
> >        */
> >       synchronize_net();
> >
> > -     if (binding->list.next)
> > -             list_del(&binding->list);
> > +     if (binding->sock_priv)
> > +             xa_erase(&binding->sock_priv->bindings, binding->id);
>
> [Severity: Low]
> Can this erase remove a different binding's entry?  priv->bindings is now
> keyed by the global ID allocated in net_devmem_bind_dmabuf():
>
>         err = xa_alloc_cyclic(&net_devmem_dmabuf_bindings, &binding->id,
>                               binding, xa_limit_32b, &id_alloc_next,
>                               GFP_KERNEL);
>
> and net_devmem_unbind_dmabuf() releases that global ID first:
>
>         xa_erase(&net_devmem_dmabuf_bindings, binding->id);
>         synchronize_net();
>         if (binding->sock_priv)
>                 xa_erase(&binding->sock_priv->bindings, binding->id);
>
> Between the global xa_erase() and the per-socket xa_erase() the ID is a
> free slot in the global allocator, and synchronize_net() sleeps in that
> window.  If a concurrent bind on the same socket is handed the recycled ID,
> its xa_store(&priv->bindings, binding->id, binding, GFP_KERNEL) overwrites
> the dying binding's entry (the returned old entry is only tested with
> xa_is_err()), and the dying binding's xa_erase() then drops the new
> binding from priv->bindings.  The new binding would no longer be visible to
> netdev_nl_sock_priv_destroy(), leaving the dma-buf attachment and the rx
> queue memory provider installed, and binding->sock_priv dangling once the
> netlink sock priv is freed.
>
> With the embedded list_head, list_del(&binding->list) operated on the
> binding's own node, so unlinking a different binding was not possible.
>
> Reaching this requires the cyclic allocator to wrap the 32-bit ID space
> (__xa_alloc_cyclic() searches [*next, UINT_MAX] before falling back to
> limit.min), so it is hard to hit in practice.  Would erasing from
> priv->bindings before releasing the global ID, and clearing
> binding->sock_priv, make the ordering safe regardless?
>
> [ ... ]

Ack. While wrapping the 32 bit ID space exactly during the
synchronize_net() window is highly unlikely, it is possible and the
dangling pointer would be nasty. Reversing the erase ordering (erasing
from priv->bindings first) will guarantee this specific race cannot
happen. I will include this ordering fix in v3.

>
> > @@ -332,10 +333,17 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
> >       if (err < 0)
> >               goto err_free_chunks;
> >
> > -     list_add(&binding->list, &priv->bindings);
> > +     binding->sock_priv = priv;
> > +     res = xa_store(&priv->bindings, binding->id, binding, GFP_KERNEL);
> > +     if (xa_is_err(res)) {
> > +             err = xa_err(res);
> > +             goto err_erase_dmabuf_bindings;
> > +     }
> >
> >       return binding;
> >
> > +err_erase_dmabuf_bindings:
> > +     xa_erase(&net_devmem_dmabuf_bindings, binding->id);
> >  err_free_chunks:
> >       gen_pool_for_each_chunk(binding->chunk_pool,
> >                               net_devmem_dmabuf_free_chunk_owner, NULL);
>
> [Severity: High]
> Can this new error path free a binding that is still reachable by a
> concurrent lookup?
>
> At the point xa_store() runs, the preceding xa_alloc_cyclic() has already
> published the binding in the global net_devmem_dmabuf_bindings xarray, so
> net_devmem_lookup_dmabuf() can find it:
>
> net/core/devmem.c:net_devmem_lookup_dmabuf() {
>         rcu_read_lock();
>         binding = xa_load(&net_devmem_dmabuf_bindings, id);
>         if (binding) {
>                 if (!net_devmem_dmabuf_binding_get(binding))
>                         binding = NULL;
>         }
>         rcu_read_unlock();
>         ...
> }
>
> This is reached from tcp_sendmsg_locked() via net_devmem_get_binding() with
> a user supplied dmabuf id.
>
> err_erase_dmabuf_bindings only does a bare xa_erase() and then falls into
> the pre-publish teardown ladder: gen_pool_destroy(), kvfree(tx_vec),
> dma_buf_unmap_attachment_unlocked(), dma_buf_detach(),
> percpu_ref_exit(&binding->ref), kfree(binding).  There is no
> synchronize_net() and no percpu_ref_kill(), which is exactly what
> net_devmem_unbind_dmabuf() documents as required for a published binding:
>
> net/core/devmem.c:net_devmem_unbind_dmabuf() {
>         xa_erase(&net_devmem_dmabuf_bindings, binding->id);
>
>         /* Ensure no tx net_devmem_lookup_dmabuf() are in flight after the
>          * erase.
>          */
>         synchronize_net();
>         ...
>         percpu_ref_kill(&binding->ref);
> }
>
> Since the ref is never killed, a racing lookup's percpu_ref_tryget()
> succeeds, and since the free is immediate the racing thread can touch
> &binding->ref after kfree(), later call net_devmem_dmabuf_binding_put() on
> an already percpu_ref_exit()ed and freed ref, and for a tx binding walk a
> freed tx_vec whose DMA mapping is already torn down.
>
> The failure point is new: before this patch list_add() could not fail, so
> there was no error exit between xa_alloc_cyclic() and the successful
> return.  Should this path call net_devmem_unbind_dmabuf(binding) instead,
> or store into priv->bindings before publishing the binding globally?
>
> Entering the path needs the GFP_KERNEL xarray node allocation to fail, so
> it takes memory pressure in addition to the race.
>
> [ ... ]

Missing the percpu_ref_kill() and synchronize_net() on the fast error
route indeed exposes a Use-After-Free if xa_store() fails and
net_devmem_lookup_dmabuf races it.
As suggested, explicitly calling net_devmem_unbind_dmabuf(binding) on
this error path should be the cleanest fix. It will pull the binding
out of the global registry and delegate the heavy teardown (and RCU
wait) to the async worker while correctly handling the reference
counts. I will route the xa_store() failure to
net_devmem_unbind_dmabuf(binding) in v3 to ensure it is handled
safely.

> --
> pw-bot: cr