| Message ID | 897379f1850a50d8c320ca3facd06c5f03943bac.1719506876.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [RFC,iproute2-next] tc: f_flower: add support for matching on tunnel metadata | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, 27 Jun 2024 18:55:47 +0200 Davide Caratti <dcaratti@redhat.com> wrote: > + } else if (matches(*argv, "enc_flags") == 0) { > + NEXT_ARG(); > + ret = flower_parse_enc_dstflags(*argv, n); > + if (ret < 0) { > + fprintf(stderr, "Illegal \"enc_flags\"\n"); > + return -1; > + } No new uses of matches since it leads to abbreviation conflicts.
Hi Davide, Thank you for the patch. Overall I think it looks good, I only have a few comments. On 6/27/24 4:55 PM, Davide Caratti wrote: > extend TC flower for matching on tunnel metadata. > smoke test: > > # ip link add name myep type dummy > # ip link set dev myep up > # ip address add dev myep 192.0.2.1/24 > # ip neigh add dev myep 192.0.2.2 lladdr 00:c1:a0:c1:a0:00 nud permanent > # ip link add name mytun type vxlan external > # ip link set dev mytun up > # tc qdisc add dev mytun clsact > # tc filter add dev mytun egress protocol ip pref 1 handle 1 matchall action tunnel_key \ > > set src_ip 192.0.2.1 dst_ip 192.0.2.2 id 42 csum nofrag continue index 1 > # tc filter add dev mytun egress protocol ip pref 2 handle 2 flower action continue index 30 > # tc filter add dev mytun egress protocol ip pref 3 handle 3 flower enc_src_ip 192.0.2.1 action continue index 30 > # tc filter add dev mytun egress protocol ip pref 4 handle 4 flower enc_flags tundf action pipe index 100 > # mausezahn mytun -c 1 -p 100 -a 00:aa:bb:cc:dd:ee -b 00:ee:dd:cc:bb:aa -t icmp -q > # expect 2 packets below > # tc -s action get action gact index 30 > # expect 1 packet below > # tc -s action get action gact index 100 > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > include/uapi/linux/pkt_cls.h | 8 +++++++ > tc/f_flower.c | 42 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index 229fc925ec3a..24795aad7651 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -554,6 +554,9 @@ enum { > TCA_FLOWER_KEY_SPI, /* be32 */ > TCA_FLOWER_KEY_SPI_MASK, /* be32 */ > > + TCA_FLOWER_KEY_ENC_FLAGS, /* u32 */ > + TCA_FLOWER_KEY_ENC_FLAGS_MASK, /* u32 */ > + FYI: I will do a v1 of the kernel side soon, where the comments above will change to be32. > __TCA_FLOWER_MAX, > }; > > @@ -674,6 +677,11 @@ enum { > enum { > TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), > TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), > + /* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */ > + TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 3), > + TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 4), > + TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 5), > + TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 6), > }; > > enum { > diff --git a/tc/f_flower.c b/tc/f_flower.c > index 08c1001af7b4..45fc31dc380f 100644 > --- a/tc/f_flower.c > +++ b/tc/f_flower.c > @@ -17,6 +17,7 @@ > #include <linux/tc_act/tc_vlan.h> > #include <linux/mpls.h> > #include <linux/ppp_defs.h> > +#include <linux/if_tunnel.h> > > #include "utils.h" > #include "tc_util.h" > @@ -28,6 +29,7 @@ > > enum flower_matching_flags { > FLOWER_IP_FLAGS, > + FLOWER_ENC_DST_FLAGS, > }; > > enum flower_endpoint { > @@ -92,6 +94,7 @@ static void explain(void) > " erspan_opts MASKED-OPTIONS |\n" > " gtp_opts MASKED-OPTIONS |\n" > " pfcp_opts MASKED-OPTIONS |\n" > + " enc_flags ENC-FLAGS |\n" Maybe add a "ENC-FLAGS := foo,bar,..." below, could be generated from flag_to_string[], properly best to do separately as it requires moving the flag_to_string[] definition. > " ip_flags IP-FLAGS |\n" IP-FLAGS is also not defined, properly a good time to fix that. > " l2_miss L2_MISS |\n" > " enc_dst_port [ port_number ] |\n" > @@ -205,6 +208,11 @@ struct flag_to_string { > static struct flag_to_string flags_str[] = { > { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, FLOWER_IP_FLAGS, "frag" }, > { TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST, FLOWER_IP_FLAGS, "firstfrag" }, > + { TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM, FLOWER_ENC_DST_FLAGS, "csum" }, > + { TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT, FLOWER_ENC_DST_FLAGS, "tundf" }, > + { TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM, FLOWER_ENC_DST_FLAGS, "oam" }, > + { TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT, FLOWER_ENC_DST_FLAGS, "crit" }, > + Nit: I would prefix all of these with "tun_". > }; > > static int flower_parse_matching_flags(char *str, > @@ -1461,6 +1469,29 @@ static int flower_parse_enc_opts_pfcp(char *str, struct nlmsghdr *n) > return 0; > } > > +static int flower_parse_enc_dstflags(char *str, struct nlmsghdr *n) > +{ > + > + __u32 dst_flags, dst_flags_mask; > + int err; > + > + err = flower_parse_matching_flags(str, > + FLOWER_ENC_DST_FLAGS, > + &dst_flags, > + &dst_flags_mask); > + > + if (err < 0 || !dst_flags_mask) > + return -1; > + err = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_ENC_FLAGS, htonl(dst_flags)); > + if (err < 0) > + return -1; > + err = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_ENC_FLAGS_MASK, htonl(dst_flags_mask)); > + if (err < 0) > + return -1; > + > + return 0; > +} > + > static int flower_parse_mpls_lse(int *argc_p, char ***argv_p, > struct nlmsghdr *nlh) > { > @@ -2248,6 +2279,13 @@ static int flower_parse_opt(const struct filter_util *qu, char *handle, > fprintf(stderr, "Illegal \"pfcp_opts\"\n"); > return -1; > } > + } else if (matches(*argv, "enc_flags") == 0) { > + NEXT_ARG(); > + ret = flower_parse_enc_dstflags(*argv, n); > + if (ret < 0) { > + fprintf(stderr, "Illegal \"enc_flags\"\n"); > + return -1; > + } I agree that flower_parse_opt() is a bit crowded, but splitting it out to it's own function like this implicitly also means that you can't specify enc_flags over multiple enc_flags argument, as addattr32() is called once per enc_flags argument, instead of after all the arguments have been handled. [..] flower ip_flags frag ip_flags firstfrag [..] is valid, and is equivalent to [..] flower ip_flags frag/firstfrag [..]. IMHO I would completely mirror the way that ip_flags are handled, except the call to matches(), so as to avoid adding any new quirks. > } else if (matches(*argv, "action") == 0) { > NEXT_ARG(); > ret = parse_action(&argc, &argv, TCA_FLOWER_ACT, n); > @@ -3262,6 +3300,10 @@ static int flower_print_opt(const struct filter_util *qu, FILE *f, > tb[TCA_FLOWER_KEY_FLAGS], > tb[TCA_FLOWER_KEY_FLAGS_MASK]); > > + flower_print_matching_flags("enc_flags", FLOWER_ENC_DST_FLAGS, > + tb[TCA_FLOWER_KEY_ENC_FLAGS], > + tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]); > + > if (tb[TCA_FLOWER_L2_MISS]) { > struct rtattr *attr = tb[TCA_FLOWER_L2_MISS]; >
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 229fc925ec3a..24795aad7651 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -554,6 +554,9 @@ enum { TCA_FLOWER_KEY_SPI, /* be32 */ TCA_FLOWER_KEY_SPI_MASK, /* be32 */ + TCA_FLOWER_KEY_ENC_FLAGS, /* u32 */ + TCA_FLOWER_KEY_ENC_FLAGS_MASK, /* u32 */ + __TCA_FLOWER_MAX, }; @@ -674,6 +677,11 @@ enum { enum { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), + /* FLOW_DIS_ENCAPSULATION (1 << 2) is not exposed to userspace */ + TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 3), + TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 4), + TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 5), + TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 6), }; enum { diff --git a/tc/f_flower.c b/tc/f_flower.c index 08c1001af7b4..45fc31dc380f 100644 --- a/tc/f_flower.c +++ b/tc/f_flower.c @@ -17,6 +17,7 @@ #include <linux/tc_act/tc_vlan.h> #include <linux/mpls.h> #include <linux/ppp_defs.h> +#include <linux/if_tunnel.h> #include "utils.h" #include "tc_util.h" @@ -28,6 +29,7 @@ enum flower_matching_flags { FLOWER_IP_FLAGS, + FLOWER_ENC_DST_FLAGS, }; enum flower_endpoint { @@ -92,6 +94,7 @@ static void explain(void) " erspan_opts MASKED-OPTIONS |\n" " gtp_opts MASKED-OPTIONS |\n" " pfcp_opts MASKED-OPTIONS |\n" + " enc_flags ENC-FLAGS |\n" " ip_flags IP-FLAGS |\n" " l2_miss L2_MISS |\n" " enc_dst_port [ port_number ] |\n" @@ -205,6 +208,11 @@ struct flag_to_string { static struct flag_to_string flags_str[] = { { TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT, FLOWER_IP_FLAGS, "frag" }, { TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST, FLOWER_IP_FLAGS, "firstfrag" }, + { TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM, FLOWER_ENC_DST_FLAGS, "csum" }, + { TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT, FLOWER_ENC_DST_FLAGS, "tundf" }, + { TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM, FLOWER_ENC_DST_FLAGS, "oam" }, + { TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT, FLOWER_ENC_DST_FLAGS, "crit" }, + }; static int flower_parse_matching_flags(char *str, @@ -1461,6 +1469,29 @@ static int flower_parse_enc_opts_pfcp(char *str, struct nlmsghdr *n) return 0; } +static int flower_parse_enc_dstflags(char *str, struct nlmsghdr *n) +{ + + __u32 dst_flags, dst_flags_mask; + int err; + + err = flower_parse_matching_flags(str, + FLOWER_ENC_DST_FLAGS, + &dst_flags, + &dst_flags_mask); + + if (err < 0 || !dst_flags_mask) + return -1; + err = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_ENC_FLAGS, htonl(dst_flags)); + if (err < 0) + return -1; + err = addattr32(n, MAX_MSG, TCA_FLOWER_KEY_ENC_FLAGS_MASK, htonl(dst_flags_mask)); + if (err < 0) + return -1; + + return 0; +} + static int flower_parse_mpls_lse(int *argc_p, char ***argv_p, struct nlmsghdr *nlh) { @@ -2248,6 +2279,13 @@ static int flower_parse_opt(const struct filter_util *qu, char *handle, fprintf(stderr, "Illegal \"pfcp_opts\"\n"); return -1; } + } else if (matches(*argv, "enc_flags") == 0) { + NEXT_ARG(); + ret = flower_parse_enc_dstflags(*argv, n); + if (ret < 0) { + fprintf(stderr, "Illegal \"enc_flags\"\n"); + return -1; + } } else if (matches(*argv, "action") == 0) { NEXT_ARG(); ret = parse_action(&argc, &argv, TCA_FLOWER_ACT, n); @@ -3262,6 +3300,10 @@ static int flower_print_opt(const struct filter_util *qu, FILE *f, tb[TCA_FLOWER_KEY_FLAGS], tb[TCA_FLOWER_KEY_FLAGS_MASK]); + flower_print_matching_flags("enc_flags", FLOWER_ENC_DST_FLAGS, + tb[TCA_FLOWER_KEY_ENC_FLAGS], + tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]); + if (tb[TCA_FLOWER_L2_MISS]) { struct rtattr *attr = tb[TCA_FLOWER_L2_MISS];