| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e2ffbc92-5a73-464f-9979-446b5373e80e@fiberby.net>
Date: Fri, 28 Jun 2024 09:46:28 +0000
From: Asbjørn Sloth Tønnesen <ast@...erby.net>
To: Davide Caratti <dcaratti@...hat.com>
Cc: David Ahern <dsahern@...nel.org>, aclaudi@...hat.com,
Ilya Maximets <i.maximets@....org>, echaudro@...hat.com,
netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [RFC iproute2-next] tc: f_flower: add support for matching on
tunnel metadata
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@...hat.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];
>
--
Best regards
Asbjørn Sloth Tønnesen
Network Engineer
Fiberby - AS42541
Powered by blists - more mailing lists