Re: [PATCH v2 3/4] cxl/memdev: Add support for multi PF devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cxl/memdev: NULL pointer dereference when accessing missing cxlmd->attach
- [High] cxl/memdev: data race leading to NULL dereference of attach->cxlr
- [High] cxl/memdev: TOCTOU NULL pointer dereference on cxlmd->cxlds
--

commit ad669ced94a784d488cc194089a6921ef73d1404
Author: Alejandro Lucero <alucerop@amd.com>
Subject: cxl/memdev: Add support for multi PF devices

This commit enables non-PF0 Physical Functions to access CXL memory by
establishing a device link between the CXL region attached to the PF0 memdev
and the non-PF0 devices. Unbinding the PF0 device will naturally release the
memdev and region, safely unwinding the non-PF0 drivers first.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9c..799cb6e756390 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
[ ... ]
> @@ -802,6 +802,98 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
>  	return ERR_PTR(rc);
>  }
>  
> +static int match_memdev_by_parent_device(struct device *dev, const void *data)
> +{
> +	const struct device *pf_dev = data;
> +	struct cxl_memdev *cxlmd;
> +
> +	if (!is_cxl_memdev(dev))
> +		return 0;
> +
> +	cxlmd = to_cxl_memdev(dev);
> +	return (cxlmd->cxlds->dev == pf_dev);

[Severity: High]
Does this create a race condition that could lead to a NULL pointer
dereference? When bus_find_device() iterates over the CXL bus using
match_memdev_by_parent_device(), it evaluates cxlmd->cxlds->dev == pf_dev
without holding a lock protecting cxlmd->cxlds.

Concurrently, if an unrelated cxl_memdev on the bus is shutting down,
cxl_memdev_shutdown() explicitly sets cxlmd->cxlds = NULL. If the matching
loop encounters this shutting-down memdev, could it trigger a deterministic
NULL pointer dereference?

Since dev->parent is stably assigned to cxlds->dev at creation, could this
check use dev->parent == pf_dev to bypass this race entirely?

> +}
> +
> +static int __cxl_get_range_and_link(struct device *pf0, struct device *pfx,
> +				    struct range *range)
> +{
> +	struct device *mem_dev __free(put_device) =
> +		bus_find_device(&cxl_bus_type, NULL, pf0,
> +				match_memdev_by_parent_device);
> +	struct cxl_attach_region *attach;
> +	struct cxl_memdev *cxlmd;
> +
> +	if (!mem_dev)
> +		return -ENODEV;
> +
> +	cxlmd = to_cxl_memdev(mem_dev);
> +	attach = container_of(cxlmd->attach, struct cxl_attach_region, attach);
> +
> +	/*
> +	 * The cxlmd object does exist and it can be found in the cxl bus after
> +	 * creation but before attach probe setting the proper HPA range. If so,
> +	 * the caller will need to try later.
> +	 */
> +	if (attach->hpa_range.end == CXL_RESOURCE_NONE)

[Severity: High]
Could this blindly dereference a missing cxlmd->attach? When
cxl_get_range_and_link() retrieves a cxl_memdev by parent device, if the
target device was created without an attach callback (e.g., via
devm_cxl_add_classdev()), cxlmd->attach is NULL.

Since container_of(NULL, struct cxl_attach_region, attach) evaluates exactly
to NULL because the member is at offset 0, evaluating attach->hpa_range.end
would trigger an immediate kernel crash. Should this exported subsystem API
validate its state first to prevent remote subsystems from inducing crashes?

> +		return -EPROBE_DEFER;
> +
> +	/*
> +	 * Create the device link between the region and the consumer device.
> +	 * AUTOREMOVE_CONSUMER means the link implicitly to be removed if the
> +	 * consumer unbinds first with no consequences for the supplier.
> +	 */
> +	if (!device_link_add(pfx, &attach->cxlr->dev,
> +			     DL_FLAG_AUTOREMOVE_CONSUMER)) {

[Severity: High]
Does reading attach->hpa_range.end and attach->cxlr here lack synchronization
against the cxl_mem probe writer?

While __cxl_get_range_and_link() relies on device_lock(pf0) to serialize
access:

cxl_get_range_and_link() {
    ...
    device_lock(pf0);
    rc = __cxl_get_range_and_link(pf0, pfx, range);
    device_unlock(pf0);
    ...
}

cxl_memdev_attach_region() writes to attach->cxlr and attach->hpa_range
during the cxl_mem driver probe, which executes asynchronously under the
child's device lock (device_lock(&cxlmd->dev)).

Due to the lack of synchronization and memory barriers between these distinct
lock contexts, could a reader observe an updated non-NONE hpa_range.end while
attach->cxlr remains NULL, passing &attach->cxlr->dev to device_link_add()
and crashing the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001132023.17032-1-alucerop@amd.com?part=3