Re: [PATCH] PCI/P2PDMA: Use RCU_INIT_POINTER() to clear pdev->p2pdma

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] pci_p2pdma_release() skips synchronize_rcu() when p2pdma->pool is NULL, causing a use-after-free of the devm-allocated p2pdma structure and corruption of its map_types xarray for concurrent RCU readers.
- [High] Missing devres action removal in pci_p2pdma_add_resource() error paths causes Use-After-Free of p2p_pgmap during devres teardown.
--

commit eb4ec2c32d602512abbeccbe720637cf83f85cb6
Author: Li RongQing <lirongqing@baidu.com>

PCI/P2PDMA: Use RCU_INIT_POINTER() to clear pdev->p2pdma

Since pdev->p2pdma is an RCU-protected pointer, directly assigning
NULL to it violates RCU semantics and triggers sparse static analysis
warnings.

Fix this by using RCU_INIT_POINTER() instead of a raw assignment,
ensuring proper RCU annotations and consistency with the subsequent
synchronize_rcu() call.

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 7c898542af8d5..fd98624208c50 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -236,7 +236,7 @@ static void pci_p2pdma_release(void *data)
>  		return;
>  
>  	/* Flush and disable pci_alloc_p2p_mem() */
> -	pdev->p2pdma = NULL;
> +	RCU_INIT_POINTER(pdev->p2pdma, NULL);
>  	if (p2pdma->pool)
>  		synchronize_rcu();

[Severity: High]
This isn't a bug introduced by this patch, but could skipping
synchronize_rcu() here result in a use-after-free for concurrent RCU readers?

If p2pdma->pool is NULL, synchronize_rcu() is bypassed before destroying the
map_types xarray and returning. Since the p2pdma structure is managed by
devres, it will be freed shortly after this callback returns.

If concurrent RCU readers (like those executing pci_p2pdma_map_type()) obtain
the pointer just before it is cleared, will they continue to access the freed
map_types xarray and p2pdma structure?

>  	xa_destroy(&p2pdma->map_types);


While reviewing this file, I noticed another pre-existing issue in
pci_p2pdma_add_resource():

drivers/pci/p2pdma.c:pci_p2pdma_add_resource()
{
[ ... ]
	error = devm_add_action_or_reset(&pdev->dev, pci_p2pdma_unmap_mappings,
					 p2p_pgmap);
	if (error)
		goto pages_free;

	p2pdma = rcu_dereference_protected(pdev->p2pdma, 1);
	error = gen_pool_add_owner(p2pdma->pool, (unsigned long)addr,
			pci_bus_address(pdev, bar) + offset,
			range_len(&pgmap->range), dev_to_node(&pdev->dev),
			&pgmap->ref);
	if (error)
		goto pages_free;
[ ... ]
pages_free:
	devm_memunmap_pages(&pdev->dev, pgmap);
pgmap_free:
	devm_kfree(&pdev->dev, p2p_pgmap);

[Severity: High]
This is also a pre-existing issue, but are we leaving a dangling pointer in
the devres list here?

If gen_pool_add_owner() fails, the code jumps to the pages_free label and
explicitly frees p2p_pgmap via devm_kfree(). However, the devres action
pci_p2pdma_unmap_mappings is not removed.

During devres teardown, the registered action will execute and call
pci_p2pdma_unmap_mappings() with the freed p2p_pgmap pointer. This
dereferences p2p_pgmap->mem->owner->kobj, resulting in a use-after-free.

Would it be safer to use devm_remove_action() here to prevent this?

	return error;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612083452.1639-1-lirongqing@baidu.com?part=1