Re: [PATCH net v5 2/4] net: devmem: detach TX bindings on NETDEV_UNREGISTER

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch's TX detach is incomplete.

--

net: devmem: detach TX bindings on NETDEV_UNREGISTER

This commit extends net_devmem_dev_unregister() to TX devmem bindings.
When binding->dev unregisters, the walk clears the LIVE mark, unmaps the
dma_buf, and clears both binding->dev and binding->vdev. When only
binding->vdev unregisters, it clears just binding->vdev. The commit also
makes __net_devmem_dmabuf_binding_free() call net_devmem_lock_dev() for
TX bindings.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 4610de805ef08..5c7d9640e9d14 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -157,8 +157,7 @@ void __net_devmem_dmabuf_binding_free(struct work_struct *wq)
>  		     binding->area.num_niovs, binding->free_count);
>  
>  	kvfree(binding->area.niovs);
> -	if (binding->direction == DMA_FROM_DEVICE)
> -		dev = net_devmem_lock_dev(binding, &dev_tracker);
> +	dev = net_devmem_lock_dev(binding, &dev_tracker);
>  	net_devmem_dmabuf_binding_unmap(binding);
>  	xa_erase(&net_devmem_dmabuf_bindings, binding->id);
>  	net_devmem_unlock_dev(dev, &dev_tracker);
> @@ -252,11 +251,20 @@ void net_devmem_dev_unregister(struct net_device *dev)
>  
>  	xa_lock(&net_devmem_dmabuf_bindings);
>  	xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) {
> -		if (binding->direction != DMA_FROM_DEVICE ||
> -		    READ_ONCE(binding->dev) != dev ||
> -		    xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> -				NET_DEVMEM_BINDING_LIVE))
> -			continue;
> +		if (binding->direction == DMA_FROM_DEVICE) {
> +			if (READ_ONCE(binding->dev) != dev ||
> +			    xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +					NET_DEVMEM_BINDING_LIVE))
> +				continue;
> +		} else {
> +			if (READ_ONCE(binding->vdev) == dev)
> +				WRITE_ONCE(binding->vdev, NULL);
> +			if (READ_ONCE(binding->dev) != dev)
> +				continue;
> +			WRITE_ONCE(binding->vdev, NULL);
> +			__xa_clear_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +					NET_DEVMEM_BINDING_LIVE);
> +		}

[Severity: High]

Can a leased bind-tx still create a TX binding on the physical device
after this walk has already run for that device?

net_devmem_dev_unregister() runs once per device. It is called from
netdev_genl_netdevice_event() under netdev_lock(dev), and does nothing
unless reg_state is NETREG_UNREGISTERING.

Take a virtual device V (for example netkit) with a queue leased from a
physical device X. When bind-tx is issued on V, netdev_nl_bind_tx_doit()
locks X and binds to it without checking X->reg_state again:

net/core/netdev-genl.c:netdev_nl_bind_tx_doit() {
    ...
	bind_dev = netdev_find_netmem_tx_dev(netdev);
    ...
	if (bind_dev != netdev)
		netdev_lock(bind_dev);

	dma_dev = netdev_queue_get_dma_dev(bind_dev, 0, NETDEV_QUEUE_TYPE_TX);

	binding = net_devmem_bind_dmabuf(bind_dev,
					 bind_dev != netdev ? netdev : NULL,
    ...
}

netdev_find_netmem_tx_dev() only checks netif_device_present(X), and
unregister_netdevice_many_notify() does not clear that bit.

netdev_genl_nb is registered at subsys_initcall and the netkit notifier
at module_init, with the same priority. On NETDEV_UNREGISTER of X, the
netdev-genl walk for X therefore finishes and unlocks X before
netkit_check_lease_unregister() queues V for unregistration. Until then
V stays NETREG_REGISTERED and its lease to X stays in place.

That seems to allow this ordering:

CPU1 (unregister of X)              CPU2 (unprivileged bind-tx on V)
netdev_genl_netdevice_event(X)
  netdev_lock(X)
  net_devmem_dev_unregister(X)
  netdev_unlock(X)
                                    netdev_nl_bind_tx_doit()
                                      netdev_get_by_index_lock(V)
                                      bind_dev = X (via the lease)
                                      netdev_lock(X)
                                      net_devmem_bind_dmabuf(X, V, ...)
                                        LIVE binding, binding->dev = X
                                      netdev_unlock(X)
netkit_check_lease_unregister(X)
  unregister of V
    net_devmem_dev_unregister(V)
      binding->dev != V, so only
      binding->vdev is cleared
    netkit_queue_unlease()
      netdev_put() on X
netdev_run_todo()
  X is freed

The walk for X never runs again, and the binding holds no reference on X,
so binding->dev is left pointing at the freed X.

When the netlink socket is closed later, wouldn't
netdev_nl_sock_priv_destroy()->net_devmem_lock_dev() call netdev_hold()
and netdev_lock() on freed memory? That is the oops quoted in the commit
message.

With the __net_devmem_dmabuf_binding_free() change above, the deferred
free work also calls net_devmem_lock_dev() for TX bindings, so it is a
second path that dereferences the stale binding->dev. The dma_buf also
stays attached and mapped to X's DMA device.

The non-leased path looks safe, because netdev_put_lock() checks
reg_state again under the instance lock. The later patches in this series
don't seem to touch this path.

Would it close the window to reject the bind with -ENODEV when
bind_dev->reg_state != NETREG_REGISTERED after netdev_lock(bind_dev)?
unregister_netdevice_many_notify() writes reg_state under that lock, and
the devmem walk runs later under the same lock.

>  
>  		xa_unlock(&net_devmem_dmabuf_bindings);
>  		if (!netif_is_queue_leasee(dev))

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030083539.777877-1-almasrymina%40google.com