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 <[email protected]> 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/[email protected]?part=12
