Re: [PATCH 14/22] cxl: Define a driver interface for DPA allocation

[Gregory Price <gourry@gourry.net>]

This review was generated by kreview-0811365ff2. 

This is not an automated email, the reviewer though this report
looked valid enough to consider discussion.

----

Cursory browse, this does look like a legitimate reference leak.

~Gregory

----

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index a172ce4e9b19..d60a697f12cc 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> +static struct cxl_endpoint_decoder *
> +cxl_find_free_decoder(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct device *dev;
> +
> +	guard(rwsem_read)(&cxl_rwsem.dpa);
> +	dev = device_find_child(&endpoint->dev, NULL,
> +				find_free_decoder);
> +	if (!dev)
> +		return NULL;
> +
> +	return to_cxl_endpoint_decoder(dev);
> +}

[ ... ]

> +struct cxl_endpoint_decoder *cxl_request_dpa(struct cxl_memdev *cxlmd,
> +					     enum cxl_partition_mode mode,
> +					     resource_size_t alloc)
> +{
> +	int rc;
> +
> +	if (!IS_ALIGNED(alloc, SZ_256M))
> +		return ERR_PTR(-EINVAL);
> +
> +	struct cxl_endpoint_decoder *cxled __free(put_cxled) =
> +		cxl_find_free_decoder(cxlmd);
> +
> +	if (!cxled)
> +		return ERR_PTR(-ENODEV);
> +
> +	rc = cxl_dpa_set_part(cxled, mode);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	rc = cxl_dpa_alloc(cxled, alloc);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return no_free_ptr(cxled);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_request_dpa, "CXL");

Does this leak the device reference obtained by device_find_child()
in cxl_find_free_decoder()?

device_find_child() increments the device refcount (reference A).
Then cxl_dpa_alloc() -> __cxl_dpa_reserve() calls get_device() on
the same device (reference B).

On the success path, no_free_ptr() inhibits the put_cxled cleanup,
so reference A is transferred to the caller.  The matching cleanup
function cxl_dpa_free() calls __cxl_dpa_release(), which drops
reference B via put_device(), but reference A is never released:

    cxl_dpa_free()
      -> devm_cxl_dpa_release()
        -> __cxl_dpa_release()
          -> put_device()     /* drops reference B only */

    /* reference A from device_find_child() is still held */

The only caller in this series (sfc efx_cxl_init/efx_cxl_exit)
never calls put_device() on the returned cxled either, so the
endpoint decoder device refcount remains permanently elevated.