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

[netdev-bot+sashiko@kernel.org]

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] The TX-binding dangling `binding->dev` use-after-free can still
  happen through a race in the leased-queue (netkit) bind-tx path.

--

net: devmem: detach TX bindings on NETDEV_UNREGISTER

This extends net_devmem_dev_unregister() to cover TX devmem bindings.
When binding->dev unregisters, the patch clears the binding's LIVE mark
and both dev and vdev, then unmaps the dma_buf after synchronize_net().
When only binding->vdev unregisters, it clears just binding->vdev.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 93cc2815d935a..dd07203076808 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -192,9 +192,55 @@ net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
>  	mutex_unlock(&binding->lock);
>  }
>  
> +static bool
> +net_devmem_dev_unregister_tx(struct net_devmem_dmabuf_binding *binding,
> +			     unsigned long xa_idx, struct net_device *dev)
> +{
> +	if (READ_ONCE(binding->dev) == dev) {

[ ... ]

> +		return true;
> +	}
> +
> +	if (READ_ONCE(binding->vdev) == dev) {
> +		mutex_lock(&binding->lock);
> +		WRITE_ONCE(binding->vdev, NULL);
> +		mutex_unlock(&binding->lock);
> +	}

[Severity: High]
Can a TX binding still end up with a dangling binding->dev through the
leased queue (netkit) bind-tx path?

In netdev_nl_bind_tx_doit(), when the target is a NETMEM_TX_NO_DMA
leasee, netdev_find_netmem_tx_dev() finds the physical device. It reads
the lease without taking the physical device's lock. It also only checks
netif_device_present(), and the unregister path never clears that:

net/core/netdev-genl.c:netdev_find_netmem_tx_dev() {
    ...
		phys_dev = lease_rxq->dev;
		if (netif_device_present(phys_dev) &&
		    phys_dev->netmem_tx == NETMEM_TX_DMA)
			return phys_dev;
    ...
}

The bind then goes ahead without rechecking bind_dev->reg_state:

net/core/netdev-genl.c:netdev_nl_bind_tx_doit() {
    ...
	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,
    ...
}

On the direct path, netdev_get_by_index_lock()->netdev_put_lock() rejects
reg_state > NETREG_REGISTERED. Nothing does the same check for bind_dev
here.

The sequence seems to be:

1. The physical device P unregisters.
   unregister_netdevice_many_notify() sets P->reg_state to
   NETREG_UNREGISTERING and calls the NETDEV_UNREGISTER notifiers.

2. The netdev-genl notifier runs before netkit's, because it was
   registered earlier with equal priority. It calls
   net_devmem_dev_unregister(P) under netdev_lock(P).

3. A concurrent unprivileged bind-tx on netkit already holds netkit's
   lock and is blocked in netdev_lock(P). It now continues. The lease is
   still in place, so it creates a binding with dev = P and
   vdev = netkit, maps the dmabuf, and marks the binding LIVE.

4. netkit_check_lease_unregister(P) unregisters netkit. In
   net_devmem_dev_unregister(netkit)->net_devmem_dev_unregister_tx(),
   binding->dev is P, so only the branch above runs and only
   binding->vdev is cleared.

5. netdev_run_todo() sets P->reg_state to NETREG_UNREGISTERED. Any
   rebroadcast NETDEV_UNREGISTER then returns early at:

	if (dev->reg_state != NETREG_UNREGISTERING)
		return;

   net_devmem_bind_dmabuf() sets binding->dev = dev without taking a
   netdev_hold(), so P can be freed.

6. When the socket closes, netdev_nl_sock_priv_destroy() reads the stale
   binding->dev and calls netdev_hold() and netdev_lock() on it. This is
   the same oops quoted in the commit message. The dma_buf also stays
   attached to the removed device until the final free.

NETDEV_CMD_BIND_TX is unprivileged. Hitting this needs a race with the
physical device's unregistration. That is still the path the commit
message and the 1abe839b34ae Fixes: tag say this patch fixes. Later
patches in the series don't appear to touch net/ or drivers/net/netkit.c
to address it.

Would it make sense to recheck bind_dev->reg_state after
netdev_lock(bind_dev) in netdev_nl_bind_tx_doit()? Extending the
unregister walk doesn't look like it can close this window on its own.

[ ... ]

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