On Thu, Jun 25, 2026 at 06:12:18PM +0000, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) > to consider: > - [High] Missing DCD response on memory allocation failure leaves the device > state machine permanently blocked while unconditionally clearing the event > log. fixed
> - [Medium] Stack-allocation optimization in `memdev_release_extent()` is > defeated by an internal dynamic allocation in `cxl_send_dc_response()`. false pos > - [Medium] Invalid `__counted_by` attribute on a little-endian struct member > breaks bounds checking on big-endian architectures. fixed > -- > > commit f5333550094fcaacb4e98dbf0c270b32ec17b506 > Author: Ira Weiny <[email protected]> > > cxl/mem: Set up framework for handling DC Events > > Adds the support for receiving DC event records but defers > the real add/release logic to subsequent commits. Simply refuse all > extents for DC_ADD and ack all DC_RELEASE events for now. Forced > release is currently unsupported. > > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > > --- a/drivers/cxl/core/mbox.c > > +++ b/drivers/cxl/core/mbox.c > > [ ... ] > > > +static void memdev_release_extent(struct cxl_memdev_state *mds, struct > > range *range) > > +{ > > + struct device *dev = mds->cxlds.dev; > > + struct cxl_extent extent = { > > + .start_dpa = cpu_to_le64(range->start), > > + .length = cpu_to_le64(range_len(range)), > > + }; > > + struct cxl_extent_list_node node = { .extent = &extent }; > > + LIST_HEAD(extent_list); > > + > > + dev_dbg(dev, "Release response dpa %pra\n", range); > > + > > + /* > > + * Stack-allocated instead of kzalloc'ed to avoid potential -ENOMEM, > > + * which would prevent sending the release DC rsp. > > + */ > > + list_add_tail(&node.list, &extent_list); > > + if (cxl_send_dc_response(mds, CXL_MBOX_OP_RELEASE_DC, &extent_list, 1)) > > [Severity: Medium] > The comment mentions stack-allocating to avoid -ENOMEM so we can ensure the > release DC response is sent. > > However, cxl_send_dc_response() uses kzalloc() to allocate the > cxl_mbox_dc_response message: > > struct cxl_mbox_dc_response *response __free(kfree) = > kzalloc(pl_size, GFP_KERNEL); > > Doesn't this dynamic allocation defeat the optimization, since memory > pressure could still cause kzalloc() to fail, preventing the release > response from being sent? > No. In cxl_send_dc_response(), we send an empty rsp if kzalloc() fails. struct cxl_mbox_dc_response *response __free(kfree) = kzalloc(pl_size, GFP_KERNEL); if (!response) { /* Refuse all extents rather than stall the device. */ struct cxl_mbox_dc_response empty = { }; return send_one_response(cxl_mbox, &empty, opcode, 0, 0); } Originally, this function kzalloc'ed the node that was the input for cxl_send_dc_response. If that failed, we would never reach cxl_dc_response(), and no rsp would be sent, which is why it was changed to stack allocation. Anyway the comment is deleted since it seems misleading and stack-allocating two small local structs for a single extent is the obvious choice and does need extra justification. There is no issue of correctness. > > + dev_dbg(dev, "Failed to release %pra\n", range); > > +} > > [ ... ] > > > +static int handle_add_event(struct cxl_memdev_state *mds, > > + struct cxl_event_dcd *event) > > +{ > > + struct device *dev = mds->cxlds.dev; > > + int rc; > > + > > + rc = add_to_pending_list(&mds->add_ctx.pending_extents, &event->extent); > > + if (rc) { > > + clear_pending_extents(mds); > > + return rc; > > + } > > [Severity: High] > If add_to_pending_list() fails and returns -ENOMEM, we return early without > sending the required CXL_MBOX_OP_ADD_DC_RESPONSE command. > > Since cxl_handle_dcd_event_records() drops the error and the event is cleared > from the device log, will this leave the device state machine permanently > blocked waiting for a response that never arrives? > Fixed. This only applies in the case for a More chain with a single event record. So the first and only event record has More=0 and the 20s watchdog is never armed. Otherwise, the watchdog would catch it. However, we may as well send an empty response anyway for both cases. So the fix is: rc = add_to_pending_list(&ctx->pending_extents, &event->extent); if (rc) { clear_pending_extents(mds); /* * If More chain only has 1 record and errors before watchdog is * armed, cxl_add_pending will never be reached, so no response * is sent. Return an empty response. Also sends empty response * for multi-record chain pre-emptively rather than waiting for * watchdog to trigger. */ if (!(event->flags & CXL_DCD_EVENT_MORE)) { ctx->armed = false; cancel_delayed_work(&ctx->timeout_work); if (cxl_send_dc_response(mds, CXL_MBOX_OP_ADD_DC_RESPONSE, &ctx->pending_extents, 0)) dev_dbg(dev, "Failed to refuse chain after staging error\n"); } return rc; } > > + > > + if (event->flags & CXL_DCD_EVENT_MORE) { > > [ ... ] > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > > --- a/drivers/cxl/cxlmem.h > > +++ b/drivers/cxl/cxlmem.h > > [ ... ] > > > +/* > > + * Add Dynamic Capacity Response > > + * CXL rev 3.1 section 8.2.9.9.9.3; Table 8-168 & Table 8-169 > > + */ > > +struct cxl_mbox_dc_response { > > + __le32 extent_list_size; > > + u8 flags; > > + u8 reserved[3]; > > + struct updated_extent_list { > > + __le64 dpa_start; > > + __le64 length; > > + u8 reserved[8]; > > + } __packed extent_list[] __counted_by(extent_list_size); > > [Severity: Medium] > Should this use __counted_by_le() instead of __counted_by()? > > Since extent_list_size is explicitly little-endian (__le32), using > __counted_by() on big-endian architectures will result in the bounds checking > instrumentation reading an inflated integer value, leading to out-of-bounds > errors or kernel panics. > Fixed. Also updated spec reference to r4.0 Section 8.2.10.9.9.3; Table 8-350 & Table 8-351 > > +} __packed; > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/[email protected]?part=12
