[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