Re: [RFC 2/2] sfc: add multipf support

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The variable `pf0_pci_dev` is used uninitialized as the starting pointer for the `pci_get_device()` search, leading to memory corruption or crashes.
- [High] The return value of `cxl_map()` is checked using inverted logic, treating success as failure (causing a Use-After-Free) and failure as success (silently bypassing initialization failure).
- [High] The `struct pci_dev` reference acquired by `get_pf0_pci_device()` is never released, resulting in a persistent object leak.
- [High] Memory is dynamically allocated for the undefined or incorrect type `struct cxl` instead of `struct efx_cxl`.
- [High] The PF0 device matching logic compares `pci_dev->slot`, which can be NULL, potentially associating unrelated physical devices.
- [Medium] Missing teardown of `cxl_get_pf0_memdev()` on error paths in `efx_cxl_init()`.
- [Medium] The manually allocated `struct efx_cxl` for non-PF0 devices is permanently leaked on driver teardown.
- [Medium] An early return in the `efx_cxl_exit()` non-PF0 path permanently leaks the I/O memory mapping.
--

--- Patch [2]: [RFC 2/2] sfc: add multipf support ---
commit e798dff2eb0bc8eafa5e5bca36069e0989d743a9
Author: Alejandro Lucero <[email protected]>

sfc: add multipf support

Use CXL core accelerator API for registering non-PF0 PFs to the memdev
linked to the PF0, along with its complementary unregister. Adapt the
ioremap call per PF to be an offset based on the PF function index and
a hardcoded per PF CXL.mem slot size.

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 348d7404cd7aa..e45c8dd1969c2 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -13,6 +13,48 @@
>  #include "efx_cxl.h"
>  
>  #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
> +#define EFX_CTPIO_BUFFER_PER_PF_SIZE	SZ_8M
> +
> +#define X4_PF_DEVICE_ID 0x0c03
> +
> +static struct pci_dev *get_pf0_pci_device(struct pci_dev *pfx)
> +{
> +	struct pci_dev *pf0_pci_dev;
> +
> +	while ((pf0_pci_dev = pci_get_device(PCI_VENDOR_ID_SOLARFLARE,
> +					     X4_PF_DEVICE_ID, pf0_pci_dev))
> +					     != NULL) {

[Severity: High]
Since pf0_pci_dev is an uninitialized stack variable on the first iteration,
will pci_get_device() attempt to drop a reference on a wild pointer via its
internal pci_dev_put() call?

> +		/* With multiple X4 installed check against pci_slot as well. */
> +		if (pf0_pci_dev->slot == pfx->slot &&

[Severity: High]
Could the slot pointers be NULL on systems where PCI slots are not described
by firmware?

If so, a NULL == NULL comparison could inadvertently match the first PF0 of
any X4 card in the system, even if it is a completely different physical
adapter. Does this require checking bus or domain topology instead?

> +		    PCI_FUNC(pf0_pci_dev->devfn) == 0)
> +			break;
> +	}
> +	return pf0_pci_dev;
> +}

[ ... ]

> @@ -20,9 +62,48 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  	struct pci_dev *pci_dev = efx->pci_dev;
>  	struct range cxl_pio_range;
>  	struct efx_cxl *cxl;
> +	u8 devfn;
>  	u16 dvsec;
>  	int rc;
>  
> +	if (efx->type->is_vf)
> +		return 0;
> +
> +	/* are we PF0? */
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn != 0) {
> +		struct pci_dev *pf0_pci_dev;
> +		struct cxl_memdev *cxlmd;
> +
> +		pf0_pci_dev = get_pf0_pci_device(pci_dev);

[Severity: High]
Are we leaking the pf0_pci_dev reference here?

The pci_get_device() function increments the reference count of the returned
PCI device. Is there a missing pci_dev_put(pf0_pci_dev) after this function
is done using it?

> +
> +		/* This should not happen! */
> +		if (!pf0_pci_dev)
> +			return 0;
> +
> +		/* Is the PF0 device configured with and using CXL? */
> +		if (!pcie_is_cxl(pf0_pci_dev))
> +			return 0;
> +
> +		cxlmd = cxl_get_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev,
> +					   devfn, &cxl_pio_range);
> +
> +		if (IS_ERR(cxlmd))
> +			return -EPROBE_DEFER;
> +
> +		cxl = kzalloc_obj(struct cxl);

[Severity: High]
Is the struct type passed to kzalloc_obj() correct?

It looks like this allocates memory for 'struct cxl' instead of the intended
'struct efx_cxl'. Will this lead to heap out-of-bounds writes when fields like
cxl->cxlmd or cxl->ctpio_cxl are accessed?

> +		if (!cxl)
> +			return -ENOMEM;

[Severity: Medium]
If this allocation fails, does the code leak the sibling entry created earlier
by cxl_get_pf0_memdev()?

Should there be a call to cxl_put_pf0_memdev() before returning?

> +
> +		cxl->cxlmd = cxlmd;
> +
> +		if (!cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {

[Severity: High]
Is the return check for cxl_map() inverted here?

Looking at cxl_map(), it returns 0 on success. If cxl_map() succeeds, the
condition (!0) evaluates to true.

Does this mean the success path incorrectly triggers error handling, which
calls kfree(cxl) while leaving a dangling pointer in probe_data->cxl,
causing a use-after-free?

> +			kfree(cxl);
> +			return -ENOMEM;
> +		}
> +		return 0;
> +	}

[ ... ]

> @@ -80,25 +161,38 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  		return PTR_ERR(cxl->cxlmd);
>  	}
>  
> -	cxl->ctpio_cxl = ioremap_wc(cxl_pio_range.start,
> -				    range_len(&cxl_pio_range));
> -	if (!cxl->ctpio_cxl) {
> +	if (!cxl_map(probe_data, cxl, 0, cxl_pio_range)) {

[Severity: High]
Does this have the same inverted return logic as the non-PF0 case?

If cxl_map() returns 0 on success, will this evaluate to true on success,
incorrectly logging an error and returning -ENOMEM?

>  		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
>  			&cxl_pio_range);
>  		return -ENOMEM;
>  	}
>  
> -	probe_data->cxl_pio_initialised = true;
> -	probe_data->cxl = cxl;
> -
>  	return 0;
>  }
>  
>  void efx_cxl_exit(struct efx_probe_data *probe_data)
>  {
> +	struct efx_nic *efx = &probe_data->efx;
> +	struct pci_dev *pci_dev = efx->pci_dev;
> +	u8 devfn;
> +
>  	if (!probe_data->cxl)
>  		return;
>  
> +	/* are we PF0? */
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn != 0) {
> +		struct pci_dev *pf0_pci_dev;
> +
> +		pf0_pci_dev = get_pf0_pci_device(pci_dev);
> +
> +		/* This should not happen! */
> +		if (!pf0_pci_dev)
> +			return;

[Severity: Medium]
If this early return is taken, does it bypass the trailing iounmap() call
and permanently leak the CXL PIO mapping?

> +
> +		cxl_put_pf0_memdev(&pf0_pci_dev->dev, &pci_dev->dev, devfn);

[Severity: High]
Does get_pf0_pci_device() leak another reference to the PCI device here,
similar to the init path?

> +	}
> +
>  	iounmap(probe_data->cxl->ctpio_cxl);

[Severity: Medium]
For non-PF0 devices, probe_data->cxl is allocated dynamically using
kzalloc_obj() during initialization. Is there a missing kfree(probe_data->cxl)
in this teardown path?

>  }

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