> 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: fix stale HWC response after command timeout
>
> This reworks the MANA HWC command slot lifetime so a late response cannot
> memcpy into a caller buffer that is already gone, and cannot complete a
> different command through a recycled msg_id. Each caller_ctx gains a
> spinlock,
> a refcount and an -EINPROGRESS sentinel, the semaphore is replaced by a
> waitqueue plus the inflight bitmap, and a first timeout latches a new
> hwc_timed_out flag.
>
> A few questions about the new slot lifetime and the timeout latch below.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > index 1603968d7989a..d92032b466af2 100644
> > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> > @@ -7,25 +7,49 @@
> > #include <linux/pci.h>
> > #include <linux/vmalloc.h>
> >
> > +/* Acquire a free message slot from the inflight bitmap. Returns
> > + * -ETIMEDOUT if a prior HWC command has timed out (preserving the
> > + * error code callers expect).
> > + */
> > static int mana_hwc_get_msg_index(struct hw_channel_context *hwc, u16
> > *msg_id) {
> > struct gdma_resource *r = &hwc->inflight_msg_res;
> > unsigned long flags;
> > u32 index;
> >
> > - down(&hwc->sema);
> > + for (;;) {
> > + spin_lock_irqsave(&r->lock, flags);
> >
> > - spin_lock_irqsave(&r->lock, flags);
> > + if (hwc->hwc_timed_out) {
> > + spin_unlock_irqrestore(&r->lock, flags);
> > + return -ETIMEDOUT;
> > + }
> >
> > - index = find_first_zero_bit(hwc->inflight_msg_res.map,
> > - hwc->inflight_msg_res.size);
> > + index = find_first_zero_bit(r->map, r->size);
> > + if (index < r->size) {
> > + struct hwc_caller_ctx *ctx;
> > +
> > + bitmap_set(r->map, index, 1);
> > + ctx = &hwc->caller_ctx[index];
> > + reinit_completion(&ctx->comp_event);
> > + refcount_set(&ctx->refcnt, 1);
> > + ctx->responded = false;
> > + ctx->msg_id = index;
> > + ctx->error = -EINPROGRESS;
> > + spin_unlock_irqrestore(&r->lock, flags);
> > + break;
> > + }
>
> [Severity: High]
> bitmap_set() publishes the slot to the response path here, while the response-
> side reference is only taken much later in
> mana_hwc_send_request():
>
> /* Take handle_resp's ref before posting ... */
> refcount_inc(&ctx->refcnt);
>
> In between, mana_hwc_handle_resp() already accepts the slot. Its only gates
> are the range check and a lockless test_bit():
>
> if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
> dev_err(hwc->dev, "hwc_rx: invalid msg_id = %u\n", msg_id);
> mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> return;
> }
>
> and it ends with an unconditional hwc_ctx_put(). msg_id comes from host-
> writable DMA memory (READ_ONCE()d out of the RX buffer), and responded is
> false for a fresh allocation, so the new duplicate guard does not catch a
> response naming a slot that is still in this pre-post window.
>
> Can such a response drop refcnt 1->0, clear the bitmap bit and wake msg_waitq
> while the sender still owns the slot? If so, does that give:
>
> - the sender's refcount_inc() running on 0, i.e. a "refcount_t:
> addition on 0; use-after-free" splat plus permanent saturation,
> leaking that slot for good;
>
> - another sender being handed the same msg_id, sharing both
> txq->msg_buf->reqs[msg_id] and the same hwc_caller_ctx, so one
> command completes with the other command's payload, which is the
> cross-completion this patch is meant to remove;
>
> - a spurious complete() letting wait_for_completion_timeout() succeed
> with ctx->error still -EINPROGRESS, so -115 is handed back to
> callers?
>
> The same window also covers the req_len > tx_wr->buf_len path that jumps to
> out:, whose new comment states "no WQE was submitted so handle_resp()
> cannot race here" and "refcount is 1". Can the slot already have been
> released
> by then, making that hwc_ctx_put() a second release?
>
> [Severity: Medium]
> comp_event, refcnt, responded, msg_id and error are (re)initialised here under
> inflight_msg_res.lock only, while mana_hwc_handle_resp() reads and writes
> responded and error under ctx->lock only and never takes
> inflight_msg_res.lock.
> The intersection of the two locksets is empty, even though the header comment
> names ctx->lock as the protecting lock.
>
> bitmap_set() runs before these field writes in the same critical section and
> handle_resp()'s visibility gate is a lockless test_bit(), so can a CQ
> interrupt on
> another CPU observe the bit, take ctx->lock and interleave with this
> initialisation? It could then see the stale responded == true from the
> previous
> generation and silently drop a real response, or set responded/error and
> complete() while they are being reset, after which its hwc_ctx_put() consumes
> the new generation's initial reference.
>
> reinit_completion() also writes x->done = 0 without the completion's
> wait.lock,
> while handle_resp() calls complete() under ctx->lock. Can a completion be
> lost
> or spuriously observed that way?
>
> > + spin_unlock_irqrestore(&r->lock, flags);
> >
> > - bitmap_set(hwc->inflight_msg_res.map, index, 1);
> > + wait_event(hwc->msg_waitq,
> > + hwc->hwc_timed_out ||
> > + !bitmap_full(r->map, r->size));
> >
> > - spin_unlock_irqrestore(&r->lock, flags);
> > + if (hwc->hwc_timed_out)
> > + return -ETIMEDOUT;
> > + }
>
> [Severity: Low]
> hwc_timed_out is written under inflight_msg_res.lock in
> mana_hwc_send_request(), but read as a plain load both in this
> wait_event() condition and in the check right after it. bitmap_full() also
> reads
> r->map outside r->lock while other CPUs modify the same words with non-
> atomic bitmap_set()/bitmap_clear(). Would READ_ONCE()/
> WRITE_ONCE() (or evaluating the bitmap under r->lock) be preferable here to
> keep KCSAN quiet?
>
> Related: the read-modify-write in the timeout path
>
> if (hwc->hwc_timeout > 1)
> hwc->hwc_timeout = 1;
>
> is unlocked while mana_hwc_init_event_handler() also writes hwc_timeout
> from EQ interrupt context. That access is not introduced by this patch, it
> is in
> the baseline, but the patch makes the value load-bearing since a zero or
> degraded value now latches the whole channel.
>
> >
> > *msg_id = index;
> > -
> > return 0;
> > }
> >
> > @@ -35,10 +59,17 @@ static void mana_hwc_put_msg_index(struct
> hw_channel_context *hwc, u16 msg_id)
> > unsigned long flags;
> >
> > spin_lock_irqsave(&r->lock, flags);
> > - bitmap_clear(hwc->inflight_msg_res.map, msg_id, 1);
> > + bitmap_clear(r->map, msg_id, 1);
> > spin_unlock_irqrestore(&r->lock, flags);
> >
> > - up(&hwc->sema);
> > + wake_up(&hwc->msg_waitq);
> > +}
> > +
> > +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);
> > }
> >
> > static int mana_hwc_verify_resp_msg(const struct hwc_caller_ctx
> > *caller_ctx, @@ -114,22 +145,44 @@ static void
> mana_hwc_handle_resp(struct hw_channel_context *hwc, u32 resp_len,
> > resp_len = 0;
> > }
> >
> > - err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> > - if (err)
> > - goto out;
> > + spin_lock(&ctx->lock);
> >
> > - ctx->status_code = resp_msg->status;
> > + if (ctx->responded) {
> > + /* A response for this slot was already delivered; this is a
> > + * duplicate or replayed one. Drop it so the hwc_ctx_put()
> > + * a first response performs is not done twice, which would
> > + * over-release the slot while the sender still owns it.
> > + */
> > + spin_unlock(&ctx->lock);
> > + mana_hwc_post_rx_wqe(hwc->rxq, rx_req);
> > + return;
> > + }
> > + ctx->responded = true;
> >
> > - memcpy(ctx->output_buf, resp_msg, resp_len);
> > -out:
> > - ctx->error = err;
> > + err = mana_hwc_verify_resp_msg(ctx, resp_msg, resp_len);
> >
> > - /* Must post rx wqe before complete(), otherwise the next rx may
> > - * hit no_wqe error.
> > + if (!err && ctx->output_buf) {
> > + ctx->status_code = resp_msg->status;
> > + memcpy(ctx->output_buf, resp_msg, resp_len);
> > + ctx->error = 0;
> > + } else if (ctx->output_buf) {
> > + /* Only overwrite error if the sender hasn't timed out
> > + * or been force-completed by destroy. When output_buf
> > + * is NULL, a terminal error (-ENODEV or timeout) has
> > + * already been set — preserve it so the sender doesn't
> > + * see a spurious success.
> > + */
> > + ctx->error = err;
> > + }
>
> [Severity: Low]
> Is this comment accurate? It says that when output_buf is NULL "a terminal
> error (-ENODEV or timeout) has already been set".
>
> On the timeout path mana_hwc_send_request() only NULLs ctx->output_buf
> and assigns its local err; ctx->error keeps the -EINPROGRESS sentinel written
> by
> mana_hwc_get_msg_index(). Grepping drivers/net/ethernet/microsoft, the
> only ENODEV in hw_channel.c is inside this comment.
>
> The comment also mentions senders "force-completed by destroy", but
> mana_hwc_destroy_channel() tears down the CQ and WQs and kfree()s
> caller_ctx without ever completing ctx->comp_event or writing ctx->error.
>
> Could the comment be reworded to describe what the code actually
> guarantees?
>
> > +
> > + /* 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);
> > }
> >
> > static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue
> > *q_self, @@ -216,7 +269,12 @@ static void
> > mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> >
> > switch (type) {
> > case HWC_DATA_CFG_HWC_TIMEOUT:
> > - hwc->hwc_timeout = val;
> > + /* A zero timeout would make every command time
> out
> > + * immediately and latch hwc_timed_out, disabling the
> > + * channel. Ignore it and keep the positive default.
> > + */
> > + if (val)
> > + hwc->hwc_timeout = val;
> > break;
>
> [Severity: Medium]
> This guard covers the reconfig EQE, but is the other device-supplied source of
> the same field covered? mana_gd_query_hwc_timeout() still
> does:
>
> *timeout_val = resp.timeout_ms;
>
> and mana_gd_verify_vf_version() calls it as
> mana_gd_query_hwc_timeout(pdev, &hwc->hwc_timeout), so a device-
> reported timeout_ms of 0 reaches hwc->hwc_timeout unfiltered.
>
> With the new code, does the next command then wait msecs_to_jiffies(0), time
> out immediately and latch hwc_timed_out, after which every command fails
> with -ETIMEDOUT out of mana_hwc_get_msg_index() without being posted, for
> the remaining life of the channel?
>
> >
> > case HWC_DATA_HW_LINK_CONNECT:
> > @@ -708,7 +766,7 @@ static int mana_hwc_init_inflight_msg(struct
> > hw_channel_context *hwc, {
> > int err;
> >
> > - sema_init(&hwc->sema, num_msg);
> > + init_waitqueue_head(&hwc->msg_waitq);
>
> [Severity: Low]
> The commit message says:
>
> Replace the depth-1 semaphore with a waitqueue + bitmap so a slot held
> past a timeout does not deadlock admission and timed-out waiters can be
> released.
>
> Was the semaphore depth-1? The removed line is sema_init(&hwc->sema,
> num_msg), and mana_hwc_init_queues() calls
> mana_hwc_init_inflight_msg(hwc, q_depth), so it admitted up to q_depth
> concurrent senders, matching the bitmap size.
>
> The reason a counting semaphore no longer fits looks like the fact that a
> bitmap
> bit can now outlive its sender (held by handle_resp's reference), which
> desynchronises the semaphore count from bitmap occupancy. Could the
> message be corrected, given this is a Fixes:-tagged patch headed for stable?
>
> >
> > err = mana_gd_alloc_res_map(num_msg, &hwc->inflight_msg_res);
> > if (err)
>
> [ ... ]
>
> > @@ -999,13 +1062,17 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
> > struct hwc_wq *txq = hwc->txq;
> > struct gdma_req_hdr *req_msg;
> > struct hwc_caller_ctx *ctx;
> > + unsigned long flags;
> > u32 dest_vrcq = 0;
> > u32 dest_vrq = 0;
> > u32 command;
> > + u32 status;
> > u16 msg_id;
> > int err;
> >
> > - mana_hwc_get_msg_index(hwc, &msg_id);
> > + err = mana_hwc_get_msg_index(hwc, &msg_id);
> > + if (err)
> > + return err;
> >
> > tx_wr = &txq->msg_buf->reqs[msg_id];
> >
>
> [ ... ]
>
> > @@ -1034,8 +1104,14 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
> > dest_vrcq = hwc->pf_dest_vrcq_id;
> > }
> >
> > + /* Take handle_resp's ref before posting — hardware can respond
> > + * immediately after the doorbell ring.
> > + */
> > + refcount_inc(&ctx->refcnt);
> > +
> > err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
> > if (err) {
> > + refcount_dec(&ctx->refcnt);
> > dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n",
> err);
> > goto out;
> > }
> > @@ -1046,31 +1122,86 @@ int mana_hwc_send_request(struct
> hw_channel_context *hwc, u32 req_len,
> > dev_err(hwc->dev, "Command 0x%x timed out: %u
> ms\n",
> > command, hwc->hwc_timeout);
> >
> > - /* Reduce further waiting if HWC no response */
> > + /* NULL out output_buf so a late handle_resp() won't write
> > + * into the caller's buffer after the sender returns, then
> > + * check whether handle_resp() already delivered a valid
> > + * response between the timeout firing and this lock
> > + * acquisition — ctx->error != -EINPROGRESS means it ran.
> > + */
> > + spin_lock_irqsave(&ctx->lock, flags);
> > + ctx->output_buf = NULL;
> > + err = ctx->error;
> > + status = ctx->status_code;
> > + spin_unlock_irqrestore(&ctx->lock, flags);
> > +
> > + if (err != -EINPROGRESS) {
> > + /* handle_resp() delivered a valid response just after
> > + * the timeout fired. The hardware is alive, so use
> > + * the response and leave the channel usable; do not
> > + * latch hwc_timed_out or degrade hwc_timeout for
> what
> > + * turned out to be a transient race.
> > + */
> > + hwc_ctx_put(hwc, ctx);
> > + goto check_status;
> > + }
> > +
> > + /* Genuine timeout: no response arrived. Reduce further
> > + * waiting, and mark the channel timed out under the bitmap
> > + * lock so get_msg_index() cannot acquire new slots after this.
> > + */
> > if (hwc->hwc_timeout > 1)
> > hwc->hwc_timeout = 1;
> >
> > + spin_lock_irqsave(&hwc->inflight_msg_res.lock, flags);
> > + hwc->hwc_timed_out = true;
> > + spin_unlock_irqrestore(&hwc->inflight_msg_res.lock, flags);
> > + wake_up_all(&hwc->msg_waitq);
>
> [Severity: High]
> Once hwc_timed_out is set here, is anything expected to clear it?
> Grepping drivers/net/ethernet/microsoft and include/net/mana, this is the only
> write besides the kzalloc zeroing, so the flag stays set for the life of the
> hw_channel_context.
>
> From then on mana_hwc_get_msg_index() bails out before a slot is even
> allocated:
>
> if (hwc->hwc_timed_out) {
> spin_unlock_irqrestore(&r->lock, flags);
> return -ETIMEDOUT;
> }
>
> so mana_hwc_post_tx_wqe() is never reached and no doorbell is rung for any
> later command. Every GDMA control command funnels through
> mana_gd_send_request()->mana_hwc_send_request(), including the teardown
> commands mana_gd_disable_queue(), mana_gd_destroy_dma_region() and
> mana_gd_deregister_device(), all of which treat errors as non-fatal and then
> dma_free_coherent() the backing pages anyway.
>
> Before this patch those commands were still built, posted and the doorbell
> rung
> (only the wait was shortened to 1 ms), so the device did act on destroy and
> disable requests. Can this leave the device holding registered DMA regions
> that
> point at freed pages, which is the situation the comment in
> mana_hwc_destroy_channel() describes as risking memory corruption on
> systems without an IOMMU?
>
> The commit message describes the latch only as:
>
> On a genuine timeout the channel is marked hwc_timed_out and further
> mana_hwc_get_msg_index() callers fail with -ETIMEDOUT instead of
> reusing a slot whose response may still arrive.
>
> Could it state that the latch is channel-wide, unconditional and never
> cleared,
> given the per-slot refcount already prevents reuse of a msg_id whose response
> is outstanding?
>
> [Severity: High]
> Is the driver's own zero-timeout mode affected here too? mana_serv_reset() in
> gdma_main.c does:
>
> /* HWC is not responding in this case, so don't wait */
> hwc->hwc_timeout = 0;
>
> dev_info(&pdev->dev, "MANA reset cycle start\n");
>
> mana_gd_suspend(pdev, PMSG_SUSPEND);
>
> and mana_gd_suspend() runs mana_rdma_remove() (which sends
> mana_gd_deregister_device()), mana_remove(&gc->mana, true) and
> mana_gd_cleanup_device(), all of which issue HWC commands.
>
> For the first of those, wait_for_completion_timeout() with
> msecs_to_jiffies(0) returns 0, ctx->error is still -EINPROGRESS, so this
> branch
> runs: hwc->hwc_timeout > 1 is false and hwc_timed_out is set unconditionally.
> Do the remaining teardown commands then abort in
> mana_hwc_get_msg_index() without ever reaching the SQ, turning the
> intended fire-and-forget teardown into no teardown at all while the buffers
> are
> still freed?
>
> The patch already guards the device-reported value with if (val) for this
> exact
> reason. Should the in-driver zero assignment be handled as well, for instance
> by not latching when hwc_timeout is 0, since mana_need_log() already treats
> hwc_timeout == 0 as a legitimate "don't wait, don't log"
> state?
>
> >
> > err = -ETIMEDOUT;
> > - goto out;
> > + hwc_ctx_put(hwc, ctx);
> > + goto done;
> > }
> >
> > - if (ctx->error) {
> > - err = ctx->error;
> > - goto out;
> > - }
> > + /* NULL output_buf so a late handle_resp() won't memcpy into
> > + * the caller's buffer after the sender exits. Read error and
> > + * status_code under the same lock — after hwc_ctx_put the slot
> > + * may be reused and these fields overwritten.
> > + */
> > + spin_lock_irqsave(&ctx->lock, flags);
> > + ctx->output_buf = NULL;
> > + err = ctx->error;
> > + status = ctx->status_code;
> > + spin_unlock_irqrestore(&ctx->lock, flags);
> > + hwc_ctx_put(hwc, ctx);
> > +
> > +check_status:
> > + if (err)
> > + goto done;
>
> [ ... ]
>
> > +
> > + err = 0;
> > + goto done;
> > out:
> > - mana_hwc_put_msg_index(hwc, msg_id);
> > + /* Pre-post error paths: no WQE was submitted so handle_resp()
> > + * cannot race here. refcount is 1 (no second ref taken).
> > + */
> > + ctx = hwc->caller_ctx + msg_id;
> > + spin_lock_irqsave(&ctx->lock, flags);
> > + ctx->output_buf = NULL;
> > + spin_unlock_irqrestore(&ctx->lock, flags);
> > + hwc_ctx_put(hwc, ctx);
> > +done:
> > return err;
> > }
> > diff --git a/include/net/mana/hw_channel.h
> > b/include/net/mana/hw_channel.h index 3d8543acb5cc0..5a55cedf0607a
> > 100644
> > --- a/include/net/mana/hw_channel.h
> > +++ b/include/net/mana/hw_channel.h
> > @@ -173,6 +173,23 @@ struct hwc_caller_ctx {
> >
> > u32 error; /* Linux error code */
> > u32 status_code;
>
> [Severity: Low]
> error stays declared as u32 while mana_hwc_get_msg_index() now stores
> ctx->error = -EINPROGRESS into it, and mana_hwc_send_request() reads it
> back into an int and compares it as signed with if (err != -EINPROGRESS).
> The decision whether to latch the whole channel therefore depends on an out-
> of-range u32 to int conversion rather than on the declared type.
>
> Should the field become int error, matching its own "Linux error code"
> comment, or the sentinel be made unsigned-safe?
>
> > +
> > + /* Protects output_buf against concurrent access from
> > + * handle_resp() (CQ interrupt) and the sender timeout path.
> > + */
> > + spinlock_t lock;
> > +
> > + /* Tracks sender + handle_resp ownership. The last put
> > + * (refcount reaches 0) releases the bitmap slot.
> > + */
> > + refcount_t refcnt;
> > + u16 msg_id;
> > +
> > + /* Set under lock by the first handle_resp() for this slot so a
> > + * duplicate or replayed response is dropped instead of consuming
> > + * the response-side reference a second time.
> > + */
> > + bool responded;
> > };
> >
> > struct hw_channel_context {
> > @@ -193,13 +210,19 @@ struct hw_channel_context {
> > u32 hwc_timeout;
> >
> > + /* Set on first HWC timeout. Causes get_msg_index() to return
> > + * -ETIMEDOUT instead of waiting, draining all queued senders.
> > + */
> > + bool hwc_timed_out;
> > +
>
> Could this comment also note that the state is never cleared, so it is
> terminal
> for the lifetime of the channel?
I will send v4 to address the comments. v4 also adds a 7th patch, "keep
max_num_cqs immutable once cq_table is allocated", referenced below.
There are the summary of v4 addressing / not addressing the comments:
Legend: [FIX] fixed in v4, [DESIGN] intentional/won't change,
[PRE] pre-existing (not introduced by this series).
== Patch 1: RCU-protect gc->cq_table ==
[FIX] "Per-CQ blocking grace periods added to hot teardown paths ...
mana_gd_destroy_cq() now ends with an unconditional synchronize_rcu()":
the netdev teardown paths will be reworked into a two-pass quiesce/free
that takes a single grace period instead of one per CQ.
[FIX] "The bound that guards every cq_table index lives outside the
RCU-published object" / "A malicious host can trigger out-of-bounds ...
by dynamically inflating gc->max_num_cqs": v4 snapshots cq->id and
max_num_cqs with READ_ONCE() so the same value sizes, bounds and indexes
the table, and a new patch keeps max_num_cqs immutable once cq_table is
allocated.
[FIX] "two *separate* loads of the same interrupt-mutable, device-
controlled values": collapsed to single READ_ONCE() snapshots.
[FIX] "read gc->cq_table with rcu_dereference_protected(..., true)" /
gdma.h lifetime comment: the gdma.h comment will be corrected to state
the actual lifetime rule (base pointer is written only at establish and
teardown, so the "true" predicate is sound).
[FIX] "Pre-existing Use-After-Free in HWC channel teardown due to
inverted destruction order": addressed by the teardown-ordering rework
(see P3/P5 below).
== Patch 2: fix HWC RQ/SQ buffer size swap ==
[FIX] "Commit message overstates the impact ... describes a reachable
buffer overflow": changelog reworded as a latent-correctness fix (both
sizes are 0x1000, so no overflow is observable).
[FIX] "the queue dimensions are hoisted above mana_hwc_create_cq()":
the hoist and its comment are dropped; the assignments stay at the end
of mana_hwc_init_queues().
(gemini's "OOB read from inline_oob_size_div4", "UAF during teardown",
and "race/UAF in send_request timeout" against this patch are addressed
in P4/P5/P6 respectively; see below.)
== Patch 3: free HWC comp_buf after destroying the EQ ==
[FIX] "hwc->txq and hwc->rxq ... are freed by mana_hwc_destroy_wq()
BEFORE mana_hwc_destroy_cq() unpublishes" / "Destroying the TX and RX
queues before the CQ and EQ ... causes a use-after-free": v4 destroys
the CQ first (which tears down the EQ + IRQ and unpublishes the
cq_table slot with a grace period) before freeing the RQ/TX WQs.
[FIX] "the comment ... misdescribe[s] the fencing mechanism": the
comment is corrected to describe the EQ-destroy -> IRQ-deregister ->
synchronize_rcu fence accurately.
== Patch 4: validate hardware-supplied values in the HWC RX path ==
[FIX] "short-response gate ... drops the completion without waking the
pending sender, turning a prompt -EPROTO ... into a full-timeout hang"
/ "a timeout ... can lead to stack corruption and use-after-free": the
short-response early return is removed; a malformed response reaches
verify_resp_msg() -> -EPROTO and completes the sender.
[FIX] "Unbounded, unrecoverable RX WQE (RQ credit) leak": the failure
paths now account leaked WQEs and trip hwc_timeout on RQ exhaustion.
[FIX] "a plain (non-READ_ONCE) load of device/host-writable DMA memory"
/ "SGE ... validates a much weaker invariant ... corrupted
inline_oob_size_div4": v4 snapshots inline_oob_size_div4 (read once
through its u32 flags word, since it is a bit-field) and sge->address
with READ_ONCE(), and rejects any value other than the exact one the
driver programs.
[FIX] "Commit message claims an out-of-bounds indexing fix ... that
does not exist": the msg_id check is reframed in the changelog as
defense in depth.
== Patch 5: HWC teardown safety (setup_active + destroy ordering) ==
[FIX] "makes setup_active the definitive signal while still arming it
too late": setup_active is armed immediately after mana_smc_setup_hwc()
succeeds.
[FIX] "Redundant HWC teardown in mana_hwc_establish_channel() causes a
double hardware timeout ... and masks the original error code": the
redundant teardown is removed; teardown happens once via the caller.
[FIX] "the CQ is destroyed and its memory freed before the EQ is
deregistered ... Use-After-Free and WARN splats": the EQ is destroyed
(IRQ deregistered, in-flight handlers drained) before the CQ.
[FIX] "removes the only reset of gc->max_num_cqs and codifies ... that
it is 'Set once'": complemented by the new immutability patch; the
field is never reset and never re-inflated.
[FIX] "An early return on HWC teardown failure leaves a stale pointer
in gc->cq_table, which a malicious host can exploit to cause an out-of-
bounds read": on teardown failure the HWC and its CQ are leaked (not
freed), so the cq_table slot is not dangling, and max_num_cqs
immutability prevents any OOB index. No UAF/OOB results.
[DESIGN] "mana_hwc_destroy_channel() ... returns early when
mana_smc_teardown_hwc() fails [and] skips the entire tail" / "the leak
path abandons that instance": intentional. If teardown fails the
device may still DMA into those buffers; freeing them without an IOMMU
to fault the stale DMA risks memory corruption, so the resources are
leaked on purpose (the code says so and keeps setup_active set).
== Patch 6: fix stale HWC response after command timeout ==
[FIX] "Refcount underflow / premature slot release: the response-side
reference is taken late" / "a maliciously early hardware response can
prematurely free the message slot before the sender takes its
reference": v4 takes both references up front in
mana_hwc_get_msg_index() (refcount initialised to 2), under the same
lock that publishes the slot, so an early/stale/forged response drops
only one reference (2->1) and cannot free the slot under the sender.
The pre-post error path latches ->responded so it cannot double-drop.
[FIX] "zero-value guard for hwc->hwc_timeout only in the ... reconfig
handler": v4 rejects a device-supplied zero in the query path too.
[FIX] "hwc_timed_out is written under inflight_msg_res.lock ... but
read as a plain load": all accesses use READ_ONCE()/WRITE_ONCE().
[FIX] "u32 error; /* Linux error code */ ... stores the negative
sentinel": caller_ctx::error is changed from u32 to int so it holds the
negative -EINPROGRESS sentinel and errno values directly.
[FIX] "the output_buf == NULL comment ... is inaccurate" and "'Replace
the depth-1 semaphore ...'": the comment and the changelog wording are
corrected.
[FIX + PRE] "Lockset mismatch: mana_hwc_get_msg_index() initialises
per-slot state under inflight_msg_res.lock, while handle_resp() uses
ctx->lock; the gate is a lockless test_bit()": the dangerous
consequence you flagged (a raced handle_resp releasing a slot the new
sender still owns) is eliminated by the refcount-initialised-to-2 change
above. The remaining consequences (dropping a stale response, lost
completion) require a duplicate/forged/unsolicited response, i.e. the
pre-existing msg_id-reuse (ABA) window; the honest in-order path never
races the initialiser. Fully closing the ABA window needs a per-request
generation token, which is a larger, separable change I plan to send on
top rather than fold in here.
[DESIGN] "Terminal, channel-wide hwc_timed_out latch": intentional.
Once the HWC stops responding the channel is unusable and further
commands should fail fast. v4 only narrows the false positive: a valid
response racing the timeout (ctx->error != -EINPROGRESS) no longer
latches the channel.
[DESIGN] "mana_serv_reset() deliberately sets hwc->hwc_timeout = 0 ...
and then runs the whole suspend/teardown sequence": also intentional --
there the HWC is known unresponsive and the driver must not wait. This
is distinct from a device-supplied zero, which is now rejected.
All changes were built per-commit (mana + mana_ib, W=0 E=0), checked
with sparse C=1 and checkpatch, and boot/traffic tested on Azure MANA
hardware.
Thanks,
Long
