[PATCH] PCI/P2PDMA: Fix freeing dev_pagemap on error

[PATCH] PCI/P2PDMA: Fix freeing dev_pagemap on error

[Keith Busch]

The devres_free() API is only to be used after the resource has been
unlinked from the device tracking it. Calling this functino directly will
hit a kernel BUG_ON. This patch fixes this to use the managed resource
release function, devm_kfree().

Fixes: 1380472e7b855 ("PCI/P2PDMA: Support peer-to-peer memory")
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/p2pdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index da66c7e31730..9c56a10eff15 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -222,7 +222,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 	return 0;
 
 pgmap_free:
-	devres_free(pgmap);
+	devm_kfree(&pci->dev, pgmap);
 	return error;
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
-- 
2.14.4

Re: [PATCH] PCI/P2PDMA: Fix freeing dev_pagemap on error

[Keith Busch]

On Fri, Oct 12, 2018 at 09:52:19AM -0600, Keith Busch wrote:
> The devres_free() API is only to be used after the resource has been
> unlinked from the device tracking it. Calling this functino directly will
> hit a kernel BUG_ON. This patch fixes this to use the managed resource
> release function, devm_kfree().
> 
> Fixes: 1380472e7b855 ("PCI/P2PDMA: Support peer-to-peer memory")
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/p2pdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index da66c7e31730..9c56a10eff15 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -222,7 +222,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  	return 0;
>  
>  pgmap_free:
> -	devres_free(pgmap);
> +	devm_kfree(&pci->dev, pgmap);
                    ^^^

That should be 'pdev', not 'pci'. I need to slow down the workflow, I'm
pointing send-email to the wrong patches again...

Re: [PATCH] PCI/P2PDMA: Fix freeing dev_pagemap on error

[Logan Gunthorpe]

On 2018-10-12 9:52 a.m., Keith Busch wrote:
> The devres_free() API is only to be used after the resource has been
> unlinked from the device tracking it. Calling this functino directly will
> hit a kernel BUG_ON. This patch fixes this to use the managed resource
> release function, devm_kfree().
> 
> Fixes: 1380472e7b855 ("PCI/P2PDMA: Support peer-to-peer memory")
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Oops, yes, nice catch! Thanks!

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>


> ---
>  drivers/pci/p2pdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index da66c7e31730..9c56a10eff15 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -222,7 +222,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  	return 0;
>  
>  pgmap_free:
> -	devres_free(pgmap);
> +	devm_kfree(&pci->dev, pgmap);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
> 

Re: [PATCH] PCI/P2PDMA: Fix freeing dev_pagemap on error

[Keith Busch]

On Fri, Oct 12, 2018 at 09:58:14AM -0600, Logan Gunthorpe wrote:
> On 2018-10-12 9:52 a.m., Keith Busch wrote:
> > The devres_free() API is only to be used after the resource has been
> > unlinked from the device tracking it. Calling this functino directly will
> > hit a kernel BUG_ON. This patch fixes this to use the managed resource
> > release function, devm_kfree().
> > 
> > Fixes: 1380472e7b855 ("PCI/P2PDMA: Support peer-to-peer memory")
> > Cc: Logan Gunthorpe <logang@deltatee.com>
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> 
> Oops, yes, nice catch! Thanks!

Yeah, I hit the bug after an nvme controller reset. The nvme driver
tries to add its CMB memory a second time, and it's correctly detected
as an error by devm_memremap_pages(), but I'm also thinking nvme should
have known better too.

Anyway, this patch (its v2, really) is the simple error handling fix,
but there will be a follow up nvme patch to avoid the error in the first
place. :)