From patchwork Mon Apr 10 21:22:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Lamparter X-Patchwork-Id: 749222 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3w236s4sKlz9s8P for ; Tue, 11 Apr 2017 07:22:29 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=googlemail.com header.i=@googlemail.com header.b="uQQHFU6u"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752869AbdDJVWV (ORCPT ); Mon, 10 Apr 2017 17:22:21 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35891 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752766AbdDJVWU (ORCPT ); Mon, 10 Apr 2017 17:22:20 -0400 Received: by mail-wm0-f66.google.com with SMTP id q125so11808385wmd.3; Mon, 10 Apr 2017 14:22:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=ksyTaNCEcoPEqRBc/U7neSCHnNKbN4a5Thaca9euh4E=; b=uQQHFU6uPfovbhjl56K5GsO7qy+Bb63My9mrMlnxbx0jUTWEWyKzZTwDn40P8yV9oX MwC5qqk3GmewiaSNQPeC2teF1BP7kpywAMaLAu+ZI80zAX1KSrsWzFXN4jMpoEhp/tWM VNmnpzrBssIU8jxEo2mRwKczv0zjO6xV3FqP1W/fQijJqUVADrbKP1m4q1RXK5Tu7v4W e+kNtsZxdNLWWSjLLIWTK9jDDMj1j1aCLSwrV+b8Klf6yh6p619lDG2wbzW9RalKERN1 jOO7cuSQqlWEfB+j6L1uGcUcQQLS7LWFmnDBt4dvM4i5wgn/0xCfWAsOcokzGWfU477K bA/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=ksyTaNCEcoPEqRBc/U7neSCHnNKbN4a5Thaca9euh4E=; b=uG2OfGWtUG6CdRxuKHNH1HieJ3lyM1k95ub/NnpK1xNcTxtxd5gAdvw5Q6ulK4Iexh cBLZj3D7QqvDQZqWVyLOkH60vm+pA/Zza0dn4Jq53Ud4LvVoqGj3wKdrhR0EO6qoOySJ RhoEv2lc68ZoVdKwkZxD/k8bl0bSBZe71JE4JoeK6bhn2a99PbrPdDkk2O7MPVpgpYf0 bRtGDtLRnqDHWCf4iHeWAoROhxQFHJJ+MvmwFYcKVvMlk9n+oyH4PbtMQF0H5yUXtb+m iYt8kGosFXlf/YwM89MYqsERjqzX2GeX2oC90tLUFQB2EnLocq2PefGVfzk7Bc3TGQBu 3rnQ== X-Gm-Message-State: AN3rC/6y5CidOFo8POSr8taRQ9FAeLiNTjnsTrTrFk8SmR3QhKwxn0ic nasA08LbMZnEvGgg X-Received: by 10.28.143.71 with SMTP id r68mr12436048wmd.61.1491859333372; Mon, 10 Apr 2017 14:22:13 -0700 (PDT) Received: from debian64.daheim (p5B0D7F40.dip0.t-ipconnect.de. [91.13.127.64]) by smtp.gmail.com with ESMTPSA id f62sm11657312wmh.33.2017.04.10.14.22.11 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 10 Apr 2017 14:22:12 -0700 (PDT) Received: from localhost.daheim ([127.0.0.1] helo=debian64.localnet) by debian64.daheim with esmtp (Exim 4.89) (envelope-from ) id 1cxglH-0002MN-Bu; Mon, 10 Apr 2017 23:22:11 +0200 From: Christian Lamparter To: Myungho Jung Cc: netdev@vger.kernel.org, David Miller , linux-wireless@vger.kernel.org, edumazet@google.com Subject: Re: [PATCH] p54: add null pointer check before releasing socket buffer Date: Mon, 10 Apr 2017 23:22:10 +0200 Message-ID: <1896493.H38pOMoCHn@debian64> User-Agent: KMail/5.2.3 (Linux/4.11.0-rc4-wt-only+; KDE/5.28.0; x86_64; ; ) In-Reply-To: <20170410205414.GB12557@fqdn.specialj.com> References: <1491801800-4371-1-git-send-email-mhjungk@gmail.com> <3716717.OUypxXNBrR@debian64> <20170410205414.GB12557@fqdn.specialj.com> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Monday, April 10, 2017 1:54:14 PM CEST Myungho Jung wrote: > On Mon, Apr 10, 2017 at 02:12:54PM +0200, Christian Lamparter wrote: > > On Sunday, April 9, 2017 10:23:20 PM CEST Myungho Jung wrote: > > > Kernel panic is caused by trying to dereference null pointer. Check if > > > the pointer is null before freeing space. > > [...] > > As for adding if (!skb) checks. I think kfree, kfree_skb, dev_kfree_skb > > (aka consume_skb) all check for null pointers already. So the logical > > thing to do would be to make dev_kfree_skb_irq (which would also fix > > dev_kfree_skb_any) consistent with kfree, kfree_skb, dev_kfree_skb and > > add the check there. > > > --- > > > drivers/net/wireless/intersil/p54/txrx.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c > > > index 1af7da0..8956061 100644 > > > --- a/drivers/net/wireless/intersil/p54/txrx.c > > > +++ b/drivers/net/wireless/intersil/p54/txrx.c > > > @@ -503,7 +503,9 @@ static void p54_rx_eeprom_readback(struct p54_common *priv, > > > > > > priv->eeprom = NULL; > > > tmp = p54_find_and_unlink_skb(priv, hdr->req_id); > > > - dev_kfree_skb_any(tmp); > > > + if (unlikely(!tmp)) > > > + dev_kfree_skb_any(tmp); > > > + > > > complete(&priv->eeprom_comp); > > > } > > > > > > @@ -597,7 +599,9 @@ static void p54_rx_stats(struct p54_common *priv, struct sk_buff *skb) > > > } > > > > > > tmp = p54_find_and_unlink_skb(priv, hdr->req_id); > > > - dev_kfree_skb_any(tmp); > > > + if (unlikely(!tmp)) > > > + dev_kfree_skb_any(tmp); > > > + > > > complete(&priv->stat_comp); > > > } > > > > > > > > > > > [...] I'm not sure it actually caused kernel panic but guessed from > a bug report [https://bugzilla.kernel.org/show_bug.cgi?id=195289]. Thank you for bringing this to my attention. Reading the bugreport, it does sound like there's a bigger issue with the USB Ports. I'll see if this can be fixed. But it does sound like a hardware issue at this point. > And correct fix will be like this: > if (likely(tmp)) > dev_kfree_skb_any(tmp); > > But, like you said, I think null pointer should be checked in > dev_kfree_skb_irq although already checking before calling it in many > other places. I'll try another patch. Thank you for your advice. Well, the patch could be as simple as this: --- --- The question is: would David or Eric support the change. Any comments, what's the prefered solution? Just patch __dev_kfree_skb_irq to make it consistent with *kfree*, or patch the driver? I'm fine either way, but I would prefere patching __dev_kfree_skb_irq. Regards, Christian diff --git a/net/core/dev.c b/net/core/dev.c index 7869ae3837ca..44f7d5a1c67c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2450,6 +2450,9 @@ void __dev_kfree_skb_irq(struct sk_buff *skb, enum skb_free_reason reason) { unsigned long flags; + if (!skb) + return; + if (likely(atomic_read(&skb->users) == 1)) { smp_rmb(); atomic_set(&skb->users, 0);