From patchwork Mon Oct 10 08:02:03 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: danborkmann@iogearbox.net X-Patchwork-Id: 118653 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 3237FB6FA5 for ; Mon, 10 Oct 2011 19:05:11 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752674Ab1JJIFB (ORCPT ); Mon, 10 Oct 2011 04:05:01 -0400 Received: from www62.your-server.de ([213.133.104.62]:60402 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752455Ab1JJIFA convert rfc822-to-8bit (ORCPT ); Mon, 10 Oct 2011 04:05:00 -0400 Received: from [78.46.5.208] (helo=webmail01.your-server.de) by www62.your-server.de with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.72) (envelope-from ) id 1RDAr5-0003fG-FP; Mon, 10 Oct 2011 10:04:59 +0200 Received: from pc-10089.ethz.ch (pc-10089.ethz.ch [82.130.102.59]) by webmail.your-server.de (Horde Framework) with HTTP; Mon, 10 Oct 2011 10:02:03 +0200 Message-ID: <20111010100203.15066m7nvqod58cb@webmail.your-server.de> Date: Mon, 10 Oct 2011 10:02:03 +0200 From: danborkmann@iogearbox.net To: Eric Dumazet Cc: "David S. Miller" , netdev@vger.kernel.org Subject: Re: [PATCH] af_packet: tpacket_destruct_skb, deref skb after BUG_ON assertion References: <20111009171919.10922hrx8qjm2f7b@webmail.your-server.de> <1318193866.21116.3.camel@edumazet-laptop> In-Reply-To: <1318193866.21116.3.camel@edumazet-laptop> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Dynamic Internet Messaging Program (DIMP) H3 (1.1.6) X-Authenticated-Sender: danborkmann@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.97.2/13776/Mon Oct 10 02:03:42 2011) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hi Eric, Quoting Eric Dumazet : > Le dimanche 09 octobre 2011 à 17:19 +0200, danborkmann@iogearbox.net a > écrit : >> This tiny patch derefs the skb only after BUG_ON(skb==NULL) was evaluated >> and not before. Patched against latest Linus tree. >> >> Thanks, >> Daniel >> >> Signed-off-by: Daniel Borkmann >> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> index fabb4fa..d9d833b 100644 >> --- a/net/packet/af_packet.c >> +++ b/net/packet/af_packet.c >> @@ -1167,11 +1167,12 @@ ring_is_full: >> >> static void tpacket_destruct_skb(struct sk_buff *skb) >> { >> - struct packet_sock *po = pkt_sk(skb->sk); >> + struct packet_sock *po; >> void *ph; >> >> BUG_ON(skb == NULL); >> >> + po = pkt_sk(skb->sk); >> if (likely(po->tx_ring.pg_vec)) { >> ph = skb_shinfo(skb)->destructor_arg; >> BUG_ON(__packet_get_status(po, ph) != TP_STATUS_SENDING); >> >> > > Well, to be honest, this BUG_ON(!skb) is absolutely useless for two > reasons. > > 1) If skb happens to be NULL, the NULL dereference is trapped and stack > trace dumped as well. > > 2) Of course, tpacket_destruct_skb() being an skb destructor, skb cannot > be NULL at this point by design. > > Please remove the BUG_ON() instead of trying to move it ;) Thanks, you're absolutely right! Here's the trivial patch: af_packet: removed unnecessary BUG_ON assertion in tpacket_destruct_skb If skb is NULL, then stack trace is thrown on anyway on dereference. Therefore, the stack trace triggered by BUG_ON is duplicate. Signed-off-by: Daniel Borkmann --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index fabb4fa..886ae50 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1170,8 +1170,6 @@ static void tpacket_destruct_skb(struct sk_buff *skb) struct packet_sock *po = pkt_sk(skb->sk); void *ph; - BUG_ON(skb == NULL); - if (likely(po->tx_ring.pg_vec)) { ph = skb_shinfo(skb)->destructor_arg; BUG_ON(__packet_get_status(po, ph) != TP_STATUS_SENDING);