Re: [PATCH v4 05/18] PCI/P2PDMA: Document the pdev->p2pdma lifetime and RCU rules

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

Pre-existing issues:
- [High] Use-after-free of `p2p_pgmap` in `pci_alloc_p2pmem()` due to incorrect devres execution ordering.
--

--- Patch [5]: [PATCH v4 05/18] PCI/P2PDMA: Document the pdev->p2pdma lifetime and RCU rules ---
commit 158fe7a750e98e97967829dbed7785e91ba2a835
Author: Leon Romanovsky <[email protected]>

PCI/P2PDMA: Document the pdev->p2pdma lifetime and RCU rules

This commit documents the lifetime, publication, and teardown rules
for pdev->p2pdma, clarifying how the RCU grace period protects the
pci_p2pdma object and its gen_pool from concurrent access during
driver unbind.

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index a77ef9deb3c65..49bc8cf062409 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
[ ... ]
> @@ -235,9 +268,19 @@ static void pci_p2pdma_release(void *data)
>  	if (!p2pdma)
>  		return;
>  
> -	/* Flush and disable pci_alloc_p2p_mem() */
> +	/*
> +	 * Stop new RCU readers and wait for readers that observed p2pdma before
> +	 * allowing devres to free it. This is required even without a pool,
> +	 * because pci_has_p2pmem() dereferences every non-NULL p2pdma it finds.
> +	 * For a pool-backed provider this also fences gen_pool_destroy().
> +	 */

[Severity: High]
This is a pre-existing issue, but could this devres teardown ordering
lead to a use-after-free of p2p_pgmap in pci_alloc_p2pmem()?

Since pcim_p2pdma_init() registers pci_p2pdma_release() before
pci_p2pdma_add_resource() allocates p2p_pgmap via devm_kzalloc(),
devres LIFO ordering means p2p_pgmap is freed before pci_p2pdma_release()
is executed to clear pdev->p2pdma and destroy the gen_pool.

If a concurrent call to pci_alloc_p2pmem() occurs while the provider
driver unbinds, could it retrieve a chunk from gen_pool pointing to the
already freed p2p_pgmap->ref and pass it to percpu_ref_tryget_live_rcu(ref),
dereferencing a dangling pointer?

>  	RCU_INIT_POINTER(pdev->p2pdma, NULL);
>  	synchronize_rcu();
> +
> +	/*
> +	 * The grace period also ensures no RCU reader can still be accessing
> +	 * map_types here.
> +	 */
>  	xa_destroy(&p2pdma->map_types);

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