Re: [PATCH 09/15] cxl/core: Add Cache ID verification

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] cxl/cache: missing rcu_read_lock during xa_for_each iteration
- [High] cxl/cache: unconditional probe failure for Restricted CXL Host (RCH) topologies
- [High] cxl/cache: use-after-free of parent_dport during asynchronous teardown
- [Medium] cxl/core: missing validation of hdmd state against hardware cache ID
- [Low] cxl/core: returning negative error code for truthy boolean condition
--

commit fb180ed6317b83962ae34e684038bcea834c1f39
Author: Ben Cheatham <Benjamin.Cheatham@amd.com>

cxl/core: Add Cache ID verification
    
Check if system firmware has pre-programmed a CXL.cache device's cache
id as part of cxl_cache::probe(). A cache id is required when multiple
CXL.cache devices are present under a host bridge, so fail probe if the
id has not been programmed and another device is present.

Also add @hdmd to struct cxl_dev_state for endpoint drivers to indicate
whether their device is using HDM-D flows. This is required for
correctly validating cache id programming.

> diff --git a/drivers/cxl/cache.c b/drivers/cxl/cache.c
> index dfefd301696a2..6c098010149ae 100644
> --- a/drivers/cxl/cache.c
> +++ b/drivers/cxl/cache.c
> @@ -84,6 +84,90 @@ static int cxl_endpoint_cache_enabled(struct cxl_port *endpoint)
>  	return 0;
>  }
>  
> +static struct cxl_port *find_host_bridge(struct cxl_port *endpoint)
> +{
> +	struct cxl_port *parent = endpoint->parent_dport->port;

[Severity: High]
Can this result in a use-after-free during asynchronous teardown?

When a CXL host bridge is hot-removed, the upstream CXL port topology can be
torn down, and devres can free parent_dport, before the PCI device is
unbound.

During PCI unbind, the cachedev's devres will call deprogram_cache_id()
which dereferences cxlcd->endpoint->parent_dport->port here, accessing
freed memory.

> +	struct cxl_port *hb = endpoint;
> +
> +	if (is_cxl_root(parent))
> +		return NULL;

[Severity: High]
Will this unconditionally fail probes for Restricted CXL Host (RCH)
topologies?

In an RCH topology, the endpoint's parent is the root port, so
is_cxl_root(parent) will evaluate to true immediately, causing
find_host_bridge() to return NULL.

Later in program_cache_id(), this NULL return causes the entire cache
programming and probe sequence to fail with -ENODEV.

> +
> +	while (!is_cxl_root(parent)) {
> +		hb = parent;
> +		parent = hb->parent_dport->port;
> +	}
> +
> +	return hb;
> +}
> +
> +static int get_num_cachedevs_present(struct cxl_port *host_bridge, u32 *num)
> +{
> +	u32 num_cachedevs = 0;
> +	unsigned long index;
> +	struct cxl_ep *ep;
> +
> +	xa_for_each(&host_bridge->endpoints, index, ep) {
> +		if (is_cxl_cachedev(ep->ep))
> +			num_cachedevs++;
> +	}

[Severity: High]
Is an rcu_read_lock() or xa_lock() required here when iterating the
host_bridge->endpoints array?

While program_cache_id() holds the host bridge device lock (hb->dev),
concurrent endpoint probes in cxl_add_ep() insert into this array without
holding the higher-level hb->dev lock. 

Iterating xa_for_each() without RCU protection or the array's lock risks
data races and use-after-free, especially since cxl_ep_remove() can free the
endpoint structures concurrently.

[ ... ]
> diff --git a/drivers/cxl/core/cache.c b/drivers/cxl/core/cache.c
> index 17df0e603b68f..be570d3dc6090 100644
> --- a/drivers/cxl/core/cache.c
> +++ b/drivers/cxl/core/cache.c
> @@ -39,3 +39,161 @@ int cxl_dport_map_cache_id_dc(struct cxl_dport *dport)
>  	return cxl_map_component_regs(map, &dport->regs.component,
>  				      BIT(CXL_CM_CAP_CAP_ID_CACHE_ID_DC));
>  }
> +
> +static int cache_decoder_committed(struct cxl_dport *dport)
> +{
> +	u32 cap, stat;
> +
> +	cap = readl(dport->regs.ciddc + CXL_CACHE_ID_DC_CAP_OFFSET);
> +	if (!FIELD_GET(CXL_CACHE_ID_DC_CAP_COMMIT_REQ, cap))
> +		return -ENXIO;

[Severity: Low]
Is it safe to return a negative error code for a truthy boolean condition?

When the COMMIT_REQ bit is not set, this returns -ENXIO, which the caller
evaluates as truthy via negation (e.g., !cache_decoder_committed(dport)
evaluates to false since !(-ENXIO) is false).

While this happens to work in C, returning a negative integer to signify a
successful or truthy condition creates a silent refactoring hazard.

> +
> +	stat = readl(dport->regs.ciddc + CXL_CACHE_ID_DC_STATUS_OFFSET);
> +	return FIELD_GET(CXL_CACHE_ID_DC_STATUS_COMMITTED, stat);
> +}
> +
> +static int cache_decoder_get_id(struct cxl_dport *dport)
> +{
> +	u32 ctrl;
> +
> +	ctrl = readl(dport->regs.ciddc + CXL_CACHE_ID_DC_CTRL_OFFSET);
> +	if (FIELD_GET(CXL_CACHE_ID_DC_CTRL_HDMD_PRESENT, ctrl))
> +		 return FIELD_GET(CXL_CACHE_ID_DC_CTRL_HDMD_ID, ctrl);
> +
> +	return FIELD_GET(CXL_CACHE_ID_DC_CTRL_LOCAL_ID, ctrl);
> +}

[Severity: Medium]
Does this miss validating the hardware's HDM-D capability against the
endpoint driver's expectation?

The commit message notes that @hdmd is added to struct cxl_dev_state to
indicate whether the device uses HDM-D flows, and that this is required for
correctly validating cache id programming. 

However, cache_decoder_get_id() parses the ID purely based on the hardware
HDMD_PRESENT bit, and cxl_cachedev_validate_cache_id() never
cross-references this with the software cxlds->hdmd state. Firmware
misconfigurations where the cache ID flow type doesn't match the endpoint
driver's requirement will go undetected.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923173342.5584-1-Benjamin.Cheatham@amd.com?part=9