Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] bpf: concurrent writers to tx_stats->syncp without mutual exclusion - [High] net: missing BH disable around u64_stats_update_begin() in mana_fold_rxq_stats() --
commit 347c8bd66ee5005e7aea875ed691f7a765b23452 Author: Long Li <[email protected]> net: mana: swap queue sets in mana_set_channels This patch introduces a replacement queue set building before quiescing TX. It drains the old TX queues and swaps pointers, handling statistics folding upon successful or failed rollback. > --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c > @@ -59,6 +59,11 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct > xdp_frame **frames, > if (unlikely(!apc->port_is_up)) > return 0; > > + /* Pair with the smp_wmb() in mana_publish_qset() before reading queue > + * state. > + */ > + smp_rmb(); > + > q_idx = smp_processor_id() % ndev->real_num_tx_queues; > > for (i = 0; i < n; i++) { [Severity: High] Does mana_xdp_xmit() require a lock when updating tx_stats->syncp further down in this function? This function acts as the ndo_xdp_xmit callback and maps CPUs to queues using smp_processor_id() % ndev->real_num_tx_queues. If multiple CPUs map to the same queue index, or if mana_start_xmit() runs concurrently on the same queue, multiple threads could concurrently invoke u64_stats_update_begin(&tx_stats->syncp). Since there isn't a per-queue mutual exclusion lock acquired here (unlike the core stack serialization in mana_start_xmit()), could this concurrency corrupt the sequence counter and cause readers to spin infinitely on 32-bit systems? [ ... ] > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c [ ... ] > @@ -1123,6 +1161,58 @@ static void mana_free_queue_stats(struct > mana_port_context *apc) > apc->txq_stats = NULL; > } > > +/* Fold under RTNL after drain_stats writers quiesce. Clear drain_stats to > + * prevent double counting on rollback. > + */ > +static void mana_fold_rxq_stats(struct mana_port_context *apc, > + struct mana_rxq *rxq) > +{ > + struct mana_stats_rx *src = &rxq->drain_stats; > + struct mana_stats_rx *dst; > + unsigned int i; > + > + ASSERT_RTNL(); > + > + if (!apc->rxq_stats_ret || rxq->rxq_idx >= apc->max_queues) > + return; > + > + dst = &apc->rxq_stats_ret[rxq->rxq_idx]; > + > + u64_stats_update_begin(&dst->syncp); > + dst->packets += src->packets; > + dst->bytes += src->bytes; [Severity: High] Could this u64_stats_update_begin() call deadlock a preempting softirq reader? This function runs in process context under rtnl_lock(), which is a mutex and does not disable bottom halves. Readers of network statistics (like ndo_get_stats64) can be executed from softirq context. If a softirq interrupts mana_fold_rxq_stats() on the same CPU while the writer holds an odd sequence counter, the reader will loop forever in u64_stats_fetch_retry(). Should this use local_bh_disable() or the _bh wrapper for the stats update to prevent this scenario? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
