[PATCH v2 net-next 0/2] net/sched: support tunnel options in cls_flower and act_tunnel_key

[PATCH v2 net-next 0/2] net/sched: support tunnel options in cls_flower and act_tunnel_key

[Simon Horman]

Allow the flower classifier to match on tunnel options and the
tunnel key action to set them.

Tunnel options are a bytestring of up to 256 bytes.
The flower classifier matching with an optional bitwise mask.
Tunnel implementations may support more or less options,
or none at all.


Discussion stemming from review of RFC:

This feature is to be used in conjunction with tunnels in collect metadata
(external) mode. As I understand it there are three tunnel netdevs that use
options metadata in the kernel at this time.

* Geneve

  In the case of Geneve options are TLVs[1]. My reading is that in collect
  metadata mode the kernel does not appear to do anything other than pass
  them around as a bytestring.

  [1] https://tools.ietf.org/html/draft-ietf-nvo3-geneve-05#section-3.5

* VXLAN-GBP

  In the case of VXLAN-GBP on RX in collect metadata mode options are used
  to carry information parsed in vxlan_parse_gbp_hdr() from the VXLAN Group
  Based Policy Extension[2]. On RX the options data is used to create an
  extension (header) by vxlan_build_gbp_hdr().

  [2] https://tools.ietf.org/html/draft-smith-vxlan-group-policy-03#section-2.1

* ERSPAN (GRE)

  In the case of ERSPAN, which is a variant of GRE, on RX in collect
  metadata mode options are used to carry the index parsed from the ERSPAN
  Type II feature header[3] in erspan_rcv().  The converse is true on TX
  and is handled by erspan_fb_xmit().

  [3] https://tools.ietf.org/html/draft-foschiano-erspan-03#section-4.2

Users of options:

* There are eBPF hooks to allow getting on and setting tunnel metadata:
  bpf_skb_set_tunnel_opt, bpf_skb_get_tunnel_opt.

* Open vSwitch is able to match and set Geneve and VXLAN-GBP options.

Neither of the above appear to assume any structure for the data.


Changes since RFC:
* Drop RFC prefix
* Correct changelogs and enhance cover letter.


Simon Horman (2):
  net/sched: add tunnel option support to act_tunnel_key
  net/sched: allow flower to match tunnel options

 include/net/flow_dissector.h              | 13 ++++++++++++
 include/uapi/linux/pkt_cls.h              |  3 +++
 include/uapi/linux/tc_act/tc_tunnel_key.h |  1 +
 net/sched/act_tunnel_key.c                | 26 ++++++++++++++++++-----
 net/sched/cls_flower.c                    | 35 ++++++++++++++++++++++++++++++-
 5 files changed, 72 insertions(+), 6 deletions(-)

-- 
2.1.4

[PATCH v2 net-next 1/2] net/sched: add tunnel option support to act_tunnel_key

[Simon Horman]

Allow setting tunnel options using the act_tunnel_key action.

Options are a bitwise maskable bytestring of up to 256 bytes.
Tunnel implementations may support less or more options,
or no options at all.

 # 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 \
            opts 0102800100800022 \
    action mirred egress redirect dev geneve0

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
v2
* Correct example which was incorrectly described matching rather
  than setting tunnel options
---
 include/uapi/linux/tc_act/tc_tunnel_key.h |  1 +
 net/sched/act_tunnel_key.c                | 26 +++++++++++++++++++++-----
 2 files changed, 22 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 afcd4be953e2..e0cb1121d132 100644
--- a/include/uapi/linux/tc_act/tc_tunnel_key.h
+++ b/include/uapi/linux/tc_act/tc_tunnel_key.h
@@ -35,6 +35,7 @@ enum {
 	TCA_TUNNEL_KEY_PAD,
 	TCA_TUNNEL_KEY_ENC_DST_PORT,	/* be16 */
 	TCA_TUNNEL_KEY_NO_CSUM,		/* u8 */
+	TCA_TUNNEL_KEY_ENC_OPTS,
 	__TCA_TUNNEL_KEY_MAX,
 };
 
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 30c96274c638..77b5890a48b9 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -66,6 +66,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_BINARY },
 };
 
 static int tunnel_key_init(struct net *net, struct nlattr *nla,
@@ -81,9 +82,11 @@ 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;
+	u8 *opts;
 	int err;
 
 	if (!nla)
@@ -121,6 +124,11 @@ 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 = nla_data(tb[TCA_TUNNEL_KEY_ENC_OPTS]);
+			opts_len = nla_len(tb[TCA_TUNNEL_KEY_ENC_OPTS]);
+		}
+
 		if (tb[TCA_TUNNEL_KEY_ENC_IPV4_SRC] &&
 		    tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]) {
 			__be32 saddr;
@@ -131,7 +139,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;
@@ -142,9 +150,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);
+						      key_id, opts_len);
 		}
 
+		if (opts_len)
+			ip_tunnel_info_opts_set(&metadata->u.tun_info,
+						opts, opts_len);
+
 		if (!metadata) {
 			ret = -EINVAL;
 			goto err_out;
@@ -264,8 +276,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) ||
@@ -273,7 +286,10 @@ 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)) ||
+		    (info->options_len &&
+		     nla_put(skb, TCA_TUNNEL_KEY_ENC_OPTS, info->options_len,
+			     info + 1)))
 			goto nla_put_failure;
 	}
 
-- 
2.1.4

[PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

Allow matching on options in tunnel headers.
This makes use of existing tunnel metadata support.

Options are a bytestring of up to 256 bytes.
Tunnel implementations may support less or more options,
or no options at all.

e.g.
 # ip link add name geneve0 type geneve dstport 0 external
 # tc qdisc add dev geneve0 ingress
 # tc filter add dev geneve0 protocol ip parent ffff: \
     flower \
       enc_src_ip 10.0.99.192 \
       enc_dst_ip 10.0.99.193 \
       enc_key_id 11 \
       enc_opts 0102800100800020/fffffffffffffff0 \
       ip_proto udp \
       action mirred egress redirect dev eth1

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

---
v2
* Correct example which was incorrectly described setting rather
  than matching tunnel options
---
 include/net/flow_dissector.h | 13 +++++++++++++
 include/uapi/linux/pkt_cls.h |  3 +++
 net/sched/cls_flower.c       | 35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index fc3dce730a6b..43f98bf0b349 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -183,6 +183,18 @@ struct flow_dissector_key_ip {
 	__u8	ttl;
 };
 
+/**
+ * struct flow_dissector_key_enc_opts:
+ * @data: data
+ * @len: len
+ */
+struct flow_dissector_key_enc_opts {
+	u8 data[256];	/* Using IP_TUNNEL_OPTS_MAX is desired here
+			 * but seems difficult to #include
+			 */
+	u8 len;
+};
+
 enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
 
 	FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index d5e2bf68d0d4..7a09a28f21e0 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -467,6 +467,9 @@ enum {
 	TCA_FLOWER_KEY_IP_TTL,		/* u8 */
 	TCA_FLOWER_KEY_IP_TTL_MASK,	/* u8 */
 
+	TCA_FLOWER_KEY_ENC_OPTS,
+	TCA_FLOWER_KEY_ENC_OPTS_MASK,
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index d230cb4c8094..e72a17c46f07 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -51,6 +51,7 @@ struct fl_flow_key {
 	struct flow_dissector_key_mpls mpls;
 	struct flow_dissector_key_tcp tcp;
 	struct flow_dissector_key_ip ip;
+	struct flow_dissector_key_enc_opts enc_opts;
 } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
 
 struct fl_flow_mask_range {
@@ -181,6 +182,11 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 		skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
 		skb_key.enc_tp.src = key->tp_src;
 		skb_key.enc_tp.dst = key->tp_dst;
+
+		if (info->options_len) {
+			skb_key.enc_opts.len = info->options_len;
+			ip_tunnel_info_opts_get(skb_key.enc_opts.data, info);
+		}
 	}
 
 	skb_key.indev_ifindex = skb->skb_iif;
@@ -421,6 +427,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_IP_TOS_MASK]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_IP_TTL]		= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_IP_TTL_MASK]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ENC_OPTS]	= { .type = NLA_BINARY },
+	[TCA_FLOWER_KEY_ENC_OPTS_MASK]	= { .type = NLA_BINARY },
 };
 
 static void fl_set_key_val(struct nlattr **tb,
@@ -712,6 +720,26 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		       &mask->enc_tp.dst, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
 		       sizeof(key->enc_tp.dst));
 
+	if (tb[TCA_FLOWER_KEY_ENC_OPTS]) {
+		key->enc_opts.len = nla_len(tb[TCA_FLOWER_KEY_ENC_OPTS]);
+
+		if (key->enc_opts.len > sizeof(key->enc_opts.data))
+			return -EINVAL;
+
+		/* enc_opts is variable length.
+		 * If present ensure the value and mask are the same length.
+		 */
+		if (tb[TCA_FLOWER_KEY_ENC_OPTS_MASK] &&
+		    nla_len(tb[TCA_FLOWER_KEY_ENC_OPTS_MASK]) != key->enc_opts.len)
+			return -EINVAL;
+
+		mask->enc_opts.len = key->enc_opts.len;
+		fl_set_key_val(tb, key->enc_opts.data, TCA_FLOWER_KEY_ENC_OPTS,
+			       mask->enc_opts.data,
+			       TCA_FLOWER_KEY_ENC_OPTS_MASK,
+			       key->enc_opts.len);
+	}
+
 	if (tb[TCA_FLOWER_KEY_FLAGS])
 		ret = fl_set_key_flags(tb, &key->control.flags, &mask->control.flags);
 
@@ -804,6 +832,8 @@ static void fl_init_dissector(struct cls_fl_head *head,
 			   enc_control);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_ENC_PORTS, enc_tp);
+	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
+			     FLOW_DISSECTOR_KEY_ENC_OPTS, enc_opts);
 
 	skb_flow_dissector_init(&head->dissector, keys, cnt);
 }
@@ -1330,7 +1360,10 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
 			    TCA_FLOWER_KEY_ENC_UDP_DST_PORT,
 			    &mask->enc_tp.dst,
 			    TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,
-			    sizeof(key->enc_tp.dst)))
+			    sizeof(key->enc_tp.dst)) ||
+	    fl_dump_key_val(skb, key->enc_opts.data, TCA_FLOWER_KEY_ENC_OPTS,
+			    mask->enc_opts.data, TCA_FLOWER_KEY_ENC_OPTS_MASK,
+			    key->enc_opts.len))
 		goto nla_put_failure;
 
 	if (fl_dump_key_flags(skb, key->control.flags, mask->control.flags))
-- 
2.1.4

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Jiri Pirko]

Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>Allow matching on options in tunnel headers.
>This makes use of existing tunnel metadata support.
>
>Options are a bytestring of up to 256 bytes.
>Tunnel implementations may support less or more options,
>or no options at all.
>
>e.g.
> # ip link add name geneve0 type geneve dstport 0 external
> # tc qdisc add dev geneve0 ingress
> # tc filter add dev geneve0 protocol ip parent ffff: \
>     flower \
>       enc_src_ip 10.0.99.192 \
>       enc_dst_ip 10.0.99.193 \
>       enc_key_id 11 \
>       enc_opts 0102800100800020/fffffffffffffff0 \
>       ip_proto udp \
>       action mirred egress redirect dev eth1
>
>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
>---
>v2
>* Correct example which was incorrectly described setting rather
>  than matching tunnel options
>---
> include/net/flow_dissector.h | 13 +++++++++++++
> include/uapi/linux/pkt_cls.h |  3 +++
> net/sched/cls_flower.c       | 35 ++++++++++++++++++++++++++++++++++-
> 3 files changed, 50 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index fc3dce730a6b..43f98bf0b349 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -183,6 +183,18 @@ struct flow_dissector_key_ip {
> 	__u8	ttl;
> };
> 
>+/**
>+ * struct flow_dissector_key_enc_opts:
>+ * @data: data
>+ * @len: len
>+ */
>+struct flow_dissector_key_enc_opts {
>+	u8 data[256];	/* Using IP_TUNNEL_OPTS_MAX is desired here
>+			 * but seems difficult to #include
>+			 */
>+	u8 len;
>+};
>+
> enum flow_dissector_key_id {
> 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */

I don't see the actual dissection implementation. Where is it?
Did you test the patchset?

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
> >Allow matching on options in tunnel headers.
> >This makes use of existing tunnel metadata support.
> >
> >Options are a bytestring of up to 256 bytes.
> >Tunnel implementations may support less or more options,
> >or no options at all.
> >
> >e.g.
> > # ip link add name geneve0 type geneve dstport 0 external
> > # tc qdisc add dev geneve0 ingress
> > # tc filter add dev geneve0 protocol ip parent ffff: \
> >     flower \
> >       enc_src_ip 10.0.99.192 \
> >       enc_dst_ip 10.0.99.193 \
> >       enc_key_id 11 \
> >       enc_opts 0102800100800020/fffffffffffffff0 \
> >       ip_proto udp \
> >       action mirred egress redirect dev eth1
> >
> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >
> >---
> >v2
> >* Correct example which was incorrectly described setting rather
> >  than matching tunnel options
> >---
> > include/net/flow_dissector.h | 13 +++++++++++++
> > include/uapi/linux/pkt_cls.h |  3 +++
> > net/sched/cls_flower.c       | 35 ++++++++++++++++++++++++++++++++++-
> > 3 files changed, 50 insertions(+), 1 deletion(-)
> >
> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >index fc3dce730a6b..43f98bf0b349 100644
> >--- a/include/net/flow_dissector.h
> >+++ b/include/net/flow_dissector.h
> >@@ -183,6 +183,18 @@ struct flow_dissector_key_ip {
> > 	__u8	ttl;
> > };
> > 
> >+/**
> >+ * struct flow_dissector_key_enc_opts:
> >+ * @data: data
> >+ * @len: len
> >+ */
> >+struct flow_dissector_key_enc_opts {
> >+	u8 data[256];	/* Using IP_TUNNEL_OPTS_MAX is desired here
> >+			 * but seems difficult to #include
> >+			 */
> >+	u8 len;
> >+};
> >+
> > enum flow_dissector_key_id {
> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
> 
> I don't see the actual dissection implementation. Where is it?
> Did you test the patchset?

Yes, I did test it. But it is also possible something went astray along the
way and I will retest.

I think that the code you are looking for is in
fl_classify() in this patch.

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Jiri Pirko]

Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
>On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>> >Allow matching on options in tunnel headers.
>> >This makes use of existing tunnel metadata support.
>> >
>> >Options are a bytestring of up to 256 bytes.
>> >Tunnel implementations may support less or more options,
>> >or no options at all.
>> >
>> >e.g.
>> > # ip link add name geneve0 type geneve dstport 0 external
>> > # tc qdisc add dev geneve0 ingress
>> > # tc filter add dev geneve0 protocol ip parent ffff: \
>> >     flower \
>> >       enc_src_ip 10.0.99.192 \
>> >       enc_dst_ip 10.0.99.193 \
>> >       enc_key_id 11 \
>> >       enc_opts 0102800100800020/fffffffffffffff0 \
>> >       ip_proto udp \
>> >       action mirred egress redirect dev eth1
>> >
>> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> >Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> >
>> >---
>> >v2
>> >* Correct example which was incorrectly described setting rather
>> >  than matching tunnel options
>> >---
>> > include/net/flow_dissector.h | 13 +++++++++++++
>> > include/uapi/linux/pkt_cls.h |  3 +++
>> > net/sched/cls_flower.c       | 35 ++++++++++++++++++++++++++++++++++-
>> > 3 files changed, 50 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>> >index fc3dce730a6b..43f98bf0b349 100644
>> >--- a/include/net/flow_dissector.h
>> >+++ b/include/net/flow_dissector.h
>> >@@ -183,6 +183,18 @@ struct flow_dissector_key_ip {
>> > 	__u8	ttl;
>> > };
>> > 
>> >+/**
>> >+ * struct flow_dissector_key_enc_opts:
>> >+ * @data: data
>> >+ * @len: len
>> >+ */
>> >+struct flow_dissector_key_enc_opts {
>> >+	u8 data[256];	/* Using IP_TUNNEL_OPTS_MAX is desired here
>> >+			 * but seems difficult to #include
>> >+			 */
>> >+	u8 len;
>> >+};
>> >+
>> > enum flow_dissector_key_id {
>> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
>> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
>> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
>> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
>> 
>> I don't see the actual dissection implementation. Where is it?
>> Did you test the patchset?
>
>Yes, I did test it. But it is also possible something went astray along the
>way and I will retest.
>
>I think that the code you are looking for is in
>fl_classify() in this patch.

The dissection should be done in the flow_dissector. That's the whole
point in having it generic. You should move it there.

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
> >> >Allow matching on options in tunnel headers.
> >> >This makes use of existing tunnel metadata support.
> >> >
> >> >Options are a bytestring of up to 256 bytes.
> >> >Tunnel implementations may support less or more options,
> >> >or no options at all.
> >> >
> >> >e.g.
> >> > # ip link add name geneve0 type geneve dstport 0 external
> >> > # tc qdisc add dev geneve0 ingress
> >> > # tc filter add dev geneve0 protocol ip parent ffff: \
> >> >     flower \
> >> >       enc_src_ip 10.0.99.192 \
> >> >       enc_dst_ip 10.0.99.193 \
> >> >       enc_key_id 11 \
> >> >       enc_opts 0102800100800020/fffffffffffffff0 \
> >> >       ip_proto udp \
> >> >       action mirred egress redirect dev eth1
> >> >
> >> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >> >Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >> >
> >> >---
> >> >v2
> >> >* Correct example which was incorrectly described setting rather
> >> >  than matching tunnel options
> >> >---
> >> > include/net/flow_dissector.h | 13 +++++++++++++
> >> > include/uapi/linux/pkt_cls.h |  3 +++
> >> > net/sched/cls_flower.c       | 35 ++++++++++++++++++++++++++++++++++-
> >> > 3 files changed, 50 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >> >index fc3dce730a6b..43f98bf0b349 100644
> >> >--- a/include/net/flow_dissector.h
> >> >+++ b/include/net/flow_dissector.h
> >> >@@ -183,6 +183,18 @@ struct flow_dissector_key_ip {
> >> > 	__u8	ttl;
> >> > };
> >> > 
> >> >+/**
> >> >+ * struct flow_dissector_key_enc_opts:
> >> >+ * @data: data
> >> >+ * @len: len
> >> >+ */
> >> >+struct flow_dissector_key_enc_opts {
> >> >+	u8 data[256];	/* Using IP_TUNNEL_OPTS_MAX is desired here
> >> >+			 * but seems difficult to #include
> >> >+			 */
> >> >+	u8 len;
> >> >+};
> >> >+
> >> > enum flow_dissector_key_id {
> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
> >> 
> >> I don't see the actual dissection implementation. Where is it?
> >> Did you test the patchset?
> >
> >Yes, I did test it. But it is also possible something went astray along the
> >way and I will retest.
> >
> >I think that the code you are looking for is in
> >fl_classify() in this patch.
> 
> The dissection should be done in the flow_dissector. That's the whole
> point in having it generic. You should move it there.
> 

Thanks, will do.

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:

...

> >> > enum flow_dissector_key_id {
> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
> >> 
> >> I don't see the actual dissection implementation. Where is it?
> >> Did you test the patchset?
> >
> >Yes, I did test it. But it is also possible something went astray along the
> >way and I will retest.
> >
> >I think that the code you are looking for is in
> >fl_classify() in this patch.
> 
> The dissection should be done in the flow_dissector. That's the whole
> point in having it generic. You should move it there.

Coming back to this after lunch, I believe what I have done in this patch
is consistent with handling of other enc fields, which are set in
fl_classify() rather than the dissector. In particular the ip_tunnel_info,
which is used by this patch, is already used in fl_classify().

Without this patch I see:


static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
                       struct tcf_result *res)
{
        ...
        struct ip_tunnel_info *info;

        ...

        info = skb_tunnel_info(skb);
        if (info) {
                struct ip_tunnel_key *key = &info->key;

                switch (ip_tunnel_info_af(info)) {
                case AF_INET:
                        skb_key.enc_control.addr_type =
                                FLOW_DISSECTOR_KEY_IPV4_ADDRS;
                        skb_key.enc_ipv4.src = key->u.ipv4.src;
                        skb_key.enc_ipv4.dst = key->u.ipv4.dst;
                        break;
                case AF_INET6:
                        skb_key.enc_control.addr_type =
                                FLOW_DISSECTOR_KEY_IPV6_ADDRS;
                        skb_key.enc_ipv6.src = key->u.ipv6.src;
                        skb_key.enc_ipv6.dst = key->u.ipv6.dst;
                        break;
                }

                skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
                skb_key.enc_tp.src = key->tp_src;
                skb_key.enc_tp.dst = key->tp_dst;
        }

	...
}

This patch adds the following inside the if() clause above:

	if (info->options_len) {
		skb_key.enc_opts.len = info->options_len;
		ip_tunnel_info_opts_get(skb_key.enc_opts.data, info);
	}

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Jiri Pirko]

Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
>On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
>> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
>> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>
>...
>
>> >> > enum flow_dissector_key_id {
>> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
>> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
>> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
>> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
>> >> 
>> >> I don't see the actual dissection implementation. Where is it?
>> >> Did you test the patchset?
>> >
>> >Yes, I did test it. But it is also possible something went astray along the
>> >way and I will retest.
>> >
>> >I think that the code you are looking for is in
>> >fl_classify() in this patch.
>> 
>> The dissection should be done in the flow_dissector. That's the whole
>> point in having it generic. You should move it there.
>
>Coming back to this after lunch, I believe what I have done in this patch
>is consistent with handling of other enc fields, which are set in
>fl_classify() rather than the dissector. In particular the ip_tunnel_info,
>which is used by this patch, is already used in fl_classify().

That means the current code is wrong. The dissection should be done in
flow_dissector, not in fl_classify.



>
>Without this patch I see:
>
>
>static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>                       struct tcf_result *res)
>{
>        ...
>        struct ip_tunnel_info *info;
>
>        ...
>
>        info = skb_tunnel_info(skb);
>        if (info) {
>                struct ip_tunnel_key *key = &info->key;
>
>                switch (ip_tunnel_info_af(info)) {
>                case AF_INET:
>                        skb_key.enc_control.addr_type =
>                                FLOW_DISSECTOR_KEY_IPV4_ADDRS;
>                        skb_key.enc_ipv4.src = key->u.ipv4.src;
>                        skb_key.enc_ipv4.dst = key->u.ipv4.dst;
>                        break;
>                case AF_INET6:
>                        skb_key.enc_control.addr_type =
>                                FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>                        skb_key.enc_ipv6.src = key->u.ipv6.src;
>                        skb_key.enc_ipv6.dst = key->u.ipv6.dst;
>                        break;
>                }
>
>                skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
>                skb_key.enc_tp.src = key->tp_src;
>                skb_key.enc_tp.dst = key->tp_dst;
>        }
>
>	...
>}
>
>This patch adds the following inside the if() clause above:
>
>	if (info->options_len) {
>		skb_key.enc_opts.len = info->options_len;
>		ip_tunnel_info_opts_get(skb_key.enc_opts.data, info);
>	}
>
>

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
> >
> >...
> >
> >> >> > enum flow_dissector_key_id {
> >> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> >> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> >> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> >> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
> >> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
> >> >> 
> >> >> I don't see the actual dissection implementation. Where is it?
> >> >> Did you test the patchset?
> >> >
> >> >Yes, I did test it. But it is also possible something went astray along the
> >> >way and I will retest.
> >> >
> >> >I think that the code you are looking for is in
> >> >fl_classify() in this patch.
> >> 
> >> The dissection should be done in the flow_dissector. That's the whole
> >> point in having it generic. You should move it there.
> >
> >Coming back to this after lunch, I believe what I have done in this patch
> >is consistent with handling of other enc fields, which are set in
> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
> >which is used by this patch, is already used in fl_classify().
> 
> That means the current code is wrong. The dissection should be done in
> flow_dissector, not in fl_classify.

Would an better approach be to move the fl_classify() below into, say,
skb_flow_dissect_tunnel_info() and call that from fl_classify().

The reason I suggest this rather than moving the code into
__skb_flow_dissect() is that currently flower assumes that tunnel_info
is used if present. While I assume other users of () assume tunnel_info
is not used even if present.

> >Without this patch I see:
> >
> >
> >static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> >                       struct tcf_result *res)
> >{
> >        ...
> >        struct ip_tunnel_info *info;
> >
> >        ...
> >
> >        info = skb_tunnel_info(skb);
> >        if (info) {
> >                struct ip_tunnel_key *key = &info->key;
> >
> >                switch (ip_tunnel_info_af(info)) {
> >                case AF_INET:
> >                        skb_key.enc_control.addr_type =
> >                                FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> >                        skb_key.enc_ipv4.src = key->u.ipv4.src;
> >                        skb_key.enc_ipv4.dst = key->u.ipv4.dst;
> >                        break;
> >                case AF_INET6:
> >                        skb_key.enc_control.addr_type =
> >                                FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> >                        skb_key.enc_ipv6.src = key->u.ipv6.src;
> >                        skb_key.enc_ipv6.dst = key->u.ipv6.dst;
> >                        break;
> >                }
> >
> >                skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
> >                skb_key.enc_tp.src = key->tp_src;
> >                skb_key.enc_tp.dst = key->tp_dst;
> >        }
> >
> >	...
> >}
> >
> >This patch adds the following inside the if() clause above:
> >
> >	if (info->options_len) {
> >		skb_key.enc_opts.len = info->options_len;
> >		ip_tunnel_info_opts_get(skb_key.enc_opts.data, info);
> >	}
> >
> >

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Jiri Pirko]

Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@netronome.com wrote:
>On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
>> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
>> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
>> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
>> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>> >
>> >...
>> >
>> >> >> > enum flow_dissector_key_id {
>> >> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>> >> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
>> >> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
>> >> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
>> >> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>> >> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
>> >> >> 
>> >> >> I don't see the actual dissection implementation. Where is it?
>> >> >> Did you test the patchset?
>> >> >
>> >> >Yes, I did test it. But it is also possible something went astray along the
>> >> >way and I will retest.
>> >> >
>> >> >I think that the code you are looking for is in
>> >> >fl_classify() in this patch.
>> >> 
>> >> The dissection should be done in the flow_dissector. That's the whole
>> >> point in having it generic. You should move it there.
>> >
>> >Coming back to this after lunch, I believe what I have done in this patch
>> >is consistent with handling of other enc fields, which are set in
>> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
>> >which is used by this patch, is already used in fl_classify().
>> 
>> That means the current code is wrong. The dissection should be done in
>> flow_dissector, not in fl_classify.
>
>Would an better approach be to move the fl_classify() below into, say,
>skb_flow_dissect_tunnel_info() and call that from fl_classify().

No. There is one flow dissection function and you just set it up in a
way you need it. Makes no sense to me to split it up in any way.


>
>The reason I suggest this rather than moving the code into
>__skb_flow_dissect() is that currently flower assumes that tunnel_info
>is used if present. While I assume other users of () assume tunnel_info
>is not used even if present.

__skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
only in case it is needed.


>
>> >Without this patch I see:
>> >
>> >
>> >static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>> >                       struct tcf_result *res)
>> >{
>> >        ...
>> >        struct ip_tunnel_info *info;
>> >
>> >        ...
>> >
>> >        info = skb_tunnel_info(skb);
>> >        if (info) {
>> >                struct ip_tunnel_key *key = &info->key;
>> >
>> >                switch (ip_tunnel_info_af(info)) {
>> >                case AF_INET:
>> >                        skb_key.enc_control.addr_type =
>> >                                FLOW_DISSECTOR_KEY_IPV4_ADDRS;
>> >                        skb_key.enc_ipv4.src = key->u.ipv4.src;
>> >                        skb_key.enc_ipv4.dst = key->u.ipv4.dst;
>> >                        break;
>> >                case AF_INET6:
>> >                        skb_key.enc_control.addr_type =
>> >                                FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>> >                        skb_key.enc_ipv6.src = key->u.ipv6.src;
>> >                        skb_key.enc_ipv6.dst = key->u.ipv6.dst;
>> >                        break;
>> >                }
>> >
>> >                skb_key.enc_key_id.keyid = tunnel_id_to_key32(key->tun_id);
>> >                skb_key.enc_tp.src = key->tp_src;
>> >                skb_key.enc_tp.dst = key->tp_dst;
>> >        }
>> >
>> >	...
>> >}
>> >
>> >This patch adds the following inside the if() clause above:
>> >
>> >	if (info->options_len) {
>> >		skb_key.enc_opts.len = info->options_len;
>> >		ip_tunnel_info_opts_get(skb_key.enc_opts.data, info);
>> >	}
>> >
>> >

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

On Wed, Sep 27, 2017 at 03:47:50PM +0200, Jiri Pirko wrote:
> Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@netronome.com wrote:
> >On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
> >> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
> >> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
> >> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
> >> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
> >> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
> >> >
> >> >...
> >> >
> >> >> >> > enum flow_dissector_key_id {
> >> >> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >> >> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> >> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> >> >> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> >> >> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> >> >> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
> >> >> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
> >> >> >> 
> >> >> >> I don't see the actual dissection implementation. Where is it?
> >> >> >> Did you test the patchset?
> >> >> >
> >> >> >Yes, I did test it. But it is also possible something went astray along the
> >> >> >way and I will retest.
> >> >> >
> >> >> >I think that the code you are looking for is in
> >> >> >fl_classify() in this patch.
> >> >> 
> >> >> The dissection should be done in the flow_dissector. That's the whole
> >> >> point in having it generic. You should move it there.
> >> >
> >> >Coming back to this after lunch, I believe what I have done in this patch
> >> >is consistent with handling of other enc fields, which are set in
> >> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
> >> >which is used by this patch, is already used in fl_classify().
> >> 
> >> That means the current code is wrong. The dissection should be done in
> >> flow_dissector, not in fl_classify.
> >
> >Would an better approach be to move the fl_classify() below into, say,
> >skb_flow_dissect_tunnel_info() and call that from fl_classify().
> 
> No. There is one flow dissection function and you just set it up in a
> way you need it. Makes no sense to me to split it up in any way.
> 
> 
> >
> >The reason I suggest this rather than moving the code into
> >__skb_flow_dissect() is that currently flower assumes that tunnel_info
> >is used if present. While I assume other users of () assume tunnel_info
> >is not used even if present.
> 
> __skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
> only in case it is needed.

Ok, do you think it is sufficient for __skb_flow_dissect to look at the
tunnel keys, say FLOW_DISSECTOR_KEY_ENC_*? I am a bit concerned this may
break flower as it look at the tunnel info unconditionally.

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Jiri Pirko]

Wed, Sep 27, 2017 at 03:50:44PM CEST, simon.horman@netronome.com wrote:
>On Wed, Sep 27, 2017 at 03:47:50PM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@netronome.com wrote:
>> >On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
>> >> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
>> >> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
>> >> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
>> >> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
>> >> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>> >> >
>> >> >...
>> >> >
>> >> >> >> > enum flow_dissector_key_id {
>> >> >> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>> >> >> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> >> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
>> >> >> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
>> >> >> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
>> >> >> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>> >> >> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
>> >> >> >> 
>> >> >> >> I don't see the actual dissection implementation. Where is it?
>> >> >> >> Did you test the patchset?
>> >> >> >
>> >> >> >Yes, I did test it. But it is also possible something went astray along the
>> >> >> >way and I will retest.
>> >> >> >
>> >> >> >I think that the code you are looking for is in
>> >> >> >fl_classify() in this patch.
>> >> >> 
>> >> >> The dissection should be done in the flow_dissector. That's the whole
>> >> >> point in having it generic. You should move it there.
>> >> >
>> >> >Coming back to this after lunch, I believe what I have done in this patch
>> >> >is consistent with handling of other enc fields, which are set in
>> >> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
>> >> >which is used by this patch, is already used in fl_classify().
>> >> 
>> >> That means the current code is wrong. The dissection should be done in
>> >> flow_dissector, not in fl_classify.
>> >
>> >Would an better approach be to move the fl_classify() below into, say,
>> >skb_flow_dissect_tunnel_info() and call that from fl_classify().
>> 
>> No. There is one flow dissection function and you just set it up in a
>> way you need it. Makes no sense to me to split it up in any way.
>> 
>> 
>> >
>> >The reason I suggest this rather than moving the code into
>> >__skb_flow_dissect() is that currently flower assumes that tunnel_info
>> >is used if present. While I assume other users of () assume tunnel_info
>> >is not used even if present.
>> 
>> __skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
>> only in case it is needed.
>
>Ok, do you think it is sufficient for __skb_flow_dissect to look at the
>tunnel keys, say FLOW_DISSECTOR_KEY_ENC_*? I am a bit concerned this may
>break flower as it look at the tunnel info unconditionally.

yeah. When flower needs that, it will get that from the flow dissector.
I don't see why it would break anything. Again, existing code is wrong:
commit bc3103f1ed405de587fa43d8b0671e615505a700
Author: Amir Vadai <amir@vadai.me>
Date:   Thu Sep 8 16:23:47 2016 +0300

    net/sched: cls_flower: Classify packet in ip tunnels

The dissection has to be moved to flow dissector.

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Simon Horman]

On Wed, Sep 27, 2017 at 04:00:11PM +0200, Jiri Pirko wrote:
> Wed, Sep 27, 2017 at 03:50:44PM CEST, simon.horman@netronome.com wrote:
> >On Wed, Sep 27, 2017 at 03:47:50PM +0200, Jiri Pirko wrote:
> >> Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@netronome.com wrote:
> >> >On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
> >> >> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
> >> >> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
> >> >> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
> >> >> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
> >> >> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
> >> >> >
> >> >> >...
> >> >> >
> >> >> >> >> > enum flow_dissector_key_id {
> >> >> >> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >> >> >> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> >> >> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
> >> >> >> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
> >> >> >> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
> >> >> >> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
> >> >> >> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
> >> >> >> >> 
> >> >> >> >> I don't see the actual dissection implementation. Where is it?
> >> >> >> >> Did you test the patchset?
> >> >> >> >
> >> >> >> >Yes, I did test it. But it is also possible something went astray along the
> >> >> >> >way and I will retest.
> >> >> >> >
> >> >> >> >I think that the code you are looking for is in
> >> >> >> >fl_classify() in this patch.
> >> >> >> 
> >> >> >> The dissection should be done in the flow_dissector. That's the whole
> >> >> >> point in having it generic. You should move it there.
> >> >> >
> >> >> >Coming back to this after lunch, I believe what I have done in this patch
> >> >> >is consistent with handling of other enc fields, which are set in
> >> >> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
> >> >> >which is used by this patch, is already used in fl_classify().
> >> >> 
> >> >> That means the current code is wrong. The dissection should be done in
> >> >> flow_dissector, not in fl_classify.
> >> >
> >> >Would an better approach be to move the fl_classify() below into, say,
> >> >skb_flow_dissect_tunnel_info() and call that from fl_classify().
> >> 
> >> No. There is one flow dissection function and you just set it up in a
> >> way you need it. Makes no sense to me to split it up in any way.
> >> 
> >> 
> >> >
> >> >The reason I suggest this rather than moving the code into
> >> >__skb_flow_dissect() is that currently flower assumes that tunnel_info
> >> >is used if present. While I assume other users of () assume tunnel_info
> >> >is not used even if present.
> >> 
> >> __skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
> >> only in case it is needed.
> >
> >Ok, do you think it is sufficient for __skb_flow_dissect to look at the
> >tunnel keys, say FLOW_DISSECTOR_KEY_ENC_*? I am a bit concerned this may
> >break flower as it look at the tunnel info unconditionally.
> 
> yeah. When flower needs that, it will get that from the flow dissector.
> I don't see why it would break anything. Again, existing code is wrong:

I understand that you think the existing code is wrong.
But I also want to try not to add new bugs.

I am concerned about the case where none of FLOW_DISSECTOR_KEY_ENC_* are
set but flower currently dissects the tunnel info anyway. If I make
dissection of tunnel info dependent on FLOW_DISSECTOR_KEY_ENC_*
that may change things.

Re: [PATCH v2 net-next 2/2] net/sched: allow flower to match tunnel options

[Jiri Pirko]

Wed, Sep 27, 2017 at 04:09:54PM CEST, simon.horman@netronome.com wrote:
>On Wed, Sep 27, 2017 at 04:00:11PM +0200, Jiri Pirko wrote:
>> Wed, Sep 27, 2017 at 03:50:44PM CEST, simon.horman@netronome.com wrote:
>> >On Wed, Sep 27, 2017 at 03:47:50PM +0200, Jiri Pirko wrote:
>> >> Wed, Sep 27, 2017 at 03:37:33PM CEST, simon.horman@netronome.com wrote:
>> >> >On Wed, Sep 27, 2017 at 02:56:03PM +0200, Jiri Pirko wrote:
>> >> >> Wed, Sep 27, 2017 at 02:52:06PM CEST, simon.horman@netronome.com wrote:
>> >> >> >On Wed, Sep 27, 2017 at 01:08:22PM +0200, Jiri Pirko wrote:
>> >> >> >> Wed, Sep 27, 2017 at 11:27:33AM CEST, simon.horman@netronome.com wrote:
>> >> >> >> >On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
>> >> >> >> >> Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
>> >> >> >
>> >> >> >...
>> >> >> >
>> >> >> >> >> > enum flow_dissector_key_id {
>> >> >> >> >> > 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>> >> >> >> >> > 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
>> >> >> >> >> >@@ -205,6 +217,7 @@ enum flow_dissector_key_id {
>> >> >> >> >> > 	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
>> >> >> >> >> > 	FLOW_DISSECTOR_KEY_TCP, /* struct flow_dissector_key_tcp */
>> >> >> >> >> > 	FLOW_DISSECTOR_KEY_IP, /* struct flow_dissector_key_ip */
>> >> >> >> >> >+	FLOW_DISSECTOR_KEY_ENC_OPTS, /* struct flow_dissector_key_enc_opts */
>> >> >> >> >> 
>> >> >> >> >> I don't see the actual dissection implementation. Where is it?
>> >> >> >> >> Did you test the patchset?
>> >> >> >> >
>> >> >> >> >Yes, I did test it. But it is also possible something went astray along the
>> >> >> >> >way and I will retest.
>> >> >> >> >
>> >> >> >> >I think that the code you are looking for is in
>> >> >> >> >fl_classify() in this patch.
>> >> >> >> 
>> >> >> >> The dissection should be done in the flow_dissector. That's the whole
>> >> >> >> point in having it generic. You should move it there.
>> >> >> >
>> >> >> >Coming back to this after lunch, I believe what I have done in this patch
>> >> >> >is consistent with handling of other enc fields, which are set in
>> >> >> >fl_classify() rather than the dissector. In particular the ip_tunnel_info,
>> >> >> >which is used by this patch, is already used in fl_classify().
>> >> >> 
>> >> >> That means the current code is wrong. The dissection should be done in
>> >> >> flow_dissector, not in fl_classify.
>> >> >
>> >> >Would an better approach be to move the fl_classify() below into, say,
>> >> >skb_flow_dissect_tunnel_info() and call that from fl_classify().
>> >> 
>> >> No. There is one flow dissection function and you just set it up in a
>> >> way you need it. Makes no sense to me to split it up in any way.
>> >> 
>> >> 
>> >> >
>> >> >The reason I suggest this rather than moving the code into
>> >> >__skb_flow_dissect() is that currently flower assumes that tunnel_info
>> >> >is used if present. While I assume other users of () assume tunnel_info
>> >> >is not used even if present.
>> >> 
>> >> __skb_flow_dissect should look at what caller wants, then check skb_tunnel_info
>> >> only in case it is needed.
>> >
>> >Ok, do you think it is sufficient for __skb_flow_dissect to look at the
>> >tunnel keys, say FLOW_DISSECTOR_KEY_ENC_*? I am a bit concerned this may
>> >break flower as it look at the tunnel info unconditionally.
>> 
>> yeah. When flower needs that, it will get that from the flow dissector.
>> I don't see why it would break anything. Again, existing code is wrong:
>
>I understand that you think the existing code is wrong.
>But I also want to try not to add new bugs.
>
>I am concerned about the case where none of FLOW_DISSECTOR_KEY_ENC_* are
>set but flower currently dissects the tunnel info anyway. If I make
>dissection of tunnel info dependent on FLOW_DISSECTOR_KEY_ENC_*
>that may change things.

If none of FLOW_DISSECTOR_KEY_ENC_* are set, flower does not care about
the fields and therefore they are masked out by fl_set_masked_key.

Otherwise it would be a bug is flower would match on something user did
not specify.

Re: [PATCH v2 net-next 0/2] net/sched: support tunnel options in cls_flower and act_tunnel_key

[Jiri Benc]

On Wed, 27 Sep 2017 10:16:32 +0200, Simon Horman wrote:
> * Geneve
> 
>   In the case of Geneve options are TLVs[1]. My reading is that in collect
>   metadata mode the kernel does not appear to do anything other than pass
>   them around as a bytestring.
> 
>   [1] https://tools.ietf.org/html/draft-ietf-nvo3-geneve-05#section-3.5
[...]
> 
> Neither of the above appear to assume any structure for the data.

But that's not true. Geneve uses TLVs, you even mentioned that
yourself. Matching on a block of TLVs as a bytestring doesn't make
sense. The TLV fields may be in any order.

We need better matching here. Bytestring is useless for Geneve.

NACK for this direction of option matching. We'd need to introduce
matching on TLVs sooner or later anyway and this would be just a never
used compat cruft that we need to keep around forever.

 Jiri

Re: [PATCH v2 net-next 0/2] net/sched: support tunnel options in cls_flower and act_tunnel_key

[David Miller]

From: Simon Horman <simon.horman@netronome.com>
Date: Wed, 27 Sep 2017 10:16:32 +0200

> Users of options:
> 
> * There are eBPF hooks to allow getting on and setting tunnel metadata:
>   bpf_skb_set_tunnel_opt, bpf_skb_get_tunnel_opt.
> 
> * Open vSwitch is able to match and set Geneve and VXLAN-GBP options.
> 
> Neither of the above appear to assume any structure for the data.

I really worry about this.

These metadata option blobs are internal kernel datastructure which we
could change at any point in time.  They are not exported to
userspace as a UAPI.

It's kinda OK for eBPF programs to access this stuff since they are
expected to cope with changes to internal data-structures.

But for anything user facing, this really doesn't work.

Re: [PATCH v2 net-next 0/2] net/sched: support tunnel options in cls_flower and act_tunnel_key

[Simon Horman]

On Fri, Sep 29, 2017 at 05:54:23AM +0100, David Miller wrote:
> From: Simon Horman <simon.horman@netronome.com>
> Date: Wed, 27 Sep 2017 10:16:32 +0200
> 
> > Users of options:
> > 
> > * There are eBPF hooks to allow getting on and setting tunnel metadata:
> >   bpf_skb_set_tunnel_opt, bpf_skb_get_tunnel_opt.
> > 
> > * Open vSwitch is able to match and set Geneve and VXLAN-GBP options.
> > 
> > Neither of the above appear to assume any structure for the data.
> 
> I really worry about this.
> 
> These metadata option blobs are internal kernel datastructure which we
> could change at any point in time.  They are not exported to
> userspace as a UAPI.
> 
> It's kinda OK for eBPF programs to access this stuff since they are
> expected to cope with changes to internal data-structures.
> 
> But for anything user facing, this really doesn't work.

Hi Dave, Hi Jiri,

the feedback I got from Jiri is that there needs to be some exposure
of TLVs. What I have in mind is to describe Geneve option TLVs in the
UAPI and for the kernel - most likely cls_flower, possibly using helpers,
to translate between that encoding and the one used internally by the kernel
- which currently happens to be the on-the-wire format.

I believe that in order to avoid per-packet overhead and at the same time
code complexity the TLVs should be described in-order. So matching on
TLV-A,TLV-B,TLV-C would be a different match to TLV-C,TLV-A,TLV-B.  An
order-independent match could be added if desired in future.

This would mean the feature is initially restricted to Geneve but could
be expended to offer a similar feature for other encapsulation protocols
as the need arises.

Would this address your concerns?

Re: [PATCH v2 net-next 0/2] net/sched: support tunnel options in cls_flower and act_tunnel_key

[Jiri Benc]

On Mon, 2 Oct 2017 09:50:15 +0200, Simon Horman wrote:
> I believe that in order to avoid per-packet overhead and at the same time
> code complexity the TLVs should be described in-order. So matching on
> TLV-A,TLV-B,TLV-C would be a different match to TLV-C,TLV-A,TLV-B.  An
> order-independent match could be added if desired in future.

Although better than the binary format, I doubt that it would be
useful. I can't imagine a real use case where you would want such match.

Instead, what you want is a match on a particular TLV, wherever it is
in the data. For start, we can support just a single TLV.

I.e. when matching on TLV-A, all of these would match:
TLV-A,TLV-B,TLV-C; TLV-B,TLV-A,TLV-C; TLV-B,TLV-C,TLV-A. And this one
won't match: TLV-B,TLV-C,TLV-D.

 Jiri