Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] The newly introduced background recovery via `queue_reset_work` races with administrative interface state changes (`ip link set down`), causing the port to become permanently stuck in a broken state. - [High] Clearing `channel_changing` before queuing `queue_reset_work` in `mana_set_channels` creates a race window where the RDMA driver can claim the vport, permanently defeating the recovery. Pre-existing issues: - [Critical] A failure in `mana_attach()` leaves the driver vulnerable to a guaranteed NULL pointer dereference when the port is later brought up via `ip link set up`. - [High] Missing RDMA synchronization via the `channel_changing` flag in `mana_set_ringparam`, `mana_change_mtu`, and `mana_xdp_set` allows the RDMA driver to hijack the vport during configuration changes. - [Medium] The fix for `mana_attach()` failures leaving the port in a broken state was applied to ethtool ring size and channel count changes, but missed MTU changes and XDP program attachment. -- --- Patch [4]: [PATCH net-next v12 4/4] net: mana: recover port on attach failure in ethtool operations --- commit 750fd303cde77a7c9585144b5c7341d83090dbb3 Author: Dipayaan Roy <[email protected]> net: mana: recover port on attach failure in ethtool operations This patch adds a recovery mechanism for mana_attach() failures during ethtool ring size or channel count changes by scheduling queue_reset_work, while adjusting retry values to maximize the chance of recovery. > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > index f77509818d07f..71e69d5a9a04a 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c > @@ -675,8 +676,19 @@ static int mana_set_channels(struct net_device *ndev, > apc->num_queues = new_count; > err = mana_attach(ndev); > if (err) { > - apc->num_queues = old_count; > netdev_err(ndev, "mana_attach failed: %d\n", err); > + > + /* Choose a retry queue count that maximizes recovery > + * chances in the reset work handler. > + */ > + if (old_count < new_count) > + apc->num_queues = old_count; > + else if (new_count > MANA_DEF_NUM_QUEUES) > + apc->num_queues = MANA_DEF_NUM_QUEUES; > + else > + apc->num_queues = 1; > + > + schedule_port_reset = true; > } [Severity: Critical] This is a pre-existing issue, but a failure in mana_attach() leaves the driver vulnerable to a NULL pointer dereference when the port is later brought up manually. If mana_attach() fails, its failure path calls mana_cleanup_port_context() which frees apc->rxqs: drivers/net/ethernet/microsoft/mana/mana_en.c:mana_cleanup_port_context() { ... kfree(apc->rxqs); apc->rxqs = NULL; ... } If the user later brings the interface up, mana_open() triggers the queue allocation chain without reinitializing the port context: drivers/net/ethernet/microsoft/mana/mana_en.c:mana_open() { ... err = mana_alloc_queues(ndev); ... } Which dereferences the NULL pointer in mana_add_rx_queues(): drivers/net/ethernet/microsoft/mana/mana_en.c:mana_add_rx_queues() { ... apc->rxqs[i] = rxq; ... } Is there a mechanism to prevent this sequence if recovery fails? > out: > @@ -685,6 +697,11 @@ static int mana_set_channels(struct net_device *ndev, > mutex_lock(&apc->vport_mutex); > apc->channel_changing = false; > mutex_unlock(&apc->vport_mutex); [Severity: High] Could clearing channel_changing before the reset work executes create a race condition where the RDMA driver claims the vport? If mana_set_channels() fails in mana_attach() and drops vport_mutex while setting apc->channel_changing = false, a concurrent RDMA QP creation could call mana_cfg_vport() and successfully claim the hardware vport since vport_use_count is 0. When the queued reset work later calls mana_attach() -> mana_create_vport(), it would see vport_use_count as 1 and fail with -EBUSY, preventing the recovery from succeeding. Should channel_changing remain true until the recovery completes? > + > + if (schedule_port_reset) > + queue_work(apc->ac->per_port_queue_reset_wq, > + &apc->queue_reset_work); > + > return err; > } [Severity: High] Can this asynchronous recovery race with administrative interface state changes, leaving the port in a state where it cannot be brought up or down? If an admin runs ip link set down before the reset workqueue executes: drivers/net/ethernet/microsoft/mana/mana_en.c:mana_close() { if (!apc->port_is_up) return 0; ... } Because mana_close() returns early, apc->port_st_save remains true, while the kernel state transitions to DOWN. When the reset work runs, it calls mana_attach() which sees port_st_save == true, reallocates queues, increments vport_use_count to 1, and sets apc->port_is_up = true. If the admin then runs ip link set up, mana_open() will fail in mana_create_vport() with -EBUSY because vport_use_count is already 1. A subsequent ip link set down will do nothing because the kernel state is already DOWN. Does the reset work handler need to synchronize against administrative state changes? [ ... ] > @@ -752,11 +770,35 @@ static int mana_set_ringparam(struct net_device *ndev, > err = mana_attach(ndev); > if (err) { [Severity: High] This isn't a bug introduced by this patch, but does the detach and attach sequence here lack synchronization against the RDMA driver? Unlike mana_set_channels(), mana_set_ringparam() does not acquire vport_mutex or set apc->channel_changing = true before calling mana_detach(). When it calls mana_detach(ndev, false), vport_use_count drops to 0. A concurrent RDMA workload could call mana_cfg_vport() and claim the vport during this window. When the Ethernet driver subsequently calls mana_attach(), it would fail with -EBUSY. This same lack of synchronization appears to be present in mana_change_mtu() and mana_xdp_set(). > netdev_err(ndev, "mana_attach failed: %d\n", err); > - apc->tx_queue_size = old_tx; > - apc->rx_queue_size = old_rx; > + NL_SET_ERR_MSG_FMT(extack, "failed to change ring params: %d", > + err); > + > + /* Choose retry ring sizes that maximize recovery > + * chances in the reset work handler. Handle RX and > + * TX independently. > + */ > + if (old_rx < new_rx) > + apc->rx_queue_size = old_rx; > + else if (new_rx > DEF_RX_BUFFERS_PER_QUEUE) > + apc->rx_queue_size = DEF_RX_BUFFERS_PER_QUEUE; > + else > + apc->rx_queue_size = MIN_RX_BUFFERS_PER_QUEUE; > + > + if (old_tx < new_tx) > + apc->tx_queue_size = old_tx; > + else if (new_tx > DEF_TX_BUFFERS_PER_QUEUE) > + apc->tx_queue_size = DEF_TX_BUFFERS_PER_QUEUE; > + else > + apc->tx_queue_size = MIN_TX_BUFFERS_PER_QUEUE; > + > + schedule_port_reset = true; > } > out: > mana_pre_dealloc_rxbufs(apc); > + > + if (schedule_port_reset) > + queue_work(apc->ac->per_port_queue_reset_wq, > + &apc->queue_reset_work); > return err; > } [Severity: Medium] This is a pre-existing issue, but should this same recovery mechanism be applied to MTU and XDP program changes? If mana_attach() fails in mana_change_mtu() or mana_xdp_set(), they return immediately without scheduling queue_reset_work. This leaves the port detached, which eventually leads to the NULL pointer dereference in mana_open() if the user tries to bring the interface up manually. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
