Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] Capping the hardware-reported HWC queue depth without negotiating it with the host causes a fatal ring buffer size mismatch, leading to out-of-bounds memory accesses. Pre-existing issues: - [High] CQ queue memory is freed without waiting for an RCU grace period, risking use-after-free in the IRQ handler. - [High] Use-After-Free during channel teardown due to incorrect queue destruction order. -- commit e9ca1d1d51e325f4557f02fd7fcfac8f36e41ace Author: Long Li <[email protected]> net: mana: add dynamic HWC queue depth with reinit path Rebuild depth-one HWC queues at the advertised maximum, capped at 128. Preserve the 24-bit depth in u32, bound allocations and round message buffers to a power of two. Advertise support in the driver-version request. > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index eb88bae2b14d8..78424e1d884f1 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > @@ -1260,15 +1260,17 @@ static void mana_gd_create_cq(const struct > gdma_queue_spec *spec, > static void mana_gd_destroy_cq(struct gdma_context *gc, > struct gdma_queue *queue) > { > + struct gdma_queue **cq_table = READ_ONCE(gc->cq_table); > u32 id = queue->id; > > - if (id >= gc->max_num_cqs) > + /* HWC re-establishment can fail before allocating the CQ table. */ > + if (!cq_table || id >= gc->max_num_cqs) > return; > > - if (!gc->cq_table[id]) > + if (!cq_table[id]) > return; > > - gc->cq_table[id] = NULL; > + cq_table[id] = NULL; [Severity: High] This isn't a bug introduced by this patch, but can this lead to a use-after-free in the EQ interrupt handler? The CQ memory is freed by the caller (mana_gd_destroy_queue()) immediately after mana_gd_destroy_cq() unpublishes it from the table, without waiting for an RCU grace period. Concurrently, the EQ interrupt processor (mana_gd_process_eqe()) runs under rcu_read_lock(), reads the CQ pointer from the table, and dereferences it. If the CQ is freed during this window, the interrupt handler will access freed memory. Should this wait for an RCU grace period before freeing the queue? > } > > int mana_gd_create_hwc_queue(struct gdma_dev *gd, > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index a4f7346d285f7..750ee8f729994 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > @@ -846,13 +878,44 @@ static int mana_hwc_init_queues(struct > hw_channel_context *hwc, u16 q_depth, > return err; > } > > +/* Bring-up only: requires no senders or concurrent lifecycle operations. > + * This helper does not unpublish the HWC or drain senders. > + */ > +static void mana_hwc_destroy_queues(struct hw_channel_context *hwc) > +{ > + struct gdma_context *gc = hwc->gdma_dev->gdma_context; > + > + /* The CQ helper deregisters the HWC EQ before returning. */ > + if (hwc->cq) { > + mana_hwc_destroy_cq(gc, hwc->cq); [Severity: High] This is a pre-existing issue, but does this queue destruction order cause a use-after-free during teardown? mana_hwc_destroy_cq() frees the CQ object before it destroys the EQ. The EQ remains active and can still receive events from the hardware. If a teardown fails or times out (which is actively exercised by the new reinit_fallback logic) and an initialization event arrives after the CQ is freed but before the EQ is disabled, the EQ interrupt handler will dereference the freed CQ when processing HWC_INIT_DATA_CQID. > + hwc->cq = NULL; > + } [ ... ] > @@ -896,8 +959,132 @@ int mana_hwc_create_channel(struct gdma_context *gc) > goto out; > } > > + if (q_depth_max > HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH) { > + /* Bound DMA allocations before using the 24-bit depth. */ > + if (q_depth_max > HW_CHANNEL_MAX_QUEUE_DEPTH) > + q_depth_max = HW_CHANNEL_MAX_QUEUE_DEPTH; [Severity: Critical] Does capping the hardware-reported HWC queue depth without negotiating it with the host cause a ring buffer size mismatch? The driver limits the queue depth to HW_CHANNEL_MAX_QUEUE_DEPTH (128) and allocates DMA rings based on this limit. However, the mana_smc_setup_hwc() protocol lacks a queue size field, meaning this restriction isn't communicated to the host hardware. If the hardware advertises a larger depth (e.g., 256), will it write up to 256 entries into the rings and exceed the 128 allocated entries, leading to an out-of-bounds memory write? > + > + /* Keep bootstrap message sizes for mandatory commands. > + * Incompatible reports skip rebuilding, not channel creation. > + */ > + if (max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE || [ ... ] > + err = mana_hwc_establish_channel(gc, &q_depth_max, > + &max_req_msg_size, > + &max_resp_msg_size); > + if (!err && > + (q_depth_max < hwc->num_inflight_msg || [Severity: Critical] Does this validation check incorrectly allow the out-of-bounds access? By verifying q_depth_max < hwc->num_inflight_msg, it permits the hardware depth (q_depth_max) to be larger than the driver's allocated depth (hwc->num_inflight_msg). Should this reject the configuration if the hardware depth exceeds the allocated depth? > + max_req_msg_size != HW_CHANNEL_MAX_REQUEST_SIZE || > + max_resp_msg_size != HW_CHANNEL_MAX_RESPONSE_SIZE)) { > + /* The rebuilt channel must support the allocated depth > + * and message sizes. > + */ -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
