Skip to contentSkip to conversation

usb: gadget: udc: add return value check of kzalloc in mv_udc_probe

PATCH Superseded void0red 4 participants 13 messages Feb 24, 2023 – Feb 25, 2023 USB

lore
Share & tools
lore

void0red

author
#0 lore raw

PATCH 1/1 usb: gadget: udc: add return value check of kzalloc in mv_udc_probe

From: Kang Chen <[email protected]>

Even an 8-byte kzalloc will fail when we don't have enough memory, so we need a nullptr check and do the cleanup when it fails.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/gadget/udc/mv_udc_core.c +4 -0
index b397f3a84..6dd6d52de 100644
@@ -2230,6 +2230,10 @@ static int mv_udc_probe(struct platform_device *pdev)
 
 	/* allocate a small amount of memory to get valid address */
 	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1
#1 lore raw
Quoted 19 lines · On Fri, Feb 24, 2023 at 5:28 PM void0red <[email protected]> wrote:

From: Kang Chen <[email protected]>

Even an 8-byte kzalloc will fail when we don't have enough memory, so we need a nullptr check and do the cleanup when it fails.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/gadget/udc/mv_udc_core.c +0 -0
index b397f3a84..6dd6d52de 100644

drivers/usb/gadget/udc/mv_udc_core.c static int mv_udc_probe(struct platform_device *pdev) · line 2232

Quoted 3 lines
drivers/usb/gadget/udc/mv_udc_core.c +0 -0
@@ -2230,6 +2230,10 @@ static int mv_udc_probe(struct platform_device *pdev)
 
        /* allocate a small amount of memory to get valid address */
        udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);

Hi Kang and gregkh,

I think there is a memory leak in this kzalloc. It seems there is no deallocation for this allocated object.

As the surrounding allocation statements suggest,
we should turn kzalloc to devm_kzalloc.

Quoted 10 lines
drivers/usb/gadget/udc/mv_udc_core.c +4 -1
+       if (!udc->status_req->req.buf) {
+               retval = -ENOMEM;
+               goto err_destroy_dma;
+       }
        udc->status_req->req.dma = DMA_ADDR_INVALID;
 
        udc->resume_state = USB_STATE_NOTATTACHED;
--

2.34.1

author
#2 lore raw

Hi, Dongliang,

I totally agree with you. I checked the other drivers using status_req->req.buf structure, they free the memory when the driver removed. But in this driver, I can't find such code. So, as you said, it needs a devm_kazlloc instead of a kzalloc to manage the memory and avoid a memory leak.

Thanks for your correction. I will post a new patch later.

Quoted 43 lines · Dongliang Mu <[email protected]> 于2023年2月24日周五 19:06写道：
Quoted 19 lines · On Fri, Feb 24, 2023 at 5:28 PM void0red <[email protected]> wrote:

From: Kang Chen <[email protected]>

Even an 8-byte kzalloc will fail when we don't have enough memory, so we need a nullptr check and do the cleanup when it fails.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/gadget/udc/mv_udc_core.c +0 -0
index b397f3a84..6dd6d52de 100644

drivers/usb/gadget/udc/mv_udc_core.c static int mv_udc_probe(struct platform_device *pdev) · line 2232

Quoted 3 lines
drivers/usb/gadget/udc/mv_udc_core.c +0 -0
@@ -2230,6 +2230,10 @@ static int mv_udc_probe(struct platform_device *pdev)
 
        /* allocate a small amount of memory to get valid address */
        udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);

Hi Kang and gregkh,

I think there is a memory leak in this kzalloc. It seems there is no deallocation for this allocated object.

As the surrounding allocation statements suggest,
we should turn kzalloc to devm_kzalloc.

Quoted 10 lines
drivers/usb/gadget/udc/mv_udc_core.c +4 -1
+       if (!udc->status_req->req.buf) {
+               retval = -ENOMEM;
+               goto err_destroy_dma;
+       }
        udc->status_req->req.dma = DMA_ADDR_INVALID;
 
        udc->resume_state = USB_STATE_NOTATTACHED;
--

2.34.1

author
#3 lore raw

PATCH 1/1 usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

In reply to Dongliang Mu

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +5 -1
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);
 
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1
maintainer
#4 lore raw

Re: [PATCH] usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

Quoted 34 lines · On Fri, Feb 24, 2023 at 09:02:35PM +0800, void0red wrote:

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +5 -1
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);
 
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

thanks,

greg k-h's patch email bot

#5 lore raw

Re: [PATCH] usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

Quoted 4 lines · Greg KH
Quoted 2 lines · On Fri, Feb 24, 2023 at 09:02:35PM +0800, void0red wrote:

From: Kang Chen <[email protected]>

Quoted 8 lines
Quoted 8 lines

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>

Please remember to send a v2 patch. Otherwise, gregkh's bot will notify you that this is an already-sent patch.

Quoted 52 lines
Quoted 22 lines
---
 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +5 -2
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
      INIT_LIST_HEAD(&udc->status_req->queue);
 
      /* allocate a small amount of memory to get valid address */
-     udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+     udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+     if (!udc->status_req->req.buf) {
+             retval = -ENOMEM;
+             goto err_destroy_dma;
+     }
      udc->status_req->req.dma = DMA_ADDR_INVALID;
 
      udc->resume_state = USB_STATE_NOTATTACHED;
--
Quoted 2 lines

2.34.1

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

thanks,

greg k-h's patch email bot

author
#6 lore raw

PATCH v2 1/1 usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +5 -1
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);
 
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1
maintainer
#7 lore raw

Re: [PATCH v2] usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

Quoted 34 lines · On Fri, Feb 24, 2023 at 10:11:01PM +0800, void0red wrote:

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +5 -1
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);
 
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.

You are receiving this message because of the following common error(s) as indicated below:

- This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.

thanks,

greg k-h's patch email bot

#8 lore raw

Re: [PATCH v2] usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

In reply to void0red

Hello!

Quoted 5 lines · On 2/24/23 5:11 PM, void0red wrote:

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

IMHO these are 2 different problems and deserve its own patch
fixing it...

Quoted 15 lines
Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +0 -0
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);

drivers/usb/gadget/udc/mv_udc_core.c static int mv_udc_probe(struct platform_device *pdev) · line 2239

Quoted 10 lines
drivers/usb/gadget/udc/mv_udc_core.c +5 -1
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;

MBR, Sergey

author
#9 lore raw

PATCH v3 1/2 usb: gadget: udc: add return value check of kzalloc in mv_udc_probe

From: Kang Chen <[email protected]>

Even an 8-byte kzalloc will fail when we don't have enough memory, so we need a nullptr check and do the cleanup when it fails.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>

---
v3 -> v2: split the patch into two
v2 -> v1: replace kzalloc with devm_kzalloc

 drivers/usb/gadget/udc/mv_udc_core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/gadget/udc/mv_udc_core.c +4 -0
index b397f3a84..6dd6d52de 100644
@@ -2230,6 +2230,10 @@ static int mv_udc_probe(struct platform_device *pdev)
 
 	/* allocate a small amount of memory to get valid address */
 	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1
author
#10 lore raw

PATCH v3 2/2 usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak.

Reported-by Dongliang Mu <[email protected]>
Link https://lore.kernel.org/all/CAD-N9QX5i5toj8cs7DxBjYWtRGf3ZRnfTAf809sFW6iX0Ktfmw@mail.gmail.com open here
Signed-off-by Kang Chen <[email protected]>

---
v3 -> v2: split the patch into two
v2 -> v1: replace kzalloc with devm_kzalloc

 drivers/usb/gadget/udc/mv_udc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +1 -1
index 6dd6d52de..08474c08d 100644
@@ -2229,7 +2229,7 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);
 
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
 	if (!udc->status_req->req.buf) {
 		retval = -ENOMEM;
 		goto err_destroy_dma;
Signature · 1 line
2.34.1
author
#11 lore raw

PATCH v2 1/1 usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

In reply to Dongliang Mu

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>

---
v2 -> v1: replace kzalloc with devm_kzalloc

 drivers/usb/gadget/udc/mv_udc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/gadget/udc/mv_udc_core.c +5 -1
index b397f3a84..08474c08d 100644
@@ -2229,7 +2229,11 @@ static int mv_udc_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&udc->status_req->queue);
 
 	/* allocate a small amount of memory to get valid address */
-	udc->status_req->req.buf = kzalloc(8, GFP_KERNEL);
+	udc->status_req->req.buf = devm_kzalloc(&pdev->dev, 8, GFP_KERNEL);
+	if (!udc->status_req->req.buf) {
+		retval = -ENOMEM;
+		goto err_destroy_dma;
+	}
 	udc->status_req->req.dma = DMA_ADDR_INVALID;
 
 	udc->resume_state = USB_STATE_NOTATTACHED;
Signature · 1 line
2.34.1
maintainer
#12 lore raw

Re: [PATCH v2] usb: gadget: udc: replace kzalloc with devm_kzalloc in mv_udc_probe and add a check for this allocation.

Quoted 6 lines · On Fri, Feb 24, 2023 at 10:31:48PM +0800, void0red wrote:

From: Kang Chen <[email protected]>

This driver uses the unified memory management api, so replace kzalloc with devm_kzalloc to avoid a memory leak and add a check for this allocation.

Quoted 6 lines
Reported-by eriri <[email protected]>
Link https://bugzilla.kernel.org/show_bug.cgi?id=217081
Signed-off-by Kang Chen <[email protected]>

---
v2 -> v1: replace kzalloc with devm_kzalloc

I see 2 v2 patches on the mailing list, how am I supposed to know which one is correct?

Please fix up and resend a v3.

thanks,

greg k-h