Re: [PATCH v7 2/2 RESEND] tools/testing/cxl: Enable zero sized decoders under hb0

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl_dpa_free() fails to release 0-length DPA reservations, rendering unlocked zero-size decoders permanently unusable by userspace and leaking memory.
--

--- Patch [2]: [PATCH v7 2/2 RESEND] tools/testing/cxl: Enable zero sized decoders under hb0 ---
commit 328b24698ee67fa6e936ca1dc4e6a01523de8a20
Author: Richard Cheng <[email protected]>

tools/testing/cxl: Enable zero sized decoders under hb0
    
The kernel now allows committed zero-size HDM decoders so BIOS can lock
empty decoders; cxl_test needs to exercise that path. Add a module
parameter to mock zero-size decoders under host-bridge0 endpoints.

> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 8ab2ce1262f30..fff2004a08c80 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c

[ ... ]

> @@ -1072,16 +1072,49 @@ static void default_mock_decoder(struct cxl_decoder *cxld)
>  	WARN_ON_ONCE(!cxld_registry_new(cxld));
>  }
>  
> -static int first_decoder(struct device *dev, const void *data)
> +static int match_decoder_by_index(struct device *dev, const void *data)
>  {
> +	int target_id = *(const int *)data;
>  	struct cxl_decoder *cxld;
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
>  	cxld = to_cxl_decoder(dev);
> -	if (cxld->id == 0)
> -		return 1;
> -	return 0;
> +	return cxld->id == target_id;
> +}
> +
> +/*
> + * Mock a committed, locked, empty decoder
> + * (CXL r4.0 8.2.4.20.12). Gated by the mock_zero_size_decoders module
> + * param so the default cxl_test topology, shared by the region test
> + * suite, is left undisturbed.
> + */
> +static void size_zero_mock_decoder_ep(struct cxl_decoder *cxld, u64 base)
> +{
> +	struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(&cxld->dev);
> +
> +	cxld->hpa_range = DEFINE_RANGE(base, base - 1);
> +	cxld->interleave_ways = 2;
> +	cxld->interleave_granularity = 4096;
> +	cxld->target_type = CXL_DECODER_HOSTONLYMEM;
> +	cxld->flags = CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> +	cxled->state = CXL_DECODER_STATE_AUTO;
> +	/* decoder[0] reserved [0, size/2), empty decoders sit at that watermark */
> +	devm_cxl_dpa_reserve(cxled, mock_auto_region_size / 2, 0, 0);

[Severity: High]
Does this 0-length DPA reservation leak when the decoder is torn down?

Looking at the core CXL code, cxl_dpa_free() returns early if cxled_empty()
is true:

drivers/cxl/core/hdm.c:cxl_dpa_free() {
	...
	if (cxled_empty(cxled))
		return 0;
	...
}

And cxled_empty() returns true for 0-length resources:

drivers/cxl/cxl.h:cxled_empty() {
	return !cxled->dpa_res || !resource_size(cxled->dpa_res);
}

Because resource_size(cxled->dpa_res) == 0, cxl_dpa_free() returns 0 without
calling devm_cxl_dpa_release().

Does this mean the 0-length kmalloc'd resource is leaked and port->hdm_end
is never decremented, rendering unlocked zero-size decoders permanently
unusable by userspace?

> +	cxld->commit = mock_decoder_commit;
> +	cxld->reset = mock_decoder_reset;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2