Re: [PATCH v4 01/18] PCI/P2PDMA: Do not tear down the allocate attribute on registration failure

[Logan Gunthorpe]

On 2026-08-21 13:38, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@xxxxxxxxxx>
> 
> pci_p2pdma_add_resource() installs pci_p2pdma_unmap_mappings() as a devres
> action with the devres allocated p2p_pgmap as its data, and only then adds
> the range to the pool:
> 
> 	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, ...);
> 	if (error)
> 		goto pages_free;
> 
> The action removes the allocate attribute for the whole device, which
> tears down existing userspace mappings of every BAR already registered on
> it. Both failures here get that wrong, in opposite ways.
> 
> devm_add_action_or_reset() runs the action when it cannot allocate its
> devres node, so an -ENOMEM while registering a second BAR unmaps the
> first one. Use devm_add_action() and let the error path unwind only what
> this call created.
> 
> gen_pool_add_owner() allocates a chunk and can also fail with -ENOMEM.
> There the action is registered, and the error path frees p2p_pgmap with
> devm_kfree() while leaving the action pointing at it. On unbind devres
> runs the action and pci_p2pdma_unmap_mappings() dereferences
> p2p_pgmap->mem->owner->kobj, which is freed memory. Give that failure its
> own label and drop the action with devm_remove_action(), which removes it
> without running it.
> 
> Tested-by: Tushar Dave <tdave@xxxxxxxxxx>
> Fixes: 7e9c7ef83d78 ("PCI/P2PDMA: Allow userspace VMA allocations through sysfs")
> Fixes: f58ef9d1d135 ("PCI/P2PDMA: Separate the mmap() support from the core logic")
> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>

Makes sense to me:

Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>