netdev - Re: [PATCH iproute2-next] tc: f_flower: add support for matching on tunnel metadata

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a5a5f36f-43f5-4486-b691-8187f6eaa818@fiberby.net>
Date: Tue, 16 Jul 2024 00:30:07 +0000
From: Asbjørn Sloth Tønnesen <ast@...erby.net>
To: Davide Caratti <dcaratti@...hat.com>
Cc: aclaudi@...hat.com, dsahern@...nel.org, echaudro@...hat.com,
 i.maximets@....org, jhs@...atatu.com, netdev@...r.kernel.org,
 stephen@...workplumber.org
Subject: Re: [PATCH iproute2-next] tc: f_flower: add support for matching on
 tunnel metadata

Hi Davide,

Minor documentation consistency nit, otherwise LGTM.

On 7/15/24 5:27 PM, Davide Caratti wrote:
> extend TC flower for matching on tunnel metadata.
> 
> Changes since RFC:
>   - update uAPI bits to Asbjørn's most recent code [1]
>   - add 'tun' prefix to all flag names (Asbjørn)
>   - allow parsing 'enc_flags' multiple times, without clearing the match
>     mask every time, like happens for 'ip_flags' (Asbjørn)
>   - don't use "matches()" for parsing argv[]  (Stephen)
>   - (hopefully) improve usage() printout (Asbjørn)
>   - update man page
> 
> [1] https://lore.kernel.org/netdev/20240709163825.1210046-1-ast@fiberby.net/
> 
> Signed-off-by: Davide Caratti <dcaratti@...hat.com>
> ---
>   include/uapi/linux/pkt_cls.h |  7 +++++++
>   man/man8/tc-flower.8         | 28 ++++++++++++++++++++++++--
>   tc/f_flower.c                | 38 +++++++++++++++++++++++++++++++++++-
>   3 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 229fc925ec3a..19e25bceb24c 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,	/* be32 */
> +	TCA_FLOWER_KEY_ENC_FLAGS_MASK,	/* be32 */
> +
>   	__TCA_FLOWER_MAX,
>   };
>   
> @@ -674,6 +677,10 @@ enum {
>   enum {
>   	TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0),
>   	TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1),
> +	TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 2),
> +	TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 3),
> +	TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 4),
> +	TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 5),
>   };
>   
>   enum {
> diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
> index 6b56640503d5..028f48571be3 100644
> --- a/man/man8/tc-flower.8
> +++ b/man/man8/tc-flower.8
> @@ -106,7 +106,9 @@ flower \- flow based traffic control filter
>   .B l2_miss
>   .IR L2_MISS " | "
>   .BR cfm
> -.IR CFM_OPTIONS " }"
> +.IR CFM_OPTIONS " | "
> +.BR enc_flags
> +.IR ENCFLAG_LIST " }"
>   
>   .ti -8
>   .IR LSE_LIST " := [ " LSE_LIST " ] " LSE
> @@ -131,6 +133,16 @@ flower \- flow based traffic control filter
>   .B op
>   .IR OPCODE "
>   
> +.ti -8
> +.IR ENCFLAG_LIST " := [ " ENCFLAG_LIST "/ ] " ENCFLAG
> +
> +.ti -8
> +.IR ENCFLAG " := { "
> +.BR [no]tuncsum " | "
> +.BR [no]tundf " | "
> +.BR [no]tunoam " | "
> +.BR [no]tuncrit " } "
> +
>   .SH DESCRIPTION
>   The
>   .B flower
> @@ -538,11 +550,23 @@ Match on the Maintenance Domain (MD) level field.
>   .BI op " OPCODE "
>   Match on the CFM opcode field. \fIOPCODE\fR is an unsigned 8 bit value in
>   decimal format.
> +.RE
> +.TP
> +.BI enc_flags " ENCFLAGS_LIST "

^^^ reference to ENCFLAGS_LIST

> +Match on tunnel control flags.
> +.I ENCFLAGS_LIST
> +is a list of the following tunnel control flags:
> +.BR [no]tuncsum ", "
> +.BR [no]tundf ", "
> +.BR [no]tunoam ", "
> +.BR [no]tuncrit ", "
> +each separated by '/'.
> +.TP
>   
>   .SH NOTES
>   As stated above where applicable, matches of a certain layer implicitly depend
>   on the matches of the next lower layer. Precisely, layer one and two matches
> -(\fBindev\fR,  \fBdst_mac\fR and \fBsrc_mac\fR)
> +(\fBindev\fR,  \fBdst_mac\fR, \fBsrc_mac\fR and \fBenc_flags\fR)
>   have no dependency,
>   MPLS and layer three matches
>   (\fBmpls\fR, \fBmpls_label\fR, \fBmpls_tc\fR, \fBmpls_bos\fR, \fBmpls_ttl\fR,
> diff --git a/tc/f_flower.c b/tc/f_flower.c
> index 08c1001af7b4..35ccc3743f46 100644
> --- a/tc/f_flower.c
> +++ b/tc/f_flower.c
> @@ -28,6 +28,7 @@
>   
>   enum flower_matching_flags {
>   	FLOWER_IP_FLAGS,
> +	FLOWER_ENC_DST_FLAGS,
>   };
>   
>   enum flower_endpoint {
> @@ -99,13 +100,16 @@ static void explain(void)
>   		"			ct_label MASKED_CT_LABEL |\n"
>   		"			ct_mark MASKED_CT_MARK |\n"
>   		"			ct_zone MASKED_CT_ZONE |\n"
> -		"			cfm CFM }\n"
> +		"			cfm CFM |\n"
> +		"			enc_flags ENC-FLAGS }\n"

^^^ reference to ENC-FLAGS

>   		"	LSE-LIST := [ LSE-LIST ] LSE\n"
>   		"	LSE := lse depth DEPTH { label LABEL | tc TC | bos BOS | ttl TTL }\n"
>   		"	FILTERID := X:Y:Z\n"
>   		"	MASKED_LLADDR := { LLADDR | LLADDR/MASK | LLADDR/BITS }\n"
>   		"	MASKED_CT_STATE := combination of {+|-} and flags trk,est,new,rel,rpl,inv\n"
>   		"	CFM := { mdl LEVEL | op OPCODE }\n"
> +		"	ENCFLAG-LIST := [ ENCFLAG-LIST/ ]ENCFLAG\n"
> +		"	ENCFLAG := { [no]tuncsum | [no]tundf | [no]tunoam | [no]tuncrit }\n"

Nit: What is ENC-FLAGS? I assume it should have been ENCFLAG-LIST.

For consistency between man and --help, is it singular or plural flags,
and is it with a dash or underscore?


-- 
Best regards
Asbjørn Sloth Tønnesen
Network Engineer
Fiberby - AS42541