[PATCH v2] staging: gdm724x: Fix DMA from stack

[PATCH v2] staging: gdm724x: Fix DMA from stack

[ameynarkhede03]

From: Amey Narkhede <ameynarkhede03@gmail.com>

Stack allocated buffers cannot be used for DMA
on all architectures so allocate hci_packet buffer
using kzalloc().

Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
---
 drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
index dc4da66c3..c4a9b90c5 100644
--- a/drivers/staging/gdm724x/gdm_usb.c
+++ b/drivers/staging/gdm724x/gdm_usb.c
@@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,

 static int request_mac_address(struct lte_udev *udev)
 {
-	u8 buf[16] = {0,};
-	struct hci_packet *hci = (struct hci_packet *)buf;
+	u8 *buf;
+	struct hci_packet *hci;
 	struct usb_device *usbdev = udev->usbdev;
 	int actual;
 	int ret = -1;
+	buf = kzalloc(16, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+	hci = (struct hci_packet *)buf;

 	hci->cmd_evt = gdm_cpu_to_dev16(udev->gdm_ed, LTE_GET_INFORMATION);
 	hci->len = gdm_cpu_to_dev16(udev->gdm_ed, 1);
@@ -70,6 +74,7 @@ static int request_mac_address(struct lte_udev *udev)
 			   &actual, 1000);

 	udev->request_mac_addr = 1;
+	kfree(buf);

 	return ret;
 }
--
2.30.0

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Greg KH]

On Wed, Feb 10, 2021 at 01:01:47AM +0530, ameynarkhede03@gmail.com wrote:
> From: Amey Narkhede <ameynarkhede03@gmail.com>
> 
> Stack allocated buffers cannot be used for DMA
> on all architectures so allocate hci_packet buffer
> using kzalloc().
> 
> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> ---
>  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> index dc4da66c3..c4a9b90c5 100644
> --- a/drivers/staging/gdm724x/gdm_usb.c
> +++ b/drivers/staging/gdm724x/gdm_usb.c
> @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> 
>  static int request_mac_address(struct lte_udev *udev)
>  {
> -	u8 buf[16] = {0,};
> -	struct hci_packet *hci = (struct hci_packet *)buf;
> +	u8 *buf;
> +	struct hci_packet *hci;
>  	struct usb_device *usbdev = udev->usbdev;
>  	int actual;
>  	int ret = -1;
> +	buf = kzalloc(16, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +	hci = (struct hci_packet *)buf;
> 
>  	hci->cmd_evt = gdm_cpu_to_dev16(udev->gdm_ed, LTE_GET_INFORMATION);
>  	hci->len = gdm_cpu_to_dev16(udev->gdm_ed, 1);
> @@ -70,6 +74,7 @@ static int request_mac_address(struct lte_udev *udev)
>  			   &actual, 1000);
> 
>  	udev->request_mac_addr = 1;
> +	kfree(buf);
> 
>  	return ret;
>  }
> --
> 2.30.0
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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:

- Your patch contains warnings and/or errors noticed by the
  scripts/checkpatch.pl tool.

- 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/SubmittingPatches 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

[PATCH v2] staging: gdm724x: Fix DMA from stack

[Amey Narkhede]

Stack allocated buffers cannot be used for DMA
on all architectures so allocate hci_packet buffer
using kzalloc().

Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
---
Changes in v2:
	- Fixed build warning
	- Fixed memory leak using kfree

 drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
index dc4da66c3..c4a9b90c5 100644
--- a/drivers/staging/gdm724x/gdm_usb.c
+++ b/drivers/staging/gdm724x/gdm_usb.c
@@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,

 static int request_mac_address(struct lte_udev *udev)
 {
-	u8 buf[16] = {0,};
-	struct hci_packet *hci = (struct hci_packet *)buf;
+	u8 *buf;
+	struct hci_packet *hci;
 	struct usb_device *usbdev = udev->usbdev;
 	int actual;
 	int ret = -1;
+	buf = kzalloc(16, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+	hci = (struct hci_packet *)buf;

 	hci->cmd_evt = gdm_cpu_to_dev16(udev->gdm_ed, LTE_GET_INFORMATION);
 	hci->len = gdm_cpu_to_dev16(udev->gdm_ed, 1);
@@ -70,6 +74,7 @@ static int request_mac_address(struct lte_udev *udev)
 			   &actual, 1000);

 	udev->request_mac_addr = 1;
+	kfree(buf);

 	return ret;
 }
--
2.30.0

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Greg KH]

On Wed, Feb 10, 2021 at 01:31:34PM +0530, Amey Narkhede wrote:
> Stack allocated buffers cannot be used for DMA
> on all architectures so allocate hci_packet buffer
> using kzalloc().
> 
> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> ---
> Changes in v2:
> 	- Fixed build warning
> 	- Fixed memory leak using kfree
> 
>  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> index dc4da66c3..c4a9b90c5 100644
> --- a/drivers/staging/gdm724x/gdm_usb.c
> +++ b/drivers/staging/gdm724x/gdm_usb.c
> @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> 
>  static int request_mac_address(struct lte_udev *udev)
>  {
> -	u8 buf[16] = {0,};
> -	struct hci_packet *hci = (struct hci_packet *)buf;
> +	u8 *buf;
> +	struct hci_packet *hci;
>  	struct usb_device *usbdev = udev->usbdev;
>  	int actual;
>  	int ret = -1;
> +	buf = kzalloc(16, GFP_KERNEL);

checkpatch did not complain about this?

And why do you need 'buf' anymore now?  Why not just allocate hci and
pass that to the request instead?  Saves you an extra cast and an extra
pointer.

thanks,

greg k-h

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Amey Narkhede]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On 21/02/10 09:06AM, Greg KH wrote:
> On Wed, Feb 10, 2021 at 01:31:34PM +0530, Amey Narkhede wrote:
> > Stack allocated buffers cannot be used for DMA
> > on all architectures so allocate hci_packet buffer
> > using kzalloc().
> >
> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > ---
> > Changes in v2:
> > 	- Fixed build warning
> > 	- Fixed memory leak using kfree
> >
> >  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> > index dc4da66c3..c4a9b90c5 100644
> > --- a/drivers/staging/gdm724x/gdm_usb.c
> > +++ b/drivers/staging/gdm724x/gdm_usb.c
> > @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> >
> >  static int request_mac_address(struct lte_udev *udev)
> >  {
> > -	u8 buf[16] = {0,};
> > -	struct hci_packet *hci = (struct hci_packet *)buf;
> > +	u8 *buf;
> > +	struct hci_packet *hci;
> >  	struct usb_device *usbdev = udev->usbdev;
> >  	int actual;
> >  	int ret = -1;
> > +	buf = kzalloc(16, GFP_KERNEL);
>
> checkpatch did not complain about this?
No. checkpatch shows no errors and warnings.
>
> And why do you need 'buf' anymore now?  Why not just allocate hci and
> pass that to the request instead?  Saves you an extra cast and an extra
> pointer.
>
> thanks,
>
> greg k-h
Thanks. I'll send v3. I assume now we don't need kzalloc anymore as we initialize
the hci_packet so kmalloc(sizeof(struct hci_packet),..) will do.

Amey

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Dan Carpenter]

On Wed, Feb 10, 2021 at 02:28:11PM +0530, Amey Narkhede wrote:
> On 21/02/10 09:06AM, Greg KH wrote:
> > On Wed, Feb 10, 2021 at 01:31:34PM +0530, Amey Narkhede wrote:
> > > Stack allocated buffers cannot be used for DMA
> > > on all architectures so allocate hci_packet buffer
> > > using kzalloc().
> > >
> > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > ---
> > > Changes in v2:
> > > 	- Fixed build warning
> > > 	- Fixed memory leak using kfree
> > >
> > >  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> > > index dc4da66c3..c4a9b90c5 100644
> > > --- a/drivers/staging/gdm724x/gdm_usb.c
> > > +++ b/drivers/staging/gdm724x/gdm_usb.c
> > > @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> > >
> > >  static int request_mac_address(struct lte_udev *udev)
> > >  {
> > > -	u8 buf[16] = {0,};
> > > -	struct hci_packet *hci = (struct hci_packet *)buf;
> > > +	u8 *buf;
> > > +	struct hci_packet *hci;
> > >  	struct usb_device *usbdev = udev->usbdev;
> > >  	int actual;
> > >  	int ret = -1;
> > > +	buf = kzalloc(16, GFP_KERNEL);
> >
> > checkpatch did not complain about this?
> No. checkpatch shows no errors and warnings.
> >
> > And why do you need 'buf' anymore now?  Why not just allocate hci and
> > pass that to the request instead?  Saves you an extra cast and an extra
> > pointer.
> >
> > thanks,
> >
> > greg k-h
> Thanks. I'll send v3. I assume now we don't need kzalloc anymore as we initialize
> the hci_packet so kmalloc(sizeof(struct hci_packet),..) will do.

We only initialize the first five bytes, but it also seems as if we only
use the first five bytes which raises the question of why we are
allocating 16 bytes.

regards,
dan carpenter

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Amey Narkhede]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On 21/02/10 12:04PM, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 02:28:11PM +0530, Amey Narkhede wrote:
> > On 21/02/10 09:06AM, Greg KH wrote:
> > > On Wed, Feb 10, 2021 at 01:31:34PM +0530, Amey Narkhede wrote:
> > > > Stack allocated buffers cannot be used for DMA
> > > > on all architectures so allocate hci_packet buffer
> > > > using kzalloc().
> > > >
> > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > 	- Fixed build warning
> > > > 	- Fixed memory leak using kfree
> > > >
> > > >  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
> > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> > > > index dc4da66c3..c4a9b90c5 100644
> > > > --- a/drivers/staging/gdm724x/gdm_usb.c
> > > > +++ b/drivers/staging/gdm724x/gdm_usb.c
> > > > @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> > > >
> > > >  static int request_mac_address(struct lte_udev *udev)
> > > >  {
> > > > -	u8 buf[16] = {0,};
> > > > -	struct hci_packet *hci = (struct hci_packet *)buf;
> > > > +	u8 *buf;
> > > > +	struct hci_packet *hci;
> > > >  	struct usb_device *usbdev = udev->usbdev;
> > > >  	int actual;
> > > >  	int ret = -1;
> > > > +	buf = kzalloc(16, GFP_KERNEL);
> > >
> > > checkpatch did not complain about this?
> > No. checkpatch shows no errors and warnings.
> > >
> > > And why do you need 'buf' anymore now?  Why not just allocate hci and
> > > pass that to the request instead?  Saves you an extra cast and an extra
> > > pointer.
> > >
> > > thanks,
> > >
> > > greg k-h
> > Thanks. I'll send v3. I assume now we don't need kzalloc anymore as we initialize
> > the hci_packet so kmalloc(sizeof(struct hci_packet),..) will do.
>
> We only initialize the first five bytes, but it also seems as if we only
> use the first five bytes which raises the question of why we are
> allocating 16 bytes.
>
> regards,
> dan carpenter
>
That makes sense. I kept 16 bytes as original implementation allocated
16 bytes on stack.

Thanks,
Amey

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Greg KH]

On Wed, Feb 10, 2021 at 02:28:11PM +0530, Amey Narkhede wrote:
> On 21/02/10 09:06AM, Greg KH wrote:
> > On Wed, Feb 10, 2021 at 01:31:34PM +0530, Amey Narkhede wrote:
> > > Stack allocated buffers cannot be used for DMA
> > > on all architectures so allocate hci_packet buffer
> > > using kzalloc().
> > >
> > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > ---
> > > Changes in v2:
> > > 	- Fixed build warning
> > > 	- Fixed memory leak using kfree
> > >
> > >  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> > > index dc4da66c3..c4a9b90c5 100644
> > > --- a/drivers/staging/gdm724x/gdm_usb.c
> > > +++ b/drivers/staging/gdm724x/gdm_usb.c
> > > @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> > >
> > >  static int request_mac_address(struct lte_udev *udev)
> > >  {
> > > -	u8 buf[16] = {0,};
> > > -	struct hci_packet *hci = (struct hci_packet *)buf;
> > > +	u8 *buf;
> > > +	struct hci_packet *hci;
> > >  	struct usb_device *usbdev = udev->usbdev;
> > >  	int actual;
> > >  	int ret = -1;
> > > +	buf = kzalloc(16, GFP_KERNEL);
> >
> > checkpatch did not complain about this?
> No. checkpatch shows no errors and warnings.

Please add a blank line after variables and before logic.

> > And why do you need 'buf' anymore now?  Why not just allocate hci and
> > pass that to the request instead?  Saves you an extra cast and an extra
> > pointer.
> >
> > thanks,
> >
> > greg k-h
> Thanks. I'll send v3. I assume now we don't need kzalloc anymore as we initialize
> the hci_packet so kmalloc(sizeof(struct hci_packet),..) will do.

Why is it needed now?  And why would that change?

thanks,

greg k-h

Re: [PATCH v2] staging: gdm724x: Fix DMA from stack

[Amey Narkhede]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On 21/02/10 10:09AM, Greg KH wrote:
> On Wed, Feb 10, 2021 at 02:28:11PM +0530, Amey Narkhede wrote:
> > On 21/02/10 09:06AM, Greg KH wrote:
> > > On Wed, Feb 10, 2021 at 01:31:34PM +0530, Amey Narkhede wrote:
> > > > Stack allocated buffers cannot be used for DMA
> > > > on all architectures so allocate hci_packet buffer
> > > > using kzalloc().
> > > >
> > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > 	- Fixed build warning
> > > > 	- Fixed memory leak using kfree
> > > >
> > > >  drivers/staging/gdm724x/gdm_usb.c | 9 +++++++--
> > > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> > > > index dc4da66c3..c4a9b90c5 100644
> > > > --- a/drivers/staging/gdm724x/gdm_usb.c
> > > > +++ b/drivers/staging/gdm724x/gdm_usb.c
> > > > @@ -56,11 +56,15 @@ static int gdm_usb_recv(void *priv_dev,
> > > >
> > > >  static int request_mac_address(struct lte_udev *udev)
> > > >  {
> > > > -	u8 buf[16] = {0,};
> > > > -	struct hci_packet *hci = (struct hci_packet *)buf;
> > > > +	u8 *buf;
> > > > +	struct hci_packet *hci;
> > > >  	struct usb_device *usbdev = udev->usbdev;
> > > >  	int actual;
> > > >  	int ret = -1;
> > > > +	buf = kzalloc(16, GFP_KERNEL);
> > >
> > > checkpatch did not complain about this?
> > No. checkpatch shows no errors and warnings.
>
> Please add a blank line after variables and before logic.
>
Will do thanks.
> > > And why do you need 'buf' anymore now?  Why not just allocate hci and
> > > pass that to the request instead?  Saves you an extra cast and an extra
> > > pointer.
> > >
> > > thanks,
> > >
> > > greg k-h
> > Thanks. I'll send v3. I assume now we don't need kzalloc anymore as we initialize
> > the hci_packet so kmalloc(sizeof(struct hci_packet),..) will do.
>
> Why is it needed now?  And why would that change?
>
> thanks,
>
> greg k-h
I was thinking about allcoating hci_packet(hci) but as Dan said
we only use first five bytes so kmalloc(5, ...) should work.

Thanks,
Amey

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]