Re: [PATCH net-next v2 3/6] net: guard memory provider RX queue open and close on unregister

[Stanislav Fomichev]

On 11/08, Mina Almasry wrote:
> When a memory provider RX queue is leased to a virtual netdev,
> netif_mp_open_rxq() and netif_mp_close_rxq() redirect through
> rxq->lease and lock the physical backing netdev directly rather than via
> netdev_get_by_index_lock(). This raises two issues during netdev
> unregistration:
> 
> 1. In __netif_mp_open_rxq(), the physical backing netdev may already be
>    in NETREG_UNREGISTERING (after dev_shutdown() and
>    dev_memory_provider_uninstall() have run on it, before cascaded
>    unregistration of the virtual leasee tears down the lease). Reject
>    opening an RX queue if dev->reg_state != NETREG_REGISTERED.
> 2. In __netif_mp_close_rxq(), when the physical backing netdev
>    unregisters before the virtual leasee, dev_memory_provider_uninstall()
>    on the physical netdev does not uninstall the virtual queues, so
>    phys_rxq->mp_params remains populated until the virtual leasee's
>    netif_rxq_cleanup_unlease() or socket teardown runs. At that point
>    the physical netdev has already gone through dev_shutdown() and must
>    not call netdev_rx_queue_reconfig(). When dev->reg_state >
>    NETREG_REGISTERED, clear rxq->mp_params and return 0.
> 
> Signed-off-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> ---
>  net/core/netdev_rx_queue.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> index a7261fedd5e76..85ae5abf16b8e 100644
> --- a/net/core/netdev_rx_queue.c
> +++ b/net/core/netdev_rx_queue.c
> @@ -193,6 +193,14 @@ static int __netif_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
>  	struct netdev_rx_queue *rxq;
>  	int ret;
>  
> +	/* A leased physical dev is locked via rxq->lease rather than
> +	 * netdev_get_by_index_lock(), so check reg_state under its lock.
> +	 */
> +	if (dev->reg_state != NETREG_REGISTERED) {
> +		NL_SET_ERR_MSG(extack, "Device is being unregistered");
> +		return -ENODEV;
> +	}

nit: wonder if we should keep lease vs non-lease handling in the callers?
netif_mp_open_rxq/netif_mp_close_rxq already handle this, and __ variants
seem to be unaware of the leasing abstractions.

Maybe put them into some new lease-valid helper? So we can keep these
tricky details in a single place, similar to netdev_put_lock?

  netif_lease_valid() { return netif_is_alive && netif_device_present } ?