Skip to contentSkip to conversation

get rid of skb->nf_bridge pointer

PATCH In review Florian Westphal 3 participants 19 messages Apr 1, 2015 – Apr 2, 2015

Subsystems (5)
Networking Network drivers netfilter IPv4/IPv6 +1 more subsystems
lore
Share & tools
lore

Florian Westphal

author maintainer
#0 lore raw

cover get rid of skb->nf_bridge pointer

[ netdev hackers are encouraged to look at patches #2 and #11 specifically,
those are the ones with largest impact outside netfilter land ]

Remove skb->nf_bridge pointer.

Unfortunately we still need some way to decide if skb is bridged or not, else kfree_skb, skb_clone etc. would have to do costly lookups in bridge netfilter.

We use a 2 bit state field in the skb for this purpose. If its zero, skb is not bridged (same as skb->nf_bridge == NULL in current kernel).

nf_bridge_info is stored in an rhashtable; bridge netfilter and the few other places (nfqueue, nflog, physdev match) that need to access bridge netfilter data do on-demand lookups in an rhashtable to access the data associated with a bridged skb.

skb_clone and skb_copy will call into netfilter core helpers for bridged skbs to duplicate the information if needed. Likewise, kfree_skb removes and frees the bridge netfilter meta data as well if needed.

In order to avoid those lookups where we're dealing with non-bridged skbs, we store 2 bit state field in the skb.

Tested, on host connected to kvm-bridge:

ping -s $bignum $ip_behind_bridge

on bridge:
-j REDIRECT
-j DNAT --to-destination $ip_behind_bridge
-m physdev match with in/outdev match in FORWARD and INPUT (indev only)
- same w. active -j NFQUEUE.

Patch 11 substitutes the pointer for on-demand lookups, most of the other patches prepare for this change by adding helpers and splitting state information into 'public' and 'bridge netfilter private'.

The alternative to the rhashtable is to store the bridge netfilter metadata in skb->cb[], but there are some caveats since we need such metadata to survive local delivery too (else we'd break use of physdev match in INPUT).

This is why external store was chosen.

Feedback and suggestions welcome.

 include/linux/netfilter.h                  |    8 
 include/linux/netfilter_bridge.h           |  104 +++++-
 include/linux/skbuff.h                     |   87 ++---
 include/net/ip.h                           |    4 
 net/bridge/br_device.c                     |   19 -
 net/bridge/br_netfilter.c                  |  482 ++++++++++++++++++++---------
 net/bridge/br_private.h                    |    2 
 net/core/skbuff.c                          |    5 
 net/ipv4/ip_output.c                       |   30 +
 net/ipv4/netfilter/nf_defrag_ipv4.c        |    3 
 net/ipv4/netfilter/nf_reject_ipv4.c        |    6 
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c  |    3 
 net/ipv6/netfilter/nf_reject_ipv6.c        |    6 
 net/netfilter/core.c                       |   49 ++
 net/netfilter/ipset/ip_set_hash_netiface.c |   32 +
 net/netfilter/nf_log_common.c              |    7 
 net/netfilter/nf_queue.c                   |   22 -
 net/netfilter/nfnetlink_log.c              |   17 -
 net/netfilter/nfnetlink_queue_core.c       |   34 +-
 net/netfilter/xt_physdev.c                 |   36 +-
 20 files changed, 670 insertions(+), 286 deletions(-)
author maintainer
#1 lore raw

PATCH 1/14 netfilter: bridge: really save frag_max_size between PRE and POST_ROUTING

We also need to save/store in forward, else br_parse_ip_options call will zero frag_max_size as well.

Fixes 93fdd47e5 ('bridge: Save frag_max_size between PRE_ROUTING and POST_ROUTING')
Signed-off-by Florian Westphal <[email protected]>
---
 net/bridge/br_netfilter.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
net/bridge/br_netfilter.c +15 -2
index f3884a1..282ed76 100644
@@ -686,6 +686,13 @@ static int br_nf_forward_finish(struct sk_buff *skb)
 	struct net_device *in;
 
 	if (!IS_ARP(skb) && !IS_VLAN_ARP(skb)) {
+		int frag_max_size;
+
+		if (skb->protocol == htons(ETH_P_IP)) {
+			frag_max_size = IPCB(skb)->frag_max_size;
+			BR_INPUT_SKB_CB(skb)->frag_max_size = frag_max_size;
+		}
+
 		in = nf_bridge->physindev;
 		if (nf_bridge->mask & BRNF_PKT_TYPE) {
 			skb->pkt_type = PACKET_OTHERHOST;

@@ -745,8 +752,14 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 		nf_bridge->mask |= BRNF_PKT_TYPE;
 	}
 
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
-		return NF_DROP;
+	if (pf == NFPROTO_IPV4) {
+		int frag_max = BR_INPUT_SKB_CB(skb)->frag_max_size;
+
+		if (br_parse_ip_options(skb))
+			return NF_DROP;
+
+		IPCB(skb)->frag_max_size = frag_max;
+	}
 
 	nf_bridge->physoutdev = skb->dev;
 	if (pf == NFPROTO_IPV4)
Signature · 1 line
2.0.5
maintainer
#2 lore raw

Re: [PATCH nf-next 01/14] netfilter: bridge: really save frag_max_size between PRE and POST_ROUTING

Quoted 2 lines · Florian Westphal

We also need to save/store in forward, else br_parse_ip_options call will zero frag_max_size as well.

I have only applied this fix to nf-next. Thanks.

maintainer
#3 lore raw

Re: [PATCH nf-next 01/14] netfilter: bridge: really save frag_max_size between PRE and POST_ROUTING

Quoted 5 lines · Pablo Neira Ayuso
Quoted 2 lines · Florian Westphal

We also need to save/store in forward, else br_parse_ip_options call will zero frag_max_size as well.

I have only applied this fix to nf-next. Thanks.

BTW, you can resubmit patches in case they are independent of the ip_fragment() change in first place. Thanks.

author maintainer
#4 lore raw

PATCH 2/14 net: untangle ip_fragment and bridge netfilter

In reply to Florian Westphal

Long time ago it was possible for the netfilter ip_conntrack core to call ip_fragment in POST_ROUTING hook.

This is no longer the case, so the only case where bridge netfilter ends up calling ip_fragment is the direct call site in br_netfilter.c.

Add mtu arguments to ip_fragment and remove the bridge netfilter mtu helper.

bridge netfilter uses following MTU choice strategy:

A - frag_max_size is set:
We're looking at skb where at least one fragment had the DF bit set. Use frag_max_size as the MTU to re-create the original fragments as close as possible.

If this would bring us over the MTU, silently drop the skb. (This can happen with bridge where not all devices have same MTU,
but its difficult to imagine such setup that doesn't already have worse problems)

B - frag_max_size not set.
This means original packet did not have DF bit set, just use the output device MTU for refragmentation. This is safe since any router in the path is free to re-fragment as needed.

C - skb too big and ignore_df not set. Means the input device mtu is bigger
than outdevice mtu. Drop skb, setup should be fixed instead.

Signed-off-by Florian Westphal <[email protected]>

---
I looked at various other alternatives for 'please undo defrag on this ipv4 packet'. But this is the most sensible thing to do.

Keeping original fragments doesn't fly since we might have performed NAT or other manipluations on the (defragmented) skb.
Building defrag/frag into GRO/GSO doesn't work either since the purpose of GRO is a lot different (we also can't rely on all fragments arriving in same napi context).

Instead we refragment using size of largest fragment seen, or using the bridge device mtu.

 include/linux/netfilter_bridge.h |  7 -------
 include/net/ip.h                 |  4 ++--
 net/bridge/br_netfilter.c        | 40 +++++++++++++++++++++++++++++++++-------
 net/ipv4/ip_output.c             | 30 ++++++++++++++++++------------
 4 files changed, 53 insertions(+), 28 deletions(-)
include/linux/netfilter_bridge.h +0 -7
index 2734977..b131613 100644
@@ -23,13 +23,6 @@ enum nf_br_hook_priorities {
 #define BRNF_8021Q			0x10
 #define BRNF_PPPoE			0x20
 
-static inline unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb)
-{
-	if (unlikely(skb->nf_bridge->mask & BRNF_PPPoE))
-		return PPPOE_SES_HLEN;
-	return 0;
-}
-
 int br_handle_frame_finish(struct sk_buff *skb);
 
 static inline void br_drop_fake_rtable(struct sk_buff *skb)
include/net/ip.h +2 -2
index d0808a3..1fe203a 100644
@@ -108,8 +108,8 @@ int ip_local_deliver(struct sk_buff *skb);
 int ip_mr_input(struct sk_buff *skb);
 int ip_output(struct sock *sk, struct sk_buff *skb);
 int ip_mc_output(struct sock *sk, struct sk_buff *skb);
-int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *));
-int ip_do_nat(struct sk_buff *skb);
+int ip_fragment(struct sk_buff *skb, unsigned int mtu,
+		int (*output)(struct sk_buff *));
 void ip_send_check(struct iphdr *ip);
 int __ip_local_out(struct sk_buff *skb);
 int ip_local_out_sk(struct sock *sk, struct sk_buff *skb);
net/bridge/br_netfilter.c +33 -7
index 282ed76..c0c8700 100644
@@ -837,30 +837,56 @@ static int br_nf_push_frag_xmit(struct sk_buff *skb)
 	return br_dev_queue_push_xmit(skb);
 }
 
+static unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb)
+{
+	if (skb->nf_bridge->mask & BRNF_PPPoE)
+		return PPPOE_SES_HLEN;
+	return 0;
+}
+
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 {
 	int ret;
 	int frag_max_size;
-	unsigned int mtu_reserved;
+	unsigned int mtu_reserved, mtu;
 
 	if (skb_is_gso(skb) || skb->protocol != htons(ETH_P_IP))
 		return br_dev_queue_push_xmit(skb);
 
 	mtu_reserved = nf_bridge_mtu_reduction(skb);
+	mtu = min(skb->dev->mtu, IP_MAX_MTU) - mtu_reserved;
+
 	/* This is wrong! We should preserve the original fragment
 	 * boundaries by preserving frag_list rather than refragmenting.
+	 *
+	 * However, the skb might have been subject to NAT.
+	 * We try to un-do the fragmentation, using largest received fragment
+	 * size as mtu if it is set.
 	 */
-	if (skb->len + mtu_reserved > skb->dev->mtu) {
+	if (skb->len > mtu) {
+		if (!skb->ignore_df) /* was not a defragmented */
+			goto err_out;
+
 		frag_max_size = BR_INPUT_SKB_CB(skb)->frag_max_size;
+		if (frag_max_size) {
+			if (frag_max_size > mtu) /* too big and DF set */
+				goto err_out;
+			mtu = frag_max_size;
+		}
+
 		if (br_parse_ip_options(skb))
-			/* Drop invalid packet */
-			return NF_DROP;
-		IPCB(skb)->frag_max_size = frag_max_size;
-		ret = ip_fragment(skb, br_nf_push_frag_xmit);
-	} else
+			goto err_out;
+
+		ret = ip_fragment(skb, mtu, br_nf_push_frag_xmit);
+	} else {
 		ret = br_dev_queue_push_xmit(skb);
+	}
 
 	return ret;
+ err_out:
+	IP_INC_STATS_BH(dev_net(skb->dev), IPSTATS_MIB_FRAGFAILS);
+	kfree_skb(skb);
+	return 0;
 }
 #else
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
net/ipv4/ip_output.c +18 -12
index 8259e77..9054995 100644
@@ -216,6 +216,7 @@ static int ip_finish_output_gso(struct sk_buff *skb)
 	netdev_features_t features;
 	struct sk_buff *segs;
 	int ret = 0;
+	unsigned int mtu;
 
 	/* common case: locally created skb or seglen is <= mtu */
 	if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||

@@ -236,6 +237,7 @@ static int ip_finish_output_gso(struct sk_buff *skb)
 		return -ENOMEM;
 	}
 
+	mtu = ip_skb_dst_mtu(skb);
 	consume_skb(skb);
 
 	do {

@@ -243,7 +245,7 @@ static int ip_finish_output_gso(struct sk_buff *skb)
 		int err;
 
 		segs->next = NULL;
-		err = ip_fragment(segs, ip_finish_output2);
+		err = ip_fragment(segs, mtu, ip_finish_output2);
 
 		if (err && ret == 0)
 			ret = err;

@@ -255,6 +257,8 @@ static int ip_finish_output_gso(struct sk_buff *skb)
 
 static int ip_finish_output(struct sk_buff *skb)
 {
+	unsigned int mtu;
+
 #if defined(CONFIG_NETFILTER) && defined(CONFIG_XFRM)
 	/* Policy lookup after SNAT yielded a new policy */
 	if (skb_dst(skb)->xfrm != NULL) {

@@ -265,8 +269,9 @@ static int ip_finish_output(struct sk_buff *skb)
 	if (skb_is_gso(skb))
 		return ip_finish_output_gso(skb);
 
-	if (skb->len > ip_skb_dst_mtu(skb))
-		return ip_fragment(skb, ip_finish_output2);
+	mtu = ip_skb_dst_mtu(skb);
+	if (skb->len > mtu)
+		return ip_fragment(skb, mtu, ip_finish_output2);
 
 	return ip_finish_output2(skb);
 }

@@ -473,20 +478,26 @@ static void ip_copy_metadata(struct sk_buff *to, struct sk_buff *from)
 	skb_copy_secmark(to, from);
 }
 
-/*
+/**
+ *	ip_fragment - fragment IP datagram or send ICMP error
+ *
+ *	@skb: the skb to fragment
+ *	@mtu: mtu to use for fragmentation
+ *	@output: transmit function used to send fragments
+ *
  *	This IP datagram is too large to be sent in one piece.  Break it up into
  *	smaller pieces (each of size equal to IP header plus
  *	a block of the data of the original IP data part) that will yet fit in a
  *	single device frame, and queue such a frame for sending.
  */
-
-int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
+int ip_fragment(struct sk_buff *skb, unsigned int mtu,
+		int (*output)(struct sk_buff *))
 {
 	struct iphdr *iph;
 	int ptr;
 	struct net_device *dev;
 	struct sk_buff *skb2;
-	unsigned int mtu, hlen, left, len, ll_rs;
+	unsigned int hlen, left, len, ll_rs;
 	int offset;
 	__be16 not_last_frag;
 	struct rtable *rt = skb_rtable(skb);

@@ -500,7 +511,6 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 
 	iph = ip_hdr(skb);
 
-	mtu = ip_skb_dst_mtu(skb);
 	if (unlikely(((iph->frag_off & htons(IP_DF)) && !skb->ignore_df) ||
 		     (IPCB(skb)->frag_max_size &&
 		      IPCB(skb)->frag_max_size > mtu))) {

@@ -517,10 +527,6 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 
 	hlen = iph->ihl * 4;
 	mtu = mtu - hlen;	/* Size of data space */
-#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (skb->nf_bridge)
-		mtu -= nf_bridge_mtu_reduction(skb);
-#endif
 	IPCB(skb)->flags |= IPSKB_FRAG_COMPLETE;
 
 	/* When frag_list is given, use it. First, check its validity:
Signature · 1 line
2.0.5
maintainer
#5 lore raw

Re: [PATCH nf-next 02/14] net: untangle ip_fragment and bridge netfilter

From: Florian Westphal <[email protected]>
Date: Wed, 1 Apr 2015 22:36:28 +0200

Quoted 2 lines

Add mtu arguments to ip_fragment and remove the bridge netfilter mtu helper.

I told you I disagree with this approach. Anything that adds an 'mtu' argument to ip_fragment() I am not even going to look at seriously, there must be device context when you call that function.

Furthermore, and even more importantly, right now what bridge netfilter does with fragmentation is _terminally_ broken. It absolutely does not guarantee to preserve the geometry of the incoming fragment stream.

This is why you must use something like GRO/GSO, which is built to positively and provably preserve the geometry of SKBs as they are packed and unpacked.

author maintainer
#6 lore raw

Re: [PATCH nf-next 02/14] net: untangle ip_fragment and bridge netfilter

Quoted 10 lines · David Miller

From: Florian Westphal <[email protected]>
Date: Wed, 1 Apr 2015 22:36:28 +0200

Quoted 2 lines

Add mtu arguments to ip_fragment and remove the bridge netfilter mtu helper.

I told you I disagree with this approach. Anything that adds an 'mtu' argument to ip_fragment() I am not even going to look at seriously, there must be device context when you call that function.

Not sure. There is one case where we must not use device mtu:
if DF was set on one of the fragments, we must not increase fragment size, thats why I added the MTU argument to make sure that largest fragment size will be the upper boundary.

I don't see where we break PMTUD or induce any other kind of breakage after this change.

For the non-df case the original sizes of the fragments don't matter since any device in the path will (re)fragment.

Quoted 2 lines

Furthermore, and even more importantly, right now what bridge netfilter does with fragmentation is _terminally_ broken.

I didn't claim it wasn't :-]

Quoted 3 lines

This is why you must use something like GRO/GSO, which is built to positively and provably preserve the geometry of SKBs as they are packed and unpacked.

Thats not as trivial as it sounds.
GRO only aggregates same-size packets.

All the fragments might have different sizes.

Futhermore we need to handle overlapping fragments, duplicates and arrival of fragments in any order.

Preserving the original skbs and sending those instead of refragmentation doesn't work either since the reassembled skb might have been subject to NAT.

So the only possible compromise I see is to record/store all fragment sizes in the correct logical order (according to offset) and use that as 'replay split points', i.e. we'd still end up not using the device mtu when undoing the defrag operation.

We'd also not have full transparency unless we would also re-create the overlapping and duplicate packets that we might have seen, and send the refragmented skbs in the same order as we received them.

I don't think thats something we want in ip stack, so we'd have to (re)implement ip (de|re)-fragmentation for the bridge in bridge netfilter.

Is that what you had in mind?

Or do you see some way to re-use existing implementation without fuglifying normal defrag operations?

Thanks.

author maintainer
#7 lore raw

PATCH 3/14 netfilter: bridge: don't use nf_bridge_info data to store mac header

In reply to Florian Westphal

Currently br_netfilter maintains an extra state, nf_bridge_info, which is attached to skb via skb->nf_bridge pointer. Amongst other things we use skb->nf_bridge->data to store the original mac header for every processed skb.

This is required for ip refragmentation when using conntrack on top of bridge, because ip_fragment doesn't copy it from original skb.

However there is no need anymore to do this unconditionally.

Move this to the one place where its needed -- when br_netfilter calls ip_fragment().

Also switch to percpu storage for this so we can handle fragmenting without accessing nf_bridge meta data.

After this change, only one user of skb->nf_bridge->data is left.

Signed-off-by Florian Westphal <[email protected]>
---
 net/bridge/br_netfilter.c | 57 ++++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 25 deletions(-)
net/bridge/br_netfilter.c +32 -25
index c0c8700..6ccb1af 100644
@@ -111,6 +111,19 @@ static inline __be16 pppoe_proto(const struct sk_buff *skb)
 	 pppoe_proto(skb) == htons(PPP_IPV6) && \
 	 brnf_filter_pppoe_tagged)
 
+/* largest possible L2 header, see br_nf_dev_queue_xmit() */
+#define NF_BRIDGE_MAX_MAC_HEADER_LENGTH (PPPOE_SES_HLEN + ETH_HLEN)
+
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
+struct brnf_frag_data {
+	char mac[NF_BRIDGE_MAX_MAC_HEADER_LENGTH];
+	u8 encap_size;
+	u8 size;
+};
+
+static DEFINE_PER_CPU(struct brnf_frag_data, brnf_frag_data_storage);
+#endif
+
 static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
 {
 	struct net_bridge_port *port;

@@ -189,14 +202,6 @@ static inline void nf_bridge_pull_encap_header_rcsum(struct sk_buff *skb)
 	skb->network_header += len;
 }
 
-static inline void nf_bridge_save_header(struct sk_buff *skb)
-{
-	int header_size = ETH_HLEN + nf_bridge_encap_header_len(skb);
-
-	skb_copy_from_linear_data_offset(skb, -header_size,
-					 skb->nf_bridge->data, header_size);
-}
-
 /* When handing a packet over to the IP layer
  * check whether we have a skb that is in the
  * expected format

@@ -810,30 +815,22 @@ static unsigned int br_nf_forward_arp(const struct nf_hook_ops *ops,
 }
 
 #if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
-static bool nf_bridge_copy_header(struct sk_buff *skb)
+static int br_nf_push_frag_xmit(struct sk_buff *skb)
 {
+	struct brnf_frag_data *data;
 	int err;
-	unsigned int header_size;
 
-	nf_bridge_update_protocol(skb);
-	header_size = ETH_HLEN + nf_bridge_encap_header_len(skb);
-	err = skb_cow_head(skb, header_size);
-	if (err)
-		return false;
-
-	skb_copy_to_linear_data_offset(skb, -header_size,
-				       skb->nf_bridge->data, header_size);
-	__skb_push(skb, nf_bridge_encap_header_len(skb));
-	return true;
-}
+	data = this_cpu_ptr(&brnf_frag_data_storage);
+	err = skb_cow_head(skb, data->size);
 
-static int br_nf_push_frag_xmit(struct sk_buff *skb)
-{
-	if (!nf_bridge_copy_header(skb)) {
+	if (err) {
 		kfree_skb(skb);
 		return 0;
 	}
 
+	skb_copy_to_linear_data_offset(skb, -data->size, data->mac, data->size);
+	__skb_push(skb, data->encap_size);
+
 	return br_dev_queue_push_xmit(skb);
 }
 

@@ -864,6 +861,8 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 	 * size as mtu if it is set.
 	 */
 	if (skb->len > mtu) {
+		struct brnf_frag_data *data;
+
 		if (!skb->ignore_df) /* was not a defragmented */
 			goto err_out;
 

@@ -877,6 +876,15 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 		if (br_parse_ip_options(skb))
 			goto err_out;
 
+		nf_bridge_update_protocol(skb);
+
+		data = this_cpu_ptr(&brnf_frag_data_storage);
+		data->encap_size = nf_bridge_encap_header_len(skb);
+		data->size = ETH_HLEN + data->encap_size;
+
+		skb_copy_from_linear_data_offset(skb, -data->size, data->mac,
+						 data->size);
+
 		ret = ip_fragment(skb, mtu, br_nf_push_frag_xmit);
 	} else {
 		ret = br_dev_queue_push_xmit(skb);

@@ -932,7 +940,6 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
 	}
 
 	nf_bridge_pull_encap_header(skb);
-	nf_bridge_save_header(skb);
 	if (pf == NFPROTO_IPV4)
 		skb->protocol = htons(ETH_P_IP);
 	else
Signature · 1 line
2.0.5
author maintainer
#8 lore raw

PATCH 4/14 netfilter: bridge: start splitting mask into public/private chunks

In reply to Florian Westphal

->mask is a bit info field that mixes various use cases.

In particular, we have flags that are mutually exlusive, and flags that are only used within br_netfilter while others need to be exposed to other parts of the kernel.

Remove BRNF_8021Q/PPPoE flags. They're mutually exclusive and only needed within br_netfilter context.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter_bridge.h |  2 --
 include/linux/skbuff.h           |  5 +++++
 net/bridge/br_netfilter.c        | 17 ++++++++++++-----
 3 files changed, 17 insertions(+), 7 deletions(-)
include/linux/netfilter_bridge.h +0 -2
index b131613..848226e 100644
@@ -20,8 +20,6 @@ enum nf_br_hook_priorities {
 #define BRNF_PKT_TYPE			0x01
 #define BRNF_BRIDGED_DNAT		0x02
 #define BRNF_NF_BRIDGE_PREROUTING	0x08
-#define BRNF_8021Q			0x10
-#define BRNF_PPPoE			0x20
 
 int br_handle_frame_finish(struct sk_buff *skb);
include/linux/skbuff.h +5 -0
index 36f3f43..fe1b557 100644
@@ -166,6 +166,11 @@ struct nf_conntrack {
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 struct nf_bridge_info {
 	atomic_t		use;
+	enum {
+		BRNF_PROTO_UNCHANGED,
+		BRNF_PROTO_8021Q,
+		BRNF_PROTO_PPPOE
+	} orig_proto;
 	unsigned int		mask;
 	struct net_device	*physindev;
 	struct net_device	*physoutdev;
net/bridge/br_netfilter.c +12 -5
index 6ccb1af..8c86f37 100644
@@ -257,10 +257,16 @@ drop:
 
 static void nf_bridge_update_protocol(struct sk_buff *skb)
 {
-	if (skb->nf_bridge->mask & BRNF_8021Q)
+	switch (skb->nf_bridge->orig_proto) {
+	case BRNF_PROTO_8021Q:
 		skb->protocol = htons(ETH_P_8021Q);
-	else if (skb->nf_bridge->mask & BRNF_PPPoE)
+		break;
+	case BRNF_PROTO_PPPOE:
 		skb->protocol = htons(ETH_P_PPP_SES);
+		break;
+	case BRNF_PROTO_UNCHANGED:
+		break;
+	}
 }
 
 /* PF_BRIDGE/PRE_ROUTING *********************************************/

@@ -498,10 +504,11 @@ static struct net_device *setup_pre_routing(struct sk_buff *skb)
 	nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;
 	nf_bridge->physindev = skb->dev;
 	skb->dev = brnf_get_logical_dev(skb, skb->dev);
+
 	if (skb->protocol == htons(ETH_P_8021Q))
-		nf_bridge->mask |= BRNF_8021Q;
+		nf_bridge->orig_proto = BRNF_PROTO_8021Q;
 	else if (skb->protocol == htons(ETH_P_PPP_SES))
-		nf_bridge->mask |= BRNF_PPPoE;
+		nf_bridge->orig_proto = BRNF_PROTO_PPPOE;
 
 	/* Must drop socket now because of tproxy. */
 	skb_orphan(skb);

@@ -836,7 +843,7 @@ static int br_nf_push_frag_xmit(struct sk_buff *skb)
 
 static unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb)
 {
-	if (skb->nf_bridge->mask & BRNF_PPPoE)
+	if (skb->nf_bridge->orig_proto == BRNF_PROTO_PPPOE)
 		return PPPOE_SES_HLEN;
 	return 0;
 }
Signature · 1 line
2.0.5
author maintainer
#9 lore raw

PATCH 5/14 netfilter: bridge: make BRNF_PKT_TYPE flag a bool

In reply to Florian Westphal

nf_bridge_info->mask is used for several things, for example to remember if skb->pkt_type was set to OTHER_HOST.

For a bridge, OTHER_HOST is expected case. For ip forward its a non-starter though -- routing expects PACKET_HOST.

Bridge netfilter thus changes OTHER_HOST to PACKET_HOST before hook invocation and then un-does it after hook traversal.

This information is irrelevant outside of br_netfilter.

After this change, ->mask now only contains flags that need to be known outside of br_netfilter in fast-path.

Next patch will change mask into a 2bit state field in sk_buff, so that we can remove skb->nf_bridge pointer for good and consider all remaining places that access nf_bridge info content a slowpath.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter_bridge.h |  1 -
 include/linux/skbuff.h           |  1 +
 net/bridge/br_netfilter.c        | 18 +++++++++---------
 3 files changed, 10 insertions(+), 10 deletions(-)
include/linux/netfilter_bridge.h +0 -1
index 848226e..66245b5 100644
@@ -17,7 +17,6 @@ enum nf_br_hook_priorities {
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 
-#define BRNF_PKT_TYPE			0x01
 #define BRNF_BRIDGED_DNAT		0x02
 #define BRNF_NF_BRIDGE_PREROUTING	0x08
include/linux/skbuff.h +1 -0
index fe1b557..afa53e4 100644
@@ -171,6 +171,7 @@ struct nf_bridge_info {
 		BRNF_PROTO_8021Q,
 		BRNF_PROTO_PPPOE
 	} orig_proto;
+	bool			pkt_otherhost;
 	unsigned int		mask;
 	struct net_device	*physindev;
 	struct net_device	*physoutdev;
net/bridge/br_netfilter.c +9 -9
index 8c86f37..02ccf38 100644
@@ -277,9 +277,9 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
 	struct rtable *rt;
 
-	if (nf_bridge->mask & BRNF_PKT_TYPE) {
+	if (nf_bridge->pkt_otherhost) {
 		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
+		nf_bridge->pkt_otherhost = false;
 	}
 	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
 

@@ -410,9 +410,9 @@ 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 (nf_bridge->pkt_otherhost) {
 		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
+		nf_bridge->pkt_otherhost = false;
 	}
 	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
 	if (dnat_took_place(skb)) {

@@ -498,7 +498,7 @@ static struct net_device *setup_pre_routing(struct sk_buff *skb)
 
 	if (skb->pkt_type == PACKET_OTHERHOST) {
 		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
+		nf_bridge->pkt_otherhost = true;
 	}
 
 	nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;

@@ -706,9 +706,9 @@ static int br_nf_forward_finish(struct sk_buff *skb)
 		}
 
 		in = nf_bridge->physindev;
-		if (nf_bridge->mask & BRNF_PKT_TYPE) {
+		if (nf_bridge->pkt_otherhost) {
 			skb->pkt_type = PACKET_OTHERHOST;
-			nf_bridge->mask ^= BRNF_PKT_TYPE;
+			nf_bridge->pkt_otherhost = false;
 		}
 		nf_bridge_update_protocol(skb);
 	} else {

@@ -761,7 +761,7 @@ 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->pkt_otherhost = true;
 	}
 
 	if (pf == NFPROTO_IPV4) {

@@ -943,7 +943,7 @@ 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;
+		nf_bridge->pkt_otherhost = true;
 	}
 
 	nf_bridge_pull_encap_header(skb);
Signature · 1 line
2.0.5
author maintainer
#10 lore raw

PATCH 6/14 netfilter: bridge: rename and resize 'data' field

In reply to Florian Westphal

Only user left is neigh resolution when DNAT is detected, to hold the original source mac address (neigh resolution builds new mac header using bridge mac).

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/skbuff.h    | 2 +-
 net/bridge/br_netfilter.c | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)
include/linux/skbuff.h +1 -1
index afa53e4..0991259 100644
@@ -175,7 +175,7 @@ struct nf_bridge_info {
 	unsigned int		mask;
 	struct net_device	*physindev;
 	struct net_device	*physoutdev;
-	unsigned long		data[32 / sizeof(unsigned long)];
+	char			neigh_header[8];
 };
 #endif
net/bridge/br_netfilter.c +6 -3
index 02ccf38..947a2f6 100644
@@ -329,7 +329,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 			 */
 			skb_copy_from_linear_data_offset(skb,
 							 -(ETH_HLEN-ETH_ALEN),
-							 skb->nf_bridge->data,
+							 nf_bridge->neigh_header,
 							 ETH_HLEN-ETH_ALEN);
 			/* tell br_dev_xmit to continue with forwarding */
 			nf_bridge->mask |= BRNF_BRIDGED_DNAT;

@@ -991,8 +991,11 @@ static void br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 	skb_pull(skb, ETH_HLEN);
 	nf_bridge->mask &= ~BRNF_BRIDGED_DNAT;
 
-	skb_copy_to_linear_data_offset(skb, -(ETH_HLEN-ETH_ALEN),
-				       skb->nf_bridge->data, ETH_HLEN-ETH_ALEN);
+	BUILD_BUG_ON(sizeof(nf_bridge->neigh_header) != (ETH_HLEN - ETH_ALEN));
+
+	skb_copy_to_linear_data_offset(skb, -(ETH_HLEN - ETH_ALEN),
+				       nf_bridge->neigh_header,
+				       ETH_HLEN - ETH_ALEN);
 	skb->dev = nf_bridge->physindev;
 	br_handle_frame_finish(skb);
 }
Signature · 1 line
2.0.5
author maintainer
#11 lore raw

PATCH 7/14 netfilter: bridge: add helpers for fetching physin/outdev

In reply to Florian Westphal

right now we store this in the nf_bridge_info struct, accessible via skb->nf_bridge. This patch prepares removal of this pointer from skb:

Instead of using skb->nf_bridge->x, we use helpers to obtain the in/out device (or ifindexes).

Followup patches to netfilter will then allow nf_bridge_info to be obtained by a call into the br_netfilter core, rather than keeping a pointer to it in sk_buff.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter_bridge.h           | 23 ++++++++++++++++++++-
 net/ipv4/netfilter/nf_reject_ipv4.c        |  4 +++-
 net/ipv6/netfilter/nf_reject_ipv6.c        |  4 +++-
 net/netfilter/ipset/ip_set_hash_netiface.c | 32 ++++++++++++++++++++++--------
 net/netfilter/nf_log_common.c              |  5 +++--
 net/netfilter/nf_queue.c                   | 18 +++++++++--------
 net/netfilter/nfnetlink_log.c              | 17 ++++++++++++----
 net/netfilter/nfnetlink_queue_core.c       | 28 +++++++++++++++++---------
 8 files changed, 97 insertions(+), 34 deletions(-)
include/linux/netfilter_bridge.h +22 -1
index 66245b5..61251e4 100644
@@ -2,7 +2,7 @@
 #define __LINUX_BRIDGE_NETFILTER_H
 
 #include <uapi/linux/netfilter_bridge.h>
-
+#include <linux/skbuff.h>
 
 enum nf_br_hook_priorities {
 	NF_BR_PRI_FIRST = INT_MIN,

@@ -30,6 +30,27 @@ static inline void br_drop_fake_rtable(struct sk_buff *skb)
 		skb_dst_drop(skb);
 }
 
+static inline int nf_bridge_get_physinif(const struct sk_buff *skb)
+{
+	return skb->nf_bridge ? skb->nf_bridge->physindev->ifindex : 0;
+}
+
+static inline int nf_bridge_get_physoutif(const struct sk_buff *skb)
+{
+	return skb->nf_bridge ? skb->nf_bridge->physoutdev->ifindex : 0;
+}
+
+static inline struct net_device *
+nf_bridge_get_physindev(const struct sk_buff *skb)
+{
+	return skb->nf_bridge ? skb->nf_bridge->physindev : NULL;
+}
+
+static inline struct net_device *
+nf_bridge_get_physoutdev(const struct sk_buff *skb)
+{
+	return skb->nf_bridge ? skb->nf_bridge->physoutdev : NULL;
+}
 #else
 #define br_drop_fake_rtable(skb)	        do { } while (0)
 #endif /* CONFIG_BRIDGE_NETFILTER */
net/ipv4/netfilter/nf_reject_ipv4.c +3 -1
index c5b794d..3262e41 100644
@@ -13,6 +13,7 @@
 #include <net/dst.h>
 #include <net/netfilter/ipv4/nf_reject.h>
 #include <linux/netfilter_ipv4.h>
+#include <linux/netfilter_bridge.h>
 #include <net/netfilter/ipv4/nf_reject.h>
 
 const struct tcphdr *nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,

@@ -146,7 +147,8 @@ void nf_send_reset(struct sk_buff *oldskb, int hook)
 	 */
 	if (oldskb->nf_bridge) {
 		struct ethhdr *oeth = eth_hdr(oldskb);
-		nskb->dev = oldskb->nf_bridge->physindev;
+
+		nskb->dev = nf_bridge_get_physindev(oldskb);
 		niph->tot_len = htons(nskb->len);
 		ip_send_check(niph);
 		if (dev_hard_header(nskb, nskb->dev, ntohs(nskb->protocol),
net/ipv6/netfilter/nf_reject_ipv6.c +3 -1
index 3afdce0..94b4c6d 100644
@@ -13,6 +13,7 @@
 #include <net/ip6_checksum.h>
 #include <net/netfilter/ipv6/nf_reject.h>
 #include <linux/netfilter_ipv6.h>
+#include <linux/netfilter_bridge.h>
 #include <net/netfilter/ipv6/nf_reject.h>
 
 const struct tcphdr *nf_reject_ip6_tcphdr_get(struct sk_buff *oldskb,

@@ -195,7 +196,8 @@ void nf_send_reset6(struct net *net, struct sk_buff *oldskb, int hook)
 	 */
 	if (oldskb->nf_bridge) {
 		struct ethhdr *oeth = eth_hdr(oldskb);
-		nskb->dev = oldskb->nf_bridge->physindev;
+
+		nskb->dev = nf_bridge_get_physindev(oldskb);
 		nskb->protocol = htons(ETH_P_IPV6);
 		ip6h->payload_len = htons(sizeof(struct tcphdr));
 		if (dev_hard_header(nskb, nskb->dev, ntohs(nskb->protocol),
net/netfilter/ipset/ip_set_hash_netiface.c +24 -8
index 758b002..380ef51 100644
@@ -19,6 +19,7 @@
 #include <net/netlink.h>
 
 #include <linux/netfilter.h>
+#include <linux/netfilter_bridge.h>
 #include <linux/netfilter/ipset/pfxlen.h>
 #include <linux/netfilter/ipset/ip_set.h>
 #include <linux/netfilter/ipset/ip_set_hash.h>

@@ -211,6 +212,22 @@ hash_netiface4_data_next(struct hash_netiface4_elem *next,
 #define HKEY_DATALEN	sizeof(struct hash_netiface4_elem_hashed)
 #include "ip_set_hash_gen.h"
 
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+static const char *get_physindev_name(const struct sk_buff *skb)
+{
+	struct net_device *dev = nf_bridge_get_physindev(skb);
+
+	return dev ? dev->name : NULL;
+}
+
+static const char *get_phyoutdev_name(const struct sk_buff *skb)
+{
+	struct net_device *dev = nf_bridge_get_physoutdev(skb);
+
+	return dev ? dev->name : NULL;
+}
+#endif
+
 static int
 hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb,
 		    const struct xt_action_param *par,

@@ -234,16 +251,15 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	e.ip &= ip_set_netmask(e.cidr);
 
 #define IFACE(dir)	(par->dir ? par->dir->name : NULL)
-#define PHYSDEV(dir)	(nf_bridge->dir ? nf_bridge->dir->name : NULL)
 #define SRCDIR		(opt->flags & IPSET_DIM_TWO_SRC)
 
 	if (opt->cmdflags & IPSET_FLAG_PHYSDEV) {
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-		const struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+		e.iface = SRCDIR ? get_physindev_name(skb) :
+				   get_phyoutdev_name(skb);
 
-		if (!nf_bridge)
+		if (!e.iface)
 			return -EINVAL;
-		e.iface = SRCDIR ? PHYSDEV(physindev) : PHYSDEV(physoutdev);
 		e.physdev = 1;
 #else
 		e.iface = NULL;

@@ -476,11 +492,11 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb,
 
 	if (opt->cmdflags & IPSET_FLAG_PHYSDEV) {
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-		const struct nf_bridge_info *nf_bridge = skb->nf_bridge;
-
-		if (!nf_bridge)
+		e.iface = SRCDIR ? get_physindev_name(skb) :
+				   get_phyoutdev_name(skb);
+		if (!e.iface)
 			return -EINVAL;
-		e.iface = SRCDIR ? PHYSDEV(physindev) : PHYSDEV(physoutdev);
+
 		e.physdev = 1;
 #else
 		e.iface = NULL;
net/netfilter/nf_log_common.c +3 -2
index 2631876..a5aa596 100644
@@ -17,6 +17,7 @@
 #include <net/route.h>
 
 #include <linux/netfilter.h>
+#include <linux/netfilter_bridge.h>
 #include <linux/netfilter/xt_LOG.h>
 #include <net/netfilter/nf_log.h>
 

@@ -163,10 +164,10 @@ nf_log_dump_packet_common(struct nf_log_buf *m, u_int8_t pf,
 		const struct net_device *physindev;
 		const struct net_device *physoutdev;
 
-		physindev = skb->nf_bridge->physindev;
+		physindev = nf_bridge_get_physindev(skb);
 		if (physindev && in != physindev)
 			nf_log_buf_add(m, "PHYSIN=%s ", physindev->name);
-		physoutdev = skb->nf_bridge->physoutdev;
+		physoutdev = nf_bridge_get_physoutdev(skb);
 		if (physoutdev && out != physoutdev)
 			nf_log_buf_add(m, "PHYSOUT=%s ", physoutdev->name);
 	}
net/netfilter/nf_queue.c +10 -8
index 4c8b68e..fb045b4 100644
@@ -10,6 +10,7 @@
 #include <linux/proc_fs.h>
 #include <linux/skbuff.h>
 #include <linux/netfilter.h>
+#include <linux/netfilter_bridge.h>
 #include <linux/seq_file.h>
 #include <linux/rcupdate.h>
 #include <net/protocol.h>

@@ -54,12 +55,14 @@ void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 		dev_put(entry->outdev);
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	if (entry->skb->nf_bridge) {
-		struct nf_bridge_info *nf_bridge = entry->skb->nf_bridge;
+		struct net_device *physdev;
 
-		if (nf_bridge->physindev)
-			dev_put(nf_bridge->physindev);
-		if (nf_bridge->physoutdev)
-			dev_put(nf_bridge->physoutdev);
+		physdev = nf_bridge_get_physindev(entry->skb);
+		if (physdev)
+			dev_put(physdev);
+		physdev = nf_bridge_get_physoutdev(entry->skb);
+		if (physdev)
+			dev_put(physdev);
 	}
 #endif
 	/* Drop reference to owner of hook which queued us. */

@@ -79,13 +82,12 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
 		dev_hold(entry->outdev);
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	if (entry->skb->nf_bridge) {
-		struct nf_bridge_info *nf_bridge = entry->skb->nf_bridge;
 		struct net_device *physdev;
 
-		physdev = nf_bridge->physindev;
+		physdev = nf_bridge_get_physindev(entry->skb);
 		if (physdev)
 			dev_hold(physdev);
-		physdev = nf_bridge->physoutdev;
+		physdev = nf_bridge_get_physoutdev(entry->skb);
 		if (physdev)
 			dev_hold(physdev);
 	}
net/netfilter/nfnetlink_log.c +13 -4
index 957b83a..51afea4 100644
@@ -23,6 +23,7 @@
 #include <linux/ipv6.h>
 #include <linux/netdevice.h>
 #include <linux/netfilter.h>
+#include <linux/netfilter_bridge.h>
 #include <net/netlink.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nfnetlink_log.h>

@@ -448,14 +449,18 @@ __build_packet_message(struct nfnl_log_net *log,
 					 htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
 				goto nla_put_failure;
 		} else {
+			struct net_device *physindev;
+
 			/* Case 2: indev is bridge group, we need to look for
 			 * physical device (when called from ipv4) */
 			if (nla_put_be32(inst->skb, NFULA_IFINDEX_INDEV,
 					 htonl(indev->ifindex)))
 				goto nla_put_failure;
-			if (skb->nf_bridge && skb->nf_bridge->physindev &&
+
+			physindev = nf_bridge_get_physindev(skb);
+			if (physindev &&
 			    nla_put_be32(inst->skb, NFULA_IFINDEX_PHYSINDEV,
-					 htonl(skb->nf_bridge->physindev->ifindex)))
+					 htonl(physindev->ifindex)))
 				goto nla_put_failure;
 		}
 #endif

@@ -479,14 +484,18 @@ __build_packet_message(struct nfnl_log_net *log,
 					 htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
 				goto nla_put_failure;
 		} else {
+			struct net_device *physoutdev;
+
 			/* Case 2: indev is a bridge group, we need to look
 			 * for physical device (when called from ipv4) */
 			if (nla_put_be32(inst->skb, NFULA_IFINDEX_OUTDEV,
 					 htonl(outdev->ifindex)))
 				goto nla_put_failure;
-			if (skb->nf_bridge && skb->nf_bridge->physoutdev &&
+
+			physoutdev = nf_bridge_get_physoutdev(skb);
+			if (physoutdev &&
 			    nla_put_be32(inst->skb, NFULA_IFINDEX_PHYSOUTDEV,
-					 htonl(skb->nf_bridge->physoutdev->ifindex)))
+					 htonl(physoutdev->ifindex)))
 				goto nla_put_failure;
 		}
 #endif
net/netfilter/nfnetlink_queue_core.c +19 -9
index 86ee8b0..94e1aaf 100644
@@ -25,6 +25,7 @@
 #include <linux/proc_fs.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/netfilter_ipv6.h>
+#include <linux/netfilter_bridge.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nfnetlink_queue.h>
 #include <linux/list.h>

@@ -396,14 +397,18 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 					 htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
 				goto nla_put_failure;
 		} else {
+			int physinif;
+
 			/* Case 2: indev is bridge group, we need to look for
 			 * physical device (when called from ipv4) */
 			if (nla_put_be32(skb, NFQA_IFINDEX_INDEV,
 					 htonl(indev->ifindex)))
 				goto nla_put_failure;
-			if (entskb->nf_bridge && entskb->nf_bridge->physindev &&
+
+			physinif = nf_bridge_get_physinif(entskb);
+			if (physinif &&
 			    nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
-					 htonl(entskb->nf_bridge->physindev->ifindex)))
+					 htonl(physinif)))
 				goto nla_put_failure;
 		}
 #endif

@@ -426,14 +431,18 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 					 htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
 				goto nla_put_failure;
 		} else {
+			int physoutif;
+
 			/* Case 2: outdev is bridge group, we need to look for
 			 * physical output device (when called from ipv4) */
 			if (nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
 					 htonl(outdev->ifindex)))
 				goto nla_put_failure;
-			if (entskb->nf_bridge && entskb->nf_bridge->physoutdev &&
+
+			physoutif = nf_bridge_get_physoutif(entskb);
+			if (physoutif &&
 			    nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
-					 htonl(entskb->nf_bridge->physoutdev->ifindex)))
+					 htonl(physoutif)))
 				goto nla_put_failure;
 		}
 #endif

@@ -765,11 +774,12 @@ dev_cmp(struct nf_queue_entry *entry, unsigned long ifindex)
 			return 1;
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	if (entry->skb->nf_bridge) {
-		if (entry->skb->nf_bridge->physindev &&
-		    entry->skb->nf_bridge->physindev->ifindex == ifindex)
-			return 1;
-		if (entry->skb->nf_bridge->physoutdev &&
-		    entry->skb->nf_bridge->physoutdev->ifindex == ifindex)
+		int physinif, physoutif;
+
+		physinif = nf_bridge_get_physinif(entry->skb);
+		physoutif = nf_bridge_get_physoutif(entry->skb);
+
+		if (physinif == ifindex || physoutif == ifindex)
 			return 1;
 	}
 #endif
Signature · 1 line
2.0.5
author maintainer
#12 lore raw

PATCH 8/14 netfilter: physdev: use helpers

In reply to Florian Westphal

Avoid skb->nf_bridge accesses where possible.

Signed-off-by Florian Westphal <[email protected]>
---
 net/netfilter/xt_physdev.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)
net/netfilter/xt_physdev.c +22 -12
index 50a5204..1caaccb 100644
@@ -25,16 +25,15 @@ MODULE_ALIAS("ip6t_physdev");
 static bool
 physdev_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
-	static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long))));
 	const struct xt_physdev_info *info = par->matchinfo;
+	const struct net_device *physdev;
 	unsigned long ret;
 	const char *indev, *outdev;
-	const struct nf_bridge_info *nf_bridge;
 
 	/* Not a bridged IP packet or no info available yet:
 	 * LOCAL_OUT/mangle and LOCAL_OUT/nat don't know if
 	 * the destination device will be a bridge. */
-	if (!(nf_bridge = skb->nf_bridge)) {
+	if (!skb->nf_bridge) {
 		/* Return MATCH if the invert flags of the used options are on */
 		if ((info->bitmask & XT_PHYSDEV_OP_BRIDGED) &&
 		    !(info->invert & XT_PHYSDEV_OP_BRIDGED))

@@ -54,30 +53,41 @@ physdev_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		return true;
 	}
 
+	physdev = nf_bridge_get_physoutdev(skb);
+	outdev = physdev ? physdev->name : NULL;
+
 	/* This only makes sense in the FORWARD and POSTROUTING chains */
 	if ((info->bitmask & XT_PHYSDEV_OP_BRIDGED) &&
-	    (!!nf_bridge->physoutdev ^ !(info->invert & XT_PHYSDEV_OP_BRIDGED)))
+	    (!!outdev ^ !(info->invert & XT_PHYSDEV_OP_BRIDGED)))
 		return false;
 
+	physdev = nf_bridge_get_physindev(skb);
+	indev = physdev ? physdev->name : NULL;
+
 	if ((info->bitmask & XT_PHYSDEV_OP_ISIN &&
-	    (!nf_bridge->physindev ^ !!(info->invert & XT_PHYSDEV_OP_ISIN))) ||
+	    (!indev ^ !!(info->invert & XT_PHYSDEV_OP_ISIN))) ||
 	    (info->bitmask & XT_PHYSDEV_OP_ISOUT &&
-	    (!nf_bridge->physoutdev ^ !!(info->invert & XT_PHYSDEV_OP_ISOUT))))
+	    (!outdev ^ !!(info->invert & XT_PHYSDEV_OP_ISOUT))))
 		return false;
 
 	if (!(info->bitmask & XT_PHYSDEV_OP_IN))
 		goto match_outdev;
-	indev = nf_bridge->physindev ? nf_bridge->physindev->name : nulldevname;
-	ret = ifname_compare_aligned(indev, info->physindev, info->in_mask);
 
-	if (!ret ^ !(info->invert & XT_PHYSDEV_OP_IN))
-		return false;
+	if (indev) {
+		ret = ifname_compare_aligned(indev, info->physindev,
+					     info->in_mask);
+
+		if (!ret ^ !(info->invert & XT_PHYSDEV_OP_IN))
+			return false;
+	}
 
 match_outdev:
 	if (!(info->bitmask & XT_PHYSDEV_OP_OUT))
 		return true;
-	outdev = nf_bridge->physoutdev ?
-		 nf_bridge->physoutdev->name : nulldevname;
+
+	if (!outdev)
+		return false;
+
 	ret = ifname_compare_aligned(outdev, info->physoutdev, info->out_mask);
 
 	return (!!ret ^ !(info->invert & XT_PHYSDEV_OP_OUT));
Signature · 1 line
2.0.5
author maintainer
#13 lore raw

PATCH 9/14 netfilter: bridge: add and use nf_bridge_info_get helper

In reply to Florian Westphal

Don't access skb->nf_bridge directly, this pointer will be removed soon.

Signed-off-by Florian Westphal <[email protected]>
---
 net/bridge/br_netfilter.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)
net/bridge/br_netfilter.c +16 -8
index 947a2f6..40009b1 100644
@@ -124,6 +124,11 @@ struct brnf_frag_data {
 static DEFINE_PER_CPU(struct brnf_frag_data, brnf_frag_data_storage);
 #endif
 
+static struct nf_bridge_info *nf_bridge_info_get(const struct sk_buff *skb)
+{
+	return skb->nf_bridge;
+}
+
 static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
 {
 	struct net_bridge_port *port;

@@ -274,7 +279,7 @@ static void nf_bridge_update_protocol(struct sk_buff *skb)
  * 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 = nf_bridge_info_get(skb);
 	struct rtable *rt;
 
 	if (nf_bridge->pkt_otherhost) {

@@ -306,7 +311,6 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
  */
 static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
 	struct neighbour *neigh;
 	struct dst_entry *dst;
 

@@ -316,6 +320,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) {
+		struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
 		int ret;
 
 		if (neigh->hh.hh_len) {

@@ -402,7 +407,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 = nf_bridge_info_get(skb);
 	struct rtable *rt;
 	int err;
 	int frag_max_size;

@@ -494,7 +499,7 @@ 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;
+	struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
 
 	if (skb->pkt_type == PACKET_OTHERHOST) {
 		skb->pkt_type = PACKET_HOST;

@@ -694,7 +699,7 @@ static unsigned int br_nf_local_in(const struct nf_hook_ops *ops,
 /* PF_BRIDGE/FORWARD *************************************************/
 static int br_nf_forward_finish(struct sk_buff *skb)
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
 	struct net_device *in;
 
 	if (!IS_ARP(skb) && !IS_VLAN_ARP(skb)) {

@@ -745,6 +750,10 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 	if (!nf_bridge_unshare(skb))
 		return NF_DROP;
 
+	nf_bridge = nf_bridge_info_get(skb);
+	if (!nf_bridge)
+		return NF_DROP;
+
 	parent = bridge_parent(out);
 	if (!parent)
 		return NF_DROP;

@@ -758,7 +767,6 @@ 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->pkt_otherhost = true;

@@ -917,7 +925,7 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
 				       const struct net_device *out,
 				       int (*okfn)(struct sk_buff *))
 {
-	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
 	struct net_device *realoutdev = bridge_parent(skb->dev);
 	u_int8_t pf;
 

@@ -986,7 +994,7 @@ static unsigned int ip_sabotage_in(const struct nf_hook_ops *ops,
  */
 static void 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 = nf_bridge_info_get(skb);
 
 	skb_pull(skb, ETH_HLEN);
 	nf_bridge->mask &= ~BRNF_BRIDGED_DNAT;
Signature · 1 line
2.0.5
author maintainer
#14 lore raw

PATCH 10/14 netfilter: bridge: move bridge netfilter state into sk_buff

In reply to Florian Westphal

skb->nf_bridge is only used when interface is part of a bridge and bridge netfilter is used with 'call-iptables' sysctl set to 1 and the skb was received on a bridge port. IOW, almost noone uses it.

This moves the skb bridge netfilter state (2 bits) to the skb itself.

This means we can now determine if an skb is subject to bridge netfilter by looking at skb->nf_bridge_state in addition to skb->nf_bridge pointer.

This makes it possible to remove the skb->nf_bridge in a followup patch, while still allowing fast-path users to termine if slow-path operations are needed by looking at skb->nf_bridge_state.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter_bridge.h          | 14 ++++++++++--
 include/linux/skbuff.h                    | 28 ++++++++++++++++-------
 net/bridge/br_device.c                    | 19 +++++++++++-----
 net/bridge/br_netfilter.c                 | 37 +++++++++++++------------------
 net/bridge/br_private.h                   |  2 +-
 net/core/skbuff.c                         |  3 ++-
 net/ipv4/netfilter/nf_defrag_ipv4.c       |  3 +--
 net/ipv4/netfilter/nf_reject_ipv4.c       |  2 +-
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c |  3 +--
 net/ipv6/netfilter/nf_reject_ipv6.c       |  2 +-
 net/netfilter/nf_log_common.c             |  2 +-
 net/netfilter/nf_queue.c                  |  4 ++--
 net/netfilter/nfnetlink_queue_core.c      |  6 ++---
 net/netfilter/xt_physdev.c                |  2 +-
 14 files changed, 75 insertions(+), 52 deletions(-)
include/linux/netfilter_bridge.h +12 -2
index 61251e4..f2d7abc 100644
@@ -16,9 +16,19 @@ enum nf_br_hook_priorities {
 };
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+enum brnf_state {
+	BRNF_STATE_NONE,
+	BRNF_STATE_SEEN = 1,
 
-#define BRNF_BRIDGED_DNAT		0x02
-#define BRNF_NF_BRIDGE_PREROUTING	0x08
+	/* IPV4/IPV6 PRE_ROUTING called from bridge netfilter */
+	BRNF_STATE_PREROUTING,
+
+	/* skb that is 'transmitted' via bridge must to be injected
+	 * back into br forwarding for delivery to the correct bridge output
+	 * port due to DNAT to a destination on the same (bridged) network.
+	 */
+	BRNF_STATE_BRIDGED_DNAT,
+};
 
 int br_handle_frame_finish(struct sk_buff *skb);
include/linux/skbuff.h +20 -8
index 0991259..c060db5 100644
@@ -172,7 +172,6 @@ struct nf_bridge_info {
 		BRNF_PROTO_PPPOE
 	} orig_proto;
 	bool			pkt_otherhost;
-	unsigned int		mask;
 	struct net_device	*physindev;
 	struct net_device	*physoutdev;
 	char			neigh_header[8];

@@ -474,6 +473,9 @@ static inline u32 skb_mstamp_us_delta(const struct skb_mstamp *t1,
  *	@pkt_type: Packet class
  *	@fclone: skbuff clone status
  *	@ipvs_property: skbuff is owned by ipvs
+ *	@inner_protocol_type: encapsulation type
+ *	@remcsum_offload: udp remote checksum offload
+ *	@nf_bridge_state: bridge netfilter skb state
  *	@peeked: this packet has been seen already, so stats have been
  *		done for it, don't do them again
  *	@nf_trace: netfilter packet trace flag

@@ -614,8 +616,8 @@ struct sk_buff {
 	__u8			ipvs_property:1;
 	__u8			inner_protocol_type:1;
 	__u8			remcsum_offload:1;
-	/* 3 or 5 bit hole */
-
+	__u8			nf_bridge_state:2;
+	/* 1 or 3 bit hole */
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */
 #ifdef CONFIG_NET_CLS_ACT

@@ -3180,8 +3182,11 @@ static inline void nf_reset(struct sk_buff *skb)
 	skb->nfct = NULL;
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	nf_bridge_put(skb->nf_bridge);
-	skb->nf_bridge = NULL;
+	if (skb->nf_bridge_state) {
+		nf_bridge_put(skb->nf_bridge);
+		skb->nf_bridge = NULL;
+		skb->nf_bridge_state = 0;
+	}
 #endif
 }
 

@@ -3203,8 +3208,12 @@ static inline void __nf_copy(struct sk_buff *dst, const struct sk_buff *src,
 		dst->nfctinfo = src->nfctinfo;
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	dst->nf_bridge  = src->nf_bridge;
-	nf_bridge_get(src->nf_bridge);
+	if (src->nf_bridge_state) {
+		dst->nf_bridge = src->nf_bridge;
+		nf_bridge_get(src->nf_bridge);
+	} else {
+		dst->nf_bridge = NULL;
+	}
 #endif
 #if IS_ENABLED(CONFIG_NETFILTER_XT_TARGET_TRACE) || defined(CONFIG_NF_TABLES)
 	if (copy)

@@ -3218,7 +3227,10 @@ static inline void nf_copy(struct sk_buff *dst, const struct sk_buff *src)
 	nf_conntrack_put(dst->nfct);
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	nf_bridge_put(dst->nf_bridge);
+	if (dst->nf_bridge_state) {
+		nf_bridge_put(dst->nf_bridge);
+		dst->nf_bridge = NULL;
+	}
 #endif
 	__nf_copy(dst, src, true);
 }
net/bridge/br_device.c +13 -6
index 4ff77a1..99b78ff 100644
@@ -36,16 +36,23 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct net_bridge_fdb_entry *dst;
 	struct net_bridge_mdb_entry *mdst;
 	struct pcpu_sw_netstats *brstats = this_cpu_ptr(br->stats);
-	const struct nf_br_ops *nf_ops;
 	u16 vid = 0;
 
 	rcu_read_lock();
-	nf_ops = rcu_dereference(nf_br_ops);
-	if (nf_ops && nf_ops->br_dev_xmit_hook(skb)) {
-		rcu_read_unlock();
-		return NETDEV_TX_OK;
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+	if (skb->nf_bridge_state == BRNF_STATE_BRIDGED_DNAT) {
+		const struct nf_br_ops *nf_ops;
+
+		nf_ops = rcu_dereference(nf_br_ops);
+		if (nf_ops) {
+			nf_ops->br_dev_dnat_hook(skb);
+		} else {
+			/* br_netfilter module removed while skb in qdisc */
+			kfree_skb(skb);
+		}
+		goto out;
 	}
-
+#endif
 	u64_stats_update_begin(&brstats->syncp);
 	brstats->tx_packets++;
 	brstats->tx_bytes += skb->len;
net/bridge/br_netfilter.c +16 -21
index 40009b1..832164e 100644
@@ -148,8 +148,11 @@ 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))
+
+	if (likely(skb->nf_bridge)) {
 		atomic_set(&(skb->nf_bridge->use), 1);
+		skb->nf_bridge_state = BRNF_STATE_SEEN;
+	}
 
 	return skb->nf_bridge;
 }

@@ -286,7 +289,7 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
 		skb->pkt_type = PACKET_OTHERHOST;
 		nf_bridge->pkt_otherhost = false;
 	}
-	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+	skb->nf_bridge_state = BRNF_STATE_SEEN;
 
 	rt = bridge_parent_rtable(nf_bridge->physindev);
 	if (!rt) {

@@ -337,8 +340,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 							 nf_bridge->neigh_header,
 							 ETH_HLEN-ETH_ALEN);
 			/* tell br_dev_xmit to continue with forwarding */
-			nf_bridge->mask |= BRNF_BRIDGED_DNAT;
-			/* FIXME Need to refragment */
+			skb->nf_bridge_state = BRNF_STATE_BRIDGED_DNAT;
 			ret = neigh->output(neigh, skb);
 		}
 		neigh_release(neigh);

@@ -419,7 +421,7 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
 		skb->pkt_type = PACKET_OTHERHOST;
 		nf_bridge->pkt_otherhost = false;
 	}
-	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+	skb->nf_bridge_state = BRNF_STATE_SEEN;
 	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);

@@ -506,7 +508,7 @@ static struct net_device *setup_pre_routing(struct sk_buff *skb)
 		nf_bridge->pkt_otherhost = true;
 	}
 
-	nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;
+	skb->nf_bridge_state = BRNF_STATE_PREROUTING;
 	nf_bridge->physindev = skb->dev;
 	skb->dev = brnf_get_logical_dev(skb, skb->dev);
 

@@ -742,7 +744,7 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
 	struct net_device *parent;
 	u_int8_t pf;
 
-	if (!skb->nf_bridge)
+	if (!skb->nf_bridge_state)
 		return NF_ACCEPT;
 
 	/* Need exclusive nf_bridge_info since we might have multiple

@@ -862,6 +864,8 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 	int frag_max_size;
 	unsigned int mtu_reserved, mtu;
 
+	skb->nf_bridge_state = BRNF_STATE_NONE;
+
 	if (skb_is_gso(skb) || skb->protocol != htons(ETH_P_IP))
 		return br_dev_queue_push_xmit(skb);
 

@@ -914,6 +918,8 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 #else
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 {
+	skb->nf_bridge_state = BRNF_STATE_NONE;
+
         return br_dev_queue_push_xmit(skb);
 }
 #endif

@@ -975,10 +981,8 @@ static unsigned int ip_sabotage_in(const struct nf_hook_ops *ops,
 				   const struct net_device *out,
 				   int (*okfn)(struct sk_buff *))
 {
-	if (skb->nf_bridge &&
-	    !(skb->nf_bridge->mask & BRNF_NF_BRIDGE_PREROUTING)) {
+	if (skb->nf_bridge_state != BRNF_STATE_PREROUTING)
 		return NF_STOP;
-	}
 
 	return NF_ACCEPT;
 }

@@ -997,7 +1001,7 @@ static void br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 	struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
 
 	skb_pull(skb, ETH_HLEN);
-	nf_bridge->mask &= ~BRNF_BRIDGED_DNAT;
+	skb->nf_bridge_state = BRNF_STATE_SEEN;
 
 	BUILD_BUG_ON(sizeof(nf_bridge->neigh_header) != (ETH_HLEN - ETH_ALEN));
 

@@ -1008,17 +1012,8 @@ static void br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 	br_handle_frame_finish(skb);
 }
 
-static int br_nf_dev_xmit(struct sk_buff *skb)
-{
-	if (skb->nf_bridge && (skb->nf_bridge->mask & BRNF_BRIDGED_DNAT)) {
-		br_nf_pre_routing_finish_bridge_slow(skb);
-		return 1;
-	}
-	return 0;
-}
-
 static const struct nf_br_ops br_ops = {
-	.br_dev_xmit_hook =	br_nf_dev_xmit,
+	.br_dev_dnat_hook = br_nf_pre_routing_finish_bridge_slow,
 };
 
 void br_netfilter_enable(void)
net/bridge/br_private.h +1 -1
index b46fa0c..7f22f09 100644
@@ -764,7 +764,7 @@ static inline int br_vlan_enabled(struct net_bridge *br)
 #endif
 
 struct nf_br_ops {
-	int (*br_dev_xmit_hook)(struct sk_buff *skb);
+	void (*br_dev_dnat_hook)(struct sk_buff *skb);
 };
 extern const struct nf_br_ops __rcu *nf_br_ops;
net/core/skbuff.c +2 -1
index cdb939b..16ccbec 100644
@@ -677,7 +677,8 @@ static void skb_release_head_state(struct sk_buff *skb)
 	nf_conntrack_put(skb->nfct);
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	nf_bridge_put(skb->nf_bridge);
+	if (skb->nf_bridge_state)
+		nf_bridge_put(skb->nf_bridge);
 #endif
 }
net/ipv4/netfilter/nf_defrag_ipv4.c +1 -2
index 7e5ca6f..5b2096e 100644
@@ -51,8 +51,7 @@ static enum ip_defrag_users nf_ct_defrag_user(unsigned int hooknum,
 #endif
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (skb->nf_bridge &&
-	    skb->nf_bridge->mask & BRNF_NF_BRIDGE_PREROUTING)
+	if (skb->nf_bridge_state == BRNF_STATE_PREROUTING)
 		return IP_DEFRAG_CONNTRACK_BRIDGE_IN + zone;
 #endif
 	if (hooknum == NF_INET_PRE_ROUTING)
net/ipv4/netfilter/nf_reject_ipv4.c +1 -1
index 3262e41..3be3f1a 100644
@@ -145,7 +145,7 @@ void nf_send_reset(struct sk_buff *oldskb, int hook)
 	 * build the eth header using the original destination's MAC as the
 	 * source, and send the RST packet directly.
 	 */
-	if (oldskb->nf_bridge) {
+	if (oldskb->nf_bridge_state) {
 		struct ethhdr *oeth = eth_hdr(oldskb);
 
 		nskb->dev = nf_bridge_get_physindev(oldskb);
net/ipv6/netfilter/nf_defrag_ipv6_hooks.c +1 -2
index e70382e..01706d3 100644
@@ -41,8 +41,7 @@ static enum ip6_defrag_users nf_ct6_defrag_user(unsigned int hooknum,
 #endif
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (skb->nf_bridge &&
-	    skb->nf_bridge->mask & BRNF_NF_BRIDGE_PREROUTING)
+	if (skb->nf_bridge_state == BRNF_STATE_PREROUTING)
 		return IP6_DEFRAG_CONNTRACK_BRIDGE_IN + zone;
 #endif
 	if (hooknum == NF_INET_PRE_ROUTING)
net/ipv6/netfilter/nf_reject_ipv6.c +1 -1
index 94b4c6d..c313bb7 100644
@@ -194,7 +194,7 @@ void nf_send_reset6(struct net *net, struct sk_buff *oldskb, int hook)
 	 * build the eth header using the original destination's MAC as the
 	 * source, and send the RST packet directly.
 	 */
-	if (oldskb->nf_bridge) {
+	if (oldskb->nf_bridge_state) {
 		struct ethhdr *oeth = eth_hdr(oldskb);
 
 		nskb->dev = nf_bridge_get_physindev(oldskb);
net/netfilter/nf_log_common.c +1 -1
index a5aa596..f433262 100644
@@ -160,7 +160,7 @@ nf_log_dump_packet_common(struct nf_log_buf *m, u_int8_t pf,
 	       in ? in->name : "",
 	       out ? out->name : "");
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (skb->nf_bridge) {
+	if (skb->nf_bridge_state) {
 		const struct net_device *physindev;
 		const struct net_device *physoutdev;
net/netfilter/nf_queue.c +2 -2
index fb045b4..2dba75c 100644
@@ -54,7 +54,7 @@ void nf_queue_entry_release_refs(struct nf_queue_entry *entry)
 	if (entry->outdev)
 		dev_put(entry->outdev);
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (entry->skb->nf_bridge) {
+	if (entry->skb->nf_bridge_state) {
 		struct net_device *physdev;
 
 		physdev = nf_bridge_get_physindev(entry->skb);

@@ -81,7 +81,7 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
 	if (entry->outdev)
 		dev_hold(entry->outdev);
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (entry->skb->nf_bridge) {
+	if (entry->skb->nf_bridge_state) {
 		struct net_device *physdev;
 
 		physdev = nf_bridge_get_physindev(entry->skb);
net/netfilter/nfnetlink_queue_core.c +3 -3
index 94e1aaf..e26abe5 100644
@@ -585,13 +585,13 @@ nf_queue_entry_dup(struct nf_queue_entry *e)
  */
 static void nf_bridge_adjust_skb_data(struct sk_buff *skb)
 {
-	if (skb->nf_bridge)
+	if (skb->nf_bridge_state)
 		__skb_push(skb, skb->network_header - skb->mac_header);
 }
 
 static void nf_bridge_adjust_segmented_data(struct sk_buff *skb)
 {
-	if (skb->nf_bridge)
+	if (skb->nf_bridge_state)
 		__skb_pull(skb, skb->network_header - skb->mac_header);
 }
 #else

@@ -773,7 +773,7 @@ dev_cmp(struct nf_queue_entry *entry, unsigned long ifindex)
 		if (entry->outdev->ifindex == ifindex)
 			return 1;
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
-	if (entry->skb->nf_bridge) {
+	if (entry->skb->nf_bridge_state) {
 		int physinif, physoutif;
 
 		physinif = nf_bridge_get_physinif(entry->skb);
net/netfilter/xt_physdev.c +1 -1
index 1caaccb..535e4be 100644
@@ -33,7 +33,7 @@ physdev_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	/* Not a bridged IP packet or no info available yet:
 	 * LOCAL_OUT/mangle and LOCAL_OUT/nat don't know if
 	 * the destination device will be a bridge. */
-	if (!skb->nf_bridge) {
+	if (skb->nf_bridge_state == 0) {
 		/* Return MATCH if the invert flags of the used options are on */
 		if ((info->bitmask & XT_PHYSDEV_OP_BRIDGED) &&
 		    !(info->invert & XT_PHYSDEV_OP_BRIDGED))
Signature · 1 line
2.0.5
author maintainer
#15 lore raw

PATCH 11/14 netfilter: bridge: remove skb->nf_bridge

In reply to Florian Westphal

Instead of carrying around the nf_bridge metadata using sk_buff, add a br_netfilter private storage table and do lookups on demand.

sk_buff now only stores a 2bit bridge netfilter state to avoid the nf_bridge_info lookups in the hot-path (i.e., if state field is zero we know that there is no nf_bridge_info structure since skb isn't bridged).

The nf_bridge_info data is stored in an rhashtable. Key is the address of the skb that the nf_bridge_info is storing data for. Each clone has its own nf_bridge_info copy.

The deep copy on skb_clone() is best-effort: if allocation fails the next processing step in the bridge netfilter chain will drop the skb if it cannot find the needed metadata supposedly associated with the skb.

SLAB_DESTROY_BY_RCU is used to avoid excess memory usage, we cannot wait for rcu grace periods in packet processing path.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter.h        |   8 ++
 include/linux/netfilter_bridge.h |  49 ++++++++-
 include/linux/skbuff.h           |  51 ++-------
 net/bridge/br_netfilter.c        | 231 ++++++++++++++++++++++++++++++---------
 net/core/skbuff.c                |   2 +-
 net/netfilter/core.c             |  49 +++++++++
 6 files changed, 289 insertions(+), 101 deletions(-)
include/linux/netfilter.h +8 -0
index 2517ece..2385135 100644
@@ -339,4 +339,12 @@ extern struct nfq_ct_hook __rcu *nfq_ct_hook;
 static inline void nf_ct_attach(struct sk_buff *new, struct sk_buff *skb) {}
 #endif
 
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+struct nf_br_hook {
+	void (*nf_br_destroy)(struct sk_buff *);
+	struct nf_bridge_info* (*nf_br_find)(const struct sk_buff *);
+	bool (*nf_br_copy)(struct sk_buff *dst, const struct sk_buff *src);
+};
+#endif
+
 #endif /*__LINUX_NETFILTER_H*/
include/linux/netfilter_bridge.h +45 -4

81 lines not on this page · raw message

include/linux/skbuff.h +9 -42

93 lines not on this page · raw message

net/bridge/br_netfilter.c +177 -54

427 lines not on this page · raw message

net/core/skbuff.c +1 -1
index 16ccbec..ae249048 100644
@@ -678,7 +678,7 @@ static void skb_release_head_state(struct sk_buff *skb)
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	if (skb->nf_bridge_state)
-		nf_bridge_put(skb->nf_bridge);
+		nf_bridge_destroy(skb);
 #endif
 }
net/netfilter/core.c +49 -0

55 lines not on this page · raw message

Signature · 1 line
2.0.5
author maintainer
#16 lore raw

PATCH 12/14 netfilter: bridge: discard nf_bridge info on xmit

In reply to Florian Westphal

nf_bridge information is only needed for -m physdev, so we can always dismantle this information after POST_ROUTING if we know we're the only owner.

Signed-off-by Florian Westphal <[email protected]>
---
 net/bridge/br_netfilter.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
net/bridge/br_netfilter.c +14 -1
index 3f1f920..715157c 100644
@@ -199,6 +199,14 @@ static void nf_bridge_info_del(struct sk_buff *skb)
 	}
 }
 
+static void nf_bridge_info_drop(struct sk_buff *skb)
+{
+	if (!skb_shared(skb)) {
+		nf_bridge_info_del(skb);
+		skb->nf_bridge_state = BRNF_STATE_NONE;
+	}
+}
+
 static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
 {
 	struct net_bridge_port *port;

@@ -924,6 +932,7 @@ static int br_nf_push_frag_xmit(struct sk_buff *skb)
 	skb_copy_to_linear_data_offset(skb, -data->size, data->mac, data->size);
 	__skb_push(skb, data->encap_size);
 
+	nf_bridge_info_drop(skb);
 	return br_dev_queue_push_xmit(skb);
 }
 

@@ -942,8 +951,10 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 	unsigned int mtu_reserved, mtu;
 	struct nf_bridge_info *nf_bridge;
 
-	if (skb_is_gso(skb) || skb->protocol != htons(ETH_P_IP))
+	if (skb_is_gso(skb) || skb->protocol != htons(ETH_P_IP)) {
+		nf_bridge_info_drop(skb);
 		return br_dev_queue_push_xmit(skb);
+	}
 
 	nf_bridge = nf_bridge_info_get(skb);
 	if (!nf_bridge)

@@ -986,6 +997,7 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 
 		ret = ip_fragment(skb, mtu, br_nf_push_frag_xmit);
 	} else {
+		nf_bridge_info_drop(skb);
 		ret = br_dev_queue_push_xmit(skb);
 	}
 

@@ -998,6 +1010,7 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 #else
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 {
+	nf_bridge_info_drop(skb);
 	return br_dev_queue_push_xmit(skb);
 }
 #endif
Signature · 1 line
2.0.5
author maintainer
#17 lore raw

PATCH 13/14 netfilter: bridge: neigh_head and physoutdev can't be used at same time

In reply to Florian Westphal

The neigh_header is only needed when we detect DNAT after prerouting and neigh cache didn't have a mac address for us.

The output port has not been chosen yet so we can re-use its memory, bringing size of meta data down to 40 bytes on x86_64.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter_bridge.h | 7 +++++--
 net/bridge/br_netfilter.c        | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)
include/linux/netfilter_bridge.h +5 -2
index 621a2e4..c31024a 100644
@@ -41,8 +41,11 @@ struct nf_bridge_info {
 	bool			pkt_otherhost;
 	unsigned long		owner;
 	struct net_device	*physindev;
-	struct net_device	*physoutdev;
-	char			neigh_header[8];
+
+	union {
+		struct net_device *physoutdev;
+		char neigh_header[8];
+	};
 };
 
 struct nf_bridge_info *nf_bridge_find(const struct sk_buff *skb);
net/bridge/br_netfilter.c +2 -0
index 715157c..0730be6 100644
@@ -1111,6 +1111,8 @@ static void br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 				       nf_bridge->neigh_header,
 				       ETH_HLEN - ETH_ALEN);
 	skb->dev = nf_bridge->physindev;
+	nf_bridge->physoutdev = NULL;
+
 	br_handle_frame_finish(skb);
 }
Signature · 1 line
2.0.5
author maintainer
#18 lore raw

PATCH 14/14 netfilter: bridge: hold physinport ref during neigh resolution

In reply to Florian Westphal

This problem existed forever: when performing DNAT resolution we keep physinport around for later use as skb->dev .

We should ensure it cannot go away.

Signed-off-by Florian Westphal <[email protected]>
---
 include/linux/netfilter_bridge.h |  1 +
 net/bridge/br_netfilter.c        | 15 +++++++++++++++
 2 files changed, 16 insertions(+)
include/linux/netfilter_bridge.h +1 -0
index c31024a..8a6a4af 100644
@@ -39,6 +39,7 @@ struct nf_bridge_info {
 		BRNF_PROTO_PPPOE
 	} orig_proto;
 	bool			pkt_otherhost;
+	bool			physin_ref;
 	unsigned long		owner;
 	struct net_device	*physindev;
net/bridge/br_netfilter.c +15 -0
index 0730be6..ae03117 100644
@@ -154,6 +154,8 @@ static struct nf_bridge_info *nf_bridge_info_get(const struct sk_buff *skb)
 
 static void nf_bridge_info_free(struct nf_bridge_info *info)
 {
+	if (info->physin_ref)
+		dev_put(info->physindev);
 	kmem_cache_free(nf_bridge_cachep, info);
 }
 

@@ -174,6 +176,9 @@ static bool nf_bridge_info_copy(struct sk_buff *dst, const struct sk_buff *src)
 
 	newinfo->owner = (unsigned long)dst;
 
+	if (newinfo->physin_ref)
+		dev_hold(newinfo->physindev);
+
 	if (rhashtable_insert_fast(&nf_bridge_info_table, &newinfo->node,
 				   nf_bridge_info_params) == 0)
 		return true;

@@ -235,6 +240,7 @@ static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
 
 	nf_bridge->orig_proto = BRNF_PROTO_UNCHANGED;
 	nf_bridge->pkt_otherhost = false;
+	nf_bridge->physin_ref = false;
 	nf_bridge->owner = (unsigned long)skb;
 	nf_bridge->physoutdev = NULL;
 

@@ -422,6 +428,10 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
 							 ETH_HLEN-ETH_ALEN);
 			/* tell br_dev_xmit to continue with forwarding */
 			skb->nf_bridge_state = BRNF_STATE_BRIDGED_DNAT;
+
+			nf_bridge->physin_ref = true;
+			dev_hold(nf_bridge->physindev);
+
 			ret = neigh->output(neigh, skb);
 		}
 		neigh_release(neigh);

@@ -1113,6 +1123,11 @@ static void br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
 	skb->dev = nf_bridge->physindev;
 	nf_bridge->physoutdev = NULL;
 
+	WARN_ON_ONCE(!nf_bridge->physin_ref);
+
+	dev_put(nf_bridge->physindev);
+	nf_bridge->physin_ref = false;
+
 	br_handle_frame_finish(skb);
 }
Signature · 1 line
2.0.5