Re: [PATCH 02/15] cxl/cache: Add cxl_cache driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] cxl/cache: kernel crash in cxl_clear_cache_enable() on non-PCI mock devices
- [High] cxl/core: race condition in RMW of PCI_DVSEC_CXL_CTRL without locking
- [High] cxl/cache: NULL pointer dereference in probe due to early clear in unregister
- [Medium] cxl/core: PCI config space corruption due to missing cxl_dvsec capability check
- [Low] cxl/cache: build link failure when built-in driver calls devm_cxl_add_cachedev
--

commit bd84836a0f10aed3ee9d665572f6ba9474a6ea11
Author: Ben Cheatham <Benjamin.Cheatham@amd.com>
cxl/cache: Add cxl_cache driver

This commit adds the cxl_cache driver for managing CXL.cache capable endpoints
and validating system configurations. It provides functionality to get cache
information from the CXL device DVSEC and enables or disables CXL.cache during
device probe.

> diff --git a/drivers/cxl/cache.c b/drivers/cxl/cache.c
> --- /dev/null
> +++ b/drivers/cxl/cache.c

[ ... ]

> +static int cxl_cache_probe(struct device *dev)
> +{
> +	struct cxl_cachedev *cxlcd = to_cxl_cachedev(dev);
> +	struct cxl_dev_state *cxlds = cxlcd->cxlds;
> +	int rc;
> +
> +	/* Disable CXL.cache until we can validate the device configuration */
> +	cxl_clear_cache_enable(cxlds);

[Severity: High]
Could cxl_clear_cache_enable() panic if a non-PCI device (such as a mock 
device used in testing) is bound to the driver? It appears we blindly cast 
to a struct pci_dev pointer inside cxl_set_protocol_enable() before any 
dev_is_pci() check occurs.

[Severity: High]
Additionally, could this probe function race with device unregistration? If a 
sysfs unbind triggers cxl_cachedev_unregister(), it sets cxlcd->cxlds to 
NULL before calling device_del(). If this probe runs concurrently, could it 
retrieve the NULL pointer and pass it to cxl_clear_cache_enable(), resulting 
in a NULL pointer dereference?

> +
> +	rc = cxl_accel_read_cache_info(cxlds);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c

[ ... ]

> -static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
> +static int cxl_set_protocol_enable(struct cxl_dev_state *cxlds, u16 val,
> +				   u16 enable_bit)
>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	int d = cxlds->cxl_dvsec;
> @@ -191,9 +193,9 @@ static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
>  	if (rc)
>  		return pcibios_err_to_errno(rc);
>  
> -	if ((ctrl & PCI_DVSEC_CXL_MEM_ENABLE) == val)
> +	if ((ctrl & enable_bit) == val)
>  		return 1;
> -	ctrl &= ~PCI_DVSEC_CXL_MEM_ENABLE;
> +	ctrl &= ~enable_bit;
>  	ctrl |= val;
>  
>  	rc = pci_write_config_word(pdev, d + PCI_DVSEC_CXL_CTRL, ctrl);

[Severity: High]
Is this read-modify-write cycle protected against concurrent modifications? If
the cxl_mem and cxl_cache drivers probe concurrently, could one thread
overwrite the modifications of the other in the shared PCI_DVSEC_CXL_CTRL
register, potentially dropping the MEM_ENABLE bit while setting CACHE_ENABLE?

[Severity: Medium]
What happens if an endpoint driver adds a cache device on hardware lacking a 
valid CXL DVSEC capability? If cxlds->cxl_dvsec is 0, this will calculate 
d + PCI_DVSEC_CXL_CTRL (which is 0x0C) and overwrite the standard PCI Cache
Line Size register. Does this need a cxlds->cxl_dvsec > 0 check?

[ ... ]

> +int cxl_accel_read_cache_info(struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_cache_state *cstate = &cxlds->cstate;
> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	int dvsec = cxlds->cxl_dvsec;
> +	u16 cap, cap2;
> +	u32 unit;
> +	int rc;
> +
> +	if (!dev_is_pci(cxlds->dev))
> +		return -EINVAL;
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CAP, &cap);

[Severity: Medium]
Similarly, could this read garbage capability data from the standard PCI 
Class Code register (0x0A) if cxlds->cxl_dvsec is 0?

> +	if (rc)
> +		return pcibios_err_to_errno(rc);

[ ... ]

> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h

[ ... ]

>  struct cxl_memdev *devm_cxl_probe_mem(struct cxl_dev_state *cxlds,
>  				      struct range *range);
> -
>  int cxl_set_capacity(struct cxl_dev_state *cxlds, u64 capacity);
> +
> +#if IS_ENABLED(CONFIG_CXL_CACHE)
> +struct cxl_cachedev *devm_cxl_add_cachedev(struct cxl_dev_state *cxlds);
> +#else
> +static inline struct cxl_cachedev *
> +devm_cxl_add_cachedev(struct cxl_dev_state *cxlds)
> +{ return ERR_PTR(-ENXIO); }
> +#endif /* CONFIG_CXL_CACHE */

[Severity: Low]
Can this cause a build link failure if CONFIG_CXL_CACHE is built as a module 
(=m) but the endpoint driver calling devm_cxl_add_cachedev() is built-in (=y)? 

IS_ENABLED(CONFIG_CXL_CACHE) evaluates to true in this case, providing the 
external declaration, but the symbol won't be available during vmlinux linking. 
Should this use IS_REACHABLE(CONFIG_CXL_CACHE) instead?

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