Re: [PATCH net-next v5 12/13] net: mana: keep the surviving queues when the channel count is reduced

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing `synchronize_net()` on the rollback path of `mana_publish_qset` allows concurrent readers to dereference newly allocated containers after they are freed.

Pre-existing issues:
- [High] u64_stats writer runs with preemption enabled, which can deadlock 32-bit readers in BH context
--

commit ac5cec529175b7a19576c74efe705cb50e0c6d42
Author: Long Li <longli@microsoft.com>

net: mana: keep the surviving queues when the channel count is reduced

Split the live set into a kept prefix and a retiring tail. Reductions
allocate only pointer arrays and steering tables, retaining the kept
queues' page pools, buffers, NAPI state and XDP references.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 78be88b29c99b..158f9a6ce4215 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -4351,9 +4459,7 @@ int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
>  	if (err)
>  		goto rollback;
>  
> -	/* Install XDP and per-RXQ references before steering reaches new
> -	 * queues.
> -	 */
> +	/* Install XDP before steering reaches the incoming RXQs. */
>  	mana_chn_setxdp(apc, mana_xdp_get(apc));
>  
>  	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);

[Severity: High]
This is a pre-existing issue, but the rollback path here appears to have a
synchronization problem. If mana_publish_qset() fails, it triggers a rollback
that ultimately calls mana_fold_rxq_stats() to update stats:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_fold_rxq_stats() {
    ...
	u64_stats_update_begin(&dst->syncp);
	dst->packets		+= src->packets;
	dst->bytes		+= src->bytes;
	u64_stats_update_end(&dst->syncp);
    ...
}

Since this update runs under the RTNL lock in process context without
disabling preemption or bottom halves, what happens if a softirq interrupts
this writer and executes dev_get_stats() (which calls u64_stats_fetch_begin())?

Could this lead to the reader spinning infinitely waiting for the seqcount
to become even, resulting in a hard lockup on 32-bit architectures?

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index acc82fa9f0057..0b8c2f61d6263 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c

[ ... ]

> @@ -737,6 +737,36 @@ static int mana_set_channels(struct net_device *ndev,
>  		goto clear_flag;
>  	}
>  
> +	if (new_count < apc->num_queues) {
> +		struct mana_qset tailq;
> +
> +		err = mana_split_qset(apc, scratch, new_count, &newq, &tailq);
> +		if (err)
> +			goto free_scratch;
> +
> +		err = mana_publish_qset(apc, &newq, &oldq);
> +		if (err) {
> +			/* Discard containers only; their queues still belong to
> +			 * the old set.
> +			 */
> +			mana_discard_split(&newq, &tailq);

[Severity: High]
If mana_publish_qset() fails, its rollback path restores the old pointers via
mana_qset_install(apc, out_old) but returns err without waiting for an RCU 
grace period (such as via synchronize_net()) to ensure readers that observed
the newq pointers have finished.

Here in mana_set_channels(), mana_discard_split() is called immediately,
which frees the newly allocated containers:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_discard_split() {
    kfree(newq->tx_qp);
    kfree(newq->rxqs);
    kfree(newq->indir_table);
    kfree(newq->rxobj_table);
    ...
}

Could concurrent RCU-protected readers like ndo_select_queue (which are not
blocked by netif_tx_disable()) observe the new pointers and execute a
use-after-free when these containers are freed?

> +			goto free_scratch;
> +		}
> +
> +		/* Wait for ndo_select_queue() readers of oldq.indir_table. Free
> +		 * only containers; the queues belong to the kept set or tail.
> +		 */
> +		synchronize_net();
> +
> +		kfree(oldq.tx_qp);
> +		kfree(oldq.rxqs);
> +		kfree(oldq.indir_table);
> +		kfree(oldq.rxobj_table);
> +
> +		mana_free_qset(scratch, &tailq);
> +		goto free_scratch;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909222416.884246-1-longli@microsoft.com?part=12