flowi6_tos fixes

PATCH In review Daniel Borkmann 2 participants 5 messages Mar 18, 2016 – Mar 20, 2016

Subsystems (4)

Networking Network drivers Tracing IPv4/IPv6

lore

Share & tools

lore

Daniel Borkmann

author

2016-03-18 17:38 UTC #0 lore raw

cover flowi6_tos fixes

This set is a follow-up to address Jiri's recent feedback [1] on the flowi6_tos issue, that it is not used for IPv6 route lookups. The three patches fix all current users of flowi6_tos and remove the define to avoid any future confusion on this. Tested the vxlan and geneve ones with IPv6 routing rules. For details, please see individual patches.

[ As fixes are currently applied against net-next tree, I've rebased
it against that. ]

Thanks!

[1] http://patchwork.ozlabs.org/patch/592055/

Daniel Borkmann (3):
vxlan: fix populating tclass in vxlan6_get_route
geneve: fix populating tclass in geneve_get_v6_dst ipv6, trace: fix tos reporting on fib6_table_lookup

 drivers/net/geneve.c        | 11 ++++++-----
 drivers/net/vxlan.c         |  3 +--
 include/net/flow.h          |  2 +-
 include/net/ipv6.h          |  6 ++++++
 include/trace/events/fib6.h |  2 +-
 5 files changed, 15 insertions(+), 9 deletions(-)

Signature · 1 line

1.9.3

Daniel Borkmann

author

2016-03-18 17:38 UTC #1 lore raw

PATCH 1/3 vxlan: fix populating tclass in vxlan6_get_route

Jiri mentioned that flowi6_tos of struct flowi6 is never used/read anywhere. In fact, rest of the kernel uses the flowi6's flowlabel, where the traffic class _and_ the flowlabel (aka flowinfo) is encoded.

For example, for policy routing, fib6_rule_match() uses ip6_tclass() that is applied on the flowlabel member for matching on tclass. Similar fix is needed for geneve, where flowi6_tos is set as well. Installing a v6 blackhole rule that f.e. matches on tos is now working with vxlan.

Fixes 1400615d64cf ("vxlan: allow setting ipv6 traffic class")

Reported-by Jiri Benc <[email protected]>

Signed-off-by Daniel Borkmann <[email protected]>

---
 drivers/net/vxlan.c | 3 +--
 include/net/ipv6.h  | 6 ++++++
 2 files changed, 7 insertions(+), 2 deletions(-)

drivers/net/vxlan.c +1 -2

index 800106a7..7bfcb9a62a 100644
@@ -1810,10 +1810,9 @@ static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan,
 
 	memset(&fl6, 0, sizeof(fl6));
 	fl6.flowi6_oif = oif;
-	fl6.flowi6_tos = RT_TOS(tos);
 	fl6.daddr = *daddr;
 	fl6.saddr = vxlan->cfg.saddr.sin6.sin6_addr;
-	fl6.flowlabel = label;
+	fl6.flowlabel = ip6_make_flowinfo(RT_TOS(tos), label);
 	fl6.flowi6_mark = skb->mark;
 	fl6.flowi6_proto = IPPROTO_UDP;

include/net/ipv6.h +6 -0

index f3c9857..d0aeb97 100644
@@ -835,6 +835,12 @@ static inline u8 ip6_tclass(__be32 flowinfo)
 {
 	return ntohl(flowinfo & IPV6_TCLASS_MASK) >> IPV6_TCLASS_SHIFT;
 }
+
+static inline __be32 ip6_make_flowinfo(unsigned int tclass, __be32 flowlabel)
+{
+	return htonl(tclass << IPV6_TCLASS_SHIFT) | flowlabel;
+}
+
 /*
  *	Prototypes exported by ipv6
  */

Signature · 1 line

1.9.3

Daniel Borkmann

author

2016-03-18 17:38 UTC #2 lore raw

PATCH 2/3 geneve: fix populating tclass in geneve_get_v6_dst

In reply to Daniel Borkmann

The struct flowi6's flowi6_tos is not used in IPv6 route lookup, the traffic class information is handled in the flowi6's flowlabel member instead. For example, for policy routing, fib6_rule_match() uses ip6_tclass() that is applied on the flowlabel for matching on tclass, which would currently not work as expected.

Fixes 3a56f86f1be6 ("geneve: handle ipv6 priority like ipv4 tos")

Signed-off-by Daniel Borkmann <[email protected]>

---
 drivers/net/geneve.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

drivers/net/geneve.c +6 -5

index 192631a..bc16889 100644
@@ -843,8 +843,8 @@ static struct dst_entry *geneve_get_v6_dst(struct sk_buff *skb,
 	if (info) {
 		fl6->daddr = info->key.u.ipv6.dst;
 		fl6->saddr = info->key.u.ipv6.src;
-		fl6->flowi6_tos = RT_TOS(info->key.tos);
-		fl6->flowlabel = info->key.label;
+		fl6->flowlabel = ip6_make_flowinfo(RT_TOS(info->key.tos),
+						   info->key.label);
 		dst_cache = &info->dst_cache;
 	} else {
 		prio = geneve->tos;

@@ -855,8 +855,8 @@ static struct dst_entry *geneve_get_v6_dst(struct sk_buff *skb,
 			use_cache = false;
 		}
 
-		fl6->flowi6_tos = RT_TOS(prio);
-		fl6->flowlabel = geneve->label;
+		fl6->flowlabel = ip6_make_flowinfo(RT_TOS(prio),
+						   geneve->label);
 		fl6->daddr = geneve->remote.sin6.sin6_addr;
 		dst_cache = &geneve->dst_cache;
 	}

@@ -1049,7 +1049,8 @@ static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		if (unlikely(err))
 			goto err;
 
-		prio = ip_tunnel_ecn_encap(fl6.flowi6_tos, iip, skb);
+		prio = ip_tunnel_ecn_encap(ip6_tclass(fl6.flowlabel),
+					   iip, skb);
 		ttl = geneve->ttl;
 		if (!ttl && ipv6_addr_is_multicast(&fl6.daddr))
 			ttl = 1;

Signature · 1 line

1.9.3

Daniel Borkmann

author

2016-03-18 17:38 UTC #3 lore raw

PATCH 3/3 ipv6, trace: fix tos reporting on fib6_table_lookup

In reply to Daniel Borkmann

flowi6_tos of struct flowi6 is unused in IPv6, therefore dumping tos on that tracepoint will also give incorrect information wrt traffic class.

If we want to fix it, we need to extract it via ip6_tclass(flp->flowlabel). While for the same test case I get a count of 0 non-zero tos values before the change, they now start to show up after the change:

  # ./perf record -e fib6:fib6_table_lookup -a sleep 10
  # ./perf script | grep -v "tos 0" | wc -l
  60

Since there's no user in the kernel tree anymore of flowi6_tos, remove the define to avoid any future confusion on this.

Fixes b811580d91e9 ("net: IPv6 fib lookup tracepoint")

Signed-off-by Daniel Borkmann <[email protected]>

---
 include/net/flow.h          | 2 +-
 include/trace/events/fib6.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

include/net/flow.h +1 -1

index 83969ee..d47ef4b 100644
@@ -127,7 +127,6 @@ struct flowi6 {
 #define flowi6_oif		__fl_common.flowic_oif
 #define flowi6_iif		__fl_common.flowic_iif
 #define flowi6_mark		__fl_common.flowic_mark
-#define flowi6_tos		__fl_common.flowic_tos
 #define flowi6_scope		__fl_common.flowic_scope
 #define flowi6_proto		__fl_common.flowic_proto
 #define flowi6_flags		__fl_common.flowic_flags

@@ -135,6 +134,7 @@ struct flowi6 {
 #define flowi6_tun_key		__fl_common.flowic_tun_key
 	struct in6_addr		daddr;
 	struct in6_addr		saddr;
+	/* Note: flowi6_tos is encoded in flowlabel, too. */
 	__be32			flowlabel;
 	union flowi_uli		uli;
 #define fl6_sport		uli.ports.sport

include/trace/events/fib6.h +1 -1

index 4cf6bac..d60096c 100644
@@ -37,7 +37,7 @@ TRACE_EVENT(fib6_table_lookup,
 		__entry->tb_id = tb_id;
 		__entry->oif = flp->flowi6_oif;
 		__entry->iif = flp->flowi6_iif;
-		__entry->tos = flp->flowi6_tos;
+		__entry->tos = ip6_tclass(flp->flowlabel);
 		__entry->scope = flp->flowi6_scope;
 		__entry->flags = flp->flowi6_flags;

Signature · 1 line

1.9.3

David Miller

maintainer

2016-03-20 17:44 UTC #4 lore raw

In reply to Daniel Borkmann

From: Daniel Borkmann <[email protected]>
Date: Fri, 18 Mar 2016 18:37:56 +0100

Quoted 7 lines

This set is a follow-up to address Jiri's recent feedback [1] on the flowi6_tos issue, that it is not used for IPv6 route lookups. The three patches fix all current users of flowi6_tos and remove the define to avoid any future confusion on this. Tested the vxlan and geneve ones with IPv6 routing rules. For details, please see individual patches.

Quoted 6 lines

[ As fixes are currently applied against net-next tree, I've rebased
it against that. ]
Thanks!
[1] http://patchwork.ozlabs.org/patch/592055/

Series applied, thanks Daniel.