Re: [PATCH v6 3/4] PCI: endpoint: Add support for DOE initialization and setup in EPC core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Unprotected check of epc->init_complete causes a TOCTOU race in pci_epc_init_notify() and pci_epc_deinit_notify(), leading to double initialization, memory leaks, and xarray corruption.
- [High] Logic Flaw: DOE mailboxes are never initialized because pci_epc_init_notify() is called before functions are bound.
- [High] Incorrect teardown ordering in pci_epc_deinit_notify() causes a Use-After-Free during shutdown.

Pre-existing issues:
- [High] TOCTOU data race in pci_epc_notify_pending_init() sends spurious epc_init events after deinitialization.
--

commit 1a369cd1d0a1ffe8ebc7c7242501b1fde9e422c9
Author: Aksh Garg <a-garg7@ti.com>

PCI: endpoint: Add support for DOE initialization and setup in EPC core
    
Add pci_epc_init_capabilities() in EPC core driver to initialize and setup
capabilities supported by the EPC driver. This calls pci_epc_doe_setup() to
setup the DOE framework for an endpoint controller, which discovers DOE
capabilities and registers mailboxes. It also adds cleanup.

> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
> index 6c3c58185fc5d..96bd624559f2a 100644
> --- a/drivers/pci/endpoint/pci-epc-core.c
> +++ b/drivers/pci/endpoint/pci-epc-core.c
[ ... ]
> @@ -842,6 +844,78 @@ void pci_epc_linkdown(struct pci_epc *epc)
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_linkdown);
>  
> +/**
> + * pci_epc_doe_setup() - Discover and setup DOE mailboxes for all functions
> + * @epc: the EPC device on which DOE mailboxes has to be setup
> + *
> + * Discover DOE (Data Object Exchange) capabilities for all physical
> + * functions in the endpoint controller and register DOE mailboxes.
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +static int pci_epc_doe_setup(struct pci_epc *epc)
> +{
> +	u8 func_no, vfunc_no = 0;
> +	u16 cap_offset;
> +	int ret;
> +
> +	if (!epc->ops || !epc->ops->find_ext_capability)
> +		return -EINVAL;
> +
> +	/* Discover DOE capabilities for all functions */
> +	for (func_no = 0; func_no < epc->max_functions; func_no++) {
> +		mutex_lock(&epc->lock);
> +		cap_offset = epc->ops->find_ext_capability(epc, func_no,
> +							   vfunc_no, 0,
> +							   PCI_EXT_CAP_ID_DOE);

[Severity: High]
Does hardcoding vfunc_no to 0 completely omit DOE mailbox discovery and
initialization for SR-IOV virtual functions?

> +		mutex_unlock(&epc->lock);
> +
> +		while (cap_offset) {
[ ... ]
> +		}
> +	}
> +
> +	dev_dbg(&epc->dev, "DOE mailboxes setup complete\n");
> +	return 0;
> +}
> +
> +/**
> + * pci_epc_init_capabilities() - Initialize EPC capabilities
> + * @epc: the EPC device whose capabilities need to be initialized
> + *
> + * Initialize capabilities supported by the EPC device.
> + */
> +static void pci_epc_init_capabilities(struct pci_epc *epc)
> +{
> +	const struct pci_epc_features *epc_features;
> +	int ret;
> +
> +	epc_features = pci_epc_get_features(epc, 0, 0);
> +	if (!epc_features)
> +		return;

[Severity: High]
Could pci_epc_get_features() return NULL here and silently bypass DOE
initialization?

If pci_epc_init_notify() is invoked during controller probe before any
Endpoint Function (EPF) drivers are bound, pci_epc_get_features() may fail.

Since the functions are not bound yet, the function_num_map could be 0,
causing the initialization to abort immediately without setting up the
mailboxes.

> +
> +	if (IS_ENABLED(CONFIG_PCI_ENDPOINT_DOE) && epc_features->doe_capable) {
[ ... ]
> +	}
> +}
> +
>  /**
>   * pci_epc_init_notify() - Notify the EPF device that EPC device initialization
>   *                         is completed.
> @@ -857,6 +931,9 @@ void pci_epc_init_notify(struct pci_epc *epc)
>  	if (IS_ERR_OR_NULL(epc))
>  		return;
>  
> +	if (!epc->init_complete)
> +		pci_epc_init_capabilities(epc);
> +

[Severity: High]
Can a concurrent caller hit this unprotected check, bypassing the intended
protection?

Because the epc->init_complete condition is evaluated locklessly outside
epc->list_lock, multiple threads could proceed to initialize the
capabilities.

This double initialization might lead to memory leaks, as xa_init() would
reset the xarray pointers without freeing the already allocated elements.

>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
[ ... ]
> @@ -890,6 +967,27 @@ void pci_epc_notify_pending_init(struct pci_epc *epc, struct pci_epf *epf)
>  }
>  EXPORT_SYMBOL_GPL(pci_epc_notify_pending_init);

[Severity: High]
This isn't a bug introduced by this patch, but does the lockless check of
init_complete in pci_epc_notify_pending_init() allow a race condition
with deinitialization?

If a hardware link-down event triggers pci_epc_deinit_notify()
concurrently, pci_epc_notify_pending_init() could read the flag before it
is cleared, and subsequently send spurious epc_init events after the
hardware has been stopped.

> +
> +/**
> + * pci_epc_deinit_capabilities() - Clean up EPC capabilities
[ ... ]
> @@ -903,6 +1001,9 @@ void pci_epc_deinit_notify(struct pci_epc *epc)
>  	if (IS_ERR_OR_NULL(epc))
>  		return;
>  
> +	if (epc->init_complete)
> +		pci_epc_deinit_capabilities(epc);
> +

[Severity: High]
Does this unprotected check of init_complete allow multiple threads to
concurrently destroy the mailboxes, potentially leading to double-free
crashes?

>  	mutex_lock(&epc->list_lock);
>  	list_for_each_entry(epf, &epc->pci_epf, list) {
>  		mutex_lock(&epf->lock);
>  		if (epf->event_ops && epf->event_ops->epc_deinit)
>  			epf->event_ops->epc_deinit(epf);
>  		mutex_unlock(&epf->lock);
>  	}

[Severity: High]
Does freeing the capability before iterating over the EPF list to send
the epc_deinit notification cause a use-after-free?

If an EPF driver attempts to flush or interact with a DOE mailbox during
its shutdown callback, it would access already freed memory. Teardown
generally needs to be the exact reverse of initialization, notifying EPFs
to stop using the hardware before destroying the underlying capabilities.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623090737.711656-1-a-garg7@ti.com?part=3