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

[Jakub Kicinski <kuba@kernel.org>]

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