Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] The NAPI struct for an RX/TX queue is scheduled before it is fully initialized, leading to a NULL pointer dereference (Kernel Panic). - [High] Dangling `cq_table` entries after RAW/RSS QP destruction cause memory leaks and prevent future QP creation (Cross-process DoS). -- --- Patch [1]: [PATCH net v4 1/7] net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy --- commit 855b59e3efafe6690d7dd204468e072eef10cb49 Author: Long Li <[email protected]> net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy This commit puts the cq_table under RCU protection to prevent a use-after-free race between the EQ interrupt handler and CQ teardown. It ensures the slot is cleared and an RCU grace period elapses before freeing the CQ memory. > diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c > index f2547989f4229..2bf4be21cede7 100644 > --- a/drivers/infiniband/hw/mana/cq.c > +++ b/drivers/infiniband/hw/mana/cq.c > @@ -131,12 +131,20 @@ static void mana_ib_cq_handler(void *ctx, struct > gdma_queue *gdma_cq) > int mana_ib_install_cq_cb(struct mana_ib_dev *mdev, struct mana_ib_cq *cq) > { > struct gdma_context *gc = mdev_to_gc(mdev); > + struct gdma_queue __rcu **cq_table; > struct gdma_queue *gdma_cq; > > - if (cq->queue.id >= gc->max_num_cqs) > + /* No rcu_read_lock(): install/remove run within the IB device > + * lifetime, which mana_rdma_remove() (ib_unregister_device) drains > + * before the base cq_table can be freed. See gdma_context::cq_table > + * in gdma.h for why "true" is sound. > + */ > + cq_table = rcu_dereference_protected(gc->cq_table, true); > + if (!cq_table || cq->queue.id >= gc->max_num_cqs) > return -EINVAL; > + > /* Create CQ table entry, sharing a CQ between WQs is not supported */ > - if (gc->cq_table[cq->queue.id]) > + if (rcu_access_pointer(cq_table[cq->queue.id])) [Severity: High] This isn't a bug introduced by this patch, but could this fail permanently due to a dangling pointer? If a userspace application creates an RDMA QP with an existing CQ, mana_ib_create_qp_raw() allocates a WQ/CQ pair and overwrites send_cq->queue.id. When the QP is destroyed, mana_ib_destroy_qp_raw() tears down the WQ/CQ in hardware but does not call mana_ib_remove_cq_cb(). This leaves a dangling pointer in cq_table. If the hardware reallocates the same CQ ID to another process or queue, won't this check see the leaked entry and prevent further QP creation? > return -EINVAL; > if (cq->queue.kmem) > gdma_cq = cq->queue.kmem; [ ... ] > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 92bb55935c1c4..5d215981bba83 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c [ ... ] > @@ -2596,12 +2619,18 @@ static int mana_create_txq(struct mana_port_context > *apc, > > cq->gdma_id = cq->gdma_cq->id; > > - if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) { > + /* No rcu_read_lock(): mana_create_txq runs under RTNL during > + * netdev bring-up, inside the netdev lifetime that > + * mana_remove() drains before the base cq_table can be freed. > + * See gdma_context::cq_table in gdma.h for why "true" is sound. > + */ > + cq_table = rcu_dereference_protected(gc->cq_table, true); > + if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) { > err = -EINVAL; > goto out; > } > > - gc->cq_table[cq->gdma_id] = cq->gdma_cq; > + rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq); [Severity: Critical] This is a pre-existing issue, but by publishing the CQ to cq_table before the NAPI struct is initialized, does this open a race window that could lead to a crash? Once rcu_assign_pointer() publishes the CQ to cq_table, an early hardware completion could trigger the EQ interrupt handler mana_gd_process_eqe(), which can access the published CQ and call mana_schedule_napi(). > > mana_create_txq_debugfs(apc, i); > > @@ -2621,25 +2650,20 @@ static int mana_create_txq(struct mana_port_context > *apc, [ ... ] > @@ -2905,12 +2966,18 @@ static struct mana_rxq *mana_create_rxq(struct > mana_port_context *apc, > if (err) > goto out; > > - if (WARN_ON(cq->gdma_id >= gc->max_num_cqs)) { > + /* No rcu_read_lock(): mana_create_rxq runs under RTNL during netdev > + * bring-up, inside the netdev lifetime that mana_remove() drains > + * before the base cq_table can be freed. See gdma_context::cq_table > + * in gdma.h for why "true" is sound. > + */ > + cq_table = rcu_dereference_protected(gc->cq_table, true); > + if (WARN_ON(!cq_table || cq->gdma_id >= gc->max_num_cqs)) { > err = -EINVAL; > goto out; > } > > - gc->cq_table[cq->gdma_id] = cq->gdma_cq; > + rcu_assign_pointer(cq_table[cq->gdma_id], cq->gdma_cq); [Severity: Critical] This is a pre-existing issue, but since mana_push_wqe(rxq) has already posted buffers to the hardware just before this, couldn't a network packet be received at this point? Since netif_napi_add_weight_locked() hasn't run yet, would an early hardware interrupt cause the EQ handler to invoke napi_schedule() on an uninitialized NAPI struct? > > netif_napi_add_weight_locked(ndev, &cq->napi, mana_poll, 1); > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
