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 <[email protected]> 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/[email protected]?part=1
