From patchwork Thu Nov 20 11:42:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pablo Neira Ayuso X-Patchwork-Id: 412656 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3FFED1400F4 for ; Thu, 20 Nov 2014 22:40:27 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756132AbaKTLk0 (ORCPT ); Thu, 20 Nov 2014 06:40:26 -0500 Received: from mail.us.es ([193.147.175.20]:48627 "EHLO mail.us.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750973AbaKTLkZ (ORCPT ); Thu, 20 Nov 2014 06:40:25 -0500 Received: (qmail 26852 invoked from network); 20 Nov 2014 12:40:21 +0100 Received: from unknown (HELO us.es) (192.168.2.11) by us.es with SMTP; 20 Nov 2014 12:40:21 +0100 Received: (qmail 21501 invoked by uid 507); 20 Nov 2014 11:40:21 -0000 X-Qmail-Scanner-Diagnostics: from 127.0.0.1 by antivirus1 (envelope-from , uid 501) with qmail-scanner-2.10 (clamdscan: 0.98.4/19655. spamassassin: 3.3.2. Clear:RC:1(127.0.0.1):SA:0(-103.2/7.5):. Processed in 2.065224 secs); 20 Nov 2014 11:40:21 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on antivirus1 X-Spam-Level: X-Spam-Status: No, score=-103.2 required=7.5 tests=BAYES_50,SMTPAUTH_US, SPF_HELO_FAIL,USER_IN_WHITELIST autolearn=disabled version=3.3.2 X-Spam-ASN: AS12715 87.216.0.0/16 X-Envelope-From: pneira@us.es Received: from unknown (HELO antivirus1) (127.0.0.1) by us.es with SMTP; 20 Nov 2014 11:40:19 -0000 Received: from 192.168.1.13 (192.168.1.13) by antivirus1 (F-Secure/fsigk_smtp/412/antivirus1); Thu, 20 Nov 2014 12:40:19 +0100 (CET) X-Virus-Status: clean(F-Secure/fsigk_smtp/412/antivirus1) Received: (qmail 16238 invoked from network); 20 Nov 2014 12:40:18 +0100 Received: from 129.166.216.87.static.jazztel.es (HELO us.es) (1984lsi@87.216.166.129) by mail.us.es with AES128-SHA encrypted SMTP; 20 Nov 2014 12:40:18 +0100 Date: Thu, 20 Nov 2014 12:42:22 +0100 From: Pablo Neira Ayuso To: Gao feng Cc: netfilter-devel@vger.kernel.org Subject: Re: [PATCH v2] netfilter: bridge: unshare bridge info before change it Message-ID: <20141120114222.GA3765@salvia> References: <1416366453-12090-1-git-send-email-gaofeng@cn.fujitsu.com> <20141119130751.GA10748@salvia> <546D40DC.6060505@cn.fujitsu.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <546D40DC.6060505@cn.fujitsu.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org On Thu, Nov 20, 2014 at 09:16:12AM +0800, Gao feng wrote: > On 11/19/2014 09:07 PM, Pablo Neira Ayuso wrote: > > On Wed, Nov 19, 2014 at 11:07:32AM +0800, Gao feng wrote: > >> diff --git a/include/linux/netfilter_bridge.h b/include/linux/netfilter_bridge.h > >> index c755e49..dca7337 100644 > >> --- a/include/linux/netfilter_bridge.h > >> +++ b/include/linux/netfilter_bridge.h > >> @@ -81,14 +81,64 @@ static inline unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb) > >> return 0; > >> } > >> > >> +static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb) > >> +{ > >> + skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC); > >> + if (likely(skb->nf_bridge)) > >> + atomic_set(&(skb->nf_bridge->use), 1); > >> + > >> + return skb->nf_bridge; > >> +} > >> + > >> +static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb) > >> +{ > >> + struct nf_bridge_info *nf_bridge = skb->nf_bridge; > >> + > >> + if (atomic_read(&nf_bridge->use) > 1) { > >> + struct nf_bridge_info *tmp = nf_bridge_alloc(skb); > > > > nf_bridge_alloc() overwrites the original skb->nf_bridge when > > unsharing, so this leaks and likely breaks other things. > > > > overwrite is what we expected, we store the original nfbridge in the var > nf_bridge, copy the original to the new. and release the reference of original > nfbridge. I cannot find anything wrong. You're right. I overlook we already work with the fetched pointer in _unshare(). I'm attaching a patch to clean up this. You can use it as starter in your patch series to split this. Thanks. From 1c6141898a7b82484370ec9c61e3acdaf22eaa91 Mon Sep 17 00:00:00 2001 From: Pablo Neira Ayuso Date: Thu, 20 Nov 2014 12:28:39 +0100 Subject: [PATCH] br_netfilter: cleanup nf_bridge_alloc() and nf_bridge_unshare() Set skb->nf_bridge out of nf_bridge_alloc() and rework clone logic in nf_bridge_unshare(). This comes in preparation of Gao feng's patches. Signed-off-by: Pablo Neira Ayuso --- net/bridge/br_netfilter.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c index 1a4f32c..79dc6f7 100644 --- a/net/bridge/br_netfilter.c +++ b/net/bridge/br_netfilter.c @@ -129,27 +129,32 @@ static inline struct net_device *bridge_parent(const struct net_device *dev) static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb) { - skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC); - if (likely(skb->nf_bridge)) - atomic_set(&(skb->nf_bridge->use), 1); + struct nf_bridge_info *nf_bridge; + + nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC); + if (nf_bridge == NULL) + return NULL; - return skb->nf_bridge; + atomic_set(&nf_bridge->use, 1); + return nf_bridge; } static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb) { - struct nf_bridge_info *nf_bridge = skb->nf_bridge; + struct nf_bridge_info *nf_bridge; - if (atomic_read(&nf_bridge->use) > 1) { - struct nf_bridge_info *tmp = nf_bridge_alloc(skb); + if (atomic_read(&skb->nf_bridge->use) == 1) + return skb->nf_bridge; + + nf_bridge = nf_bridge_alloc(skb); + if (nf_bridge == NULL) + return NULL; + + memcpy(nf_bridge, skb->nf_bridge, sizeof(struct nf_bridge_info)); + atomic_set(&nf_bridge->use, 1); + nf_bridge_put(skb->nf_bridge); + skb->nf_bridge = nf_bridge; - if (tmp) { - memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info)); - atomic_set(&tmp->use, 1); - } - nf_bridge_put(nf_bridge); - nf_bridge = tmp; - } return nf_bridge; } @@ -556,7 +561,8 @@ static unsigned int br_nf_pre_routing_ipv6(const struct nf_hook_ops *ops, return NF_DROP; nf_bridge_put(skb->nf_bridge); - if (!nf_bridge_alloc(skb)) + skb->nf_bridge = nf_bridge_alloc(skb); + if (skb->nf_bridge == NULL) return NF_DROP; if (!setup_pre_routing(skb)) return NF_DROP; @@ -612,7 +618,8 @@ static unsigned int br_nf_pre_routing(const struct nf_hook_ops *ops, return NF_DROP; nf_bridge_put(skb->nf_bridge); - if (!nf_bridge_alloc(skb)) + skb->nf_bridge = nf_bridge_alloc(skb); + if (skb->nf_bridge == NULL) return NF_DROP; if (!setup_pre_routing(skb)) return NF_DROP; -- 1.7.10.4