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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes two bugs that came in with the leased-queue
  proxy in commit 5602ad61ebee ("net: Proxy netif_mp_{open,close}_rxq…

--

net: guard memory provider RX queue open and close on unregister

This commit adds a reg_state check to __netif_mp_open_rxq() in
net/core/netdev_rx_queue.c. The check stops a memory provider from being
installed on a physical netdev that is being unregistered when the device
is reached through a leased rxq. It also changes __netif_mp_close_rxq() to
clear rxq->mp_params and return early whenever the device is past
NETREG_REGISTERED, not only when mp_ops is already NULL.

> 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:

[ ... ]

>    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>

[Severity: Low]
Should this have a Fixes: tag?

The rxq->lease redirect in netif_mp_open_rxq() was added by the commit
below. So was netif_rxq_cleanup_unlease(), which calls
__netif_mp_close_rxq() on the physical device:

Fixes: 5602ad61ebee ("net: Proxy netif_mp_{open,close}_rxq for leased queues")

Before that commit, every caller reached __netif_mp_open_rxq() and
__netif_mp_close_rxq() through netdev_get_by_index_lock(). That helper
already rejects devices that are being unregistered.

Could the commit message also say what goes wrong without the fix?

On the open side, unregister_netdevice_many_notify() sets the physical
device to NETREG_UNREGISTERING. It then runs dev_shutdown() and
dev_memory_provider_uninstall() on it before NETDEV_UNREGISTER cascades
into the virtual device's teardown. In that window, a bind-rx on the
virtual device, which is still registered, could install a memory
provider on the physical rxq after the uninstall has already run.

On the close side, devmem's uninstall leaves phys_rxq->mp_params
populated. The old early return only fired when mp_ops was NULL, so it
did not trigger here. As a result,
netif_rxq_cleanup_unlease()->__netif_mp_close_rxq() went on to call
netdev_rx_queue_reconfig() on the shut-down device.

Please state the visible result, for example a queue restart on a
dismantled device, a WARN, or a provider left installed after shutdown.
That would let stable maintainers judge which trees need this and how
urgent it is.

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