Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 (2) · Medium: 2 (2) · Low: 0
- [Medium] Undisclosed behavioural changes bundled into a patch whose
changelog says "No functional change is intended" and never mentions…
- [Medium] The patch adds a guard in mana_gd_query_hwc_timeout()
(gdma_main.c:343) plus an in-tree comment declaring that a device-…
Pre-existing issues:
- [High] mana_hwc_rx_event_handler()
(drivers/net/ethernet/microsoft/mana/hw_channel.c:315-318) returns…
- [High] mana_gd_query_hwc_timeout() sanitises only the zero case of the
device-reported timeout and still accepts any other u32 (`struct…
- [Medium] DISCARDED (fixed later in this series).
- [Medium] DISCARDED (fixed later in this series).
--
net: mana: give each HWC message slot its own completion state
Each HWC message slot gains a per-slot spinlock, a refcount held by the
sender and the response handler, and a responded flag, so the sender's
timeout path and mana_hwc_handle_resp() can own the slot independently.
ctx->error also changes from u32 to int, and mana_gd_query_hwc_timeout()
now ignores a device-reported timeout of zero.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index f92b2d0bf926e..a023d3e1a95de 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -331,7 +331,13 @@ static int mana_gd_query_hwc_timeout(struct pci_dev
> *pdev, u32 *timeout_val)
> if (err || resp.hdr.status)
> return err ? err : -EPROTO;
>
> - *timeout_val = resp.timeout_ms;
> + /* Zero is the driver's own "do not wait, do not log" sentinel, set by
> + * mana_serv_reset() when the HWC has stopped responding. A zero from
> + * the device would enter that state instead: ignore it and keep the
> + * caller's positive value.
> + */
> + if (resp.timeout_ms)
> + *timeout_val = resp.timeout_ms;
>
> return 0;
> }
[Severity: Medium]
The changelog says "No functional change is intended at the current queue
depth of one" and never mentions gdma_main.c, but this hunk changes
behaviour on its own. The caller is:
mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout);
and hwc_timeout == 0 is the driver's own sentinel:
mana_need_log():
if (hwc && hwc->hwc_timeout == 0)
return false;
So a device answering GDMA_QUERY_HWC_TIMEOUT with 0 previously disabled
all HWC waiting and all HWC logging. Is this hunk a fix that wants its
own patch and a Fixes: tag rather than being folded into a preparation
patch?
Two further semantic changes in mana_hwc_send_request() are not described
either. The timeout path now reports a response that raced in after the
wait expired:
if (err != -EINPROGRESS) {
hwc_ctx_put(hwc, ctx);
goto check_status;
}
which returns that response's status (possibly -EPROTO) instead of
-ETIMEDOUT, and skips the hwc->hwc_timeout = 1 clamp. And the new
"!ctx->output_buf || ctx->responded" gate in mana_hwc_handle_resp() stops
a response arriving after its sender timed out from being memcpy'd through
a stale ctx->output_buf that usually pointed into a dead stack frame.
Could the changelog spell these out, or could they be split off?
[Severity: Medium]
The new comment states that a device-supplied value of 0 must be rejected
because 0 is reserved for internal driver state. The other writer of the
same field, fed from the same device-controlled source, is not changed:
mana_hwc_init_event_handler():
case HWC_DATA_CFG_HWC_TIMEOUT:
hwc->hwc_timeout = val;
break;
Can a GDMA_EQE_HWC_SOC_RECONFIG_DATA event carrying HWC_DATA_CFG_HWC_TIMEOUT
with val == 0 install exactly the state this guard is meant to prevent?
msecs_to_jiffies(0) would make every subsequent command expire at once,
mana_hwc_send_request() would take the new "No-wait teardown
(hwc_timeout == 0) is expected to expire" branch for ordinary commands
(skipping both the error log and the clamp), and mana_need_log() would
suppress the diagnostics.
Should the zero check live where hwc_timeout is assigned, so both ingest
points agree?
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 75fdccdc8c482..0056bdd8c53f5 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -19,6 +21,17 @@ static int mana_hwc_get_msg_index(struct
> hw_channel_context *hwc, u16 *msg_id)
> index = find_first_zero_bit(hwc->inflight_msg_res.map,
> hwc->inflight_msg_res.size);
>
> + ctx = &hwc->caller_ctx[index];
> + reinit_completion(&ctx->comp_event);
> + /* Take both references (sender + handle_resp) before publishing the
> + * slot, so an early response cannot free it under the sender.
> + */
> + refcount_set(&ctx->refcnt, 2);
> + ctx->responded = false;
> + 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);
>
> spin_unlock_irqrestore(&r->lock, flags);
> @@ -40,6 +53,13 @@ static void mana_hwc_put_msg_index(struct
> hw_channel_context *hwc, u16 msg_id)
> up(&hwc->sema);
> }
>
> +static void hwc_ctx_put(struct hw_channel_context *hwc,
> + struct hwc_caller_ctx *ctx)
> +{
> + if (refcount_dec_and_test(&ctx->refcnt))
> + mana_hwc_put_msg_index(hwc, ctx->msg_id);
> +}
> +
[ ... ]
> @@ -90,22 +110,35 @@ static void mana_hwc_handle_resp(struct
> hw_channel_context *hwc, u32 resp_len,
> }
>
> ctx = hwc->caller_ctx + msg_id;
> - err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> - if (err)
> - goto out;
>
> - ctx->status_code = resp_msg->status;
> + spin_lock(&ctx->lock);
>
> - memcpy(ctx->output_buf, resp_msg, resp_len);
> -out:
> + /* Honour a response only while the sender owns the slot (output_buf
> + * published) and has not already been answered; otherwise drop it as
> + * premature, stale or duplicate without touching the refcount.
> + */
> + if (!ctx->output_buf || ctx->responded) {
> + spin_unlock(&ctx->lock);
> + mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> + return;
> + }
> + ctx->responded = true;
[Severity: High]
This isn't a bug introduced by this patch, but every drop path in
mana_hwc_handle_resp() now carefully reposts the RX WQE, including the new
premature/stale/duplicate path above and the invalid-bitmap-bit path, while
the caller still has one path that does not:
mana_hwc_rx_event_handler():
msg_id = READ_ONCE(resp->response.hwc_msg_id);
if (msg_id >= hwc->num_inflight_msg) {
dev_err(hwc->dev, "HWC RX: wrong msg_id=%u\n", msg_id);
return;
}
rx_req has already been resolved at that point, and RX WQEs are armed only
once at bring-up:
mana_hwc_test_channel():
for (i = 0; i < q_depth; i++) {
req = &hwc_rxq->msg_buf->reqs[i];
err = mana_hwc_post_rx_wqe(hwc_rxq, req);
afterwards they are only re-armed from mana_hwc_handle_resp(). Since
msg_id comes from the shared DMA response buffer, can a host writing a
value >= hwc->num_inflight_msg drop the only RQ buffer at the bootstrap
depth of one, leaving every later mana_hwc_send_request() to time out
forever? The same early return is still there at the end of the series.
>
> + err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> + if (!err) {
> + ctx->status_code = resp_msg->status;
> + memcpy(ctx->output_buf, resp_msg, resp_len);
> + }
> ctx->error = err;
>
> - /* Must post rx wqe before complete(), otherwise the next rx may
> - * hit no_wqe error.
> + /* Post RX WQE before completing — the next response may arrive
> + * immediately and needs a posted buffer.
> */
> mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> -
> complete(&ctx->comp_event);
> + spin_unlock(&ctx->lock);
> +
> + hwc_ctx_put(hwc, ctx);
> }
[Severity: Medium]
This is a pre-existing ordering issue rather than something this patch
introduces, but the patch adds more accesses after complete().
mana_hwc_destroy_channel() frees the work queues before the CQ/EQ:
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);
and only mana_hwc_destroy_cq() reaches mana_gd_deregiser_irq(), whose
list_del_rcu() plus synchronize_rcu() fences off an in-flight EQ handler.
hwc->rxq and hwc->txq are not set to NULL either.
If mana_smc_teardown_hwc() failed and the device still delivers a response
in that window, can mana_hwc_post_rx_wqe(hwc->rxq, rx_req) here touch the
freed hwc_wq and msg_buf? The hwc_ctx_put(hwc, ctx) added after complete()
also touches ctx->refcnt, ctx->msg_id, hwc->inflight_msg_res and hwc->sema.
For the record, the destroy order is corrected later in this series, where
the CQ is destroyed before the TXQ and RQ after channel_up is cleared and
active_senders drained, so this comment only applies to the intermediate
state at this commit.
[ ... ]
> @@ -902,43 +952,108 @@ int mana_hwc_send_request(struct hw_channel_context
> *hwc, u32 req_len,
> dest_vrcq = hwc->pf_dest_vrcq_id;
> }
>
> + /* The response-side reference (from get_msg_index) keeps the slot
> + * alive if hardware responds right after the doorbell.
> + */
> err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
> if (err) {
> dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
> goto out;
> }
>
> + wait_ms = hwc->hwc_timeout;
> if (!wait_for_completion_timeout(&ctx->comp_event,
> - (msecs_to_jiffies(hwc->hwc_timeout))))
> {
> + msecs_to_jiffies(wait_ms))) {
[Severity: High]
This problem wasn't introduced by this patch, but the patch hardens the
same assignment against the benign value while leaving the damaging one
unchecked. resp.timeout_ms is a plain u32 and lands directly in
hwc->hwc_timeout, so wait_ms here is fully device-controlled.
__msecs_to_jiffies() returns MAX_JIFFY_OFFSET for any value whose signed
interpretation is negative, i.e. >= 0x80000000, and
wait_for_completion_timeout() sleeps in TASK_UNINTERRUPTIBLE.
Can a device that reports timeout_ms = 0xffffffff and then stops answering
wedge the calling thread indefinitely, with no way to kill it? Even
0x7fffffff gives roughly a 24 day wait, and HWC commands are issued from
paths holding rtnl_lock. The event path in mana_hwc_init_event_handler()
stores up to 0xffffff ms unbounded as well. Would a clamp against a sane
maximum at both writers be appropriate? The gap is still present at the
end of the series.
[ ... ]
> out:
> - mana_hwc_put_msg_index(hwc, msg_id);
> + /* Error, no-wait teardown, or timeout: drop the sender's and the
> + * response-side references. Latch ->responded so a racing response
> + * is a no-op, and only drop the response-side ref if it has not.
> + */
> + ctx = hwc->caller_ctx + msg_id;
> + spin_lock_irqsave(&ctx->lock, flags);
> + ctx->output_buf = NULL;
> + drop_resp_ref = !ctx->responded;
> + ctx->responded = true;
> + spin_unlock_irqrestore(&ctx->lock, flags);
> + if (drop_resp_ref)
> + refcount_dec(&ctx->refcnt);
> + hwc_ctx_put(hwc, ctx);
> +done:
> return err;
> }
[Severity: Medium]
This is pre-existing behaviour rather than something the patch introduces,
and the changelog already acknowledges it, but it may be worth spelling out
what happens in this intermediate state.
On a genuine timeout this path drops both references, so
mana_hwc_put_msg_index() clears the bitmap bit and posts the semaphore
while a response for that msg_id may still be in flight.
mana_hwc_get_msg_index() then hands the same index (always 0 at depth one)
to the next command and resets the slot:
refcount_set(&ctx->refcnt, 2);
ctx->responded = false;
so the old response passes the gate in mana_hwc_handle_resp():
if (!ctx->output_buf || ctx->responded) {
Does that mean the stale payload is memcpy'd into the new caller's buffer,
ctx->error set to 0, and complete() waking the new sender, which returns
success carrying another command's response?
The next patch in the series keeps the response-side reference across a
timeout so the slot is never handed out again until the device answers,
which removes this window; the note is only about the state at this commit.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901200018.3194525-1-longli%40microsoft.com
