cdc_ncm uses stack memory for DMA

cdc_ncm uses stack memory for DMA

[Josh Boyer]

Hi,

We've gotten a report that the cdc_ncm driver generates a stacktrace due
to using stack variables that are passed to cdc_ncm_do_request.  You can
find the stack trace here:

https://bugzilla.redhat.com/show_bug.cgi?id=720128

It's generated by lib/dma-debug.c and we have CONFIG_DMA_API_DEBUG set
in our development kernels.

Looking through the code, it seems that cdc_ncm_setup is still passing
stack variables around lines 270, 276, 375, and 406.  Which of those
exactly is triggering the backtrace, I'm not sure but all of them seem
incorrect from what I can tell.

Do you have any thoughts on a solution for this?  Off the top of my
head, either the local variables being used could be added to the ctx
structure or the variables can be changed to pointers and then be
allocated via kmalloc and torn down shortly after.

josh

Re: cdc_ncm uses stack memory for DMA

[Oliver Neukum]

Am Montag, 1. August 2011, 21:59:50 schrieb Josh Boyer:

Hi,

> Looking through the code, it seems that cdc_ncm_setup is still passing
> stack variables around lines 270, 276, 375, and 406.  Which of those
> exactly is triggering the backtrace, I'm not sure but all of them seem
> incorrect from what I can tell.

They are incorrect.
 
> Do you have any thoughts on a solution for this?  Off the top of my
> head, either the local variables being used could be added to the ctx

Careful. You also need to confirm to the rules about cached coherency
on some architectures. You cannot do DMA into the middle of a structure
unless you are sure it is not touched any other way.

> structure or the variables can be changed to pointers and then be
> allocated via kmalloc and torn down shortly after.

This looks like the easiest way.

	Regards
		Oliver

Re: cdc_ncm uses stack memory for DMA

[Josh Boyer]

On Mon, Aug 01, 2011 at 10:26:16PM +0200, Oliver Neukum wrote:
> Am Montag, 1. August 2011, 21:59:50 schrieb Josh Boyer:
> 
> Hi,
> 
> > Looking through the code, it seems that cdc_ncm_setup is still passing
> > stack variables around lines 270, 276, 375, and 406.  Which of those
> > exactly is triggering the backtrace, I'm not sure but all of them seem
> > incorrect from what I can tell.
> 
> They are incorrect.
>  
> > Do you have any thoughts on a solution for this?  Off the top of my
> > head, either the local variables being used could be added to the ctx
> 
> Careful. You also need to confirm to the rules about cached coherency
> on some architectures. You cannot do DMA into the middle of a structure
> unless you are sure it is not touched any other way.
> 
> > structure or the variables can be changed to pointers and then be
> > allocated via kmalloc and torn down shortly after.
> 
> This looks like the easiest way.

OK.  How does the following look?  It's been compile tested, but I don't
have the hardware.

josh

commit 208ae00b437bec1f46fa93f37c65a66a831e55fb
Author: Josh Boyer <jwboyer@redhat.com>
Date:   Mon Aug 1 21:09:43 2011 -0400

    The cdc_ncm driver still has a few places where stack variables are passed
    to the cdc_ncm_do_request function.  This triggers a stack trace in
    lib/dma-debug.c if the CONFIG_DEBUG_DMA_API option is set.
    
    Adjust these calls to pass parameters that have been allocated with kzalloc.
    
    Signed-off-by: Josh Boyer <jwboyer@redhat.com>

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index fd622a6..7415265 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -260,23 +260,38 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 		req.wIndex = cpu_to_le16(iface_no);
 
 		if (flags & USB_CDC_NCM_NCAP_NTB_INPUT_SIZE) {
-			struct usb_cdc_ncm_ndp_input_size ndp_in_sz;
+			struct usb_cdc_ncm_ndp_input_size *ndp_in_sz;
+
+			ndp_in_sz = kzalloc(sizeof(*ndp_in_sz), GFP_KERNEL);
+			if (!ndp_in_sz) {
+				err = -ENOMEM;
+				goto size_err;
+			}
 
 			req.wLength = 8;
-			ndp_in_sz.dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
-			ndp_in_sz.wNtbInMaxDatagrams =
+			ndp_in_sz->dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
+			ndp_in_sz->wNtbInMaxDatagrams =
 					cpu_to_le16(CDC_NCM_DPT_DATAGRAMS_MAX);
-			ndp_in_sz.wReserved = 0;
-			err = cdc_ncm_do_request(ctx, &req, &ndp_in_sz, 0, NULL,
+			ndp_in_sz->wReserved = 0;
+			err = cdc_ncm_do_request(ctx, &req, ndp_in_sz, 0, NULL,
 									1000);
+			kfree(ndp_in_sz);
 		} else {
-			__le32 dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
+			__le32 *dwNtbInMaxSize;
+			dwNtbInMaxSize = kzalloc(sizeof(*dwNtbInMaxSize), GFP_KERNEL);
+			if (!dwNtbInMaxSize) {
+				err = -ENOMEM;
+				goto size_err;
+			}
+			*dwNtbInMaxSize = cpu_to_le32(ctx->rx_max);
 
 			req.wLength = 4;
-			err = cdc_ncm_do_request(ctx, &req, &dwNtbInMaxSize, 0,
+			err = cdc_ncm_do_request(ctx, &req, dwNtbInMaxSize, 0,
 								NULL, 1000);
+			kfree(dwNtbInMaxSize);
 		}
 
+size_err:
 		if (err)
 			pr_debug("Setting NTB Input Size failed\n");
 	}
@@ -362,9 +377,15 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 
 	/* set Max Datagram Size (MTU) */
 	if (flags & USB_CDC_NCM_NCAP_MAX_DATAGRAM_SIZE) {
-		__le16 max_datagram_size;
+		__le16 *max_datagram_size;
 		u16 eth_max_sz = le16_to_cpu(ctx->ether_desc->wMaxSegmentSize);
 
+		max_datagram_size = kzalloc(sizeof(*max_datagram_size), GFP_KERNEL);
+		if (!max_datagram_size) {
+			err = -ENOMEM;
+			goto max_dgram_err;
+		}
+
 		req.bmRequestType = USB_TYPE_CLASS | USB_DIR_IN |
 							USB_RECIP_INTERFACE;
 		req.bNotificationType = USB_CDC_GET_MAX_DATAGRAM_SIZE;
@@ -372,13 +393,14 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 		req.wIndex = cpu_to_le16(iface_no);
 		req.wLength = cpu_to_le16(2);
 
-		err = cdc_ncm_do_request(ctx, &req, &max_datagram_size, 0, NULL,
+		err = cdc_ncm_do_request(ctx, &req, max_datagram_size, 0, NULL,
 									1000);
+
 		if (err) {
 			pr_debug("GET_MAX_DATAGRAM_SIZE failed, use size=%u\n",
 						CDC_NCM_MIN_DATAGRAM_SIZE);
 		} else {
-			ctx->max_datagram_size = le16_to_cpu(max_datagram_size);
+			ctx->max_datagram_size = le16_to_cpu(*max_datagram_size);
 			/* Check Eth descriptor value */
 			if (eth_max_sz < CDC_NCM_MAX_DATAGRAM_SIZE) {
 				if (ctx->max_datagram_size > eth_max_sz)
@@ -401,10 +423,12 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
 			req.wValue = 0;
 			req.wIndex = cpu_to_le16(iface_no);
 			req.wLength = 2;
-			max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
+			*max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
 
-			err = cdc_ncm_do_request(ctx, &req, &max_datagram_size,
+			err = cdc_ncm_do_request(ctx, &req, max_datagram_size,
 								0, NULL, 1000);
+			kfree(max_datagram_size);
+max_dgram_err:
 			if (err)
 				pr_debug("SET_MAX_DATAGRAM_SIZE failed\n");
 		}

RE: cdc_ncm uses stack memory for DMA

[Alexey ORISHKO]

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org]
> On Behalf Of Josh Boyer
> Sent: Tuesday, August 02, 2011 1:02 PM

> 
> -		err = cdc_ncm_do_request(ctx, &req, &max_datagram_size, 0, NULL,
> +		err = cdc_ncm_do_request(ctx, &req, max_datagram_size, 0, NULL,
>  									1000);
> +
>  		if (err) {
>  			pr_debug("GET_MAX_DATAGRAM_SIZE failed, use size=%u\n",
>  						CDC_NCM_MIN_DATAGRAM_SIZE);

You miss to free it here.

>  		} else {
> -			ctx->max_datagram_size = le16_to_cpu(max_datagram_size);
> +			ctx->max_datagram_size = le16_to_cpu(*max_datagram_size);
>  			/* Check Eth descriptor value */
>  			if (eth_max_sz < CDC_NCM_MAX_DATAGRAM_SIZE) {
>  				if (ctx->max_datagram_size > eth_max_sz)
> @@ -401,10 +423,12 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
>  			req.wValue = 0;
>  			req.wIndex = cpu_to_le16(iface_no);
>  			req.wLength = 2;
> -			max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
> +			*max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
> 
> -			err = cdc_ncm_do_request(ctx, &req, &max_datagram_size,
> +			err = cdc_ncm_do_request(ctx, &req, max_datagram_size,
>  								0, NULL, 1000);
> +			kfree(max_datagram_size);
> +max_dgram_err:
>  			if (err)
>  				pr_debug("SET_MAX_DATAGRAM_SIZE failed\n");
>  		}
> --


You have mem leak in case of request failure. Will you resubmit it with this change?
I have HW available and can test this patch on Ubuntu 11.04. 

FYI: There is another bug in ncm driver and I'm about to post a patch for it.

Alexey

Re: cdc_ncm uses stack memory for DMA

[Josh Boyer]

On Tue, Aug 02, 2011 at 02:00:16PM +0200, Alexey ORISHKO wrote:
> > -----Original Message-----
> > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org]
> > On Behalf Of Josh Boyer
> > Sent: Tuesday, August 02, 2011 1:02 PM
> 
> > 
> > -		err = cdc_ncm_do_request(ctx, &req, &max_datagram_size, 0, NULL,
> > +		err = cdc_ncm_do_request(ctx, &req, max_datagram_size, 0, NULL,
> >  									1000);
> > +
> >  		if (err) {
> >  			pr_debug("GET_MAX_DATAGRAM_SIZE failed, use size=%u\n",
> >  						CDC_NCM_MIN_DATAGRAM_SIZE);
> 
> You miss to free it here.

Ah, yes.  Will fix it.

> 
> >  		} else {
> > -			ctx->max_datagram_size = le16_to_cpu(max_datagram_size);
> > +			ctx->max_datagram_size = le16_to_cpu(*max_datagram_size);
> >  			/* Check Eth descriptor value */
> >  			if (eth_max_sz < CDC_NCM_MAX_DATAGRAM_SIZE) {
> >  				if (ctx->max_datagram_size > eth_max_sz)
> > @@ -401,10 +423,12 @@ static u8 cdc_ncm_setup(struct cdc_ncm_ctx *ctx)
> >  			req.wValue = 0;
> >  			req.wIndex = cpu_to_le16(iface_no);
> >  			req.wLength = 2;
> > -			max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
> > +			*max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
> > 
> > -			err = cdc_ncm_do_request(ctx, &req, &max_datagram_size,
> > +			err = cdc_ncm_do_request(ctx, &req, max_datagram_size,
> >  								0, NULL, 1000);
> > +			kfree(max_datagram_size);
> > +max_dgram_err:
> >  			if (err)
> >  				pr_debug("SET_MAX_DATAGRAM_SIZE failed\n");
> >  		}
> > --
> 
> 
> You have mem leak in case of request failure. Will you resubmit it with this change?

Yes.

> I have HW available and can test this patch on Ubuntu 11.04.

Great!

josh