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:
>
> On Tue, 11 Aug 2026 14:29:39 +0000 Shivaji Kant wrote:
> > netdev_nl_sock previously used a struct list_head bindings to keep
> > track of active netdev netlink bindings, protected by a per-socket
> > struct mutex lock (priv->lock).
> >
> > 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.
> >
> > Convert bindings in struct netdev_nl_sock from struct list_head to
> > struct xarray and remove priv->lock. This simplifies the code by
> > removing explicit mutex locking in netdev_nl_bind_rx_doit(),
> > netdev_nl_bind_tx_doit(), and socket initialization/teardown functions.
>
> I don't think this is worth touching if you're just trying to simplify
> the code. The mutex is in struct netdev_nl_sock, which is meant for all
> netdev socket state. It took us a bit of massaging to get the locking
> into shape, I think that keeping the mutex around can serve as a more
> generic blueprint for others who need to add state to the struct?
>
> Clashiko also doesn't believe in the correctness, I'll send out the
> review separately. I didn't look closely but the usual workaround
> for store issues, off the top of my head, is to store NULL first,
> then swap it for the real value. The store can only fail if the xarray
> needs to alloc memory, swap does not fail. I could be wrong tho.

I understand, my thinking for this was removing priv->lock is more
than just simplification, it can be a concurrency win. Sorry for not
making it more explicit in the patch description.
As you pointed out, priv->lock current serializes the entire doit
block (including netdev_get_by_index_lock() and the entire
net_devmem_bind_dmabuf_to_queue() loop). Furthermore,
net_devmem_unbind_dmabuf() blocks on synchronize_net(). With the
current mutex implementation, if one thread is destroying a socket or
unbinding a dmabuf, it holds priv->lock across synchronize_net(),
completely blocking any concurrent netlink commands on the same socket
(e.g., binding to a different queue/interface).
Converting to an xarray allows us to cleanly dissolve this wide
serialization scope. xa_for_each() handles the RCU grace periods
internally without holding a manual lock across synchronize_net(). I
agree the changelog must be updated to clearly state that reducing
this serialization scope is the explicit goal.
Also, For the race conditions you found:
Yes, the erase wrap-around race is possible. Reversing the xa_erase
order should fix it.
For the high severity UAF on the error path, calling
net_devmem_unbind_dmabuf(binding) instead of manually jumping down the
synchronous error ladder perfectly resolves the missing
synchronize_net() and percpu_ref_kill().

Let me know if this sounds positive, i will send out a v3 with the fixes.