Re: cdc_ncm uses stack memory for DMA

[Josh Boyer <jwboyer@redhat.com>]

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");
 		}