Re: [PATCH v9 06/11] cxl: Validate HDM ranges before CXL reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cxl_hdm_ranges_collect() reads HDM decoder settings without holding the required cxl_rwsem.dpa lock, leading to data races with concurrent modifications.
- [High] cxl_hdm_range_context_destroy() releases memory regions outside the cxl_rwsem.region lock, causing spurious -EBUSY errors for concurrent resets.
- [Medium] Functions contain both goto-based error handling and scope-based cleanup (__free() / scoped_guard()), violating subsystem guidelines.
--

--- Patch [6]: [PATCH v9 06/11] cxl: Validate HDM ranges before CXL reset ---
commit c90294fef588f0141e340a7198edc3285f1a401d
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Validate HDM ranges before CXL reset

Before reset, collect enabled cached HDM decoder ranges and reserve them
with request_mem_region(). This rejects reset while affected CXL memory is
busy and keeps the validation stable through reset.

[Severity: Medium]
Is it expected that cxl_restore_hdm() and __pci_cxl_hdm_init() mix
goto-based error handling with scope-based cleanup (__free() and
scoped_guard())?

The cleanup subsystem guidelines strictly mandate converting all resources
to scope-based cleanup or none of them to avoid LIFO ordering mistakes.
Mixing these paradigms increases the risk of double-free or resource leak
bugs due to confusing ownership semantics.

> diff --git a/drivers/cxl/core/reset.c b/drivers/cxl/core/reset.c
> index 7a9b1ecfadcf9..fb741b66e5022 100644
> --- a/drivers/cxl/core/reset.c
> +++ b/drivers/cxl/core/reset.c
[ ... ]
> @@ -467,6 +469,231 @@ static const u32 cxl_reset_timeout_ms[] = {
[ ... ]
> +static int cxl_hdm_ranges_collect(struct cxl_hdm_range_context *ctx,
> +				  struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_info *info = READ_ONCE(pdev->hdm);
> +	int rc;
> +
> +	if (!info) {
> +		pci_err(pdev, "CXL HDM decoder state unavailable\n");
> +		return -ENXIO;
> +	}
> +
> +	for (int i = 0; i < info->decoder_count; i++) {
> +		struct cxl_decoder_settings *settings = &info->settings[i];

[Severity: High]
Is it safe to read info->settings[i] here without holding the
cxl_rwsem.dpa lock?

If a concurrent userspace PCI reset via sysfs triggers
cxl_reset_function() and calls cxl_hdm_ranges_collect(), this code
iterates over info->settings holding only cxl_rwsem.region.

Concurrently, an asynchronous device probe calling
cxl_hdm_info_set_decoder() could modify info->settings holding only
cxl_rwsem.dpa for write.

Could this lead to a torn read of settings->hpa_range, which is then
passed to request_mem_region(), potentially causing memory corruption or
a kernel panic by requesting a garbage memory region?

> +
> +		if (!(settings->flags & CXL_DECODER_F_ENABLE))
> +			continue;
[ ... ]
> @@ -662,7 +889,9 @@ static int cxl_reset_execute(struct pci_dev *pdev, int dvsec)
[ ... ]
>  int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  {
> +	struct cxl_hdm_range_context range_ctx;
>  	int dvsec;
> +	int rc;
>  
>  	dvsec = cxl_reset_dvsec(pdev);
>  	if (dvsec < 0)
> @@ -671,5 +900,14 @@ int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  	if (probe)
>  		return 0;
>  
> -	return cxl_reset_execute(pdev, dvsec);
> +	cxl_hdm_range_context_init(&range_ctx);
> +
> +	scoped_guard(rwsem_write, &cxl_rwsem.region) {
> +		rc = cxl_hdm_ranges_prepare(&range_ctx, pdev);
> +		if (!rc)
> +			rc = cxl_reset_execute(pdev, dvsec);
> +	}
> +
> +	cxl_hdm_range_context_destroy(&range_ctx);

[Severity: High]
Could this cause spurious -EBUSY errors for concurrent resets?

When two endpoints of an interleaved CXL region are reset concurrently,
they both attempt to lock the same hpa_range.

If thread A acquires cxl_rwsem.region, calls request_mem_region(), and
finishes the reset block, it then releases cxl_rwsem.region before calling
cxl_hdm_range_context_destroy() to release the memory region.

If thread B immediately acquires cxl_rwsem.region and calls
request_mem_region() on the same hpa_range, it seems thread B would fail
with -EBUSY because thread A still holds the global iomem_resource for
this range.

Would it be safer to place the context destruction inside the lock scope
to ensure proper serialization?

> +	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709010304.680422-1-smadhavan@nvidia.com?part=6