Skip to contentSkip to conversation

net: sched: act_tunnel_key: UDP checksums

PATCH Applied Jiri Benc 3 participants 9 messages Jun 14, 2017 – Jun 16, 2017

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Jiri Benc

author
#0 lore raw

cover net: sched: act_tunnel_key: UDP checksums

Currently, the tunnel_key tc action does not set TUNNEL_CSUM, thus transmitting packets with zero UDP checksum. This is inconsistent with how we treat non-lwt UDP tunnels where the default is to fill in the UDP checksum. Non-zero UDP checksum is the better default anyway for various reasons previously discussed.

Make this configurable for the tunnel_key tc action with the default being non-zero checksum. Saves a lot of surprises especially with IPv6.

Signed-off-by Jiri Benc <[email protected]>

Jiri Benc (2):
net: sched: act_tunnel_key: request UDP checksum by default
net: sched: act_tunnel_key: make UDP checksum configurable

 include/uapi/linux/tc_act/tc_tunnel_key.h |  1 +
 net/sched/act_tunnel_key.c                | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH 1/2 net: sched: act_tunnel_key: request UDP checksum by default

There's currently no way to request (outer) UDP checksum with act_tunnel_key. This is problem especially for IPv6. Right now, tunnel_key action with IPv6 does not work without going through hassles: both sides have to have udp6zerocsumrx configured on the tunnel interface. This is obviously not a good solution universally.

It makes more sense to compute the UDP checksum by default even for IPv4. Just set the default to request the checksum when using act_tunnel_key.

Signed-off-by Jiri Benc <[email protected]>
---
 net/sched/act_tunnel_key.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/sched/act_tunnel_key.c +2 -2
index b9a2f241a5b3..b90effa10eb5 100644
@@ -125,7 +125,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 			daddr = nla_get_in_addr(tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]);
 
 			metadata = __ip_tun_set_dst(saddr, daddr, 0, 0,
-						    dst_port, TUNNEL_KEY,
+						    dst_port, TUNNEL_KEY | TUNNEL_CSUM,
 						    key_id, 0);
 		} else if (tb[TCA_TUNNEL_KEY_ENC_IPV6_SRC] &&
 			   tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]) {

@@ -136,7 +136,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 			daddr = nla_get_in6_addr(tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]);
 
 			metadata = __ipv6_tun_set_dst(&saddr, &daddr, 0, 0, dst_port,
-						      0, TUNNEL_KEY,
+						      0, TUNNEL_KEY | TUNNEL_CSUM,
 						      key_id, 0);
 		}
Signature · 1 line
1.8.3.1
author
#2 lore raw

PATCH 1/2 tc: m_tunnel_key: reformat the usage text

In reply to Jiri Benc

Adding new tunnel key fields would cause the usage line overflow 80 chars. Make the usage text similar to other commands.

Signed-off-by Jiri Benc <[email protected]>
---
 tc/m_tunnel_key.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
tc/m_tunnel_key.c +7 -1
index fb418a76bd79..6faf9632058a 100644
@@ -22,7 +22,13 @@
 static void explain(void)
 {
 	fprintf(stderr, "Usage: tunnel_key unset\n");
-	fprintf(stderr, "       tunnel_key set id TUNNELID src_ip IP dst_ip IP dst_port UDP_PORT\n");
+	fprintf(stderr, "       tunnel_key set <TUNNEL_KEY>\n");
+	fprintf(stderr,
+		"Where TUNNEL_KEY is a combination of:\n"
+		"id <TUNNELID> (mandatory)\n"
+		"src_ip <IP> (mandatory)\n"
+		"dst_ip <IP> (mandatory)\n"
+		"dst_port <UDP_PORT>\n");
 }
 
 static void usage(void)
Signature · 1 line
1.8.3.1
author
#3 lore raw

PATCH 2/2 net: sched: act_tunnel_key: make UDP checksum configurable

In reply to Jiri Benc

Allow requesting of zero UDP checksum for encapsulated packets. The name and meaning of the attribute is "NO_CSUM" in order to have the same meaning of the attribute missing and being 0.

Signed-off-by Jiri Benc <[email protected]>
---
 include/uapi/linux/tc_act/tc_tunnel_key.h |  1 +
 net/sched/act_tunnel_key.c                | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)
include/uapi/linux/tc_act/tc_tunnel_key.h +1 -0
index 84ea55e1076b..afcd4be953e2 100644
@@ -34,6 +34,7 @@ enum {
 	TCA_TUNNEL_KEY_ENC_KEY_ID,	/* be64 */
 	TCA_TUNNEL_KEY_PAD,
 	TCA_TUNNEL_KEY_ENC_DST_PORT,	/* be16 */
+	TCA_TUNNEL_KEY_NO_CSUM,		/* u8 */
 	__TCA_TUNNEL_KEY_MAX,
 };
net/sched/act_tunnel_key.c +12 -3
index b90effa10eb5..fd7e75679c69 100644
@@ -67,6 +67,7 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
 	[TCA_TUNNEL_KEY_ENC_IPV6_DST] = { .len = sizeof(struct in6_addr) },
 	[TCA_TUNNEL_KEY_ENC_KEY_ID]   = { .type = NLA_U32 },
 	[TCA_TUNNEL_KEY_ENC_DST_PORT] = {.type = NLA_U16},
+	[TCA_TUNNEL_KEY_NO_CSUM]      = { .type = NLA_U8 },
 };
 
 static int tunnel_key_init(struct net *net, struct nlattr *nla,

@@ -83,6 +84,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 	bool exists = false;
 	__be16 dst_port = 0;
 	__be64 key_id;
+	__be16 flags;
 	int ret = 0;
 	int err;
 

@@ -113,6 +115,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 
 		key_id = key32_to_tunnel_id(nla_get_be32(tb[TCA_TUNNEL_KEY_ENC_KEY_ID]));
 
+		flags = TUNNEL_KEY | TUNNEL_CSUM;
+		if (tb[TCA_TUNNEL_KEY_NO_CSUM] &&
+		    nla_get_u8(tb[TCA_TUNNEL_KEY_NO_CSUM]))
+			flags &= ~TUNNEL_CSUM;
+
 		if (tb[TCA_TUNNEL_KEY_ENC_DST_PORT])
 			dst_port = nla_get_be16(tb[TCA_TUNNEL_KEY_ENC_DST_PORT]);
 

@@ -125,7 +132,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 			daddr = nla_get_in_addr(tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]);
 
 			metadata = __ip_tun_set_dst(saddr, daddr, 0, 0,
-						    dst_port, TUNNEL_KEY | TUNNEL_CSUM,
+						    dst_port, flags,
 						    key_id, 0);
 		} else if (tb[TCA_TUNNEL_KEY_ENC_IPV6_SRC] &&
 			   tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]) {

@@ -136,7 +143,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 			daddr = nla_get_in6_addr(tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]);
 
 			metadata = __ipv6_tun_set_dst(&saddr, &daddr, 0, 0, dst_port,
-						      0, TUNNEL_KEY | TUNNEL_CSUM,
+						      0, flags,
 						      key_id, 0);
 		}
 

@@ -266,7 +273,9 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
 		if (nla_put_be32(skb, TCA_TUNNEL_KEY_ENC_KEY_ID, key_id) ||
 		    tunnel_key_dump_addresses(skb,
 					      &params->tcft_enc_metadata->u.tun_info) ||
-		    nla_put_be16(skb, TCA_TUNNEL_KEY_ENC_DST_PORT, key->tp_dst))
+		    nla_put_be16(skb, TCA_TUNNEL_KEY_ENC_DST_PORT, key->tp_dst) ||
+		    nla_put_u8(skb, TCA_TUNNEL_KEY_NO_CSUM,
+			       !(key->tun_flags & TUNNEL_CSUM)))
 			goto nla_put_failure;
 	}
Signature · 1 line
1.8.3.1
author
#4 lore raw

PATCH 2/2 tc: m_tunnel_key: add csum/nocsum option

In reply to Jiri Benc

Allows control of UDP zero checksum.

Signed-off-by Jiri Benc <[email protected]>
---
 man/man8/tc-tunnel_key.8 | 18 ++++++++++++++++++
 tc/m_tunnel_key.c        | 21 ++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)
man/man8/tc-tunnel_key.8 +18 -0
index 2e569730abbb..e979a74715cb 100644
@@ -16,6 +16,7 @@ tunnel_key - Tunnel metadata manipulation
 .IR ADDRESS
 .BI id " KEY_ID"
 .BI dst_port " UDP_PORT"
+.RB "[ " csum " | " nocsum " ]"
 
 .SH DESCRIPTION
 The

@@ -77,6 +78,23 @@ Outer header destination IP address (IPv4 or IPv6)
 .TP
 .B dst_port
 Outer header destination UDP port
+.TP
+.RB [ no ] csum
+Controlls outer UDP checksum. When set to
+.B csum
+(which is default), the outer UDP checksum is calculated and included in the
+packets. When set to
+.BR nocsum ,
+outer UDP checksum is zero. Note that when using zero UDP checksums with
+IPv6, the other tunnel endpoint must be configured to accept such packets.
+In Linux, this would be the
+.B udp6zerocsumrx
+option for the VXLAN tunnel interface.
+.IP
+If using
+.B nocsum
+with IPv6, be sure you know what you are doing. Zero UDP checksums provide
+weaker protection against corrupted packets. See RFC6935 for details.
 .RE
 .SH EXAMPLES
 The following example encapsulates incoming ICMP packets on eth0 into a vxlan
tc/m_tunnel_key.c +20 -1
index 6faf9632058a..1cdd03560c35 100644
@@ -28,7 +28,8 @@ static void explain(void)
 		"id <TUNNELID> (mandatory)\n"
 		"src_ip <IP> (mandatory)\n"
 		"dst_ip <IP> (mandatory)\n"
-		"dst_port <UDP_PORT>\n");
+		"dst_port <UDP_PORT>\n"
+		"csum | nocsum (default is \"csum\")\n");
 }
 
 static void usage(void)

@@ -92,6 +93,7 @@ static int parse_tunnel_key(struct action_util *a, int *argc_p, char ***argv_p,
 	int has_src_ip = 0;
 	int has_dst_ip = 0;
 	int has_key_id = 0;
+	int csum = 1;
 
 	if (matches(*argv, "tunnel_key") != 0)
 		return -1;

@@ -156,6 +158,10 @@ static int parse_tunnel_key(struct action_util *a, int *argc_p, char ***argv_p,
 				fprintf(stderr, "Illegal \"dst port\"\n");
 				return -1;
 			}
+		} else if (matches(*argv, "csum") == 0) {
+			csum = 1;
+		} else if (matches(*argv, "nocsum") == 0) {
+			csum = 0;
 		} else if (matches(*argv, "help") == 0) {
 			usage();
 		} else {

@@ -164,6 +170,8 @@ static int parse_tunnel_key(struct action_util *a, int *argc_p, char ***argv_p,
 		NEXT_ARG_FWD();
 	}
 
+	addattr8(n, MAX_MSG, TCA_TUNNEL_KEY_NO_CSUM, !csum);
+
 	parse_action_control_dflt(&argc, &argv, &parm.action,
 				  false, TC_ACT_PIPE);
 

@@ -233,6 +241,15 @@ static void tunnel_key_print_dst_port(FILE *f, char *name,
 	fprintf(f, "\n\t%s %d", name, rta_getattr_be16(attr));
 }
 
+static void tunnel_key_print_flag(FILE *f, const char *name_on,
+				  const char *name_off,
+				  struct rtattr *attr)
+{
+	if (!attr)
+		return;
+	fprintf(f, "\n\t%s", rta_getattr_u8(attr) ? name_on : name_off);
+}
+
 static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
 {
 	struct rtattr *tb[TCA_TUNNEL_KEY_MAX + 1];

@@ -269,6 +286,8 @@ static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
 					tb[TCA_TUNNEL_KEY_ENC_KEY_ID]);
 		tunnel_key_print_dst_port(f, "dst_port",
 					  tb[TCA_TUNNEL_KEY_ENC_DST_PORT]);
+		tunnel_key_print_flag(f, "nocsum", "csum",
+				      tb[TCA_TUNNEL_KEY_NO_CSUM]);
 		break;
 	}
 	print_action_control(f, " ", parm->action, "");
Signature · 1 line
1.8.3.1
#5 lore raw

Re: [PATCH iproute2 2/2] tc: m_tunnel_key: add csum/nocsum option

Quoted 21 lines · On Wed, 14 Jun 2017 21:30:18 +0200 Jiri Benc <[email protected]> wrote:

Allows control of UDP zero checksum.

Signed-off-by Jiri Benc <[email protected]>
---
 man/man8/tc-tunnel_key.8 | 18 ++++++++++++++++++
 tc/m_tunnel_key.c        | 21 ++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)
man/man8/tc-tunnel_key.8 +1 -0
index 2e569730abbb..e979a74715cb 100644
@@ -16,6 +16,7 @@ tunnel_key - Tunnel metadata manipulation
 .IR ADDRESS
 .BI id " KEY_ID"
 .BI dst_port " UDP_PORT"
+.RB "[ " csum " | " nocsum " ]"
 
 .SH DESCRIPTION
 The

man/man8/tc-tunnel_key.8 Outer header destination IP address (IPv4 or IPv6) · line 87

Quoted 10 lines
man/man8/tc-tunnel_key.8 +7 -0
@@ -77,6 +78,23 @@ Outer header destination IP address (IPv4 or IPv6)
 .TP
 .B dst_port
 Outer header destination UDP port
+.TP
+.RB [ no ] csum
+Controlls outer UDP checksum. When set to
+.B csum
+(which is default), the outer UDP checksum is calculated and included in the
+packets. When set to
+.BR nocsum ,

Does this change the default? Before your patches what was the checksum setting for the new tunnel.

author
#6 lore raw

Re: [PATCH iproute2 2/2] tc: m_tunnel_key: add csum/nocsum option

Quoted 2 lines · Stephen Hemminger

Does this change the default? Before your patches what was the checksum setting for the new tunnel.

Yes, it does. See the kernel patches.

I realize it's a user visible change; however, given that the real reason act_tunnel_key was introduced is offloading of openvswitch and openvswitch doesn't use it yet and I very much doubt there are other users, I'm still proposing the change. It makes things aligned between the non-lwt tunnels, tc and even the current openvswitch (which includes the csum by default as well). When someone someday starts using lwtunnels with tc for other uses, this will make it much less surprising and will prevent user errors. Not mentioning performance which is in general going to be better with the csum.

Furthermore, the only use case for nocsum is a compatibility with broken VTEPs from hw vendors. This is not a setup act_tunnel_key is going to be used in: in such case, you pretty much want the VXLAN internal control plane, not an external one.

We're also not breaking existing Linux setups, even if there were any. Even with udp6zerocsumrx, the non-zero csumed packets are still accepted.

I was notified about the current confusing behavior by Red Hat QA when they were trying to test the tunnel_key action and couldn't make it work for IPv6. When those guys needed a help of the kernel developer and the kernel developer in question had to insert a few printks to realize what's going on, no regular user would be able to use it ;-) (Of course, that's "fixable" by documentation.)

Jiri

maintainer
#8 lore raw
In reply to Jiri Benc

From: Jiri Benc <[email protected]>
Date: Wed, 14 Jun 2017 21:19:29 +0200

Quoted 10 lines

Currently, the tunnel_key tc action does not set TUNNEL_CSUM, thus transmitting packets with zero UDP checksum. This is inconsistent with how we treat non-lwt UDP tunnels where the default is to fill in the UDP checksum. Non-zero UDP checksum is the better default anyway for various reasons previously discussed.

Make this configurable for the tunnel_key tc action with the default being non-zero checksum. Saves a lot of surprises especially with IPv6.

Signed-off-by Jiri Benc <[email protected]>

Seems reasonable, series applied, thanks.