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. - [Medium] Stack-allocation optimization in `memdev_release_extent()` is defeated by an internal dynamic allocation in `cxl_send_dc_response()`. - [Medium] Invalid `__counted_by` attribute on a little-endian struct member breaks bounds checking on big-endian architectures. --
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? > + 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? > + > + 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. > +} __packed; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
