Re: [PATCH net-next v2 5/6] net: devmem: resolve physical map_dev for leased RX bindings

[Stanislav Fomichev]

On 11/08, Mina Almasry wrote:
> Previously, netdev_nl_bind_rx_doit() passed netdev as both map_dev and
> bind_dev to net_devmem_bind_dmabuf(), so leased RX bindings stored the
> virtual netdev in binding->map_dev without tracking the physical backing
> netdev at all. Unregistering the physical backing netdev while a leased
> RX binding was active failed to detach and unmap the dmabuf.
> 
> Return the physical backing net_device from netdev_nl_get_dma_dev() when
> the RX queue is leased from another netdev, lock map_dev around
> net_devmem_bind_dmabuf() when map_dev != netdev (matching
> netdev_nl_bind_tx_doit()), and assert in net_devmem_bind_dmabuf() that
> both bind_dev and map_dev are locked.
> 
> Because a leased map_dev is reached via rxq->lease->dev and locked with
> raw netdev_lock(map_dev) rather than netdev_get_by_index_lock(), it may
> already have entered NETREG_UNREGISTERING and completed
> net_devmem_dev_unregister(map_dev) before cascaded unregistration of
> bind_dev tears down the lease. Reject binding in
> net_devmem_bind_dmabuf() if map_dev->reg_state != NETREG_REGISTERED.
> 
> Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> ---
>  net/core/devmem.c      | 12 ++++++++++++
>  net/core/netdev-genl.c | 19 ++++++++++++++++---
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index bb785a871d1c4..2dac581a57e13 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -321,6 +321,18 @@ net_devmem_bind_dmabuf(struct net_device *map_dev,
>  	size_t i;
>  	long err;
>  
> +	netdev_assert_locked(bind_dev);
> +	netdev_assert_locked(map_dev);
> +
> +	/* When map_dev != bind_dev, map_dev is locked via the queue lease
> +	 * rather than netdev_get_by_index_lock(), so it may already be
> +	 * unregistering before cascaded bind_dev unregistration runs.
> +	 */
> +	if (map_dev->reg_state != NETREG_REGISTERED) {
> +		NL_SET_ERR_MSG(extack, "Device is being unregistered");
> +		return ERR_PTR(-ENODEV);
> +	}

And you can call this new lease-valid here as well? Otherwise, the changes
are too LLM-y, testing some super low-level conditions and are a bit hard
to read/follow?