From: Simon Horman <hidden> Date: 2017-09-27 08:16:48
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
From: Simon Horman <hidden> Date: 2017-09-27 08:16:51
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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
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(-)
From: Simon Horman <hidden> Date: 2017-09-27 08:16:56
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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
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(-)
@@ -51,6 +51,7 @@ struct fl_flow_key {structflow_dissector_key_mplsmpls;structflow_dissector_key_tcptcp;structflow_dissector_key_ipip;+structflow_dissector_key_enc_optsenc_opts;}__aligned(BITS_PER_LONG/8);/* Ensure that we can do comparisons as longs. */structfl_flow_mask_range{
Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
quoted hunk
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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
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(-)
From: Simon Horman <hidden> Date: 2017-09-27 09:27:39
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:
quoted
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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
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(-)
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.
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:
quoted
Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
quoted
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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
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(-)
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.
From: Simon Horman <hidden> Date: 2017-09-27 11:54:23
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:
quoted
On Wed, Sep 27, 2017 at 11:10:05AM +0200, Jiri Pirko wrote:
quoted
Wed, Sep 27, 2017 at 10:16:34AM CEST, simon.horman@netronome.com wrote:
quoted
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 <redacted>
Reviewed-by: Jakub Kicinski <redacted>
---
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(-)
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.
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);
}
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);
}
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.
quoted
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);
}
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.
quoted
quoted
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);
}
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.
quoted
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.
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.
quoted
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 [off-list ref]
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.
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.
quoted
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.
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.
quoted
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.
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
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.
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?
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