This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: mana: RCU-protect gc->cq_table lookups against concurrent CQ destroy
This puts gc->cq_table (base pointer and entries) under RCU so the EQ
interrupt handler can no longer dereference a CQ that a concurrent teardown
has freed. Teardown clears the slot and takes a grace period before the CQ
memory is released, and the Ethernet paths are converted to a two-pass
quiesce/free so the grace periods are batched instead of one per CQ.
A few questions below.
> diff --git a/drivers/infiniband/hw/mana/cq.c b/drivers/infiniband/hw/mana/cq.c
> index f2547989f4229..73d97b2f5cf9b 100644
> --- a/drivers/infiniband/hw/mana/cq.c
> +++ b/drivers/infiniband/hw/mana/cq.c
[ ... ]
> @@ -149,23 +157,54 @@ 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;
> + rcu_assign_pointer(cq_table[cq->queue.id], gdma_cq);
> return 0;
> }
>
> void mana_ib_remove_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 || cq->queue.id == INVALID_QUEUE_ID)
> + if (cq->queue.id == INVALID_QUEUE_ID)
> return;
>
> if (cq->queue.kmem)
> /* Then it will be cleaned and removed by the mana */
> return;
>
> - kfree(gc->cq_table[cq->queue.id]);
> - gc->cq_table[cq->queue.id] = NULL;
> + /* No rcu_read_lock(): like mana_ib_install_cq_cb(), this runs within
> + * the IB device lifetime that mana_rdma_remove() drains before the
> + * base cq_table can be freed. See gdma_context::cq_table in gdma.h.
> + */
> + cq_table = rcu_dereference_protected(gc->cq_table, true);
> + if (!cq_table || cq->queue.id >= gc->max_num_cqs)
> + return;
> + /* Removers for a given CQ are serialized by the IB core, so the slot
> + * is read and cleared without rcu_read_lock() or atomicity: a CQ is
> + * never torn down while a live QP references it (cq->usecnt), nor
> + * while the QP-create that installed the entry is still running (that
> + * create holds a reference on the CQ uobject across its error path,
> + * before usecnt is taken). Any double-remove is therefore sequential
> + * -- the later caller sees the NULL stored below and returns.
> + */
> + gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true);
> + /* Clear the slot only if it still holds the entry this CQ installed
> + * (gdma_cq->cq.context == cq). If the id was already removed, or was
> + * recycled and republished for another CQ, leave the current entry
> + * intact instead of wiping a live one.
> + */
> + if (!gdma_cq || gdma_cq->cq.context != cq)
> + return;
> +
> + rcu_assign_pointer(cq_table[cq->queue.id], NULL);
[Severity: Low]
The comment says the identity test protects against an id that "was
recycled and republished for another CQ", but the compare and the clear
are two separate accesses with nothing making them indivisible. Both
mana_ib_remove_cq_cb() here and mana_gd_unpublish_cq() do:
/* read */
gdma_cq = rcu_dereference_protected(cq_table[cq->queue.id], true);
if (!gdma_cq || gdma_cq->cq.context != cq)
return;
/* separate store */
rcu_assign_pointer(cq_table[cq->queue.id], NULL);
The two sides of the table are also asymmetric: mana_ib_install_cq_cb()
refuses to publish into a non-NULL slot, but mana_create_txq() and
mana_create_rxq() publish with rcu_assign_pointer(cq_table[cq->gdma_id],
cq->gdma_cq) and no occupancy check at all.
So can this interleaving wipe a live entry?
remover publisher
reads slot, sees A
stores B' into the same slot
stores NULL (B' is now gone)
Since mana_gd_process_eqe() now drops a NULL slot silently, would every
completion for B' then be discarded with no diagnostic, and would B''s own
remover later see NULL and skip its kfree()? Would a cmpxchg on the slot,
or a small lock covering cq_table entry mutation, make the conditional
clear actually atomic?
This does need the device to hand the same CQ id to two live consumers, so
I could not demonstrate it in practice; the pre-patch code cleared the slot
unconditionally, so the guard is still an improvement.
> +
> + /* Wait for in-flight EQ handlers that may have loaded the old
> + * pointer via rcu_dereference() to finish before freeing.
> + */
> + synchronize_rcu();
> + kfree(gdma_cq);
> }
>
> int mana_ib_arm_cq(struct ib_cq *ibcq, enum ib_cq_notify_flags flags)
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index e8b7ffb47eb98..d40f25a1a74a7 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
[ ... ]
> @@ -743,11 +744,30 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
> switch (type) {
> case GDMA_EQE_COMPLETION:
> cq_id = eqe->details[0] & 0xFFFFFF;
> + cq_table = rcu_dereference(gc->cq_table);
> + if (WARN_ON_ONCE(!cq_table))
> + break;
> +
> + /* Pair with the rcu_assign_pointer(gc->cq_table) release in
> + * mana_hwc_establish_channel(), which publishes the table
> + * after storing gc->max_num_cqs. The rmb keeps this bound
> + * read ordered after the table load, so a shrinking
> + * re-establish cannot pair a stale, larger max_num_cqs with a
> + * newly published, smaller table and index out of bounds.
> + */
> + smp_rmb();
> if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
> break;
>
> - cq = gc->cq_table[cq_id];
> - if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id))
> + cq = rcu_dereference(cq_table[cq_id]);
[Severity: High]
This isn't a bug introduced by this patch, but at this commit the bound
used here is still mutable while a table is published.
mana_hwc_init_event_handler() writes it straight from EQ interrupt context
from a device-supplied value:
case HWC_INIT_DATA_MAX_NUM_CQS:
gd->gdma_context->max_num_cqs = val;
while the table itself is sized once in mana_hwc_establish_channel() with
vcalloc(gc->max_num_cqs, sizeof(*cq_table)). A later, larger value would
inflate every bound test (cq_id >= gc->max_num_cqs here, cq->gdma_id >=
gc->max_num_cqs in mana_create_txq()/mana_create_rxq(), cq->queue.id >=
gc->max_num_cqs in mana_ib_install_cq_cb()) past the allocation, and a
smaller one would make mana_gd_unpublish_cq() return false so
mana_gd_destroy_cq() skips the grace period for a still-published CQ.
The gdma.h comment added by this patch states the invariant as already
holding ("max_num_cqs above is the size of cq_table and an upper bound on
valid CQ indices for the table's lifetime"), which only becomes true with
the last patch in this series, "net: mana: keep max_num_cqs immutable once
cq_table is allocated" -- that one moves the handler to
WRITE_ONCE(hwc->hwc_init_max_num_cqs, val) and has
mana_hwc_establish_channel() commit a single READ_ONCE() snapshot as both
the vcalloc() size and gc->max_num_cqs. Should the documented contract
land together with the change that establishes it, or reference it?
> + /* A NULL entry is expected while a concurrent teardown
> + * (e.g. ifdown or an MTU change) has unpublished this CQ but
> + * not yet freed it; the completion is stale, so drop it
> + * quietly rather than warning.
> + */
> + if (!cq)
> + break;
> + if (WARN_ON_ONCE(cq->type != GDMA_CQ || cq->id != cq_id))
> break;
>
> if (cq->cq.callback)
> @@ -1050,18 +1070,47 @@ static void mana_gd_create_cq(const struct
> gdma_queue_spec *spec,
> queue->cq.callback = spec->cq.callback;
> }
>
> -static void mana_gd_destroy_cq(struct gdma_context *gc,
> - struct gdma_queue *queue)
> +bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue)
> {
> + struct gdma_queue __rcu **cq_table;
> u32 id = queue->id;
>
> - if (id >= gc->max_num_cqs)
> - return;
> + /* No rcu_read_lock() here: unpublish runs only on the
> + * CQ-destroy/teardown path, where the base cq_table is stable. See
> + * the lifecycle note on gdma_context::cq_table in gdma.h for why the
> + * "true" predicate is sound.
> + */
> + cq_table = rcu_dereference_protected(gc->cq_table, true);
> + if (!cq_table || id >= gc->max_num_cqs)
> + return false;
> +
> + /* Clear the slot only if it still refers to this queue. The
> + * Ethernet two-pass teardown unpublishes the same index twice, a
> + * grace period apart, and a CQ that legitimately recycled this id in
> + * between (e.g. a new RDMA CQ via mana_ib_install_cq_cb()) must not
> + * have its fresh entry wiped by the second pass.
> + */
> + if (rcu_access_pointer(cq_table[id]) != queue)
> + return false;
> +
> + rcu_assign_pointer(cq_table[id], NULL);
> + return true;
> +}
>
> - if (!gc->cq_table[id])
> +static void mana_gd_destroy_cq(struct gdma_context *gc,
> + struct gdma_queue *queue)
> +{
> + /* A batched teardown may already have cleared the slot and taken the
> + * grace period; then there is nothing left to wait for.
> + */
> + if (!mana_gd_unpublish_cq(gc, queue))
> return;
>
> - gc->cq_table[id] = NULL;
> + /* Wait for in-flight EQ handlers that may have loaded the old
> + * pointer via rcu_dereference() to finish before the caller
> + * frees the CQ memory.
> + */
> + synchronize_rcu();
> }
[Severity: Medium]
mana_gd_unpublish_cq() returns false for two different states, and
mana_gd_destroy_cq() treats both as "nothing left to wait for":
a) the slot is already NULL because a batched pass cleared it and
already took the grace period, and
b) rcu_access_pointer(cq_table[id]) != queue, i.e. this queue's entry
was overwritten by another publisher and no grace period has been
taken for this pointer.
In case (b), mana_gd_destroy_queue() then kfree()s the queue with no wait.
Can an in-flight mana_gd_process_eqe() that already did
cq = rcu_dereference(cq_table[cq_id]);
on exactly that pointer go on to read cq->type / cq->id and call
cq->cq.callback(cq->cq.context, cq) after the free?
One route into state (b): for an RDMA kernel (kmem) CQ,
mana_ib_install_cq_cb() publishes the entry, mana_ib_remove_cq_cb()
returns early for kmem ("Then it will be cleaned and removed by the mana"),
so the entry stays published across mana_ib_gd_destroy_cq() which releases
the CQ id on the device, and is only cleared later via
mana_ib_destroy_queue() -> mana_gd_destroy_queue() -> mana_gd_destroy_cq().
If the device hands that id to an Ethernet CQ being created in that window,
mana_create_txq()/mana_create_rxq() overwrite the slot unconditionally, and
the identity test above then fails.
Would it be safer to distinguish "we cleared our slot", "never published"
and "our entry was overwritten", and still synchronize in the last case?
I could not confirm the device actually recycles ids inside that window, so
this may not be reachable in practice.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index e3c24d50dad07..409e20caeccdc 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -698,11 +699,15 @@ static int mana_hwc_establish_channel(struct
> gdma_context *gc, u16 *q_depth,
> if (WARN_ON(cq->id >= gc->max_num_cqs))
> return -EPROTO;
>
> - gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *));
> - if (!gc->cq_table)
> + cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> + if (!cq_table)
> return -ENOMEM;
>
> - gc->cq_table[cq->id] = cq;
> + rcu_assign_pointer(cq_table[cq->id], cq);
> + /* Publish the fully-initialised table last; pairs with the
> + * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
> + */
> + rcu_assign_pointer(gc->cq_table, cq_table);
>
> return 0;
> }
[ ... ]
> 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
> @@ -2427,12 +2427,18 @@ static void mana_deinit_txq(struct mana_port_context
> *apc, struct mana_txq *txq)
>
> static void mana_destroy_txq(struct mana_port_context *apc)
> {
> + struct gdma_context *gc = apc->ac->gdma_dev->gdma_context;
> struct napi_struct *napi;
> int i;
>
> if (!apc->tx_qp)
> return;
>
> + /* Pass 1: quiesce each CQ on the device and clear its cq_table slot.
> + * Taking one grace period below for the whole port avoids up to
> + * apc->num_queues serialized synchronize_rcu() calls (one per CQ in
> + * mana_gd_destroy_cq()) under RTNL on every teardown.
> + */
[Severity: Low]
This isn't a bug, but is "the whole port" accurate here? RX and TX are
batched independently: mana_destroy_rxqs() unpublishes only the RX CQs and
calls synchronize_rcu(), then mana_destroy_txq() unpublishes only the TX
CQs and calls synchronize_rcu() again, and mana_detach() runs both back to
back:
mana_destroy_rxqs(apc);
mana_destroy_txq(apc);
The same pairing happens on the attach error path. So each ifdown, MTU
change or ring reconfigure takes two grace periods per port rather than
one.
The commit message says the same thing:
Clear all of a port's CQ slots first and take a single grace period
per teardown instead
and the comment in mana_destroy_rxqs() reads "One grace period for the
whole port instead of one per RXQ". Would "one grace period per RX batch
and one per TX batch" describe the result more precisely?
> for (i = 0; i < apc->num_queues; i++) {
> if (!apc->tx_qp[i])
> continue;
> @@ -2448,8 +2454,24 @@ static void mana_destroy_txq(struct mana_port_context
> *apc)
> apc->tx_qp[i]->txq.napi_initialized = false;
> }
>
> - if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
> - mana_destroy_wq_obj(apc, GDMA_SQ,
> apc->tx_qp[i]->tx_object);
> + if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE) {
> + mana_destroy_wq_obj(apc, GDMA_SQ,
> + apc->tx_qp[i]->tx_object);
> + apc->tx_qp[i]->tx_object = INVALID_MANA_HANDLE;
> + }
> +
> + if (apc->tx_qp[i]->tx_cq.gdma_cq)
> + mana_gd_unpublish_cq(gc, apc->tx_qp[i]->tx_cq.gdma_cq);
> + }
> +
> + synchronize_rcu();
> +
> + /* Pass 2: the slots are clear, so mana_gd_destroy_cq() skips its own
> + * grace period; free the CQ, the TXQ and the queue pair.
> + */
> + for (i = 0; i < apc->num_queues; i++) {
> + if (!apc->tx_qp[i])
> + continue;
>
> mana_deinit_cq(apc, &apc->tx_qp[i]->tx_cq);
>
[ ... ]
> @@ -2987,16 +3054,31 @@ static void mana_destroy_rxqs(struct
> mana_port_context *apc)
> struct mana_rxq *rxq;
> u32 rxq_idx;
>
> - if (apc->rxqs) {
> + if (!apc->rxqs)
> + return;
>
> - for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> - rxq = apc->rxqs[rxq_idx];
> - if (!rxq)
> - continue;
> + /* Pass 1: quiesce every RXQ's CQ and clear its cq_table slot. */
> + for (rxq_idx = 0; rxq_idx < apc->num_queues; rxq_idx++) {
> + rxq = apc->rxqs[rxq_idx];
> + if (!rxq)
> + continue;
>
> - mana_destroy_rxq(apc, rxq, true);
> - apc->rxqs[rxq_idx] = NULL;
> - }
> + mana_quiesce_rxq(apc, rxq, true);
> + }
> +
> + /* One grace period for the whole port instead of one per RXQ. */
> + synchronize_rcu();
[ ... ]
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 0c395917b2144..0f591029d363b 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
[ ... ]
> @@ -496,6 +525,12 @@ int mana_gd_create_mana_wq_cq(struct gdma_dev *gd,
>
> void mana_gd_destroy_queue(struct gdma_context *gc, struct gdma_queue
> *queue);
>
> +/* Clear a CQ's cq_table slot without waiting for a grace period. Batched
> + * teardown paths clear several slots and then take a single
> synchronize_rcu();
> + * single-CQ callers use mana_gd_destroy_cq() instead, which also waits.
> + */
> +bool mana_gd_unpublish_cq(struct gdma_context *gc, struct gdma_queue *queue);
[Severity: Low]
This isn't a bug, but the comment directs includers of this header to an
API they cannot reach: mana_gd_destroy_cq() is static in gdma_main.c and is
not declared anywhere. The reachable entry point is
mana_gd_destroy_queue(), which is EXPORT_SYMBOL_NS("NET_MANA") and
dispatches GDMA_CQ into the private helper.
mana_gd_unpublish_cq() itself is non-static and declared here, but has no
EXPORT_SYMBOL_NS("NET_MANA") unlike its header-declared siblings, so a
mana_ib user of it would not link; its only users (gdma_main.c and
mana_en.c) are both in mana.ko.
Would keeping the declaration in a mana-private header, or exporting it and
pointing the comment at mana_gd_destroy_queue(), avoid the situation this
patch already runs into -- mana_ib_remove_cq_cb() open-coding the same
unpublish-then-wait sequence with a different identity predicate
(gdma_cq->cq.context != cq) from the one in mana_gd_unpublish_cq()
(rcu_access_pointer(cq_table[id]) != queue)?
> int mana_gd_poll_cq(struct gdma_queue *cq, struct gdma_comp *comp, int
> num_cqe);
>
> void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit);
--
pw-bot: cr
