From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753596Ab1HBLBw (ORCPT ); Tue, 2 Aug 2011 07:01:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51462 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752330Ab1HBLBr (ORCPT ); Tue, 2 Aug 2011 07:01:47 -0400 Date: Tue, 2 Aug 2011 07:01:38 -0400 From: Josh Boyer To: Oliver Neukum Cc: Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: cdc_ncm uses stack memory for DMA Message-ID: <20110802110138.GD2096@zod.bos.redhat.com> References: <20110801195949.GB2096@zod.bos.redhat.com> <201108012226.16180.oliver@neukum.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201108012226.16180.oliver@neukum.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 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"); }