Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 2
- [High] mana_hwc_get_msg_index() converts pure FIFO-slot contention into
-ETIMEDOUT: `if (down_timeout(&hwc->sema,…
- [Medium] The newly added gc->hwc_lock is documented as protecting
hwc->hwc_timeout accesses in mana_need_log()/mana_serv_reset(), but…
- [Low] The patch closes a real, user-visible late-response
misassociation bug (a timed-out request's slot was released…
- [Low] The new hwc_caller_ctx.resp_pending field is documented in
include/net/mana/hw_channel.h as 'Response-side reference outstanding;…
Pre-existing issues:
- [High] mana_hwc_destroy_cq() frees the completion buffer as its very
first action (`kfree(hwc_cq->comp_buf);`), then destroys the GDMA CQ,…
- [Medium] The new channel_up admission gate is evaluated only after
blocking on hwc->sema and after acquiring hwc->inflight_msg_res.lock,…
--
net: mana: support concurrent HWC requests
This adds hwc_lock to protect HWC publication and sender accounting,
serializes SQ posting with a new per-wq spinlock, bounds FIFO slot
admission with down_timeout(), and quarantines timed-out requests until a
response or teardown releases the slot. Teardown now stops admission,
force-completes outstanding requests and drains senders before destroying
the CQ, TXQ and RXQ.
A few questions below.
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 6605e7a9c481b..a4f7346d285f7 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -6,7 +6,6 @@
> #include <net/mana/hw_channel.h>
> #include <linux/vmalloc.h>
>
> -/* Acquire a free inflight message slot, waiting for one if all are in use.
> */
> static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16
> *msg_id)
> {
> struct gdma_resource *r = &hwc->inflight_msg_res;
> @@ -14,12 +13,30 @@ static int mana_hwc_get_msg_index(struct
> hw_channel_context *hwc, u16 *msg_id)
> unsigned long flags;
> u32 index;
>
> - down(&hwc->sema);
> + /* FIFO slot admission has a separate budget from the response wait.
> + * Expiry reports -ETIMEDOUT even while earlier requests make progress,
> + * so callers may initiate recovery on contention alone.
> + */
> + if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
> + return -ETIMEDOUT;
[Severity: High]
Can pure slot contention here trigger a device reset on healthy hardware?
The channel is built at HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH == 1 in
mana_hwc_create_channel(), so sema_init() grants a single permit and any
two concurrent mana_gd_send_request() callers contend for it. The loser
gets -ETIMEDOUT, which mana_gd_send_request() propagates unchanged.
Existing callers read that code as "the HWC is dead":
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_probe() {
if (err == -ETIMEDOUT || err == -EPROTO) {
...
schedule_delayed_work(&mana_dev_recovery_work.work, ...);
}
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
ret = mana_gd_resume(pdev);
if (ret == -ETIMEDOUT || ret == -EPROTO) {
... mana_serv_rescan(pdev);
}
mana_serv_rescan() calls pci_stop_and_remove_bus_device() followed by
pci_rescan_bus(). In mana_en the gf-stats work latches
ac->hwc_timeout_occurred = true on -ETIMEDOUT and stops collecting stats
permanently.
The admission budget is also the same field the response-timeout path in
mana_hwc_send_request() shrinks:
if (hwc->hwc_timeout > 1)
hwc->hwc_timeout = 1;
and the field mana_serv_reset() sets to 0, so after either event
down_timeout() takes the expired path immediately for a second caller even
though the channel is responsive.
Would returning a distinguishable code for contention, such as -EBUSY or
-EAGAIN, avoid teaching every caller to reset the device on queue depth
pressure? The commit message acknowledges this:
"Admission expiry returns -ETIMEDOUT even for contention, so existing
callers may reset a responsive channel."
Checking the end of the series, mana_hwc_get_msg_index() is byte-identical
there, so the later dynamic HWC queue depth work does not address this; it
only makes exhaustion less likely when the device advertises a depth
greater than one.
>
> spin_lock_irqsave(&r->lock, flags);
>
> - index = find_first_zero_bit(hwc->inflight_msg_res.map,
> - hwc->inflight_msg_res.size);
> + if (!hwc->channel_up) {
> + spin_unlock_irqrestore(&r->lock, flags);
> + up(&hwc->sema);
> + return -ENODEV;
> + }
> +
> + /* The semaphore admits at most r->size holders at a time, so a slot
> + * acquired above always has a free bit waiting for it here.
> + */
> + index = find_first_zero_bit(r->map, r->size);
> + if (WARN_ON_ONCE(index >= r->size)) {
> + spin_unlock_irqrestore(&r->lock, flags);
> + up(&hwc->sema);
> + return -EIO;
> + }
>
> ctx = &hwc->caller_ctx[index];
> reinit_completion(&ctx->comp_event);
> @@ -28,11 +45,12 @@ static int mana_hwc_get_msg_index(struct
> hw_channel_context *hwc, u16 *msg_id)
> */
> refcount_set(&ctx->refcnt, 2);
> ctx->responded = false;
> + ctx->resp_pending = true;
[Severity: Low]
The new field is documented in include/net/mana/hw_channel.h as:
/* Response-side reference outstanding; protected by lock. */
bool resp_pending;
but this initializing write runs under hwc->inflight_msg_res.lock only,
and ctx->lock is never taken in mana_hwc_get_msg_index(). Every other
write to the field (mana_hwc_handle_resp(), the timeout path and the out:
path of mana_hwc_send_request(), and mana_hwc_destroy_channel()) does hold
ctx->lock.
On the consumer side, mana_hwc_handle_resp() gates on a lock-free
test_bit(msg_id, hwc->inflight_msg_res.map) before acquiring ctx->lock, so
the publishing stores to responded/resp_pending and the bitmap bit share no
lock or barrier with the reader that consumes them.
Since msg_id comes from the RX buffer and is read with READ_ONCE() in
mana_hwc_rx_event_handler() precisely because the host can modify it, a
response for the freshly published msg_id can arrive between bitmap_set()
and the sender's first spin_lock(&ctx->lock). If that handler observed the
new resp_pending == true together with a stale responded == true it would
consume the response-side reference, and a later sender timeout would then
drop the refcount to zero and reopen the late-response reuse window this
patch closes.
Should the comment name inflight_msg_res.lock plus the publication
ordering, or should the initialization move under ctx->lock?
> ctx->msg_id = index;
> ctx->error = -EINPROGRESS;
>
> /* Publish the slot last, after it is fully initialised. */
> - bitmap_set(hwc->inflight_msg_res.map, index, 1);
> + bitmap_set(r->map, index, 1);
>
> spin_unlock_irqrestore(&r->lock, flags);
>
[ ... ]
> @@ -817,6 +873,11 @@ int mana_hwc_create_channel(struct gdma_context *gc)
> gd->pdid = INVALID_PDID;
> gd->doorbell = INVALID_DOORBELL;
>
> + /* Publish for setup; queue initialization below must precede senders.
> */
> + spin_lock_irqsave(&gc->hwc_lock, flags);
> + gc->hwc.driver_data = hwc;
> + spin_unlock_irqrestore(&gc->hwc_lock, flags);
> +
> /* mana_hwc_init_queues() only creates the required data structures,
> * and doesn't touch the HWC device.
> */
[Severity: Medium]
This isn't a bug introduced by this patch, since the baseline also assigned
gd->driver_data = hwc before mana_hwc_init_queues(), but does the new
channel_up gate give the impression that lookups are now safe when it is
ordered after the blocking acquire?
driver_data is published here, while sema_init() and
mana_gd_alloc_res_map() (which allocates the bitmap and initializes
inflight_msg_res.lock) only run inside mana_hwc_init_queues() ->
mana_hwc_init_inflight_msg(), and caller_ctx plus channel_up = true only
appear later in mana_hwc_test_channel().
mana_gd_send_request() gates on nothing else:
hwc = gc->hwc.driver_data;
if (!hwc) { ... return -ENODEV; }
hwc->active_senders++;
so a sender that observes the freshly published pointer reaches
mana_hwc_get_msg_index() and executes:
if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
on a kzalloc'ed semaphore (count == 0, wait_list.next/prev == NULL, so
___down_common()'s list_add_tail() walks NULL pointers), then
spin_lock_irqsave(&r->lock, flags) on a never-initialized spinlock, all
before the channel_up check can reject it.
The same window exists in the depth-renegotiation path added later in the
series, which calls mana_hwc_destroy_queues() (frees the bitmap, kfrees
caller_ctx) and then mana_hwc_init_queues() again while driver_data stays
published.
I could not exhibit a concurrent sender, since mana_hwc_create_channel()
runs from mana_gd_setup() during probe and resume where the clients have
been removed or not yet probed. Is the requirement stated by the two new
comments ("queue initialization below must precede senders", "The setup
caller must exclude concurrent HWC teardown") enforced anywhere in code?
[ ... ]
> @@ -864,14 +974,28 @@ void mana_hwc_destroy_channel(struct gdma_context *gc)
> }
> gc->max_num_cqs = 0;
>
> + /* Deregister the HWC EQ before freeing the work queues. */
> + if (hwc->cq)
> + mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> +
> if (hwc->txq)
> mana_hwc_destroy_wq(hwc, hwc->txq);
>
> if (hwc->rxq)
> mana_hwc_destroy_wq(hwc, hwc->rxq);
>
> - if (hwc->cq)
> - mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
[Severity: High]
This is a pre-existing issue in the helper rather than something this patch
introduces, but since the comment here now makes mana_hwc_destroy_cq() the
fence for the interrupt handler, does that helper actually fence anything
before it frees the buffers the handler writes?
mana_hwc_destroy_cq() does:
kfree(hwc_cq->comp_buf);
if (hwc_cq->gdma_cq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_cq);
if (hwc_cq->gdma_eq)
mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
Only the last step reaches the barrier, via mana_gd_destroy_eq() ->
mana_gd_deregister_irq():
list_del_rcu(&eq->entry);
...
synchronize_rcu();
Meanwhile the hard-IRQ callback writes into the buffer that was already
freed and dereferences the queue that was already destroyed:
completions = hwc_cq->comp_buf;
comp_read = mana_gd_poll_cq(q_self, completions, hwc_cq->queue_depth);
An interrupt taken in that interval reaches mana_gd_intr() -> the HWC EQ,
which is still on gic->eq_list -> mana_gd_process_eqe() ->
mana_hwc_comp_event(), so can this write into the kfree()d comp_buf?
The gc->max_num_cqs = 0 store just above helps for the completion path,
because mana_gd_process_eqe() does:
if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs))
break;
but that plain store carries no barrier or RCU synchronization, so an
interrupt already inside mana_gd_process_eqe() on another CPU can still
pass the check, and the EQ-callback path has no such guard at all:
mana_hwc_init_event_handler() dereferences hwc->cq (for example
GDMA_EQE_HWC_INIT_EQ_ID_DB writing hwc->cq->gdma_eq->id) after it has been
kfree()d, and hwc->cq is not set to NULL here.
The window is also reachable when mana_smc_teardown_hwc() fails, since
mana_hwc_destroy_channel() only logs "Failed to tear down HWC" and
continues while the device may still be generating HWC EQEs and CQEs.
Would destroying and deregistering the EQ first, and freeing comp_buf and
the CQ last, close this?
[ ... ]
> @@ -965,43 +1101,40 @@ int mana_hwc_send_request(struct hw_channel_context
> *hwc, u32 req_len,
> ctx->output_buf = NULL;
> err = ctx->error;
> status = ctx->status_code;
> + if (err == -EINPROGRESS) {
> + /* Publish abandonment with buffer withdrawal so a late
> + * response can reclaim the slot. Keep its reference.
> + */
> + ctx->responded = true;
> + abandoned = true;
> + }
> spin_unlock_irqrestore(&ctx->lock, flags);
[Severity: Low]
This hunk, together with the resp_pending handling in
mana_hwc_handle_resp(), fixes a real misassociation on existing kernels:
the baseline timeout path did
out:
mana_hwc_put_msg_index(hwc, msg_id);
while the hardware response was still outstanding, and
mana_hwc_handle_resp() dispatches purely on msg_id, so the next request
could take the same slot and be completed with the previous command's
response and buffer contents.
Should this carry a Fixes: tag and a description of the observable failure?
As posted the fix is bundled with unrelated behavioural changes (admission
timeout, sender draining, SQ posting serialization), so stable maintainers
have nothing to select on.
[ ... ]
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 308950f9b54b0..571a533e62e64 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -468,6 +468,15 @@ struct gdma_context {
> /* Hardware communication channel (HWC) */
> struct gdma_dev hwc;
>
> + /* Sender drain; the final wakeup runs under hwc_lock. */
> + wait_queue_head_t hwc_drain_waitq;
> +
> + /* Protects HWC publication, sender references, and short accesses in
> + * mana_need_log()/mana_serv_reset(). Setup and DMA-region readers
> + * still require lifecycle ordering. Not all timeout writers use it.
> + */
> + spinlock_t hwc_lock;
[Severity: Medium]
Does taking hwc_lock around the hwc_timeout accesses in mana_need_log() and
mana_serv_reset() provide any mutual exclusion, given that the remaining
writers and readers of that field do not take it?
Writers without the lock:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_init_event_handler() {
case HWC_DATA_CFG_HWC_TIMEOUT:
hwc->hwc_timeout = val;
}
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
if (hwc->hwc_timeout > 1)
hwc->hwc_timeout = 1;
}
and mana_gd_verify_vf_version() passes &hwc->hwc_timeout to
mana_gd_query_hwc_timeout(), which writes through it unlocked.
Readers without the lock include wait_ms = hwc->hwc_timeout and the new
admission budget:
if (down_timeout(&hwc->sema, msecs_to_jiffies(hwc->hwc_timeout)))
The first of those writers runs in hard-IRQ context, so
spin_lock_irqsave() in mana_need_log() only masks the local CPU and does
not exclude the same store on another CPU.
There is also an intent problem now that the field is a blocking budget:
the value is sampled before sleeping, so mana_serv_reset() setting
hwc->hwc_timeout = 0;
under hwc_lock, specifically so that nothing waits, cannot shorten a wait
already in progress, and a reconfiguration EQE can restore a long timeout
in the middle of a reset cycle.
Would a single IRQ-safe lock for the field, or an explicit
READ_ONCE()/WRITE_ONCE() policy with defined update precedence, be a better
fit than the current "Not all timeout writers use it"?
>
> /* Azure network adapter */
> struct gdma_dev mana;
>
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908035201.402424-1-longli%40microsoft.com
