[PATCH net-next 0/2] net: sched: act_tunnel_key: UDP checksums

[PATCH net-next 0/2] net: sched: act_tunnel_key: UDP checksums

[Jiri Benc]

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 <jbenc@redhat.com>


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(-)

-- 
1.8.3.1

[PATCH net-next 1/2] net: sched: act_tunnel_key: request UDP checksum by default

[Jiri Benc]

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 <jbenc@redhat.com>
---
 net/sched/act_tunnel_key.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index b9a2f241a5b3..b90effa10eb5 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -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);
 		}
 
-- 
1.8.3.1

[PATCH net-next 2/2] net: sched: act_tunnel_key: make UDP checksum configurable

[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 <jbenc@redhat.com>
---
 include/uapi/linux/tc_act/tc_tunnel_key.h |  1 +
 net/sched/act_tunnel_key.c                | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/tc_act/tc_tunnel_key.h b/include/uapi/linux/tc_act/tc_tunnel_key.h
index 84ea55e1076b..afcd4be953e2 100644
--- a/include/uapi/linux/tc_act/tc_tunnel_key.h
+++ b/include/uapi/linux/tc_act/tc_tunnel_key.h
@@ -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,
 };
 
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index b90effa10eb5..fd7e75679c69 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -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;
 	}
 
-- 
1.8.3.1

[PATCH iproute2 1/2] tc: m_tunnel_key: reformat the usage text

[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 <jbenc@redhat.com>
---
 tc/m_tunnel_key.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tc/m_tunnel_key.c b/tc/m_tunnel_key.c
index fb418a76bd79..6faf9632058a 100644
--- a/tc/m_tunnel_key.c
+++ b/tc/m_tunnel_key.c
@@ -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)
-- 
1.8.3.1

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

[Jiri Benc]

Allows control of UDP zero checksum.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 man/man8/tc-tunnel_key.8 | 18 ++++++++++++++++++
 tc/m_tunnel_key.c        | 21 ++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/man/man8/tc-tunnel_key.8 b/man/man8/tc-tunnel_key.8
index 2e569730abbb..e979a74715cb 100644
--- a/man/man8/tc-tunnel_key.8
+++ b/man/man8/tc-tunnel_key.8
@@ -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
diff --git a/tc/m_tunnel_key.c b/tc/m_tunnel_key.c
index 6faf9632058a..1cdd03560c35 100644
--- a/tc/m_tunnel_key.c
+++ b/tc/m_tunnel_key.c
@@ -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, "");
-- 
1.8.3.1

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

[Stephen Hemminger]

On Wed, 14 Jun 2017 21:30:18 +0200
Jiri Benc <jbenc@redhat.com> wrote:

> Allows control of UDP zero checksum.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  man/man8/tc-tunnel_key.8 | 18 ++++++++++++++++++
>  tc/m_tunnel_key.c        | 21 ++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/man/man8/tc-tunnel_key.8 b/man/man8/tc-tunnel_key.8
> index 2e569730abbb..e979a74715cb 100644
> --- a/man/man8/tc-tunnel_key.8
> +++ b/man/man8/tc-tunnel_key.8
> @@ -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 ,

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

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

[Jiri Benc]

On Wed, 14 Jun 2017 13:38:07 -0700, Stephen Hemminger wrote:
> 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

Re: [PATCH net-next 0/2] net: sched: act_tunnel_key: UDP checksums

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Wed, 14 Jun 2017 21:19:29 +0200

> 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 <jbenc@redhat.com>

Seems reasonable, series applied, thanks.

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

[Stephen Hemminger]

On Wed, 14 Jun 2017 21:30:18 +0200
Jiri Benc <jbenc@redhat.com> wrote:

> Allows control of UDP zero checksum.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>

Applied both patches to net-next