Re: [PATCH net-next v4 2/4] net: mana: give each HWC message slot its own completion state

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.

[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?

[ ... ]

[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.

[ ... ]

[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