diff mbox

[v2,nf-next,3/6] netfilter: bridge: use skb->cb to track otherhost mangling

Message ID 1426179925-18220-4-git-send-email-fw@strlen.de
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Florian Westphal March 12, 2015, 5:05 p.m. UTC 
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.

For this, cb[] can be used since the skb will never be used outside
(fake inet) bridge forwarding while in 'fake PACKET_HOST' state.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netfilter_bridge.h |  1 -
 net/bridge/br_netfilter.c        | 71 ++++++++++++++++++++++++++--------------
 2 files changed, 47 insertions(+), 25 deletions(-)

Comments

Oliver Hartkopp March 12, 2015, 6:02 p.m. UTC | #1 
On 03/12/2015 06:05 PM, Florian Westphal wrote:
> 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.
> 
> For this, cb[] can be used since the skb will never be used outside
> (fake inet) bridge forwarding while in 'fake PACKET_HOST' state.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/linux/netfilter_bridge.h |  1 -
>  net/bridge/br_netfilter.c        | 71 ++++++++++++++++++++++++++--------------


> @@ -1104,6 +1125,8 @@ static int __init br_netfilter_init(void)
>  {
>  	int ret;
>  
> +	BUILD_BUG_ON(sizeof(struct nf_bridge_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> +

Please use sock_skb_cb_check_size(size) for cb size checking which is the
'new' check for cb sizes for netdev.

See:

http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit?id=b4772ef879a8f7d8c56118c2ae5a296fcf6f81d2

>  	ret = nf_register_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
>  	if (ret < 0)
>  		return ret;
> 

Regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal March 12, 2015, 6:31 p.m. UTC | #2 
Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 03/12/2015 06:05 PM, Florian Westphal wrote:
> > 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.
> > 
> > For this, cb[] can be used since the skb will never be used outside
> > (fake inet) bridge forwarding while in 'fake PACKET_HOST' state.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  include/linux/netfilter_bridge.h |  1 -
> >  net/bridge/br_netfilter.c        | 71 ++++++++++++++++++++++++++--------------
> 
> 
> > @@ -1104,6 +1125,8 @@ static int __init br_netfilter_init(void)
> >  {
> >  	int ret;
> >  
> > +	BUILD_BUG_ON(sizeof(struct nf_bridge_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> > +
> 
> Please use sock_skb_cb_check_size(size) for cb size checking which is the
> 'new' check for cb sizes for netdev.

I can do this for conistency, but its technically not needed here.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal March 12, 2015, 6:35 p.m. UTC | #3 
Florian Westphal <fw@strlen.de> wrote:
> > > +	BUILD_BUG_ON(sizeof(struct nf_bridge_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
> > > +
> > 
> > Please use sock_skb_cb_check_size(size) for cb size checking which is the
> > 'new' check for cb sizes for netdev.
> 
> I can do this for conistency, but its technically not needed here.

I take that back, it would actually be inconsistent to use
sock_skb_cb_check_size since nf_bridge skbs will not be associated with
sockets.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Hartkopp March 12, 2015, 6:40 p.m. UTC | #4 
On 03/12/2015 07:35 PM, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
>>>> +	BUILD_BUG_ON(sizeof(struct nf_bridge_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
>>>> +
>>>
>>> Please use sock_skb_cb_check_size(size) for cb size checking which is the
>>> 'new' check for cb sizes for netdev.
>>
>> I can do this for conistency, but its technically not needed here.
> 
> I take that back, it would actually be inconsistent to use
> sock_skb_cb_check_size since nf_bridge skbs will not be associated with
> sockets.
> 

Ok. I just wanted to make sure that the use of the cb[]-space doesn't clash
with the new space that's acquired by the dropcount stuff.

Sorry for the noise :-)

Best regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/netfilter_bridge.h b/include/linux/netfilter_bridge.h
index b131613..05437f8 100644
--- a/include/linux/netfilter_bridge.h
+++ b/include/linux/netfilter_bridge.h
@@ -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
 #define BRNF_8021Q			0x10
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 669b4fa..8649ef5 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -47,6 +47,17 @@ 
 #include <linux/sysctl.h>
 #endif
 
+struct nf_bridge_skb_cb {
+	union {
+		struct inet_skb_parm i;
+		struct inet6_skb_parm i6;
+	} u;
+
+	u8 packet_otherhost:1;
+};
+
+#define BRNF_CB(skb) ((struct nf_bridge_skb_cb *)(skb)->cb)
+
 #ifdef CONFIG_SYSCTL
 static struct ctl_table_header *brnf_sysctl_header;
 static int brnf_call_iptables __read_mostly = 1;
@@ -259,6 +270,29 @@  static void nf_bridge_update_protocol(struct sk_buff *skb)
 		skb->protocol = htons(ETH_P_PPP_SES);
 }
 
+static void nf_bridge_restore_otherhost(struct sk_buff *skb)
+{
+	struct nf_bridge_skb_cb *cb = BRNF_CB(skb);
+
+	if (cb->packet_otherhost) {
+		cb->packet_otherhost = 0;
+		skb->pkt_type = PACKET_OTHERHOST;
+	}
+}
+
+static void nf_bridge_save_otherhost(struct sk_buff *skb)
+{
+	struct nf_bridge_skb_cb *cb;
+
+	if (skb->pkt_type != PACKET_OTHERHOST)
+		return;
+
+	cb = BRNF_CB(skb);
+
+	cb->packet_otherhost = 1;
+	skb->pkt_type = PACKET_HOST;
+}
+
 /* PF_BRIDGE/PRE_ROUTING *********************************************/
 /* Undo the changes made for ip6tables PREROUTING and continue the
  * bridge PRE_ROUTING hook. */
@@ -267,10 +301,8 @@  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) {
-		skb->pkt_type = PACKET_OTHERHOST;
-		nf_bridge->mask ^= BRNF_PKT_TYPE;
-	}
+	nf_bridge_restore_otherhost(skb);
+
 	nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
 
 	rt = bridge_parent_rtable(nf_bridge->physindev);
@@ -400,10 +432,7 @@  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_restore_otherhost(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))) {
@@ -485,11 +514,10 @@  static struct net_device *brnf_get_logical_dev(struct sk_buff *skb, const struct
 static struct net_device *setup_pre_routing(struct sk_buff *skb)
 {
 	struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+	struct nf_bridge_skb_cb *cb = BRNF_CB(skb);
 
-	if (skb->pkt_type == PACKET_OTHERHOST) {
-		skb->pkt_type = PACKET_HOST;
-		nf_bridge->mask |= BRNF_PKT_TYPE;
-	}
+	cb->packet_otherhost = 0;
+	nf_bridge_save_otherhost(skb);
 
 	nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING;
 	nf_bridge->physindev = skb->dev;
@@ -687,11 +715,8 @@  static int br_nf_forward_finish(struct sk_buff *skb)
 	struct net_device *in;
 
 	if (!IS_ARP(skb) && !IS_VLAN_ARP(skb)) {
+		nf_bridge_restore_otherhost(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_update_protocol(skb);
 	} else {
 		in = *((struct net_device **)(skb->cb));
@@ -741,10 +766,8 @@  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;
-	}
+
+	nf_bridge_save_otherhost(skb);
 
 	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
 		return NF_DROP;
@@ -911,10 +934,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;
-	}
+
+	nf_bridge_save_otherhost(skb);
 
 	nf_bridge_pull_encap_header(skb);
 	if (pf == NFPROTO_IPV4)
@@ -1104,6 +1125,8 @@  static int __init br_netfilter_init(void)
 {
 	int ret;
 
+	BUILD_BUG_ON(sizeof(struct nf_bridge_skb_cb) > FIELD_SIZEOF(struct sk_buff, cb));
+
 	ret = nf_register_hooks(br_nf_ops, ARRAY_SIZE(br_nf_ops));
 	if (ret < 0)
 		return ret;