Re: [RFC 1/2] cxl/memdev: add support for mutipf device

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] `cxl_get_pf0_memdev()` returns a pointer after dropping its device reference via `__free()`, causing a Use-After-Free.
- [High] Flawed execution context detection using `mutex_trylock()` leads to silently skipped driver releases.
- [High] `cxl_get_pf0_memdev()` leaves the sibling device in the xarray on the `-EPROBE_DEFER` error path.
- [High] Unconditional dereference of `attach->hpa_range.end` causes a kernel panic if `cxlmd->attach` is NULL.
- [High] Sibling device pointers are stored in the `cxlmd->siblings` xarray without taking a reference count.
- [Medium] The return value of `xa_store()` is ignored, potentially leaving siblings unregistered.
- [Medium] Missing `xa_destroy()` for the `cxlmd->siblings` xarray leads to leaked internal nodes.
--

--- Patch [1]: [RFC 1/2] cxl/memdev: add support for mutipf device ---
commit be73e6cb4711f45100bc8fb00094b292d5fbc718
Author: Alejandro Lucero <[email protected]>

cxl/memdev: add support for mutipf device

A PCI device can present multiple Physical Functions(PFs) but the CXL
specs restrict to the first one, PF0, the discovery and management of
CXL capabilities accessed through a PF0 BAR. Other non-PF0 PFs need to
obtain the CXL.mem range to work with somehow.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9c..327c4da3208fe 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -26,6 +26,35 @@ static void cxl_memdev_release(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct device *parent = dev->parent;
> +	struct device *sibling;
> +	unsigned long index;
> +
> +	/*
> +	 * Type2 multipf support implies other non-PF0 PFs could be having a

[ ... ]

> +		/* Several possibilities trigger a memdev release with one being
> +		 * its parent device (Type2 device) released from its driver. If
> +		 * so, such release is the context for this function, precluding
> +		 * the mutex lock and therefore safely avoiding to invoke the
> +		 * release again which would trigger a deadlock.
> +		 */
> +		if (mutex_trylock(&cxlmd->dev.parent->mutex)) {

[Severity: High]
Does this trylock reliably detect if it's executing within the context of the
parent device's release callback? If the trylock fails, the code assumes it
is already on the parent release path and skips calling
device_release_driver(). However, couldn't the lock be held by a completely
unrelated thread performing sysfs access, hotplug operations, or power
management? If lock contention occurs, the parent driver release might be
skipped entirely, leaving the parent device permanently bound.

> +			mutex_unlock(&cxlmd->dev.parent->mutex);
> +			device_release_driver(cxlmd->dev.parent);
> +		}
> +	}
>  
>  	ida_free(&cxl_memdev_ida, cxlmd->id);

[Severity: Medium]
Is it necessary to call xa_destroy() for the cxlmd->siblings xarray before
freeing the struct? The xarray is dynamically initialized during alloc, and
if it allocates memory for internal tree nodes, skipping xa_destroy() could
lead to leaked internal nodes even if all entries were logically erased.

>  	kfree(cxlmd);
>  	put_device(parent);
>  }

[ ... ]

> @@ -802,6 +832,98 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,

[ ... ]

> +struct cxl_memdev *cxl_get_pf0_memdev(struct device *pf0, struct device *pfx,
> +				      unsigned long index, struct range *range)
> +{
> +	struct cxl_attach_region *attach;
> +	struct cxl_memdev *cxlmd;
> +	struct device *mem_dev __free(put_device) =
> +		bus_find_device(&cxl_bus_type, NULL, pf0,
> +				match_memdev_by_parent_device);
> +
> +	if (!mem_dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	cxlmd = to_cxl_memdev(mem_dev);
> +
> +	/*
> +	 * we got the cxl_memdev and the implicit get_device in bus_find_device
> +	 * makes the next steps safe.
> +	 */
> +
> +	xa_store(&cxlmd->siblings, index, pfx, GFP_KERNEL);

[Severity: High]
Should this xarray store take a reference count on the pfx device before
storing it? Since cxl_get_pf0_memdev() registers the pfx sibling device but
doesn't increment its reference count, if the sibling device is independently
removed and freed, the xarray could retain a dangling pointer. When
cxl_memdev_release() later iterates the xarray and invokes
device_release_driver(sibling), it would dereference this dangling pointer
and cause a use-after-free.

[Severity: Medium]
Is it safe to ignore the return value of xa_store()? Since xa_store()
dynamically allocates memory for new elements and can fail returning an error
pointer, an unhandled failure means the sibling device would be omitted from
the xarray while the function returns success. This might silently compromise
cross-device unbinding logic.

> +	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 == -1)

[Severity: High]
Can this unconditionally dereference a NULL attach pointer? Because attach is
at offset 0 within struct cxl_attach_region, if cxlmd->attach is NULL, the
attach pointer evaluates identically to NULL. Standard Type 3 CXL memdevs
lack an attach region, so cxlmd->attach would be NULL, causing an immediate
kernel panic when trying to access attach->hpa_range.end.

[Severity: High]
Does this error path need to clean up the xarray entry before returning?
Since the sibling device was already inserted into cxlmd->siblings, returning
-EPROBE_DEFER here without calling xa_erase() leaves a stale pointer in the
xarray. Later, cxl_memdev_release() could iterate over the xarray and invoke
device_release_driver() on a dangling pointer.

> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	range->start =  attach->hpa_range.start;
> +	range->end =  attach->hpa_range.end;
> +
> +	return to_cxl_memdev(mem_dev);

[Severity: High]
Could this return a pointer after dropping its device reference, causing a
use-after-free? bus_find_device() acquires a reference to the matched device
stored in mem_dev, which is decorated with __free(put_device). Because this
return statement does not inhibit cleanup (for example, by using
no_free_ptr() or return_ptr()), put_device(mem_dev) is automatically executed
at scope exit, instantly dropping the reference while returning an unpinned
pointer.

> +}

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