Re: [PATCH v11 12/31] cxl/mem: Set up framework for handling DC Events

[sashiko-bot@kernel.org]

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 <iweiny@kernel.org>

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/20260625112638.550691-1-anisa.su@samsung.com?part=12