> From: 'Simon Horman' <[email protected]> > > This is an AI-generated review of your patch. The human sending this email has > considered the AI review valid, or at least plausible. > Full review at: > https://sashiko.d/ > ev%2F&data=05%7C02%7Clongli%40microsoft.com%7C3344dd1ec8e44fc6d4a50 > 8dea85caa2a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63913331 > 3118303655%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi > OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0 > %7C%7C%7C&sdata=GJTF8DNw%2FzYNsPz0lC%2FDFZpcCwqjuCmfVu7smim81x > E%3D&reserved=0 > --- > net: mana: Create separate EQs for each vPort > > This commit modifies the mana driver to prepare for assigning vPorts to > dedicated > MSI-X vectors by removing EQ sharing among vPorts. It moves the EQ definition > from the mana_context to the mana_port_context and exports EQ lifecycle > functions for use by the MANA RDMA driver. > > > diff --git a/drivers/infiniband/hw/mana/qp.c > > b/drivers/infiniband/hw/mana/qp.c index 645581359cee..6f1043383e8c > > 100644 > > --- a/drivers/infiniband/hw/mana/qp.c > > +++ b/drivers/infiniband/hw/mana/qp.c > > @@ -168,7 +168,15 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, > struct ib_pd *pd, > > cq_spec.gdma_region = cq->queue.gdma_region; > > cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE; > > cq_spec.modr_ctx_id = 0; > > - eq = &mpc->ac->eqs[cq->comp_vector]; > > + /* EQs are created when a raw QP configures the vport. > > + * A raw QP must be created before creating rwq_ind_tbl. > > + */ > > + if (!mpc->eqs) { > > + ret = -EINVAL; > > + i--; > > + goto fail; > > + } > > + eq = &mpc->eqs[cq->comp_vector % mpc->num_queues]; > > Is it possible for the EQs to be destroyed while this RSS QP is still > actively using > them?
No. The EQ id is consumed only at creation time - it is passed to firmware as cq_spec.attached_eq during mana_create_wq_obj(). After that call the CQ-to-EQ binding lives entirely in firmware. The kernel never dereferences mpc->eqs again for that RSS QP's lifetime, so there is no ongoing kernel-side access to the EQ struct from an active RSS QP. > > If the EQs are created by the Ethernet interface being brought up, or by a RAW > QP configuring the vport, this RSS QP will attach to them without incrementing > pd->vport_use_count or taking any vport reference count. This is by design. The RSS QP (RX side) does not take a vport reference, and symmetrically mana_ib_destroy_qp_rss() does not release one either. Only raw QPs (SQ side) take and release vport references. The refcount is balanced: RSS QPs are pure consumers of an already-configured vport. > > If the Ethernet interface is subsequently brought down, or the RAW QP is > destroyed, mana_destroy_eq() will be called, freeing the mpc->eqs array and > destroying the underlying DMA regions while this RSS QP remains active. This > regression could allow the hardware to DMA completion events into freed EQ > memory. Destroying the raw QP also calls mana_uncfg_vport(), which deconfigures the vport entirely. After that, firmware will not route any traffic or generate completions on this vport, so there are no in-flight DMA writes to the EQ. This is the same pre-existing behavior: the raw QP has always been the vport lifecycle anchor, and destroying it while an RSS QP is active would have broken the vport regardless - this patch does not change that relationship. Before this patch the EQs simply outlived the vport (device lifetime vs vport lifetime), which masked the dependency but did not make the out-of-order teardown any safer at the vport level. > > Additionally, since mpc->eqs is accessed here without holding pd->vport_mutex, > could a concurrent teardown of the EQs lead to a use-after-free when reading > eq->eq->id? The RDMA core serializes QP creation and destruction on the same device context. A concurrent teardown would require destroying the raw QP on the same PD simultaneously with RSS QP creation, which the IB core does not permit. The !mpc->eqs check is a defensive guard against wrong call ordering (creating an RSS QP before any raw QP), not a synchronization point for concurrent access.
