LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] device core: fix dma_mask handling in platform_device_register_full
@ 2020-03-11 16:07 Christoph Hellwig
  2020-03-11 16:10 ` Artem S. Tashkinov
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-03-11 16:07 UTC (permalink / raw)
  To: torvalds, gregkh; +Cc: aros, linux-kernel, iommu

Ever since the generic platform device code started allocating DMA masks
itself the code to allocate and leak a private DMA mask in
platform_device_register_full has been superflous.  More so the fact that
it unconditionally frees the DMA mask allocation in the failure path
can lead to slab corruption if the function fails later on for a device
where it didn't allocate the mask.  Just remove the offending code.

Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
Reported-by: Artem S. Tashkinov <aros@gmx.com>
Tested-by: Artem S. Tashkinov <aros@gmx.com>
---
 drivers/base/platform.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7fa654f1288b..47d3e6187a1c 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -662,19 +662,6 @@ struct platform_device *platform_device_register_full(
 	pdev->dev.of_node_reused = pdevinfo->of_node_reused;
 
 	if (pdevinfo->dma_mask) {
-		/*
-		 * This memory isn't freed when the device is put,
-		 * I don't have a nice idea for that though.  Conceptually
-		 * dma_mask in struct device should not be a pointer.
-		 * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
-		 */
-		pdev->dev.dma_mask =
-			kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
-		if (!pdev->dev.dma_mask)
-			goto err;
-
-		kmemleak_ignore(pdev->dev.dma_mask);
-
 		*pdev->dev.dma_mask = pdevinfo->dma_mask;
 		pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
 	}
@@ -700,7 +687,6 @@ struct platform_device *platform_device_register_full(
 	if (ret) {
 err:
 		ACPI_COMPANION_SET(&pdev->dev, NULL);
-		kfree(pdev->dev.dma_mask);
 		platform_device_put(pdev);
 		return ERR_PTR(ret);
 	}
-- 
2.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 16:07 [PATCH] device core: fix dma_mask handling in platform_device_register_full Christoph Hellwig
@ 2020-03-11 16:10 ` Artem S. Tashkinov
  2020-03-11 16:14 ` Greg KH
  2020-03-11 16:19 ` Robin Murphy
  2 siblings, 0 replies; 8+ messages in thread
From: Artem S. Tashkinov @ 2020-03-11 16:10 UTC (permalink / raw)
  To: Christoph Hellwig, torvalds, gregkh; +Cc: linux-kernel, iommu

On 3/11/20 4:07 PM, Christoph Hellwig wrote:
> Ever since the generic platform device code started allocating DMA masks
> itself the code to allocate and leak a private DMA mask in
> platform_device_register_full has been superflous.  More so the fact that
> it unconditionally frees the DMA mask allocation in the failure path
> can lead to slab corruption if the function fails later on for a device
> where it didn't allocate the mask.  Just remove the offending code.
>
> Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> Reported-by: Artem S. Tashkinov <aros@gmx.com>
> Tested-by: Artem S. Tashkinov <aros@gmx.com>
> ---
>   drivers/base/platform.c | 14 --------------
>   1 file changed, 14 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 7fa654f1288b..47d3e6187a1c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -662,19 +662,6 @@ struct platform_device *platform_device_register_full(
>   	pdev->dev.of_node_reused = pdevinfo->of_node_reused;
>
>   	if (pdevinfo->dma_mask) {
> -		/*
> -		 * This memory isn't freed when the device is put,
> -		 * I don't have a nice idea for that though.  Conceptually
> -		 * dma_mask in struct device should not be a pointer.
> -		 * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> -		 */
> -		pdev->dev.dma_mask =
> -			kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> -		if (!pdev->dev.dma_mask)
> -			goto err;
> -
> -		kmemleak_ignore(pdev->dev.dma_mask);
> -
>   		*pdev->dev.dma_mask = pdevinfo->dma_mask;
>   		pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
>   	}
> @@ -700,7 +687,6 @@ struct platform_device *platform_device_register_full(
>   	if (ret) {
>   err:
>   		ACPI_COMPANION_SET(&pdev->dev, NULL);
> -		kfree(pdev->dev.dma_mask);
>   		platform_device_put(pdev);
>   		return ERR_PTR(ret);
>   	}
>

I'd love to see this patch applied to the next round of stable kernels
(at the very least 5.5.x) ASAP because I've been stuck with kernel 5.3
on Fedora for quite some time already.

Thank you!


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 16:07 [PATCH] device core: fix dma_mask handling in platform_device_register_full Christoph Hellwig
  2020-03-11 16:10 ` Artem S. Tashkinov
@ 2020-03-11 16:14 ` Greg KH
  2020-03-11 16:15   ` Christoph Hellwig
  2020-03-11 16:19 ` Robin Murphy
  2 siblings, 1 reply; 8+ messages in thread
From: Greg KH @ 2020-03-11 16:14 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: torvalds, aros, linux-kernel, iommu

On Wed, Mar 11, 2020 at 05:07:10PM +0100, Christoph Hellwig wrote:
> Ever since the generic platform device code started allocating DMA masks
> itself the code to allocate and leak a private DMA mask in
> platform_device_register_full has been superflous.  More so the fact that
> it unconditionally frees the DMA mask allocation in the failure path
> can lead to slab corruption if the function fails later on for a device
> where it didn't allocate the mask.  Just remove the offending code.
> 
> Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> Reported-by: Artem S. Tashkinov <aros@gmx.com>
> Tested-by: Artem S. Tashkinov <aros@gmx.com>

No s-o-b from you?  :(

I can take this, or Linus, you can take this now if you want to as well:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 16:14 ` Greg KH
@ 2020-03-11 16:15   ` Christoph Hellwig
  2020-03-11 17:18     ` Greg KH
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2020-03-11 16:15 UTC (permalink / raw)
  To: Greg KH; +Cc: Christoph Hellwig, torvalds, aros, linux-kernel, iommu

On Wed, Mar 11, 2020 at 05:14:23PM +0100, Greg KH wrote:
> On Wed, Mar 11, 2020 at 05:07:10PM +0100, Christoph Hellwig wrote:
> > Ever since the generic platform device code started allocating DMA masks
> > itself the code to allocate and leak a private DMA mask in
> > platform_device_register_full has been superflous.  More so the fact that
> > it unconditionally frees the DMA mask allocation in the failure path
> > can lead to slab corruption if the function fails later on for a device
> > where it didn't allocate the mask.  Just remove the offending code.
> > 
> > Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> > Reported-by: Artem S. Tashkinov <aros@gmx.com>
> > Tested-by: Artem S. Tashkinov <aros@gmx.com>
> 
> No s-o-b from you?  :(
> 
> I can take this, or Linus, you can take this now if you want to as well:

Sorry, here it is:

Signed-off-by: Christoph Hellwig <hch@lst.de>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 16:07 [PATCH] device core: fix dma_mask handling in platform_device_register_full Christoph Hellwig
  2020-03-11 16:10 ` Artem S. Tashkinov
  2020-03-11 16:14 ` Greg KH
@ 2020-03-11 16:19 ` Robin Murphy
  2 siblings, 0 replies; 8+ messages in thread
From: Robin Murphy @ 2020-03-11 16:19 UTC (permalink / raw)
  To: Christoph Hellwig, torvalds, gregkh; +Cc: aros, linux-kernel, iommu

On 11/03/2020 4:07 pm, Christoph Hellwig wrote:
> Ever since the generic platform device code started allocating DMA masks
> itself the code to allocate and leak a private DMA mask in
> platform_device_register_full has been superflous.  More so the fact that
> it unconditionally frees the DMA mask allocation in the failure path
> can lead to slab corruption if the function fails later on for a device
> where it didn't allocate the mask.  Just remove the offending code.

I'm sure I mentioned this in passing at the time, but only in the 
context of a cleanup; I never noticed it could be cause for an actual bug :)

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> Reported-by: Artem S. Tashkinov <aros@gmx.com>
> Tested-by: Artem S. Tashkinov <aros@gmx.com>
> ---
>   drivers/base/platform.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 7fa654f1288b..47d3e6187a1c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -662,19 +662,6 @@ struct platform_device *platform_device_register_full(
>   	pdev->dev.of_node_reused = pdevinfo->of_node_reused;
>   
>   	if (pdevinfo->dma_mask) {
> -		/*
> -		 * This memory isn't freed when the device is put,
> -		 * I don't have a nice idea for that though.  Conceptually
> -		 * dma_mask in struct device should not be a pointer.
> -		 * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> -		 */
> -		pdev->dev.dma_mask =
> -			kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> -		if (!pdev->dev.dma_mask)
> -			goto err;
> -
> -		kmemleak_ignore(pdev->dev.dma_mask);
> -
>   		*pdev->dev.dma_mask = pdevinfo->dma_mask;
>   		pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
>   	}
> @@ -700,7 +687,6 @@ struct platform_device *platform_device_register_full(
>   	if (ret) {
>   err:
>   		ACPI_COMPANION_SET(&pdev->dev, NULL);
> -		kfree(pdev->dev.dma_mask);
>   		platform_device_put(pdev);
>   		return ERR_PTR(ret);
>   	}
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 16:15   ` Christoph Hellwig
@ 2020-03-11 17:18     ` Greg KH
  2020-03-11 17:23       ` Christoph Hellwig
  2020-03-11 17:25       ` Linus Torvalds
  0 siblings, 2 replies; 8+ messages in thread
From: Greg KH @ 2020-03-11 17:18 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: torvalds, aros, linux-kernel, iommu

On Wed, Mar 11, 2020 at 05:15:51PM +0100, Christoph Hellwig wrote:
> On Wed, Mar 11, 2020 at 05:14:23PM +0100, Greg KH wrote:
> > On Wed, Mar 11, 2020 at 05:07:10PM +0100, Christoph Hellwig wrote:
> > > Ever since the generic platform device code started allocating DMA masks
> > > itself the code to allocate and leak a private DMA mask in
> > > platform_device_register_full has been superflous.  More so the fact that
> > > it unconditionally frees the DMA mask allocation in the failure path
> > > can lead to slab corruption if the function fails later on for a device
> > > where it didn't allocate the mask.  Just remove the offending code.
> > > 
> > > Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> > > Reported-by: Artem S. Tashkinov <aros@gmx.com>
> > > Tested-by: Artem S. Tashkinov <aros@gmx.com>
> > 
> > No s-o-b from you?  :(
> > 
> > I can take this, or Linus, you can take this now if you want to as well:
> 
> Sorry, here it is:
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Is this still needed with the patch that Linus just committed to his
tree?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 17:18     ` Greg KH
@ 2020-03-11 17:23       ` Christoph Hellwig
  2020-03-11 17:25       ` Linus Torvalds
  1 sibling, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-03-11 17:23 UTC (permalink / raw)
  To: Greg KH; +Cc: Christoph Hellwig, torvalds, aros, linux-kernel, iommu

On Wed, Mar 11, 2020 at 06:18:02PM +0100, Greg KH wrote:
> > Sorry, here it is:
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Is this still needed with the patch that Linus just committed to his
> tree?

No.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] device core: fix dma_mask handling in platform_device_register_full
  2020-03-11 17:18     ` Greg KH
  2020-03-11 17:23       ` Christoph Hellwig
@ 2020-03-11 17:25       ` Linus Torvalds
  1 sibling, 0 replies; 8+ messages in thread
From: Linus Torvalds @ 2020-03-11 17:25 UTC (permalink / raw)
  To: Greg KH
  Cc: Christoph Hellwig, Artem S. Tashkinov, Linux Kernel Mailing List, iommu

On Wed, Mar 11, 2020 at 10:18 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Is this still needed with the patch that Linus just committed to his
> tree?

My patch is basically the same, just with the field renamed too, and
not blindly just assigning to "*pdev->dev.dma_mask" (my variant does

        pdev->platform_dma_mask = pdevinfo->dma_mask;
        pdev->dev.dma_mask = &pdev->platform_dma_mask;

instead of that incomprehensible

        *pdev->dev.dma_mask = pdevinfo->dma_mask;

which depends on that dev.dma_mask pointer having been initialized in
a random place earlier).

I had the cleanups (uncommited) in my tree, and just removed the
kfree() as per Christoph.

             Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-03-11 17:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 16:07 [PATCH] device core: fix dma_mask handling in platform_device_register_full Christoph Hellwig
2020-03-11 16:10 ` Artem S. Tashkinov
2020-03-11 16:14 ` Greg KH
2020-03-11 16:15   ` Christoph Hellwig
2020-03-11 17:18     ` Greg KH
2020-03-11 17:23       ` Christoph Hellwig
2020-03-11 17:25       ` Linus Torvalds
2020-03-11 16:19 ` Robin Murphy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).