[PATCH/RFC net-next 0/3] introduce Geneve options support in TC tunnel key

[PATCH/RFC net-next 0/3] introduce Geneve options support in TC tunnel key

[Simon Horman]

Hi all,

This set adds Geneve options support to the TC tunnel key action.
Options TLVs are exposed to userspace using a list of netlink attributes
with each option described using attributes for class, type and data.

The series also enhances the tunnel key action to use extended ack.

Simon Horman (3):
  net/sched: act_tunnel_key: disambiguate metadata dst error cases
  net/sched: act_tunnel_key: add extended ack support
  net/sched: add tunnel option support to act_tunnel_key

 include/uapi/linux/tc_act/tc_tunnel_key.h |  26 ++++
 net/sched/act_tunnel_key.c                | 212 ++++++++++++++++++++++++++++--
 2 files changed, 230 insertions(+), 8 deletions(-)

-- 
2.11.0

[PATCH/RFC 1/3] net/sched: act_tunnel_key: disambiguate metadata dst error cases

[Simon Horman]

Metadata may be NULL for one of two reasons:
* Missing user input
* Failure to allocate the metadata dst

Disambiguate these case by returning -EINVAL for the former and -ENOMEM
for the latter rather than -EINVAL for both cases.

This is in preparation for using extended ack to provide more information
to users when parsing their input.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/sched/act_tunnel_key.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 9169b7e78ada..7ce95538177b 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -143,10 +143,13 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 			metadata = __ipv6_tun_set_dst(&saddr, &daddr, 0, 0, dst_port,
 						      0, flags,
 						      key_id, 0);
+		} else {
+			ret = -EINVAL;
+			goto err_out;
 		}
 
 		if (!metadata) {
-			ret = -EINVAL;
+			ret = -ENOMEM;
 			goto err_out;
 		}
 
-- 
2.11.0

[PATCH/RFC 2/3] net/sched: act_tunnel_key: add extended ack support

[Simon Horman]

Add extended ack support for the tunnel key action by using NL_SET_ERR_MSG
during validation of user input.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/sched/act_tunnel_key.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 7ce95538177b..c0bbdec50022 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -90,12 +90,14 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 		return -EINVAL;
 
 	err = nla_parse_nested(tb, TCA_TUNNEL_KEY_MAX, nla, tunnel_key_policy,
-			       NULL);
+			       extack);
 	if (err < 0)
 		return err;
 
-	if (!tb[TCA_TUNNEL_KEY_PARMS])
+	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
+		NL_SET_ERR_MSG(extack, "Missing tunnel key parameter");
 		return -EINVAL;
+	}
 
 	parm = nla_data(tb[TCA_TUNNEL_KEY_PARMS]);
 	exists = tcf_idr_check(tn, parm->index, a, bind);
@@ -107,6 +109,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 		break;
 	case TCA_TUNNEL_KEY_ACT_SET:
 		if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
+			NL_SET_ERR_MSG(extack, "Missing tunnel key enc id");
 			ret = -EINVAL;
 			goto err_out;
 		}
@@ -144,6 +147,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 						      0, flags,
 						      key_id, 0);
 		} else {
+			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");
 			ret = -EINVAL;
 			goto err_out;
 		}
-- 
2.11.0

[PATCH/RFC 3/3] net/sched: add tunnel option support to act_tunnel_key

[Simon Horman]

Allow setting tunnel options using the act_tunnel_key action.

Options are expressed as class:type:data and multiple options
may be listed using a comma delimiter.

 # ip link add name geneve0 type geneve dstport 0 external
 # tc qdisc add dev eth0 ingress
 # tc filter add dev eth0 protocol ip parent ffff: \
     flower indev eth0 \
        ip_proto udp \
        action tunnel_key \
            set src_ip 10.0.99.192 \
            dst_ip 10.0.99.193 \
            dst_port 6081 \
            id 11 \
            geneve_opts 0102:80:00800022,0102:80:00800022 \
    action mirred egress redirect dev geneve0

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/uapi/linux/tc_act/tc_tunnel_key.h |  26 ++++
 net/sched/act_tunnel_key.c                | 199 +++++++++++++++++++++++++++++-
 2 files changed, 220 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/tc_act/tc_tunnel_key.h b/include/uapi/linux/tc_act/tc_tunnel_key.h
index 72bbefe5d1d1..1b7bdd841b98 100644
--- a/include/uapi/linux/tc_act/tc_tunnel_key.h
+++ b/include/uapi/linux/tc_act/tc_tunnel_key.h
@@ -36,9 +36,35 @@ enum {
 	TCA_TUNNEL_KEY_PAD,
 	TCA_TUNNEL_KEY_ENC_DST_PORT,	/* be16 */
 	TCA_TUNNEL_KEY_NO_CSUM,		/* u8 */
+	TCA_TUNNEL_KEY_ENC_OPTS,	/* Nested TCA_TUNNEL_KEY_ENC_OPTS_
+					 * attributes
+					 */
 	__TCA_TUNNEL_KEY_MAX,
 };
 
 #define TCA_TUNNEL_KEY_MAX (__TCA_TUNNEL_KEY_MAX - 1)
 
+enum {
+	TCA_TUNNEL_KEY_ENC_OPTS_UNSPEC,
+	TCA_TUNNEL_KEY_ENC_OPTS_GENEVE,		/* Nested
+						 * TCA_TUNNEL_KEY_ENC_OPTS_
+						 * attributes
+						 */
+	__TCA_TUNNEL_KEY_ENC_OPTS_MAX,
+};
+
+#define TCA_TUNNEL_KEY_ENC_OPTS_MAX (__TCA_TUNNEL_KEY_ENC_OPTS_MAX - 1)
+
+enum {
+	TCA_TUNNEL_KEY_ENC_OPT_GENEVE_UNSPEC,
+	TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS,		/* u16 */
+	TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE,		/* u8 */
+	TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA,		/* 4 to 128 bytes */
+
+	__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX,
+};
+
+#define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \
+	(__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1)
+
 #endif
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index c0bbdec50022..8f3fa4c8dfaf 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
 #include <linux/rtnetlink.h>
+#include <net/geneve.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/dst.h>
@@ -57,6 +58,129 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
 	return action;
 }
 
+static const struct nla_policy
+enc_opts_policy[TCA_TUNNEL_KEY_ENC_OPTS_MAX + 1] = {
+	[TCA_TUNNEL_KEY_ENC_OPTS_GENEVE]	= { .type = NLA_NESTED },
+};
+
+static const struct nla_policy
+geneve_opt_policy[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1] = {
+	[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS]	   = { .type = NLA_U16 },
+	[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE]	   = { .type = NLA_U8 },
+	[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]	   = { .type = NLA_BINARY,
+						       .len = 128 },
+};
+
+static int
+tunnel_key_copy_geneve_opt(const struct nlattr *nla, int dst_len, void *dst,
+			   struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1];
+	int err, data_len, opt_len;
+	u8 *data;
+
+	err = nla_parse_nested(tb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX,
+			       nla, geneve_opt_policy, extack);
+	if (err < 0)
+		return err;
+
+	if (!tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS] ||
+	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE] ||
+	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]) {
+		NL_SET_ERR_MSG(extack, "Missing tunnel key enc geneve option class, type or data");
+		return -EINVAL;
+	}
+
+	data = nla_data(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
+	data_len = nla_len(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
+	if (data_len < 4) {
+		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is less than 4 bytes long");
+		return -ERANGE;
+	}
+	if (data_len % 4) {
+		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is not a multiple of 4 bytes long");
+		return -ERANGE;
+	}
+
+	opt_len = sizeof(struct geneve_opt) + data_len;
+	if (dst) {
+		struct geneve_opt *opt = dst;
+		u16 class;
+
+		if (dst_len < opt_len) {
+			NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data length is longer than the supplied data");
+			return -EINVAL;
+		}
+
+		class = nla_get_u16(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS]);
+		put_unaligned_be16(class, &opt->opt_class);
+
+		opt->type = nla_get_u8(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE]);
+		opt->length = data_len / 4; /* length is in units of 4 bytes */
+		opt->r1 = 0;
+		opt->r2 = 0;
+		opt->r3 = 0;
+
+		memcpy(opt + 1, data, data_len);
+	}
+
+	return opt_len;
+}
+
+static int tunnel_key_copy_opts(const struct nlattr *nla, u8 *dst,
+				int dst_len, struct netlink_ext_ack *extack)
+{
+	int err, rem, opt_len, len = nla_len(nla), opts_len = 0;
+	const struct nlattr *attr, *head = nla_data(nla);
+
+	err = nla_validate(head, len, TCA_TUNNEL_KEY_ENC_OPTS_MAX,
+			   enc_opts_policy, extack);
+	if (err)
+		return err;
+
+	nla_for_each_attr(attr, head, len, rem) {
+		switch (nla_type(attr)) {
+		case TCA_TUNNEL_KEY_ENC_OPTS_GENEVE:
+			opt_len = tunnel_key_copy_geneve_opt(attr, dst_len,
+							     dst, extack);
+			if (opt_len < 0)
+				return opt_len;
+			opts_len += opt_len;
+			if (dst) {
+				dst_len -= opt_len;
+				dst += opt_len;
+			}
+			break;
+		}
+	}
+
+	if (!opts_len) {
+		NL_SET_ERR_MSG(extack, "Empty list of tunnel options");
+		return -EINVAL;
+	}
+
+	if (rem > 0) {
+		NL_SET_ERR_MSG(extack, "Trailing data after parsing tunnel key enc options attributes");
+		return -EINVAL;
+	}
+
+	return opts_len;
+}
+
+static int tunnel_key_get_opts_len(struct nlattr *nla,
+				   struct netlink_ext_ack *extack)
+{
+	return tunnel_key_copy_opts(nla, NULL, 0, extack);
+}
+
+static int tunnel_key_opts_set(struct nlattr *nla, struct ip_tunnel_info *info,
+			       int opts_len, struct netlink_ext_ack *extack)
+{
+	info->options_len = opts_len;
+	return tunnel_key_copy_opts(nla, ip_tunnel_info_opts(info), opts_len,
+				    extack);
+}
+
 static const struct nla_policy tunnel_key_policy[TCA_TUNNEL_KEY_MAX + 1] = {
 	[TCA_TUNNEL_KEY_PARMS]	    = { .len = sizeof(struct tc_tunnel_key) },
 	[TCA_TUNNEL_KEY_ENC_IPV4_SRC] = { .type = NLA_U32 },
@@ -66,6 +190,7 @@ static const struct nla_policy tunnel_key_policy[TCA_TUNNEL_KEY_MAX + 1] = {
 	[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 },
+	[TCA_TUNNEL_KEY_ENC_OPTS]     = { .type = NLA_NESTED },
 };
 
 static int tunnel_key_init(struct net *net, struct nlattr *nla,
@@ -81,6 +206,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 	struct tcf_tunnel_key *t;
 	bool exists = false;
 	__be16 dst_port = 0;
+	int opts_len = 0;
 	__be64 key_id;
 	__be16 flags;
 	int ret = 0;
@@ -124,6 +250,15 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 		if (tb[TCA_TUNNEL_KEY_ENC_DST_PORT])
 			dst_port = nla_get_be16(tb[TCA_TUNNEL_KEY_ENC_DST_PORT]);
 
+		if (tb[TCA_TUNNEL_KEY_ENC_OPTS]) {
+			opts_len = tunnel_key_get_opts_len(tb[TCA_TUNNEL_KEY_ENC_OPTS],
+							   extack);
+			if (opts_len < 0) {
+				ret = opts_len;
+				goto err_out;
+			}
+		}
+
 		if (tb[TCA_TUNNEL_KEY_ENC_IPV4_SRC] &&
 		    tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]) {
 			__be32 saddr;
@@ -134,7 +269,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 
 			metadata = __ip_tun_set_dst(saddr, daddr, 0, 0,
 						    dst_port, flags,
-						    key_id, 0);
+						    key_id, opts_len);
 		} else if (tb[TCA_TUNNEL_KEY_ENC_IPV6_SRC] &&
 			   tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]) {
 			struct in6_addr saddr;
@@ -145,7 +280,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 
 			metadata = __ipv6_tun_set_dst(&saddr, &daddr, 0, 0, dst_port,
 						      0, flags,
-						      key_id, 0);
+						      key_id, opts_len);
 		} else {
 			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");
 			ret = -EINVAL;
@@ -157,6 +292,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 			goto err_out;
 		}
 
+		if (opts_len)
+			tunnel_key_opts_set(tb[TCA_TUNNEL_KEY_ENC_OPTS],
+					    &metadata->u.tun_info, opts_len,
+					    extack);
+
 		metadata->u.tun_info.mode |= IP_TUNNEL_INFO_TX;
 		break;
 	default:
@@ -221,6 +361,53 @@ static void tunnel_key_release(struct tc_action *a)
 	kfree_rcu(params, rcu);
 }
 
+static int tunnel_key_geneve_opts_dump(struct sk_buff *skb,
+				       const struct ip_tunnel_info *info)
+{
+	int len = info->options_len;
+	u8 *src = (u8 *)(info + 1);
+
+	while (len > 0) {
+		struct geneve_opt *opt = (struct geneve_opt *)src;
+		u16 class;
+
+		class = get_unaligned_be16(&opt->opt_class);
+		if (nla_put_u16(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS,
+				class) ||
+		    nla_put_u8(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE,
+			       opt->type) ||
+		    nla_put(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA,
+			    opt->length * 4, opt + 1))
+			return -EMSGSIZE;
+
+		len -= sizeof(struct geneve_opt) + opt->length * 4;
+		src += sizeof(struct geneve_opt) + opt->length * 4;
+	}
+
+	return 0;
+}
+
+static int tunnel_key_opts_dump(struct sk_buff *skb,
+				const struct ip_tunnel_info *info)
+{
+	struct nlattr *start;
+	int err;
+
+	if (!info->options_len)
+		return 0;
+
+	start = nla_nest_start(skb, TCA_TUNNEL_KEY_ENC_OPTS);
+	if (!start)
+		return -EMSGSIZE;
+
+	err = tunnel_key_geneve_opts_dump(skb, info);
+	if (err)
+		return err;
+
+	nla_nest_end(skb, start);
+	return 0;
+}
+
 static int tunnel_key_dump_addresses(struct sk_buff *skb,
 				     const struct ip_tunnel_info *info)
 {
@@ -271,8 +458,9 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
 		goto nla_put_failure;
 
 	if (params->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
-		struct ip_tunnel_key *key =
-			&params->tcft_enc_metadata->u.tun_info.key;
+		struct ip_tunnel_info *info =
+			&params->tcft_enc_metadata->u.tun_info;
+		struct ip_tunnel_key *key = &info->key;
 		__be32 key_id = tunnel_id_to_key32(key->tun_id);
 
 		if (nla_put_be32(skb, TCA_TUNNEL_KEY_ENC_KEY_ID, key_id) ||
@@ -280,7 +468,8 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
 					      &params->tcft_enc_metadata->u.tun_info) ||
 		    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)))
+			       !(key->tun_flags & TUNNEL_CSUM)) ||
+		    tunnel_key_opts_dump(skb, info))
 			goto nla_put_failure;
 	}
 
-- 
2.11.0

Re: [PATCH/RFC net-next 0/3] introduce Geneve options support in TC tunnel key

[Or Gerlitz]

On Tue, Mar 6, 2018 at 7:08 PM, Simon Horman <simon.horman@netronome.com> wrote:
> Hi all,
>
> This set adds Geneve options support to the TC tunnel key action.
> Options TLVs are exposed to userspace using a list of netlink attributes
> with each option described using attributes for class, type and data.

can you comment re what are the change vs your earlier post (will
appreciate an arch link to the thread..)

>
> The series also enhances the tunnel key action to use extended ack.
>
> Simon Horman (3):
>   net/sched: act_tunnel_key: disambiguate metadata dst error cases
>   net/sched: act_tunnel_key: add extended ack support
>   net/sched: add tunnel option support to act_tunnel_key
>
>  include/uapi/linux/tc_act/tc_tunnel_key.h |  26 ++++
>  net/sched/act_tunnel_key.c                | 212 ++++++++++++++++++++++++++++--
>  2 files changed, 230 insertions(+), 8 deletions(-)
>
> --
> 2.11.0
>

Re: [PATCH/RFC net-next 0/3] introduce Geneve options support in TC tunnel key

[Simon Horman]

On Tue, Mar 06, 2018 at 07:11:12PM +0200, Or Gerlitz wrote:
> On Tue, Mar 6, 2018 at 7:08 PM, Simon Horman <simon.horman@netronome.com> wrote:
> > Hi all,
> >
> > This set adds Geneve options support to the TC tunnel key action.
> > Options TLVs are exposed to userspace using a list of netlink attributes
> > with each option described using attributes for class, type and data.
> 
> can you comment re what are the change vs your earlier post (will
> appreciate an arch link to the thread..)

The main change is that the netlink interface exposes netlink attribute for
the class, type and data of each option, rather than a single attribute
that describes all options as a byte-string.

I believe the previous discussion is here:
https://www.spinics.net/lists/netdev/msg457058.html

Re: [PATCH/RFC 2/3] net/sched: act_tunnel_key: add extended ack support

[Jiri Benc]

On Tue,  6 Mar 2018 18:08:04 +0100, Simon Horman wrote:
> -	if (!tb[TCA_TUNNEL_KEY_PARMS])
> +	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
> +		NL_SET_ERR_MSG(extack, "Missing tunnel key parameter");

"parameters" (it's not just one parameter)

> @@ -107,6 +109,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  		break;
>  	case TCA_TUNNEL_KEY_ACT_SET:
>  		if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
> +			NL_SET_ERR_MSG(extack, "Missing tunnel key enc id");

This is not much helpful to the user. What's "enc"? I guess "Missing
tunnel key id" would be enough and better.

> @@ -144,6 +147,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  						      0, flags,
>  						      key_id, 0);
>  		} else {
> +			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");

We don't need both but only one of them. And again, "enc" does not have
a clear meaning.

"Missing either IPv4 or IPv6 source and destination address"?

In addition, it makes little sense to me to add extacks to just some of
the errors in the tunnel_key_init function. Please add extacks to all
of them.

Thanks,

 Jiri

Re: [PATCH/RFC 3/3] net/sched: add tunnel option support to act_tunnel_key

[Jiri Benc]

On Tue,  6 Mar 2018 18:08:05 +0100, Simon Horman wrote:
> +static int
> +tunnel_key_copy_geneve_opt(const struct nlattr *nla, int dst_len, void *dst,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct nlattr *tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1];
> +	int err, data_len, opt_len;
> +	u8 *data;
> +
> +	err = nla_parse_nested(tb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX,
> +			       nla, geneve_opt_policy, extack);
> +	if (err < 0)
> +		return err;
> +
> +	if (!tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS] ||
> +	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE] ||
> +	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]) {
> +		NL_SET_ERR_MSG(extack, "Missing tunnel key enc geneve option class, type or data");

I think it's obvious by now that I don't like the "enc" :-)

> +		return -EINVAL;
> +	}
> +
> +	data = nla_data(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
> +	data_len = nla_len(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
> +	if (data_len < 4) {
> +		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is less than 4 bytes long");
> +		return -ERANGE;
> +	}
> +	if (data_len % 4) {
> +		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is not a multiple of 4 bytes long");
> +		return -ERANGE;
> +	}
> +
> +	opt_len = sizeof(struct geneve_opt) + data_len;
> +	if (dst) {
> +		struct geneve_opt *opt = dst;
> +		u16 class;
> +
> +		if (dst_len < opt_len) {
> +			NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data length is longer than the supplied data");

I don't understand this error. What can the user do about it?
Furthermore, the error is not propagated to the user space (see also
below).

Shouldn't this be WARN_ON or something?

> +			return -EINVAL;
> +		}
> +
> +		class = nla_get_u16(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS]);
> +		put_unaligned_be16(class, &opt->opt_class);

How this can be unaligned, given we check that the option length is a
multiple of 4 bytes and the option header is 4 bytes?

> +
> +		opt->type = nla_get_u8(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE]);
> +		opt->length = data_len / 4; /* length is in units of 4 bytes */
> +		opt->r1 = 0;
> +		opt->r2 = 0;
> +		opt->r3 = 0;
> +
> +		memcpy(opt + 1, data, data_len);
> +	}
> +
> +	return opt_len;
> +}
> +
> +static int tunnel_key_copy_opts(const struct nlattr *nla, u8 *dst,
> +				int dst_len, struct netlink_ext_ack *extack)

Please be consistent with parameter ordering, dst and dst_len are in a
different order here and in tunnel_key_copy_geneve_opt.

[...]
> @@ -157,6 +292,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  			goto err_out;
>  		}
>  
> +		if (opts_len)
> +			tunnel_key_opts_set(tb[TCA_TUNNEL_KEY_ENC_OPTS],
> +					    &metadata->u.tun_info, opts_len,
> +					    extack);

You need to propagate the error here. The previous validation is not
enough as errors while copying to tun_info were not covered.

> +
>  		metadata->u.tun_info.mode |= IP_TUNNEL_INFO_TX;
>  		break;
>  	default:
> @@ -221,6 +361,53 @@ static void tunnel_key_release(struct tc_action *a)
>  	kfree_rcu(params, rcu);
>  }
>  
> +static int tunnel_key_geneve_opts_dump(struct sk_buff *skb,
> +				       const struct ip_tunnel_info *info)
> +{
> +	int len = info->options_len;
> +	u8 *src = (u8 *)(info + 1);
> +
> +	while (len > 0) {
> +		struct geneve_opt *opt = (struct geneve_opt *)src;
> +		u16 class;
> +
> +		class = get_unaligned_be16(&opt->opt_class);

I don't think this can be unaligned.

> +		if (nla_put_u16(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS,
> +				class) ||
> +		    nla_put_u8(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE,
> +			       opt->type) ||
> +		    nla_put(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA,
> +			    opt->length * 4, opt + 1))
> +			return -EMSGSIZE;
> +
> +		len -= sizeof(struct geneve_opt) + opt->length * 4;
> +		src += sizeof(struct geneve_opt) + opt->length * 4;
> +	}

All of this needs to be nested in TCA_TUNNEL_KEY_ENC_OPTS_GENEVE.

> +
> +	return 0;
> +}
> +
> +static int tunnel_key_opts_dump(struct sk_buff *skb,
> +				const struct ip_tunnel_info *info)
> +{
> +	struct nlattr *start;
> +	int err;
> +
> +	if (!info->options_len)
> +		return 0;
> +
> +	start = nla_nest_start(skb, TCA_TUNNEL_KEY_ENC_OPTS);
> +	if (!start)
> +		return -EMSGSIZE;
> +
> +	err = tunnel_key_geneve_opts_dump(skb, info);

How do you know it is geneve opts and not some other (future) tunnel
type options?

This is actually more fundamental problem with this patch. The
metadata_dst options are blindly set to the provided data, yet nowhere
we check whether the tunnel type matches. This must be done somehow. We
probably need to store the options type in metadata_dst.

Thanks,

 Jiri

Re: [PATCH/RFC 2/3] net/sched: act_tunnel_key: add extended ack support

[Simon Horman]

On Fri, Mar 09, 2018 at 12:22:48PM +0100, Jiri Benc wrote:
> On Tue,  6 Mar 2018 18:08:04 +0100, Simon Horman wrote:
> > -	if (!tb[TCA_TUNNEL_KEY_PARMS])
> > +	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
> > +		NL_SET_ERR_MSG(extack, "Missing tunnel key parameter");
> 
> "parameters" (it's not just one parameter)
> 
> > @@ -107,6 +109,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> >  		break;
> >  	case TCA_TUNNEL_KEY_ACT_SET:
> >  		if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
> > +			NL_SET_ERR_MSG(extack, "Missing tunnel key enc id");
> 
> This is not much helpful to the user. What's "enc"? I guess "Missing
> tunnel key id" would be enough and better.
> 
> > @@ -144,6 +147,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> >  						      0, flags,
> >  						      key_id, 0);
> >  		} else {
> > +			NL_SET_ERR_MSG(extack, "Missing both ipv4 and ipv6 enc src and dst");
> 
> We don't need both but only one of them. And again, "enc" does not have
> a clear meaning.
> 
> "Missing either IPv4 or IPv6 source and destination address"?

Sure, I'll work on making the messages clearer.

> In addition, it makes little sense to me to add extacks to just some of
> the errors in the tunnel_key_init function. Please add extacks to all
> of them.

At the time I wrote the patch I tried to cover those errors that could
result from user-input. I can extend the coverage if that is preferred.

Re: [PATCH/RFC 3/3] net/sched: add tunnel option support to act_tunnel_key

[Simon Horman]

On Fri, Mar 09, 2018 at 12:53:17PM +0100, Jiri Benc wrote:
> On Tue,  6 Mar 2018 18:08:05 +0100, Simon Horman wrote:
> > +static int
> > +tunnel_key_copy_geneve_opt(const struct nlattr *nla, int dst_len, void *dst,
> > +			   struct netlink_ext_ack *extack)
> > +{
> > +	struct nlattr *tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1];
> > +	int err, data_len, opt_len;
> > +	u8 *data;
> > +
> > +	err = nla_parse_nested(tb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX,
> > +			       nla, geneve_opt_policy, extack);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	if (!tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS] ||
> > +	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE] ||
> > +	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]) {
> > +		NL_SET_ERR_MSG(extack, "Missing tunnel key enc geneve option class, type or data");
> 
> I think it's obvious by now that I don't like the "enc" :-)
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	data = nla_data(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
> > +	data_len = nla_len(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
> > +	if (data_len < 4) {
> > +		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is less than 4 bytes long");
> > +		return -ERANGE;
> > +	}
> > +	if (data_len % 4) {
> > +		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is not a multiple of 4 bytes long");
> > +		return -ERANGE;
> > +	}
> > +
> > +	opt_len = sizeof(struct geneve_opt) + data_len;
> > +	if (dst) {
> > +		struct geneve_opt *opt = dst;
> > +		u16 class;
> > +
> > +		if (dst_len < opt_len) {
> > +			NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data length is longer than the supplied data");
> 
> I don't understand this error. What can the user do about it?
> Furthermore, the error is not propagated to the user space (see also
> below).
> 
> Shouldn't this be WARN_ON or something?

Sure, that is fine by me.

> 
> > +			return -EINVAL;
> > +		}
> > +
> > +		class = nla_get_u16(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS]);
> > +		put_unaligned_be16(class, &opt->opt_class);
> 
> How this can be unaligned, given we check that the option length is a
> multiple of 4 bytes and the option header is 4 bytes?

True.

> > +
> > +		opt->type = nla_get_u8(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE]);
> > +		opt->length = data_len / 4; /* length is in units of 4 bytes */
> > +		opt->r1 = 0;
> > +		opt->r2 = 0;
> > +		opt->r3 = 0;
> > +
> > +		memcpy(opt + 1, data, data_len);
> > +	}
> > +
> > +	return opt_len;
> > +}
> > +
> > +static int tunnel_key_copy_opts(const struct nlattr *nla, u8 *dst,
> > +				int dst_len, struct netlink_ext_ack *extack)
> 
> Please be consistent with parameter ordering, dst and dst_len are in a
> different order here and in tunnel_key_copy_geneve_opt.

Sure, will do.

> [...]
> > @@ -157,6 +292,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> >  			goto err_out;
> >  		}
> >  
> > +		if (opts_len)
> > +			tunnel_key_opts_set(tb[TCA_TUNNEL_KEY_ENC_OPTS],
> > +					    &metadata->u.tun_info, opts_len,
> > +					    extack);
> 
> You need to propagate the error here. The previous validation is not
> enough as errors while copying to tun_info were not covered.

Thanks, sorry for missing that.

> > +
> >  		metadata->u.tun_info.mode |= IP_TUNNEL_INFO_TX;
> >  		break;
> >  	default:
> > @@ -221,6 +361,53 @@ static void tunnel_key_release(struct tc_action *a)
> >  	kfree_rcu(params, rcu);
> >  }
> >  
> > +static int tunnel_key_geneve_opts_dump(struct sk_buff *skb,
> > +				       const struct ip_tunnel_info *info)
> > +{
> > +	int len = info->options_len;
> > +	u8 *src = (u8 *)(info + 1);
> > +
> > +	while (len > 0) {
> > +		struct geneve_opt *opt = (struct geneve_opt *)src;
> > +		u16 class;
> > +
> > +		class = get_unaligned_be16(&opt->opt_class);
> 
> I don't think this can be unaligned.

Thanks, I'm not sure why I thought otherwise.

> > +		if (nla_put_u16(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS,
> > +				class) ||
> > +		    nla_put_u8(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE,
> > +			       opt->type) ||
> > +		    nla_put(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA,
> > +			    opt->length * 4, opt + 1))
> > +			return -EMSGSIZE;
> > +
> > +		len -= sizeof(struct geneve_opt) + opt->length * 4;
> > +		src += sizeof(struct geneve_opt) + opt->length * 4;
> > +	}
> 
> All of this needs to be nested in TCA_TUNNEL_KEY_ENC_OPTS_GENEVE.

Agreed.

> > +
> > +	return 0;
> > +}
> > +
> > +static int tunnel_key_opts_dump(struct sk_buff *skb,
> > +				const struct ip_tunnel_info *info)
> > +{
> > +	struct nlattr *start;
> > +	int err;
> > +
> > +	if (!info->options_len)
> > +		return 0;
> > +
> > +	start = nla_nest_start(skb, TCA_TUNNEL_KEY_ENC_OPTS);
> > +	if (!start)
> > +		return -EMSGSIZE;
> > +
> > +	err = tunnel_key_geneve_opts_dump(skb, info);
> 
> How do you know it is geneve opts and not some other (future) tunnel
> type options?
> 
> This is actually more fundamental problem with this patch. The
> metadata_dst options are blindly set to the provided data, yet nowhere
> we check whether the tunnel type matches. This must be done somehow. We
> probably need to store the options type in metadata_dst.

Thanks for pointing that out. Its a pretty fundamental oversight.