Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] mana_ib_remove_cq_cb: Use-After-Free due to missing RCU synchronization - [High] net: mana: Unbounded RTNL stall during queue teardown if EQ is dead - [High] mana: NULL pointer dereference in mana_gd_process_eqe() --
commit 5cede1970aed52e8e7cc0e961f84924a35708274 Author: Long Li <[email protected]> net: mana: share the EQ pool across a queue-set swap This patch makes the EQ pool port-owned so overlapping queue sets share EQs instead of requiring new allocations. It initializes EQ callbacks and throttles before IRQ publication, introduces a lockless RCU pattern for CQ table publication, and modifies CQ teardown to flush events and wait for RCU readers. > diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c > index d4e5e3f912689..0b6ad2547e0ef 100644 > --- a/drivers/infiniband/hw/mana/cq.c > +++ b/drivers/infiniband/hw/mana/cq.c > @@ -155,7 +155,8 @@ int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, > struct mana_ib_cq *cq) > gdma_cq->type = GDMA_CQ; > gdma_cq->cq.callback = mana_ib_cq_handler; > gdma_cq->id = cq->queue.id; > - gc->cq_table[cq->queue.id] = gdma_cq; > + /* Pairs with the acquire load in mana_gd_process_eqe(). */ > + smp_store_release(&gc->cq_table[cq->queue.id], gdma_cq); > return 0; > } [Severity: High] This patch converts the IB CQ table publication into a lockless RCU pattern, but does the teardown path in mana_ib_remove_cq_cb() also need to be updated? If a hardware EQ completion occurs concurrently while mana_ib_remove_cq_cb() is tearing down the CQ, it calls kfree(gc->cq_table[cq->queue.id]) directly without an RCU grace period. The hardirq handler (mana_gd_process_eqe) reads this via smp_load_acquire() and dereferences the freed pointer, leading to a use-after-free. Should mana_ib_remove_cq_cb() use cmpxchg() and synchronize_rcu() for teardown, similar to what was added for Ethernet CQs in mana_gd_destroy_cq()? [ ... ] > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index ae0ed700b3b93..078b3666947a4 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > @@ -768,7 +768,8 @@ static void mana_gd_process_eqe(struct gdma_queue *eq) > if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs)) > break; > > - cq = gc->cq_table[cq_id]; > + /* Match release publication of the CQ and its callback state. > */ > + cq = smp_load_acquire(&gc->cq_table[cq_id]); [Severity: High] Can gc->cq_table be NULL when this acquire load executes? During channel initialization, mana_hwc_establish_channel() communicates with the hardware. The hardware begins executing and sends GDMA_EQE_HWC_INIT_DATA events to the EQ, including HWC_INIT_DATA_MAX_NUM_CQS, which populates gc->max_num_cqs. However, gc->cq_table is not allocated via vcalloc() until after the hardware initialization completes and mana_hwc_establish_channel() resumes. If the hardware sends a spurious or unexpected GDMA_EQE_COMPLETION event in this window, the bounds check passes because gc->max_num_cqs is already set. The handler then executes smp_load_acquire() before the table is actually allocated, which would result in a NULL pointer dereference. [ ... ] > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 7586972e78e09..4b2c8039b8fa3 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c [ ... ] > @@ -2653,12 +2712,25 @@ static void mana_schedule_napi(void *context, struct > gdma_queue *gdma_queue) > > static void mana_deinit_cq(struct mana_port_context *apc, struct mana_cq *cq) > { > - struct gdma_dev *gd = apc->ac->gdma_dev; > + struct gdma_context *gc = apc->ac->gdma_dev->gdma_context; > + struct gdma_queue *gdma_cq = cq->gdma_cq; > + struct gdma_queue *eq; > + int err; > > - if (!cq->gdma_cq) > + if (!gdma_cq) > return; > > - mana_gd_destroy_queue(gd->gdma_context, cq->gdma_cq); > + eq = gdma_cq->cq.parent; > + if (gdma_cq->id < gc->max_num_cqs && eq && > + eq->id != INVALID_QUEUE_ID) { > + /* Flush queued events after WQ teardown, before removing this > CQ. */ > + err = mana_gd_test_eq(gc, eq); > + if (err && mana_en_need_log(apc, err)) > + netdev_err(apc->ndev, "Failed to flush EQ %u for CQ %u: > %d\n", > + eq->id, gdma_cq->id, err); > + } > + > + mana_gd_destroy_queue(gc, gdma_cq); > } [Severity: High] Could this sequential mana_gd_test_eq() call lead to an unbounded RTNL stall if an EQ stops delivering interrupts? If an EQ is dead but the hardware channel remains responsive, mana_gd_test_eq() will wait its full 30-second timeout. Since mana_deinit_cq() is called sequentially for every single TX and RX queue during teardown (via mana_destroy_txq() and mana_destroy_rxqs()), a typical device with 64 queues would experience a 30-second timeout per queue. This could result in an unrecoverable stall of over an hour while holding the global RTNL lock, blocking all networking management. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
