Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:
Critical: 0 · High: 3 (3) · Medium: 1 · Low: 2
- [High] The HWC doorbell page index is taken straight from the device
(`gd->doorbell = eq_db.doorbell` in mana_hwc_init_event_handler(),…
- [High] NULL pointer dereference on the new reinit_fallback path.
- [High] The new INVALID_DOORBELL guard is placed after the write it is
supposed to prevent.
- [Medium] Negotiated-dimension validation is not applied on every
establish, contradicting the commit message.
- [Low] Commit message claims the pre-existing 'first establish omits the
doorbell' unchecked-MMIO path is not addressed, while the new…
- [Low] Contradictory comments inside mana_hwc_create_channel(): the
early publish stores gc->hwc.driver_data under gc->hwc_lock with a…
Pre-existing issues:
- [High] mana_hwc_destroy_cq() frees the CQ completion state before the
EQ interrupt is fenced: it does kfree(hwc_cq->comp_buf), then…
- [High] The service-reset path and the PCI PM lifecycle can enter HWC
teardown concurrently.
- [High] Service work is queued on the system workqueue and is never
cancelled or flushed during driver teardown, so it can run against a…
--
net: mana: add dynamic HWC queue depth with reinit path
The HWC is bootstrapped at queue depth 1, then torn down and rebuilt at the
device-reported maximum depth so more management commands can be in flight,
with a fallback back to the bootstrap depth when the rebuild fails. A new
capability flag advertises the support and the negotiated dimensions are
validated before they size DMA allocations.
A few questions on the doorbell handling, the dimension validation and the
fallback ordering below.
>From the commit message:
> Because that reset
> runs on every establish, a firmware that supplied the doorbell on the
> first one but omits it on the second would leave INVALID_DOORBELL
> behind, so refuse the channel in that case rather than let
> mana_gd_ring_doorbell() turn it into an unchecked write far outside the
> BAR. The same write is already reachable without this patch when the
> very first establish omits the doorbell; that is pre-existing and not
> addressed here.
[Severity: Low]
Is this description of the scope accurate? The new check sits in the
common body of mana_hwc_establish_channel(), so it also runs on the first
establish, not just the second one.
What remains unaddressed is something different: the doorbell is rung from
hard IRQ in mana_gd_process_eq_events() before the check can run at all, on
either establish (see the separate comment on the reset hunk). Could the
wording say which part is actually still reachable?
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 91fcf7c092113..83df4f194ee3b 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
[ ... ]
> @@ -797,6 +806,21 @@ static int mana_hwc_establish_channel(struct
> gdma_context *gc, u16 *q_depth,
> struct gdma_queue *cq = hwc->cq->gdma_cq;
> int err;
>
> + /* Clear the values a previous establish left behind so a firmware
> + * that omits an HWC_INIT_DATA_* item on this cycle cannot silently
> + * reuse stale dimensions from the last one. The same applies to the
> + * routing identities: the queues are rebuilt from scratch, so a
> + * doorbell, PDID or PF destination left over from the previous
> + * channel does not describe them.
> + */
> + hwc->hwc_init_q_depth_max = 0;
> + hwc->hwc_init_max_req_msg_size = 0;
> + hwc->hwc_init_max_resp_msg_size = 0;
> + gc->hwc.doorbell = INVALID_DOORBELL;
[Severity: High]
Can this write happen before the guard added below can catch it?
The HWC EQ is armed from hard IRQ while the handshake is still running:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eq_events() {
...
mana_gd_ring_doorbell(gc, eq->gdma_dev->doorbell, eq->type, eq->id,
head, SET_ARM_BIT);
...
}
So the call chain is:
mana_gd_intr() -> mana_gd_process_eq_events() -> mana_gd_ring_doorbell()
If an EQE batch (INIT_DATA, INIT_DONE) is processed before
GDMA_EQE_HWC_INIT_EQ_ID_DB is consumed, gd->doorbell is still the sentinel
this hunk just stored, and mana_gd_ring_doorbell() does writeq() at
gc->db_page_base + gc->db_page_size * 0xffffffff, which is far outside the
ioremap'ed BAR0 window. The guard below only runs after
wait_for_completion_timeout() returns.
On the second establish this also discards a doorbell index that was
already known good. Would keeping (and range-checking) the index be
preferable to clearing it and checking afterwards?
> + gc->hwc.pdid = INVALID_PDID;
> + hwc->pf_dest_vrq_id = 0;
> + hwc->pf_dest_vrcq_id = 0;
> +
> init_completion(&hwc->hwc_init_eqe_comp);
>
> err = mana_smc_setup_hwc(&gc->shm_channel, false,
> @@ -815,6 +839,20 @@ static int mana_hwc_establish_channel(struct
> gdma_context *gc, u16 *q_depth,
> *max_req_msg_size = hwc->hwc_init_max_req_msg_size;
> *max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>
> + /* The doorbell was cleared before the handshake, so a firmware that
> + * signals INIT_DONE without sending GDMA_EQE_HWC_INIT_EQ_ID_DB
> + * leaves INVALID_DOORBELL behind. mana_gd_ring_doorbell() turns
> + * that into gc->db_page_base + gc->db_page_size * 0xffffffff, an
> + * unchecked MMIO write far outside the mapped BAR, and the channel
> + * test below rings it. Everything else the device reports either
> + * fails the dimension checks in mana_hwc_create_channel() or leaves
> + * the queues unable to complete, which that test already catches.
> + */
> + if (gc->hwc.doorbell == INVALID_DOORBELL) {
> + dev_err(hwc->dev, "HWC: no doorbell in init data\n");
> + return -EPROTO;
> + }
[Severity: High]
Does this catch anything other than the omission of
GDMA_EQE_HWC_INIT_EQ_ID_DB?
The doorbell index comes straight from the device in
mana_hwc_init_event_handler():
case GDMA_EQE_HWC_INIT_EQ_ID_DB:
eq_db.as_uint32 = event->details[0];
hwc->cq->gdma_eq->id = eq_db.eq_id;
gd->doorbell = eq_db.doorbell;
union hwc_init_eq_id_db declares doorbell as a 16-bit field, so the device
can never report UINT_MAX; only an omitted EQ_ID_DB produces the sentinel.
An in-field but out-of-BAR value such as 0xffff still reaches:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_ring_doorbell() {
void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index;
...
writeq(e.as_uint64, addr);
}
with a 4 KB doorbell page that is roughly 256 MB past the mapping. The
driver already has the corresponding check on the peer path:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_register_device() {
if (gc->db_page_off + gc->db_page_size * ((u64)resp.db_id + 1) >
gc->bar0_size) {
dev_err(gc->dev, "Doorbell ID %u out of range\n", resp.db_id);
return -EPROTO;
}
}
Would the same range test here work better, given it also subsumes the
sentinel case?
> /* Both were set in mana_hwc_init_event_handler(). */
> if (WARN_ON(cq->id >= gc->max_num_cqs))
> return -EPROTO;
[ ... ]
> @@ -872,13 +916,64 @@ static int mana_hwc_init_queues(struct
> hw_channel_context *hwc, u16 q_depth,
> return err;
> }
>
> +/* Tear down all HWC queues and free associated resources. Used on
> + * the reinit-with-higher-queue-depth path and reinit fallback.
> + *
> + * PRECONDITION: must be called only during channel bring-up in
> + * mana_hwc_create_channel(), before the channel carries traffic:
> + * channel_up is still false, caller_ctx is not yet allocated, the
> + * data path is not probed yet, and active_senders is 0 — so no
> + * request or response user can reach these queues. That is why this
> + * skips the hwc_lock-protected driver_data clear + active_senders
> + * drain that mana_hwc_destroy_channel() needs for the runtime
> + * teardown race; only the CQ-first ordering below (to fence off a
> + * pending interrupt) is required. Bring-up itself runs under the
> + * PCI/PM device_lock, or under GC_IN_SERVICE on the service path;
> + * those two do not exclude each other, so a service reset racing a PM
> + * transition is not serialized — but that is pre-existing and applies
> + * equally to mana_hwc_destroy_channel(), which frees the same
> + * objects. Calling this on a live, published channel would be a
> + * use-after-free.
[Severity: High]
This is a pre-existing issue and not introduced by this patch, but since
the comment documents the gap: can the two teardown contexts free the same
hwc twice?
mana_serv_reset() drives the suspend/resume cycle from a workqueue with no
device_lock held:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_serv_reset() {
spin_unlock_irqrestore(&gc->hwc_lock, flags);
dev_info(&pdev->dev, "MANA reset cycle start\n");
mana_gd_suspend(pdev, PMSG_SUSPEND);
...
}
while .suspend and .remove run under device_lock, and GC_IN_SERVICE only
serializes service jobs against each other. mana_hwc_destroy_channel()
then reads the pointer outside hwc_lock:
struct hw_channel_context *hwc = gc->hwc.driver_data;
if (!hwc)
return;
so both callers can observe the same non-NULL hwc and both proceed to
destroy the CQ/TXQ/RXQ and kfree(hwc). Is something else preventing that?
[Severity: High]
This is also pre-existing and not introduced by this patch, but related to
the service path named in the comment: is the service work ever cancelled
before gc is freed?
mana_schedule_serv_work() puts the item on the system workqueue:
INIT_WORK(&mns_wk->serv_work, mana_serv_func);
schedule_work(&mns_wk->serv_work);
while cleanup only drains the unrelated ordered queue:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_cleanup_device() {
if (gc->service_wq) {
destroy_workqueue(gc->service_wq);
gc->service_wq = NULL;
}
}
and mana_gd_remove() then does vfree(gc). mana_serv_func() ->
mana_do_service() -> mana_serv_reset() takes gc from
pci_get_drvdata(pdev) and dereferences gc->hwc_lock and
gc->hwc.driver_data, so a worker that already read a valid pointer would
operate on freed memory. Would a cancel_work_sync() for the pending item
in the remove path close that?
> + */
> +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc)
> +{
> + struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> +
> + /* Destroy CQ first to deregister the EQ from the interrupt
> + * handler list before freeing caller_ctx, TXQ, or RXQ memory.
> + * A pending interrupt handler could still reach handle_resp()
> + * which dereferences caller_ctx.
> + */
> + if (hwc->cq) {
> + mana_hwc_destroy_cq(gc, hwc->cq);
> + hwc->cq = NULL;
> + }
[Severity: High]
This isn't a bug introduced by this patch, but does mana_hwc_destroy_cq()
actually fence the interrupt before it frees its own state?
mana_hwc_destroy_cq() frees comp_buf and the GDMA CQ first, and only then
the EQ:
drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_cq() {
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);
...
}
but the IRQ removal and the RCU fence only happen in the EQ teardown:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_deregister_irq() {
list_del_rcu(&eq->entry);
...
synchronize_rcu();
}
So a handler already inside its read section via mana_gd_intr() ->
mana_gd_process_eq_events() -> mana_gd_process_eqe() ->
mana_hwc_comp_event() can still touch the freed gdma_cq and the freed
comp_buf. The comment here relies on the CQ-first ordering fencing off a
pending interrupt, which only holds for what is freed after
mana_hwc_destroy_cq() returns. Would destroying/deregistering the EQ
before freeing comp_buf and the CQ be the right order?
> +
> + kfree(hwc->caller_ctx);
> + hwc->caller_ctx = NULL;
[ ... ]
> @@ -926,8 +1021,200 @@ int mana_hwc_create_channel(struct gdma_context *gc)
> goto out;
> }
>
> + /* The channel was bootstrapped at a minimal queue depth. If the
> + * device reports a higher maximum, tear down and rebuild with
> + * the larger depth so more HWC commands can be in flight.
> + */
> + if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) {
[ ... ]
> + if (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE ||
> + max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE ||
> + (u64)q_depth_max * max_req_msg_size >
> + U32_MAX - MANA_PAGE_SIZE ||
> + (u64)q_depth_max * max_resp_msg_size >
> + U32_MAX - MANA_PAGE_SIZE) {
> + dev_err(hwc->dev,
> + "HWC: invalid dims q=%u req=%u resp=%u\n",
> + q_depth_max, max_req_msg_size,
> + max_resp_msg_size);
> + q_depth_max = HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH;
> + goto skip_reinit;
> + }
[Severity: Medium]
The commit message says "Reject a device-reported message size above the
driver maximum", but is that what happens here?
This is the only message-size validation added, and it is nested under
q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH. A device that reports
depth 0 or 1 together with arbitrary or zero HWC_INIT_DATA_MAX_REQUEST /
HWC_INIT_DATA_MAX_RESPONSE values never reaches this check;
mana_hwc_init_event_handler() stores them verbatim and
mana_hwc_establish_channel() hands them back unchecked.
When the depth is larger and the sizes do mismatch, the mismatch is not
rejected either - q_depth_max is reset and the code jumps to skip_reinit,
continuing on the already-established channel.
The backstop the commit message names also does not look like one:
static int mana_hwc_test_channel(struct hw_channel_context *hwc, u16
q_depth,
u32 max_req_msg_size, u32
max_resp_msg_size)
neither max_req_msg_size nor max_resp_msg_size is read in the body, so it
cannot catch a contradictory or omitted size report.
Was returning an error intended for the mismatch case, and should the check
move out of the depth > bootstrap branch?
[ ... ]
> + }
> +
> + goto skip_reinit;
> +
> +reinit_fallback:
> + /* Restore bootstrap-depth channel so the device remains functional.
> + * Free cq_table if it was allocated by a partially successful
> + * establish attempt.
> + */
> + dev_warn(hwc->dev, "HWC reinit failed, falling back to bootstrap
> depth\n");
> +
> + mana_hwc_destroy_queues(hwc);
> +
> + old_cq_table = gc->cq_table;
> + gc->cq_table = NULL;
> + /* Clear the bound with the table, as above. */
> + gc->max_num_cqs = 0;
[Severity: High]
Can this order oops on a NULL gc->cq_table?
mana_hwc_destroy_queues() runs before gc->max_num_cqs is cleared here, and
on this path gc->cq_table was already set to NULL by the reinit branch
above. If the re-establish then failed after
mana_hwc_init_event_handler() stored HWC_INIT_DATA_MAX_NUM_CQS and
HWC_INIT_DATA_CQID from hard IRQ, but before gc->cq_table was allocated,
max_num_cqs is non-zero with a NULL table. The new -EPROTO return for the
missing doorbell, the wait_for_completion_timeout() -ETIMEDOUT return and
the vcalloc() -ENOMEM return all leave that state.
mana_hwc_destroy_queues() -> mana_hwc_destroy_cq() ->
mana_gd_destroy_queue() then reaches:
drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_cq() {
u32 id = queue->id;
if (id >= gc->max_num_cqs)
return;
if (!gc->cq_table[id])
return;
...
}
which passes the bound and dereferences the NULL table. The runtime
teardown avoids this by setting gc->max_num_cqs = 0 before
mana_hwc_destroy_cq(). Would zeroing gc->max_num_cqs and gc->cq_table
before the mana_hwc_destroy_queues() call here (and resetting
gc->max_num_cqs alongside the other stale init fields in
mana_hwc_establish_channel()) be enough?
> + synchronize_rcu();
> + vfree(old_cq_table);
> +
[ ... ]
> +skip_reinit:
> +
> + /* No RCU needed: still in mana_hwc_create_channel, the
> + * pointer has not been published to concurrent senders yet.
> + */
[Severity: Low]
This comment and the publish earlier in the same function seem to disagree.
Earlier in mana_hwc_create_channel():
/* Publish driver_data last, under hwc_lock: the lock orders the hwc
* initialisation above before the pointer becomes visible and
* serialises the publish against the control-plane readers in
* mana_gd_send_request(), mana_need_log() and mana_serv_reset().
*/
spin_lock_irqsave(&gc->hwc_lock, flags);
gc->hwc.driver_data = hwc;
spin_unlock_irqrestore(&gc->hwc_lock, flags);
so the pointer has been published by the time skip_reinit is reached. What
keeps senders out is hwc->channel_up, which mana_hwc_get_msg_index()
tests. Could the comment say that instead?
> err = mana_hwc_test_channel(gc->hwc.driver_data,
> - HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH,
> + hwc->num_inflight_msg,
> max_req_msg_size, max_resp_msg_size);
> if (err) {
> dev_err(hwc->dev, "Failed to test HWC: %d\n", err);
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901200018.3194525-1-longli%40microsoft.com
