From patchwork Mon Dec 22 22:41:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Larry Finger X-Patchwork-Id: 5529221 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id AF78D9F1CD for ; Mon, 22 Dec 2014 22:41:34 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id ED91D2018E for ; Mon, 22 Dec 2014 22:41:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 221F92017A for ; Mon, 22 Dec 2014 22:41:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752127AbaLVWl0 (ORCPT ); Mon, 22 Dec 2014 17:41:26 -0500 Received: from mail-ob0-f180.google.com ([209.85.214.180]:53883 "EHLO mail-ob0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751918AbaLVWlZ (ORCPT ); Mon, 22 Dec 2014 17:41:25 -0500 Received: by mail-ob0-f180.google.com with SMTP id wp4so23175771obc.11; Mon, 22 Dec 2014 14:41:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type; bh=wvxlrJyEyidkySYRvsrCkqq8d1XYns+c4UfvzVDbnBA=; b=iYDs73flzydoVvuUWslfXvE8adN7F6w4oVaF4XiuSOn2ASoQaEjdtmFxrVgBM3xuyJ t6wODZ4xRVWEW4uW7jD3WBcaA6TvWYpTA8MQj25JfnY4dEoDz8sC8et8s+ijAmxf/69G cwlGHfVDpmtxkuxbaL3HS1wgRgSw7FFRxT6e3gRuuXnkz/5wWQLffpFnuynf7A5Gi5oE AUiuSWbuoUT0xdPf86crh3bShVJdx6pyNvCiZdBgbXTBzLSKnYPyeLBhwnHMXgA5rC1/ dO6ndlITol2Y8j9svH6DODoycHUnp2zkzkSuz6z38jiQhaa/rVYbiBKR3gW6bHlwaKVa h5CA== X-Received: by 10.182.225.232 with SMTP id rn8mr14675672obc.23.1419288084852; Mon, 22 Dec 2014 14:41:24 -0800 (PST) Received: from linux.site ([69.76.245.152]) by mx.google.com with ESMTPSA id yd7sm9479047obc.12.2014.12.22.14.41.23 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 22 Dec 2014 14:41:24 -0800 (PST) Message-ID: <54989E12.6050808@lwfinger.net> Date: Mon, 22 Dec 2014 16:41:22 -0600 From: Larry Finger User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Eric Biggers CC: kvalo@codeaurora.org, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, Stable Subject: Re: [PATCH for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb References: <1419269826-12552-1-git-send-email-Larry.Finger@lwfinger.net> <20141222194843.GA7575@zzz> In-Reply-To: <20141222194843.GA7575@zzz> Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 12/22/2014 01:48 PM, Eric Biggers wrote: > Is this really the same behavior as 3.17? In 3.17, allocating the new skb is > one of the first things the interrupt handler does, and if that fails it drops > the packet and keeps using the old skb. In this proposal, it's only after the > packet has been received and the old skb has been freed that a new one is > allocated. And if that fails --- well, what are you expecting to happen > exactly? You are correct. In trying to get a small patch for stable, I missed some important points. Please look at the attached patch. I think it handles the skb allocations correctly. The critical point is that _rtl_pci_init_one_rxdesc() cannot be allowed to fail to allocate an skb while in the interrupt path. Now, I have already allocated the skb before the call and bypassed this routine if the allocation fails. After a couple of crashes, this one now works for the case when the allocation wouldn't fail anyway. I will likely pull the allocation out of _rtl_pci_init_one_rxdesc() in all cases for the final patch. @Kalle: Please drop the patch I submitted this morning with this subject. It would not help the problem. I will resubmit after I am sure of the proper fix. Thanks, Larry Index: wireless-drivers/drivers/net/wireless/rtlwifi/pci.c =================================================================== --- wireless-drivers.orig/drivers/net/wireless/rtlwifi/pci.c +++ wireless-drivers/drivers/net/wireless/rtlwifi/pci.c @@ -666,6 +666,7 @@ tx_status_ok: } static int _rtl_pci_init_one_rxdesc(struct ieee80211_hw *hw, + struct sk_buff *new_skb, u8 *entry, int rxring_idx, int desc_idx) { struct rtl_priv *rtlpriv = rtl_priv(hw); @@ -674,7 +675,10 @@ static int _rtl_pci_init_one_rxdesc(stru u8 tmp_one = 1; struct sk_buff *skb; - skb = dev_alloc_skb(rtlpci->rxbuffersize); + if (new_skb) + skb = new_skb; + else + skb = dev_alloc_skb(rtlpci->rxbuffersize); if (!skb) return 0; rtlpci->rx_ring[rxring_idx].rx_buf[desc_idx] = skb; @@ -772,6 +776,7 @@ static void _rtl_pci_rx_interrupt(struct /*RX NORMAL PKT */ while (count--) { struct ieee80211_hdr *hdr; + struct sk_buff *new_skb = NULL; __le16 fc; u16 len; /*rx buffer descriptor */ @@ -800,6 +805,12 @@ static void _rtl_pci_rx_interrupt(struct return; } + new_skb = dev_alloc_skb(rtlpci->rxbuffersize); + if (unlikely(!new_skb)) { + RT_TRACE(rtlpriv, (COMP_INTR | COMP_RECV), DBG_DMESG, + "can't alloc skb for rx\n"); + goto end; + } /* Reaching this point means: data is filled already * AAAAAAttention !!! * We can NOT access 'skb' before 'pci_unmap_single' @@ -845,6 +856,7 @@ static void _rtl_pci_rx_interrupt(struct if (rtlpriv->cfg->ops->rx_command_packet && rtlpriv->cfg->ops->rx_command_packet(hw, stats, skb)) { dev_kfree_skb_any(skb); + skb = new_skb; goto end; } @@ -895,6 +907,7 @@ static void _rtl_pci_rx_interrupt(struct } else { dev_kfree_skb_any(skb); } + skb = new_skb; if (rtlpriv->use_new_trx_flow) { rtlpci->rx_ring[hw_queue].next_rx_rp += 1; rtlpci->rx_ring[hw_queue].next_rx_rp %= @@ -912,12 +925,13 @@ static void _rtl_pci_rx_interrupt(struct } end: if (rtlpriv->use_new_trx_flow) { - if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)buffer_desc, - rxring_idx, - rtlpci->rx_ring[rxring_idx].idx)) + /* TODO: Can the following fail? */ + if (!_rtl_pci_init_one_rxdesc(hw, skb, + (u8 *)buffer_desc, rxring_idx, + rtlpci->rx_ring[rxring_idx].idx)) return; } else { - if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)pdesc, + if (!_rtl_pci_init_one_rxdesc(hw, skb, (u8 *)pdesc, rxring_idx, rtlpci->rx_ring[rxring_idx].idx)) return; @@ -1309,7 +1323,7 @@ static int _rtl_pci_init_rx_ring(struct rtlpci->rx_ring[rxring_idx].idx = 0; for (i = 0; i < rtlpci->rxringcount; i++) { entry = &rtlpci->rx_ring[rxring_idx].buffer_desc[i]; - if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)entry, + if (!_rtl_pci_init_one_rxdesc(hw, NULL, (u8 *)entry, rxring_idx, i)) return -ENOMEM; } @@ -1334,7 +1348,7 @@ static int _rtl_pci_init_rx_ring(struct for (i = 0; i < rtlpci->rxringcount; i++) { entry = &rtlpci->rx_ring[rxring_idx].desc[i]; - if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)entry, + if (!_rtl_pci_init_one_rxdesc(hw, NULL, (u8 *)entry, rxring_idx, i)) return -ENOMEM; }