Re: [PATCH v5 1/9] PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Skipping `synchronize_rcu()` when `p2pdma->pool` is NULL causes a Use-After-Free of the devres-managed `pci_p2pdma` structure and its `map_types` xarray for concurrent RCU readers.
--

--- Patch [1]: [PATCH v5 1/9] PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release() ---
commit d0b08915340cc1e9c83ccd0a7d506d8fb8505da6
Author: Matt Evans <matt@ozlabs.org>

PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release()

This patch splits the pool-related release code into a new
pci_p2pdma_release_pool() function. This prepares for future compile-time
selection of a null implementation during planned refactoring.

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index b2d5266f86530..498bca2574193 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c

[ ... ]

> @@ -226,6 +226,17 @@ static const struct dev_pagemap_ops p2pdma_pgmap_ops = {
>  	.folio_free = p2pdma_folio_free,
>  };
>  
> +static void pci_p2pdma_release_pool(struct pci_dev *pdev,
> +				    struct pci_p2pdma *p2pdma)
> +{
> +	if (!p2pdma->pool)
> +		return;
> +
> +	synchronize_rcu();

[Severity: High]
This is a pre-existing issue, but does skipping synchronize_rcu() here when
the pool is empty cause a use-after-free for concurrent RCU readers?

If a driver initializes P2P DMA but does not add resources, the pool
remains uninitialized. When the driver unbinds, devres cleanup triggers
pci_p2pdma_release(), which clears pdev->p2pdma and then calls this helper.

Since the pool is missing, the early return entirely bypasses the RCU grace
period. When pci_p2pdma_release() finishes, devres immediately frees the
pci_p2pdma structure.

Concurrently, another thread executing pci_p2pdma_map_type() might take the
RCU read lock, dereference the old pdev->p2pdma pointer, and access the
freed memory via the map_types xarray.

Moving synchronize_rcu() into this pool-specific helper seems to imply that
the grace period only protects the pool, when it actually protects the
overall pci_p2pdma structure. Should synchronize_rcu() be moved back out to
pci_p2pdma_release() so it executes unconditionally?

> +	gen_pool_destroy(p2pdma->pool);
> +	sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
> +}
> +
>  static void pci_p2pdma_release(void *data)
>  {
>  	struct pci_dev *pdev = data;
> @@ -237,15 +248,8 @@ static void pci_p2pdma_release(void *data)
>  
>  	/* Flush and disable pci_alloc_p2p_mem() */
>  	pdev->p2pdma = NULL;
> -	if (p2pdma->pool)
> -		synchronize_rcu();
> +	pci_p2pdma_release_pool(pdev, p2pdma);
>  	xa_destroy(&p2pdma->map_types);
> -
> -	if (!p2pdma->pool)
> -		return;
> -
> -	gen_pool_destroy(p2pdma->pool);
> -	sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715174737.15287-1-matt@ozlabs.org?part=1