Re: [PATCH v4 07/15] cxl: Add FWCTL support to CXL

[Dan Williams <dan.j.williams@intel.com>]

Dave Jiang wrote:
> Add fwctl support code to allow sending of CXL feature commands from
> userspace through as ioctls via FWCTL. Provide initial setup bits. The
> CXL PCI probe function will call cxl_setup_fwctl() after the cxl_memdev
> has been enumerated in order to setup FWCTL char device under the
> cxl_memdev like the existing memdev char device for issuing CXL raw
> mailbox commands from userspace via ioctls.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/features.c  | 74 ++++++++++++++++++++++++++++++++++++
>  drivers/cxl/pci.c            |  4 ++
>  include/cxl/features.h       | 14 +++++++
>  include/uapi/fwctl/fwctl.h   |  1 +
>  tools/testing/cxl/test/mem.c |  4 ++
>  5 files changed, 97 insertions(+)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 82f21f64452a..81e8ff66c12e 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2024-2025 Intel Corporation. All rights reserved. */
> +#include <linux/fwctl.h>
>  #include <linux/device.h>
>  #include <cxl/mailbox.h>
>  #include <cxl/features.h>
> @@ -167,6 +168,13 @@ static void free_cxlfs(void *_cxlfs)
>  
>  	cxlds->cxlfs = NULL;
>  	kvfree(cxlfs->entries);
> +
> +	if (cxlfs->cxl_fwctl) {
> +		struct cxl_fwctl *fwctl = cxlfs->cxl_fwctl;
> +
> +		fwctl_unregister(&fwctl->fwctl_dev);
> +		fwctl_put(&fwctl->fwctl_dev);
> +	}

I think this is going to lead to use after free bugs.

Consider that this free_cxlfs() call is a devm action that was
registered *before* devm_cxl_add_memdev(). That means that the memdev
gets destroyed before fwctl is unregistered. That sounds like a recipe
for use after free bugs because userspace could still be using the fwctl
interface after the memdev has been unregistered.

More below...

[..]
> +DEFINE_FREE(free_fwctl, struct cxl_fwctl *, if (_T) fwctl_put(&_T->fwctl_dev))
> +
> +int cxl_setup_fwctl(struct cxl_memdev *cxlmd)

This needs to be devm_...

> +{
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +	struct cxl_features_state *cxlfs;
> +	int rc;
> +
> +	cxlfs = to_cxlfs(cxlds);
> +	if (!cxlfs)
> +		return -ENODEV;
> +
> +	/* No need to setup FWCTL if there are no user allowed features found */
> +	if (!cxlfs->entries->num_user_features)
> +		return -ENODEV;
> +
> +	struct cxl_fwctl *fwctl __free(free_fwctl) =
> +		fwctl_alloc_device(&cxlmd->dev, &cxlctl_ops,
> +				   struct cxl_fwctl, fwctl_dev);
> +	if (!fwctl)
> +		return -ENOMEM;
> +
> +	fwctl->cxlmd = cxlmd;
> +	rc = fwctl_register(&fwctl->fwctl_dev);
> +	if (rc)
> +		return rc;

...and a devm action needs to be registered here to make sure fwctl is
torn down before the memdev is torn down.

> +
> +	cxlfs->cxl_fwctl = no_free_ptr(fwctl);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_setup_fwctl, "CXL");
> +
> +MODULE_IMPORT_NS("FWCTL");
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 3e666ec51580..b093cb16de3e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1013,6 +1013,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> +	rc = cxl_setup_fwctl(cxlmd);
> +	if (rc)
> +		dev_dbg(&pdev->dev, "No CXL FWCTL setup\n");
> +
>  	pmu_count = cxl_count_regblock(pdev, CXL_REGLOC_RBI_PMU);
>  	if (pmu_count < 0)
>  		return pmu_count;
> diff --git a/include/cxl/features.h b/include/cxl/features.h
> index 1ab97e676c03..d0c94756e452 100644
> --- a/include/cxl/features.h
> +++ b/include/cxl/features.h
> @@ -4,6 +4,7 @@
>  #define __CXL_FEATURES_H__
>  
>  #include <linux/uuid.h>
> +#include <linux/fwctl.h>
>  
>  /* Feature UUIDs used by the kernel */
>  #define CXL_FEAT_PATROL_SCRUB_UUID						\
> @@ -158,6 +159,9 @@ enum cxl_set_feat_flag_data_transfer {
>   * @entries: CXl feature entry context
>   *	@num_features: total Features supported by the device
>   *	@ent: Flex array of Feature detail entries from the device
> + * @fwctl: CXL Firmware Control context
> + *	@fwctl_dev: Firmware Control device
> + *	@cxlfs: Pointer to CXL Features state
>   */
>  struct cxl_features_state {
>  	struct cxl_dev_state *cxlds;
> @@ -166,12 +170,17 @@ struct cxl_features_state {
>  		int num_user_features;
>  		struct cxl_feat_entry ent[] __counted_by(num_features);
>  	} *entries;
> +	struct cxl_fwctl {
> +		struct fwctl_device fwctl_dev;
> +		struct cxl_memdev *cxlmd;

Is this backpointer necessary?

Just look it up from the fwctl dev.

static inline struct cxl_memdev *fwctl_to_memdev(struct fwctl_device *fwctl_dev)
{
	return to_cxl_memdev(fwctl_dev->dev.parent);
}

...then no need for 'struct cxl_fwctl' definition.