[PATCH v2] netfilter: bridge: unshare bridge info before change it

[PATCH v2] netfilter: bridge: unshare bridge info before change it

[Gao feng]

Many packets may share the same bridge information,
we should unshare the bridge info before we change it,
otherwise other packets will go to PF_INET(6)/PRE_ROUTING
second time or the pkt_type of other packets will be
incorrect.

The problem occurs in below case.

Firstly setup NFQUEUE rule on ipv4 PREROUTING chain.

When gso packet came in from bridge, br_nf_pre_routing will
allocate nf_bridge_info for this gso packet. and call setup_pre_routing
to setup nf_bridge_info.(such as nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING)

Then this packet goes to ipv4 prerouting chain, nfqnl_enqueue_packet
will call skb_segment to segment this gso packet. in skb_segment, the new
packets will copy gso packet's header(__copy_skb_header), so there will
be many packets share the same nf_bridge_info.

When these segmented packets being reinjected into kernel, they will continue
going through bridge netfilter, br_nf_pre_routing_finish will clean the
BRNF_NF_BRIDGE_PREROUTING for the first packet, setup it for the secondary
packet, clean it for the third packet...

If the dest of these packets is local machine, they will come into br_pass_frame_up.
then go to ipv4 prerouting chain again through netif_receive_skb. so ip_sabotage_in
will not stop half of these packets.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 include/linux/netfilter_bridge.h |  54 ++++++++++++++++++++-
 net/bridge/br_netfilter.c        | 101 +++++++++++++++++++--------------------
 2 files changed, 100 insertions(+), 55 deletions(-)

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);
+
+		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;
+}
+
+static inline struct nf_bridge_info *
+nf_bridge_set_mask(struct sk_buff *skb, unsigned int mask)
+{
+	if (!nf_bridge_unshare(skb))
+		return NULL;
+
+	skb->nf_bridge->mask |= mask;
+	return skb->nf_bridge;
+}
+
+static inline struct nf_bridge_info *
+nf_bridge_unset_mask(struct sk_buff *skb, unsigned int mask)
+{
+	if (!nf_bridge_unshare(skb))
+		return NULL;
+
+	skb->nf_bridge->mask &= ~mask;
+	return skb->nf_bridge;
+}
+
 int br_handle_frame_finish(struct sk_buff *skb);
 /* Only used in br_device.c */
 static inline int br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge;
 
 	skb_pull(skb, ETH_HLEN);
-	nf_bridge->mask ^= BRNF_BRIDGED_DNAT;
+	nf_bridge = nf_bridge_unset_mask(skb, BRNF_BRIDGED_DNAT);
+	if (nf_bridge == NULL) {
+		kfree_skb(skb);
+		return 0;
+	}
 	skb_copy_to_linear_data_offset(skb, -(ETH_HLEN-ETH_ALEN),
 				       skb->nf_bridge->data, ETH_HLEN-ETH_ALEN);
 	skb->dev = nf_bridge->physindev;
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 1a4f32c..eb00150 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -127,32 +127,6 @@ static inline struct net_device *bridge_parent(const struct net_device *dev)
 	return port ? port->br->dev : NULL;
 }
 
-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);
-
-		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;
-}
-
 static inline void nf_bridge_push_encap_header(struct sk_buff *skb)
 {
 	unsigned int len = nf_bridge_encap_header_len(skb);
@@ -243,20 +217,25 @@ drop:
  * bridge PRE_ROUTING hook. */
 static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge;
 	struct rtable *rt;
 
-	if (nf_bridge->mask & BRNF_PKT_TYPE) {
+	if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
 		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
+		nf_bridge = nf_bridge_unset_mask(skb,
+				(BRNF_PKT_TYPE | BRNF_NF_BRIDGE_PREROUTING));
+	} else {
+		nf_bridge = nf_bridge_unset_mask(skb,
+				BRNF_NF_BRIDGE_PREROUTING);
 	}
-	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+
+	if (nf_bridge == NULL)
+		goto drop;
 
 	rt = bridge_parent_rtable(nf_bridge->physindev);
-	if (!rt) {
-		kfree_skb(skb);
-		return 0;
-	}
+	if (!rt)
+		goto drop;
+
 	skb_dst_set_noref(skb, &rt->dst);
 
 	skb->dev = nf_bridge->physindev;
@@ -264,8 +243,11 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 	nf_bridge_push_encap_header(skb);
 	NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
 		       br_handle_frame_finish, 1);
-
+out:
 	return 0;
+drop:
+	kfree_skb(skb);
+	goto out;
 }
 
 /* Obtain the correct destination MAC address, while preserving the original
@@ -285,7 +267,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 	dst = skb_dst(skb);
 	neigh = dst_neigh_lookup_skb(dst, skb);
 	if (neigh) {
-		int ret;
+		int ret = 0;
 
 		if (neigh->hh.hh_len) {
 			neigh_hh_bridge(&neigh->hh, skb);
@@ -302,8 +284,12 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 							 ETH_HLEN-ETH_ALEN);
 			/* tell br_dev_xmit to continue with forwarding */
 			nf_bridge->mask |= BRNF_BRIDGED_DNAT;
-			/* FIXME Need to refragment */
-			ret = neigh->output(neigh, skb);
+			if (!nf_bridge_set_mask(skb, BRNF_BRIDGED_DNAT)) {
+				kfree_skb(skb);
+			} else {
+				/* FIXME Need to refragment */
+				ret = neigh->output(neigh, skb);
+			}
 		}
 		neigh_release(neigh);
 		return ret;
@@ -355,7 +341,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
 	struct iphdr *iph = ip_hdr(skb);
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge;
 	struct rtable *rt;
 	int err;
 	int frag_max_size;
@@ -363,11 +349,18 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 	frag_max_size = IPCB(skb)->frag_max_size;
 	BR_INPUT_SKB_CB(skb)->frag_max_size = frag_max_size;
 
-	if (nf_bridge->mask & BRNF_PKT_TYPE) {
+	if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
 		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
+		nf_bridge = nf_bridge_unset_mask(skb,
+				(BRNF_PKT_TYPE | BRNF_NF_BRIDGE_PREROUTING));
+	} else {
+		nf_bridge = nf_bridge_unset_mask(skb,
+				BRNF_NF_BRIDGE_PREROUTING);
 	}
-	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+
+	if (!nf_bridge)
+		goto free_skb;
+
 	if (dnat_took_place(skb)) {
 		if ((err = ip_route_input(skb, iph->daddr, iph->saddr, iph->tos, dev))) {
 			struct in_device *in_dev = __in_dev_get_rcu(dev);
@@ -653,7 +646,11 @@ static int br_nf_forward_finish(struct sk_buff *skb)
 		in = nf_bridge->physindev;
 		if (nf_bridge->mask & BRNF_PKT_TYPE) {
 			skb->pkt_type = PACKET_OTHERHOST;
-			nf_bridge->mask ^= BRNF_PKT_TYPE;
+
+			if (!nf_bridge_unset_mask(skb, BRNF_PKT_TYPE)) {
+				kfree_skb(skb);
+				return 0;
+			}
 		}
 		nf_bridge_update_protocol(skb);
 	} else {
@@ -685,11 +682,6 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 	if (!skb->nf_bridge)
 		return NF_ACCEPT;
 
-	/* Need exclusive nf_bridge_info since we might have multiple
-	 * different physoutdevs. */
-	if (!nf_bridge_unshare(skb))
-		return NF_DROP;
-
 	parent = bridge_parent(out);
 	if (!parent)
 		return NF_DROP;
@@ -706,14 +698,16 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 	nf_bridge = skb->nf_bridge;
 	if (skb->pkt_type == PACKET_OTHERHOST) {
 		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
+		nf_bridge = nf_bridge_set_mask(skb,
+				(BRNF_PKT_TYPE | BRNF_BRIDGED));
+	} else {
+		/* The physdev module checks on this */
+		nf_bridge = nf_bridge_set_mask(skb, BRNF_BRIDGED);
 	}
 
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
+	if (!nf_bridge || (pf == NFPROTO_IPV4 && br_parse_ip_options(skb)))
 		return NF_DROP;
 
-	/* The physdev module checks on this */
-	nf_bridge->mask |= BRNF_BRIDGED;
 	nf_bridge->physoutdev = skb->dev;
 	if (pf == NFPROTO_IPV4)
 		skb->protocol = htons(ETH_P_IP);
@@ -820,7 +814,8 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
 	 * about the value of skb->pkt_type. */
 	if (skb->pkt_type == PACKET_OTHERHOST) {
 		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
+		if (!nf_bridge_set_mask(skb, BRNF_PKT_TYPE))
+			return NF_DROP;
 	}
 
 	nf_bridge_pull_encap_header(skb);
-- 
1.9.3

[PATCH] use NF_BR_PRI_BRNF in NF_HOOK_THRESH

[Gao feng]

packets jump to ip/ipv6/arp netfilter from bridge
netfilter hooks whose priority are NF_BR_PRI_BRNF,
so when packets return to bridge netfilter, the
thresh is NF_BR_PRI_BRNF + 1.

this patch use marco NF_BR_PRI_BRNF + 1 to replace
the number 1.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/bridge/br_netfilter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index eb00150..6c90696 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -242,7 +242,7 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 	nf_bridge_update_protocol(skb);
 	nf_bridge_push_encap_header(skb);
 	NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
-		       br_handle_frame_finish, 1);
+		       br_handle_frame_finish, NF_BR_PRI_BRNF + 1);
 out:
 	return 0;
 drop:
@@ -399,7 +399,7 @@ bridged_dnat:
 					       NF_BR_PRE_ROUTING,
 					       skb, skb->dev, NULL,
 					       br_nf_pre_routing_finish_bridge,
-					       1);
+					       NF_BR_PRI_BRNF + 1);
 				return 0;
 			}
 			ether_addr_copy(eth_hdr(skb)->h_dest, dev->dev_addr);
@@ -418,7 +418,7 @@ bridged_dnat:
 	nf_bridge_update_protocol(skb);
 	nf_bridge_push_encap_header(skb);
 	NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
-		       br_handle_frame_finish, 1);
+		       br_handle_frame_finish, NF_BR_PRI_BRNF + 1);
 
 	return 0;
 }
@@ -659,7 +659,7 @@ static int br_nf_forward_finish(struct sk_buff *skb)
 	nf_bridge_push_encap_header(skb);
 
 	NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_FORWARD, skb, in,
-		       skb->dev, br_forward_finish, 1);
+		       skb->dev, br_forward_finish, NF_BR_PRI_BRNF + 1);
 	return 0;
 }
 
-- 
1.9.3

Re: [PATCH v2] netfilter: bridge: unshare bridge info before change it

[Pablo Neira Ayuso]

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.

> +
> +		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;
> +}

Re: [PATCH v2] netfilter: bridge: unshare bridge info before change it

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

On Wed, Nov 19, 2014 at 02:07:51PM +0100, 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.

I took over your patch and gave it another spin.

Specifically, I added some helper functions to handle the skb->pkttype
mangling (see nf_br_mangle_pkttype(), nf_br_restore_pkttype().
Also fixed the problem above, compile tested only.

Not your fault, this br_netfilter code is not in good shape, but I
would like not to get it worse.

Would you take this over and split it in smaller patches? I'd suggest:

1) Cleanup patch to use &=~ instead of ^=. The existing code looks
fine, but I think a bug may invert the bits when use xor to handle
flags.

2) Patch to move nf_bridge_alloc() and so on to the header file, to
prepare the unshare fix.

3) Patch to add the pkttype helper functions.

4) Your unshare fix for br_netfilter.

Would you work on that? Thanks.

[-- Attachment #2: 0001-netfilter-bridge-unshare-bridge-info-before-change-i.patch --]
[-- Type: text/x-diff, Size: 11412 bytes --]

>From 6fd546e3866fe5a60be84aebd5fbc1e93ef14f46 Mon Sep 17 00:00:00 2001
From: Gao feng <gaofeng@cn.fujitsu.com>
Date: Wed, 19 Nov 2014 11:07:32 +0800
Subject: [PATCH] netfilter: bridge: unshare bridge info before change it

Many packets may share the same bridge information, we should unshare
the bridge info before we change it, otherwise other packets will go to
PF_INET(6)/PRE_ROUTING second time or the pkt_type of other packets
will be incorrect.

The problem occurs in below case.

Firstly setup NFQUEUE rule on ipv4 PREROUTING chain.

When gso packet came in from bridge, br_nf_pre_routing will
allocate nf_bridge_info for this gso packet and call setup_pre_routing
to setup nf_bridge_info (such as nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING)

Then this packet goes to ipv4 prerouting chain, nfqnl_enqueue_packet
will call skb_segment to segment this gso packet. in skb_segment, the
new packets will copy gso packet's header(__copy_skb_header), so there
will be many packets share the same nf_bridge_info.

When these segmented packets being reinjected into kernel, they will
continue going through bridge netfilter, br_nf_pre_routing_finish will
clean the BRNF_NF_BRIDGE_PREROUTING for the first packet, setup it for
the secondary packet, clean it for the third packet...

If the dest of these packets is local machine, they will come into
br_pass_frame_up. then go to ipv4 prerouting chain again through
netif_receive_skb. so ip_sabotage_in will not stop half of these
packets.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
NOTE: This still needs another review. The patch is rather large and it would
be good to split this in smaller chunks for easier review.

 include/linux/netfilter_bridge.h |   57 +++++++++++++++-
 net/bridge/br_netfilter.c        |  135 +++++++++++++++++---------------------
 2 files changed, 116 insertions(+), 76 deletions(-)

diff --git a/include/linux/netfilter_bridge.h b/include/linux/netfilter_bridge.h
index c755e49..cad76d6 100644
--- a/include/linux/netfilter_bridge.h
+++ b/include/linux/netfilter_bridge.h
@@ -81,14 +81,67 @@ 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)
+{
+	struct nf_bridge_info *nf_bridge;
+
+	nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
+	if (nf_bridge == NULL)
+		return NULL;
+
+	atomic_set(&nf_bridge->use, 1);
+
+	return nf_bridge;
+}
+
+static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
+{
+	if (atomic_read(&skb->nf_bridge->use) > 1) {
+		struct nf_bridge_info *clone = nf_bridge_alloc(skb);
+
+		if (clone) {
+			memcpy(clone, skb->nf_bridge,
+			       sizeof(struct nf_bridge_info));
+			atomic_set(&clone->use, 1);
+		}
+		nf_bridge_put(skb->nf_bridge);
+		return clone;
+	}
+	return skb->nf_bridge;
+}
+
+static inline struct nf_bridge_info *
+nf_bridge_set_mask(struct sk_buff *skb, unsigned int mask)
+{
+	if (!nf_bridge_unshare(skb))
+		return NULL;
+
+	skb->nf_bridge->mask |= mask;
+	return skb->nf_bridge;
+}
+
+static inline struct nf_bridge_info *
+nf_bridge_unset_mask(struct sk_buff *skb, unsigned int mask)
+{
+	if (!nf_bridge_unshare(skb))
+		return NULL;
+
+	skb->nf_bridge->mask &= ~mask;
+	return skb->nf_bridge;
+}
+
 int br_handle_frame_finish(struct sk_buff *skb);
 /* Only used in br_device.c */
 static inline int br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge;
 
 	skb_pull(skb, ETH_HLEN);
-	nf_bridge->mask ^= BRNF_BRIDGED_DNAT;
+	nf_bridge = nf_bridge_unset_mask(skb, BRNF_BRIDGED_DNAT);
+	if (nf_bridge == NULL) {
+		kfree_skb(skb);
+		return 0;
+	}
 	skb_copy_to_linear_data_offset(skb, -(ETH_HLEN-ETH_ALEN),
 				       skb->nf_bridge->data, ETH_HLEN-ETH_ALEN);
 	skb->dev = nf_bridge->physindev;
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index f81dc33..4647cd2 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -128,32 +128,6 @@ static inline struct net_device *bridge_parent(const struct net_device *dev)
 	return port ? port->br->dev : NULL;
 }
 
-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);
-
-		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;
-}
-
 static inline void nf_bridge_push_encap_header(struct sk_buff *skb)
 {
 	unsigned int len = nf_bridge_encap_header_len(skb);
@@ -253,25 +227,43 @@ drop:
 	return -1;
 }
 
+static inline struct nf_bridge_info *nf_br_mangle_pkttype(struct sk_buff *skb)
+{
+	if (skb->pkt_type != PACKET_OTHERHOST)
+		return skb->nf_bridge;
+
+	skb->pkt_type = PACKET_HOST;
+	return nf_bridge_set_mask(skb, BRNF_PKT_TYPE);
+}
+
+static inline struct nf_bridge_info *nf_br_restore_pkttype(struct sk_buff *skb)
+{
+	if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
+		skb->pkt_type = PACKET_OTHERHOST;
+		return nf_bridge_unset_mask(skb, BRNF_PKT_TYPE);
+	}
+
+	return skb->nf_bridge;
+}
+
 /* PF_BRIDGE/PRE_ROUTING *********************************************/
 /* Undo the changes made for ip6tables PREROUTING and continue the
  * bridge PRE_ROUTING hook. */
 static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge;
 	struct rtable *rt;
 
-	if (nf_bridge->mask & BRNF_PKT_TYPE) {
-		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
-	}
-	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+	nf_bridge = nf_br_restore_pkttype(skb);
+	if (nf_bridge == NULL)
+		goto drop;
+
+	nf_bridge->mask &= ~BRNF_NF_BRIDGE_PREROUTING;
 
 	rt = bridge_parent_rtable(nf_bridge->physindev);
-	if (!rt) {
-		kfree_skb(skb);
-		return 0;
-	}
+	if (!rt)
+		goto drop;
+
 	skb_dst_set_noref(skb, &rt->dst);
 
 	skb->dev = nf_bridge->physindev;
@@ -279,7 +271,9 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 	nf_bridge_push_encap_header(skb);
 	NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
 		       br_handle_frame_finish, 1);
-
+	return 0;
+drop:
+	kfree_skb(skb);
 	return 0;
 }
 
@@ -300,7 +294,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 	dst = skb_dst(skb);
 	neigh = dst_neigh_lookup_skb(dst, skb);
 	if (neigh) {
-		int ret;
+		int ret = 0;
 
 		if (neigh->hh.hh_len) {
 			neigh_hh_bridge(&neigh->hh, skb);
@@ -317,8 +311,12 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 							 ETH_HLEN-ETH_ALEN);
 			/* tell br_dev_xmit to continue with forwarding */
 			nf_bridge->mask |= BRNF_BRIDGED_DNAT;
-			/* FIXME Need to refragment */
-			ret = neigh->output(neigh, skb);
+			if (!nf_bridge_set_mask(skb, BRNF_BRIDGED_DNAT)) {
+				kfree_skb(skb);
+			} else {
+				/* FIXME Need to refragment */
+				ret = neigh->output(neigh, skb);
+			}
 		}
 		neigh_release(neigh);
 		return ret;
@@ -370,7 +368,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
 	struct iphdr *iph = ip_hdr(skb);
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge;
 	struct rtable *rt;
 	int err;
 	int frag_max_size;
@@ -378,11 +376,12 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 	frag_max_size = IPCB(skb)->frag_max_size;
 	BR_INPUT_SKB_CB(skb)->frag_max_size = frag_max_size;
 
-	if (nf_bridge->mask & BRNF_PKT_TYPE) {
-		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
-	}
-	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+	nf_bridge = nf_br_restore_pkttype(skb);
+	if (nf_bridge == NULL)
+		goto free_skb;
+
+	nf_bridge->mask &= ~BRNF_NF_BRIDGE_PREROUTING;
+
 	if (dnat_took_place(skb)) {
 		if ((err = ip_route_input(skb, iph->daddr, iph->saddr, iph->tos, dev))) {
 			struct in_device *in_dev = __in_dev_get_rcu(dev);
@@ -462,13 +461,9 @@ static struct net_device *brnf_get_logical_dev(struct sk_buff *skb, const struct
 /* Some common code for IPv4/IPv6 */
 static struct net_device *setup_pre_routing(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
-
-	if (skb->pkt_type == PACKET_OTHERHOST) {
-		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
-	}
+	struct nf_bridge_info *nf_bridge;
 
+	nf_bridge = nf_br_mangle_pkttype(skb);
 	nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;
 	nf_bridge->physindev = skb->dev;
 	skb->dev = brnf_get_logical_dev(skb, skb->dev);
@@ -571,7 +566,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;
@@ -627,7 +623,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;
@@ -666,10 +663,11 @@ static int br_nf_forward_finish(struct sk_buff *skb)
 
 	if (!IS_ARP(skb) && !IS_VLAN_ARP(skb)) {
 		in = nf_bridge->physindev;
-		if (nf_bridge->mask & BRNF_PKT_TYPE) {
-			skb->pkt_type = PACKET_OTHERHOST;
-			nf_bridge->mask ^= BRNF_PKT_TYPE;
-		}
+
+		nf_bridge = nf_br_restore_pkttype(skb);
+		if (nf_bridge == NULL)
+			return 0;
+
 		nf_bridge_update_protocol(skb);
 	} else {
 		in = *((struct net_device **)(skb->cb));
@@ -700,11 +698,6 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 	if (!skb->nf_bridge)
 		return NF_ACCEPT;
 
-	/* Need exclusive nf_bridge_info since we might have multiple
-	 * different physoutdevs. */
-	if (!nf_bridge_unshare(skb))
-		return NF_DROP;
-
 	parent = bridge_parent(out);
 	if (!parent)
 		return NF_DROP;
@@ -718,13 +711,9 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 
 	nf_bridge_pull_encap_header(skb);
 
-	nf_bridge = skb->nf_bridge;
-	if (skb->pkt_type == PACKET_OTHERHOST) {
-		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
-	}
-
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
+	nf_bridge = nf_br_mangle_pkttype(skb);
+	if (nf_bridge == NULL ||
+	    (pf == NFPROTO_IPV4 && br_parse_ip_options(skb)))
 		return NF_DROP;
 
 	/* The physdev module checks on this */
@@ -833,10 +822,8 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
 
 	/* We assume any code from br_dev_queue_push_xmit onwards doesn't care
 	 * about the value of skb->pkt_type. */
-	if (skb->pkt_type == PACKET_OTHERHOST) {
-		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
-	}
+	if (nf_br_mangle_pkttype(skb) == NULL)
+		return NF_DROP;
 
 	nf_bridge_pull_encap_header(skb);
 	nf_bridge_save_header(skb);
-- 
1.7.10.4

Re: [PATCH] use NF_BR_PRI_BRNF in NF_HOOK_THRESH

[Pablo Neira Ayuso]

On Wed, Nov 19, 2014 at 11:07:33AM +0800, Gao feng wrote:
> packets jump to ip/ipv6/arp netfilter from bridge
> netfilter hooks whose priority are NF_BR_PRI_BRNF,
> so when packets return to bridge netfilter, the
> thresh is NF_BR_PRI_BRNF + 1.
> 
> this patch use marco NF_BR_PRI_BRNF + 1 to replace
> the number 1.

This code has been using this for long long time. It would be great if
you can include in the description what was broken before this patch
or if this is a simple cleanup. Thanks Feng.

Re: [PATCH v2] netfilter: bridge: unshare bridge info before change it

[Gao feng]

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.

>> +
>> +		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;
>> +}
> 

Re: [PATCH v2] netfilter: bridge: unshare bridge info before change it

[Gao feng]

On 11/19/2014 09:32 PM, Pablo Neira Ayuso wrote:
> On Wed, Nov 19, 2014 at 02:07:51PM +0100, 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.
> I took over your patch and gave it another spin.
> 
> Specifically, I added some helper functions to handle the skb->pkttype
> mangling (see nf_br_mangle_pkttype(), nf_br_restore_pkttype().
> Also fixed the problem above, compile tested only.
> 
> Not your fault, this br_netfilter code is not in good shape, but I
> would like not to get it worse.
> 
> Would you take this over and split it in smaller patches? I'd suggest:
> 

Looks good,I will take this over if you have no objection to my comment below.

> 1) Cleanup patch to use &=~ instead of ^=. The existing code looks
> fine, but I think a bug may invert the bits when use xor to handle
> flags.
> 
> 2) Patch to move nf_bridge_alloc() and so on to the header file, to
> prepare the unshare fix.
> 
> 3) Patch to add the pkttype helper functions.
> 
> 4) Your unshare fix for br_netfilter.
> 
> Would you work on that? Thanks.
> 
> 
> 0001-netfilter-bridge-unshare-bridge-info-before-change-i.patch
> 
> 
>>From 6fd546e3866fe5a60be84aebd5fbc1e93ef14f46 Mon Sep 17 00:00:00 2001
> From: Gao feng <gaofeng@cn.fujitsu.com>
> Date: Wed, 19 Nov 2014 11:07:32 +0800
> Subject: [PATCH] netfilter: bridge: unshare bridge info before change it
> 
> Many packets may share the same bridge information, we should unshare
> the bridge info before we change it, otherwise other packets will go to
> PF_INET(6)/PRE_ROUTING second time or the pkt_type of other packets
> will be incorrect.
> 
> The problem occurs in below case.
> 
> Firstly setup NFQUEUE rule on ipv4 PREROUTING chain.
> 
> When gso packet came in from bridge, br_nf_pre_routing will
> allocate nf_bridge_info for this gso packet and call setup_pre_routing
> to setup nf_bridge_info (such as nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING)
> 
> Then this packet goes to ipv4 prerouting chain, nfqnl_enqueue_packet
> will call skb_segment to segment this gso packet. in skb_segment, the
> new packets will copy gso packet's header(__copy_skb_header), so there
> will be many packets share the same nf_bridge_info.
> 
> When these segmented packets being reinjected into kernel, they will
> continue going through bridge netfilter, br_nf_pre_routing_finish will
> clean the BRNF_NF_BRIDGE_PREROUTING for the first packet, setup it for
> the secondary packet, clean it for the third packet...
> 
> If the dest of these packets is local machine, they will come into
> br_pass_frame_up. then go to ipv4 prerouting chain again through
> netif_receive_skb. so ip_sabotage_in will not stop half of these
> packets.
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> NOTE: This still needs another review. The patch is rather large and it would
> be good to split this in smaller chunks for easier review.
> 
>  include/linux/netfilter_bridge.h |   57 +++++++++++++++-
>  net/bridge/br_netfilter.c        |  135 +++++++++++++++++---------------------
>  2 files changed, 116 insertions(+), 76 deletions(-)
> 
> diff --git a/include/linux/netfilter_bridge.h b/include/linux/netfilter_bridge.h
> index c755e49..cad76d6 100644
> --- a/include/linux/netfilter_bridge.h
> +++ b/include/linux/netfilter_bridge.h
> @@ -81,14 +81,67 @@ 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)
> +{
> +	struct nf_bridge_info *nf_bridge;
> +
> +	nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
> +	if (nf_bridge == NULL)
> +		return NULL;
> +
> +	atomic_set(&nf_bridge->use, 1);
> +
> +	return nf_bridge;
> +}
> +
> +static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
> +{
> +	if (atomic_read(&skb->nf_bridge->use) > 1) {
> +		struct nf_bridge_info *clone = nf_bridge_alloc(skb);
> +
> +		if (clone) {
> +			memcpy(clone, skb->nf_bridge,
> +			       sizeof(struct nf_bridge_info));
> +			atomic_set(&clone->use, 1);
> +		}
> +		nf_bridge_put(skb->nf_bridge);

You haven't assign clone to the skb->nf_bridge. the other place still access the
original skb->nf_bridge and you already release the skb->nf_bridge here. ant the
cloned nf_bridge will never be freed.

I still think my patch has no problem. the nf_bridge_unshare means if one packet
wants to change the shared nfbridge, then allocate a new one for this packet.

Re: [PATCH v2] netfilter: bridge: unshare bridge info before change it

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

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.

[-- Attachment #2: 0001-br_netfilter-cleanup-nf_bridge_alloc-and-nf_bridge_u.patch --]
[-- Type: text/x-diff, Size: 2695 bytes --]

>From 1c6141898a7b82484370ec9c61e3acdaf22eaa91 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
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 <pablo@netfilter.org>
---
 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