Re: [PATCH net-next v12 4/4] net: mana: recover port on attach failure in ethtool operations

[sashiko-bot@kernel.org]

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 <dipayanroy@linux.microsoft.com>

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/20260711041415.3008868-1-dipayanroy@linux.microsoft.com?part=4