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

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

[Shivaji Kant]

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.

Reviewed-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Shivaji Kant <shivajikant@google.com>
---
v1 -> v2:
  - Add explicit err_erase_dmabuf_bindings err label in net_devmem_bind_dmabuf()
    to align cleanup paths with the rest of the function (feedback from reviewer).

v1: https://lore.kernel.org/all/20260810065759.3735934-1-shivajikant@google.com/

 include/net/netdev_netlink.h |  5 ++---
 net/core/devmem.c            | 14 +++++++++++---
 net/core/devmem.h            |  4 ++--
 net/core/netdev-genl.c       | 25 ++++++-------------------
 4 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/include/net/netdev_netlink.h b/include/net/netdev_netlink.h
index 075962dbe743..45a4a482d42e 100644
--- a/include/net/netdev_netlink.h
+++ b/include/net/netdev_netlink.h
@@ -2,11 +2,10 @@
 #ifndef __NET_NETDEV_NETLINK_H
 #define __NET_NETDEV_NETLINK_H
 
-#include <linux/list.h>
+#include <linux/xarray.h>
 
 struct netdev_nl_sock {
-	struct mutex lock;
-	struct list_head bindings;
+	struct xarray bindings;
 };
 
 #endif	/* __NET_NETDEV_NETLINK_H */
diff --git a/net/core/devmem.c b/net/core/devmem.c
index 957d6b96216b..bdf8a0799a42 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -134,8 +134,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);
 
 	xa_for_each(&binding->bound_rxqs, xa_idx, rxq) {
 		const struct pp_memory_provider_params mp_params = {
@@ -193,6 +193,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
 	struct dma_buf *dmabuf;
 	unsigned int sg_idx, i;
 	unsigned long virtual;
+	void *res;
 	int err;
 
 	if (!dma_dev) {
@@ -325,10 +326,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);
diff --git a/net/core/devmem.h b/net/core/devmem.h
index 3852a56036cb..bf77e5a9a8fe 100644
--- a/net/core/devmem.h
+++ b/net/core/devmem.h
@@ -49,10 +49,10 @@ struct net_devmem_dmabuf_binding {
 	 */
 	struct percpu_ref ref;
 
-	/* The list of bindings currently active. Used for netlink to notify us
+	/* The socket priv this binding belongs to. Used for netlink to notify us
 	 * of the user dropping the bind.
 	 */
-	struct list_head list;
+	struct netdev_nl_sock *sock_priv;
 
 	/* rxq's this binding is active on. */
 	struct xarray bound_rxqs;
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index c15d8d4ca1f8..d3f3a91a6d93 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -1044,13 +1044,11 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_genlmsg_free;
 	}
 
-	mutex_lock(&priv->lock);
-
 	err = 0;
 	netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
 	if (!netdev) {
 		err = -ENODEV;
-		goto err_unlock_sock;
+		goto err_genlmsg_free;
 	}
 	if (!netif_device_present(netdev))
 		err = -ENODEV;
@@ -1102,8 +1100,6 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 
 	netdev_unlock(netdev);
 
-	mutex_unlock(&priv->lock);
-
 	return err < 0 ? err : 0;
 
 err_unbind:
@@ -1112,8 +1108,6 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
 	bitmap_free(rxq_bitmap);
 err_unlock:
 	netdev_unlock(netdev);
-err_unlock_sock:
-	mutex_unlock(&priv->lock);
 err_genlmsg_free:
 	nlmsg_free(rsp);
 	return err;
@@ -1185,12 +1179,10 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_genlmsg_free;
 	}
 
-	mutex_lock(&priv->lock);
-
 	netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
 	if (!netdev) {
 		err = -ENODEV;
-		goto err_unlock_sock;
+		goto err_genlmsg_free;
 	}
 
 	if (!netif_device_present(netdev)) {
@@ -1233,7 +1225,6 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
 	if (bind_dev != netdev)
 		netdev_unlock(bind_dev);
 	netdev_unlock(netdev);
-	mutex_unlock(&priv->lock);
 
 	return genlmsg_reply(rsp, info);
 
@@ -1242,8 +1233,6 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
 		netdev_unlock(bind_dev);
 err_unlock_netdev:
 	netdev_unlock(netdev);
-err_unlock_sock:
-	mutex_unlock(&priv->lock);
 err_genlmsg_free:
 	nlmsg_free(rsp);
 	return err;
@@ -1418,19 +1407,17 @@ int netdev_nl_queue_create_doit(struct sk_buff *skb, struct genl_info *info)
 
 void netdev_nl_sock_priv_init(struct netdev_nl_sock *priv)
 {
-	INIT_LIST_HEAD(&priv->bindings);
-	mutex_init(&priv->lock);
+	xa_init(&priv->bindings);
 }
 
 void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
 {
 	struct net_devmem_dmabuf_binding *binding;
-	struct net_devmem_dmabuf_binding *temp;
 	netdevice_tracker dev_tracker;
 	struct net_device *dev;
+	unsigned long xa_idx;
 
-	mutex_lock(&priv->lock);
-	list_for_each_entry_safe(binding, temp, &priv->bindings, list) {
+	xa_for_each(&priv->bindings, xa_idx, binding) {
 		mutex_lock(&binding->lock);
 		dev = binding->dev;
 		if (!dev) {
@@ -1446,7 +1433,7 @@ void netdev_nl_sock_priv_destroy(struct netdev_nl_sock *priv)
 		netdev_unlock(dev);
 		netdev_put(dev, &dev_tracker);
 	}
-	mutex_unlock(&priv->lock);
+	xa_destroy(&priv->bindings);
 }
 
 static int netdev_genl_netdevice_event(struct notifier_block *nb,
-- 
2.55.0.654.g21b8a5bc05-goog

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

[Nikolay Aleksandrov]

On 11/08/2026 17:29, 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.
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Shivaji Kant <shivajikant@google.com>
> ---
> v1 -> v2:
>    - Add explicit err_erase_dmabuf_bindings err label in net_devmem_bind_dmabuf()
>      to align cleanup paths with the rest of the function (feedback from reviewer).
> 
> v1: https://lore.kernel.org/all/20260810065759.3735934-1-shivajikant@google.com/
> 
>   include/net/netdev_netlink.h |  5 ++---
>   net/core/devmem.c            | 14 +++++++++++---
>   net/core/devmem.h            |  4 ++--
>   net/core/netdev-genl.c       | 25 ++++++-------------------
>   4 files changed, 21 insertions(+), 27 deletions(-)
> 

Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Jakub Kicinski]

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.

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

[Jakub Kicinski]

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@google.com/

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?

> 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?

[ ... ]

> @@ -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.

[ ... ]
-- 
pw-bot: cr

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@kernel.org> 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.

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@kernel.org> 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@google.com/
>
> 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

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

[Jakub Kicinski]

On Fri, 14 Aug 2026 12:04:59 +0530 Shivaji Kant wrote:
> 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.

If it's an optimization you have to show prod data (as in not a micro
benchmark) that the problem actually exists.

And please describe how your application manages the sockets, we're
talking about a lock on a management socket, not some global lock
blocking the whole system.

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

[Mina Almasry]

On Fri, Aug 14, 2026 at 6:50 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 14 Aug 2026 12:04:59 +0530 Shivaji Kant wrote:
> > 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.
>
> If it's an optimization you have to show prod data (as in not a micro
> benchmark) that the problem actually exists.
>

Sorry for the late reply. My reasoning for this change is code
simplification, not optimization.

The history of priv->lock is that it was added to protect concurrent
access to priv->bindings and only locked around list access to
priv->bindings. But then Stan and Taehee ran into deadlocks in
relation with locking order with the netdev_lock and the binding->lock
so we ended up defining intricate (undocumented) locking order between
the 3 locks and expanding the code serialized by priv->lock and the
code now feels confusing. Removing 1 of these 3 locks to simplify the
mental model seemed like a clear win for me. We do not need priv->lock
if we use a data structure that manages its own concurrency like
xarray.

But this is not a huge issue worth burning your review cycles on. If
you don't want us to resolve the clashiko issues and follow up on
this, we will drop it indeed :-). Sorry about that!

-- 
Thanks,
Mina