Define new TCA_FLOWER_KEY_FLAGS_* flags for use in struct
flow_dissector_key_control, covering the same flags
as currently exposed through TCA_FLOWER_KEY_ENC_FLAGS,
but assign them new bit positions in so that they don't
conflict with existing TCA_FLOWER_KEY_FLAGS_* flags.

Synchronize FLOW_DIS_* flags, but put the new flags
under FLOW_DIS_F_*. The idea is that we can later, move
the existing flags under FLOW_DIS_F_* as well.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 include/net/flow_dissector.h | 17 +++++++++++++----
 include/uapi/linux/pkt_cls.h |  5 +++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 3e47e123934d..f560e2c8d0e7 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -16,7 +16,8 @@ struct sk_buff;
  * struct flow_dissector_key_control:
  * @thoff:     Transport header offset
  * @addr_type: Type of key. One of FLOW_DISSECTOR_KEY_*
- * @flags:     Key flags. Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAGENCAPSULATION)
+ * @flags:     Key flags.
+ *             Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAG|ENCAPSULATION|F_*)
  */
 struct flow_dissector_key_control {
 	u16	thoff;
@@ -24,9 +25,17 @@ struct flow_dissector_key_control {
 	u32	flags;
 };
 
-#define FLOW_DIS_IS_FRAGMENT	BIT(0)
-#define FLOW_DIS_FIRST_FRAG	BIT(1)
-#define FLOW_DIS_ENCAPSULATION	BIT(2)
+/* Please keep these flags in sync with TCA_FLOWER_KEY_FLAGS_*
+ * in include/uapi/linux/pkt_cls.h, as these bit flags are exposed
+ * to userspace in some error paths, ie. unsupported flags.
+ */
+#define FLOW_DIS_IS_FRAGMENT		BIT(0)
+#define FLOW_DIS_FIRST_FRAG		BIT(1)
+#define FLOW_DIS_ENCAPSULATION		BIT(2)
+#define FLOW_DIS_F_TUNNEL_CSUM		BIT(3)
+#define FLOW_DIS_F_TUNNEL_DONT_FRAGMENT	BIT(4)
+#define FLOW_DIS_F_TUNNEL_OAM		BIT(5)
+#define FLOW_DIS_F_TUNNEL_CRIT_OPT	BIT(6)
 
 enum flow_dissect_ret {
 	FLOW_DISSECT_RET_OUT_GOOD,
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index b6d38f5fd7c0..24795aad7651 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -677,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 {
-- 
2.45.2

From: Asbjørn Sloth Tønnesen <ast@fiberby.net>
Date: Wed,  3 Jul 2024 10:45:50 +0000

> Define new TCA_FLOWER_KEY_FLAGS_* flags for use in struct
> flow_dissector_key_control, covering the same flags
> as currently exposed through TCA_FLOWER_KEY_ENC_FLAGS,
> but assign them new bit positions in so that they don't
> conflict with existing TCA_FLOWER_KEY_FLAGS_* flags.
> 
> Synchronize FLOW_DIS_* flags, but put the new flags
> under FLOW_DIS_F_*. The idea is that we can later, move
> the existing flags under FLOW_DIS_F_* as well.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  include/net/flow_dissector.h | 17 +++++++++++++----
>  include/uapi/linux/pkt_cls.h |  5 +++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 3e47e123934d..f560e2c8d0e7 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -16,7 +16,8 @@ struct sk_buff;
>   * struct flow_dissector_key_control:
>   * @thoff:     Transport header offset
>   * @addr_type: Type of key. One of FLOW_DISSECTOR_KEY_*
> - * @flags:     Key flags. Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAGENCAPSULATION)
> + * @flags:     Key flags.
> + *             Any of FLOW_DIS_(IS_FRAGMENT|FIRST_FRAG|ENCAPSULATION|F_*)
>   */
>  struct flow_dissector_key_control {
>  	u16	thoff;
> @@ -24,9 +25,17 @@ struct flow_dissector_key_control {
>  	u32	flags;
>  };
>  
> -#define FLOW_DIS_IS_FRAGMENT	BIT(0)
> -#define FLOW_DIS_FIRST_FRAG	BIT(1)
> -#define FLOW_DIS_ENCAPSULATION	BIT(2)
> +/* Please keep these flags in sync with TCA_FLOWER_KEY_FLAGS_*
> + * in include/uapi/linux/pkt_cls.h, as these bit flags are exposed
> + * to userspace in some error paths, ie. unsupported flags.
> + */
> +#define FLOW_DIS_IS_FRAGMENT		BIT(0)
> +#define FLOW_DIS_FIRST_FRAG		BIT(1)
> +#define FLOW_DIS_ENCAPSULATION		BIT(2)
> +#define FLOW_DIS_F_TUNNEL_CSUM		BIT(3)
> +#define FLOW_DIS_F_TUNNEL_DONT_FRAGMENT	BIT(4)
> +#define FLOW_DIS_F_TUNNEL_OAM		BIT(5)
> +#define FLOW_DIS_F_TUNNEL_CRIT_OPT	BIT(6)

1. enum? Then they will be included in BTF info, maybe they might come
   handy later in BPF...
2. Maybe "sync" them automatically?

	FLOW_DIS_FIRST_FRAG	= TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST,
	...

(with the only exception for BIT(2))

>  
>  enum flow_dissect_ret {
>  	FLOW_DISSECT_RET_OUT_GOOD,
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index b6d38f5fd7c0..24795aad7651 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -677,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 */

Should uAPI header contain this comment? FLOW_DIS_ENCAPSULATION is an
internal kernel definition, so I believe its name shouldn't leak to the
userspace header.

> +	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 {

Thanks,
Olek

On Wed, 3 Jul 2024 12:59:54 +0200 Alexander Lobakin wrote:
> >  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 */  
> 
> Should uAPI header contain this comment? FLOW_DIS_ENCAPSULATION is an
> internal kernel definition, so I believe its name shouldn't leak to the
> userspace header.

Also since it's internal, can avoid the gap in uAPI and make
ENCAPSULATION be something like "last uAPI bit + 1" ?

Hi Kuba and Alexander,

On 7/4/24 12:20 AM, Jakub Kicinski wrote:
> On Wed, 3 Jul 2024 12:59:54 +0200 Alexander Lobakin wrote:
>>>   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 */
>>
>> Should uAPI header contain this comment? FLOW_DIS_ENCAPSULATION is an
>> internal kernel definition, so I believe its name shouldn't leak to the
>> userspace header.
> 
> Also since it's internal, can avoid the gap in uAPI and make
> ENCAPSULATION be something like "last uAPI bit + 1" ?

Would the below work?

-#define FLOW_DIS_IS_FRAGMENT   BIT(0)
-#define FLOW_DIS_FIRST_FRAG    BIT(1)
-#define FLOW_DIS_ENCAPSULATION BIT(2)
+/* The control flags are kept in sync with TCA_FLOWER_KEY_FLAGS_*, as those
+ * flags are exposed to userspace in some error paths, ie. unsupported flags.
+ */
+enum flow_dissector_ctrl_flags {
+       FLOW_DIS_IS_FRAGMENT            = TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT,
+       FLOW_DIS_FIRST_FRAG             = TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST,
+       FLOW_DIS_F_TUNNEL_CSUM          = TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM,
+       FLOW_DIS_F_TUNNEL_DONT_FRAGMENT = TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT,
+       FLOW_DIS_F_TUNNEL_OAM           = TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM,
+       FLOW_DIS_F_TUNNEL_CRIT_OPT      = TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT,
+
+       /* These flags are internal to the kernel */
+       FLOW_DIS_ENCAPSULATION          = (TCA_FLOWER_KEY_FLAGS_MAX << 1),
+};

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

On Thu, 4 Jul 2024 21:45:14 +0000 Asbjørn Sloth Tønnesen wrote:
> +       /* These flags are internal to the kernel */
> +       FLOW_DIS_ENCAPSULATION          = (TCA_FLOWER_KEY_FLAGS_MAX << 1),

Looks good (assuming TCA_FLOWER_KEY_FLAGS_MAX == TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT)