[RFC PATCH net-next 0/9] flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage

[RFC PATCH net-next 0/9] flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage

[Asbjørn Sloth Tønnesen]

This series reworks the recently added TCA_FLOWER_KEY_ENC_FLAGS
attribute, to be more like TCA_FLOWER_KEY_FLAGS, and use
the unused u32 flags field in TCA_FLOWER_KEY_ENC_CONTROL,
instead of adding another u32 in FLOW_DISSECTOR_KEY_ENC_FLAGS.

I have defined the new FLOW_DIS_F_* and TCA_FLOWER_KEY_FLAGS_*
flags to coexists for now, so the meaning of the flags field
in struct flow_dissector_key_control is not depending on the
context that it is used in. If we run out of bits then we can
always make split them up later, if we really want to.

Davide and Ilya would this work for you?

Currently this series is only compile-tested.

Preliminary discussion about these changes:
https://lore.kernel.org/netdev/ZmFuxElwZiYJzBkh@dcaratti.users.ipa.redhat.com/

---
If this series is put directly on top of:
668b6a2ef832 ("flow_dissector: add support for tunnel control
flags") and 1d17568e74de ("net/sched: cls_flower: add support
for matching tunnel control flags") as can be done by reverting
them, cherry-picking them, and then applying this series.

Then it gives this combined diffstat:
$ git diff --stat ...
 include/net/flow_dissector.h |  17 ++++-
 include/uapi/linux/pkt_cls.h |   8 ++
 net/core/flow_dissector.c    |  34 +++++---
 net/sched/cls_flower.c       | 103 ++++++++++++++++++++++----
 4 files changed, 136 insertions(+), 26 deletions(-)

Normal shortlog and diffstat below:

Asbjørn Sloth Tønnesen (9):
  net/sched: flower: define new tunnel flags
  net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS
  net/sched: cls_flower: add policy for TCA_FLOWER_KEY_FLAGS
  flow_dissector: prepare for encapsulated control flags
  flow_dissector: set encapsulated control flags from tun_flags
  net/sched: cls_flower: add tunnel flags to fl_{set,dump}_key_flags()
  net/sched: cls_flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage
  flow_dissector: cleanup FLOW_DISSECTOR_KEY_ENC_FLAGS
  flow_dissector: set encapsulation control flags for non-IP

 include/net/flow_dissector.h |  26 +++----
 include/net/ip_tunnels.h     |  12 ----
 include/uapi/linux/pkt_cls.h |   5 ++
 net/core/flow_dissector.c    |  50 ++++++-------
 net/sched/cls_flower.c       | 135 ++++++++++++++++++++---------------
 5 files changed, 123 insertions(+), 105 deletions(-)

-- 
2.45.1

[RFC PATCH net-next 1/9] net/sched: flower: define new tunnel flags

[Asbjørn Sloth Tønnesen]

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 99626475c3f4a..1f0fddb29a0d8 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_FRAGENCAPSULATION|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 b6d38f5fd7c05..24795aad76518 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.1

Re: [RFC PATCH net-next 1/9] net/sched: flower: define new tunnel flags

[Davide Caratti]

On Tue, Jun 11, 2024 at 11:53:34PM +0000, Asbjørn Sloth Tønnesen wrote:
> 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 99626475c3f4a..1f0fddb29a0d8 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_FRAGENCAPSULATION|F_*)

^^ nit: there was a typo in the original line above. Maybe this is a
good chance to put the missing '|' before 'ENCAPSULATION'

[RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Asbjørn Sloth Tønnesen]

Prepare fl_set_key_flags/fl_dump_key_flags() for use with
TCA_FLOWER_KEY_ENC_FLAGS{,_MASK}.

This patch adds an encap argument, similar to fl_set_key_ip/
fl_dump_key_ip(), and determine the flower keys based on the
encap argument, and use them in the rest of the two functions.

Since these functions are so far, only called with encap set false,
then there is no functional change.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/sched/cls_flower.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eef570c577ac7..6a5cecfd95619 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1166,19 +1166,28 @@ static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
 	}
 }
 
-static int fl_set_key_flags(struct nlattr **tb, u32 *flags_key,
+static int fl_set_key_flags(struct nlattr **tb, bool encap, u32 *flags_key,
 			    u32 *flags_mask, struct netlink_ext_ack *extack)
 {
+	int fl_key, fl_mask;
 	u32 key, mask;
 
+	if (encap) {
+		fl_key = TCA_FLOWER_KEY_ENC_FLAGS;
+		fl_mask = TCA_FLOWER_KEY_ENC_FLAGS_MASK;
+	} else {
+		fl_key = TCA_FLOWER_KEY_FLAGS;
+		fl_mask = TCA_FLOWER_KEY_FLAGS_MASK;
+	}
+
 	/* mask is mandatory for flags */
-	if (!tb[TCA_FLOWER_KEY_FLAGS_MASK]) {
+	if (NL_REQ_ATTR_CHECK(extack, NULL, tb, fl_mask)) {
 		NL_SET_ERR_MSG(extack, "Missing flags mask");
 		return -EINVAL;
 	}
 
-	key = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS]));
-	mask = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS_MASK]));
+	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
+	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));
 
 	*flags_key  = 0;
 	*flags_mask = 0;
@@ -2086,7 +2095,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		return ret;
 
 	if (tb[TCA_FLOWER_KEY_FLAGS]) {
-		ret = fl_set_key_flags(tb, &key->control.flags,
+		ret = fl_set_key_flags(tb, false, &key->control.flags,
 				       &mask->control.flags, extack);
 		if (ret)
 			return ret;
@@ -3084,12 +3093,22 @@ static void fl_get_key_flag(u32 dissector_key, u32 dissector_mask,
 	}
 }
 
-static int fl_dump_key_flags(struct sk_buff *skb, u32 flags_key, u32 flags_mask)
+static int fl_dump_key_flags(struct sk_buff *skb, bool encap,
+			     u32 flags_key, u32 flags_mask)
 {
-	u32 key, mask;
+	int fl_key, fl_mask;
 	__be32 _key, _mask;
+	u32 key, mask;
 	int err;
 
+	if (encap) {
+		fl_key = TCA_FLOWER_KEY_ENC_FLAGS;
+		fl_mask = TCA_FLOWER_KEY_ENC_FLAGS_MASK;
+	} else {
+		fl_key = TCA_FLOWER_KEY_FLAGS;
+		fl_mask = TCA_FLOWER_KEY_FLAGS_MASK;
+	}
+
 	if (!memchr_inv(&flags_mask, 0, sizeof(flags_mask)))
 		return 0;
 
@@ -3105,11 +3124,11 @@ static int fl_dump_key_flags(struct sk_buff *skb, u32 flags_key, u32 flags_mask)
 	_key = cpu_to_be32(key);
 	_mask = cpu_to_be32(mask);
 
-	err = nla_put(skb, TCA_FLOWER_KEY_FLAGS, 4, &_key);
+	err = nla_put(skb, fl_key, 4, &_key);
 	if (err)
 		return err;
 
-	return nla_put(skb, TCA_FLOWER_KEY_FLAGS_MASK, 4, &_mask);
+	return nla_put(skb, fl_mask, 4, &_mask);
 }
 
 static int fl_dump_key_geneve_opt(struct sk_buff *skb,
@@ -3632,7 +3651,8 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
 	if (fl_dump_key_ct(skb, &key->ct, &mask->ct))
 		goto nla_put_failure;
 
-	if (fl_dump_key_flags(skb, key->control.flags, mask->control.flags))
+	if (fl_dump_key_flags(skb, false, key->control.flags,
+			      mask->control.flags))
 		goto nla_put_failure;
 
 	if (fl_dump_key_val(skb, &key->hash.hash, TCA_FLOWER_KEY_HASH,
-- 
2.45.1

Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Davide Caratti]

hello Asbjørn,

some update on this work: I tested your patch after adapting iproute2
bits (e.g. using TCA_FLOWER_KEY_FLAGS_TUNNEL_<CSUM|DONT_FRAGMENT|OAM|CRIT>

from

https://lore.kernel.org/netdev/20240611235355.177667-2-ast@fiberby.net/

Now: functional tests on TCA_FLOWER_KEY_ENC_FLAGS systematically fail. I must
admit that I didn't complete 100% of the analysis, but IMO there is at least an
endianness problem here. See below:

On Tue, Jun 11, 2024 at 11:53:35PM +0000, Asbjørn Sloth Tønnesen wrote:
> Prepare fl_set_key_flags/fl_dump_key_flags() for use with
> TCA_FLOWER_KEY_ENC_FLAGS{,_MASK}.
> 
> This patch adds an encap argument, similar to fl_set_key_ip/
> fl_dump_key_ip(), and determine the flower keys based on the
> encap argument, and use them in the rest of the two functions.
> 
> Since these functions are so far, only called with encap set false,
> then there is no functional change.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  net/sched/cls_flower.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index eef570c577ac7..6a5cecfd95619 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -1166,19 +1166,28 @@ static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
>  	}
>  }
>  
> -static int fl_set_key_flags(struct nlattr **tb, u32 *flags_key,
> +static int fl_set_key_flags(struct nlattr **tb, bool encap, u32 *flags_key,
>  			    u32 *flags_mask, struct netlink_ext_ack *extack)
>  {
> +	int fl_key, fl_mask;
>  	u32 key, mask;
>  
> +	if (encap) {
> +		fl_key = TCA_FLOWER_KEY_ENC_FLAGS;
> +		fl_mask = TCA_FLOWER_KEY_ENC_FLAGS_MASK;
> +	} else {
> +		fl_key = TCA_FLOWER_KEY_FLAGS;
> +		fl_mask = TCA_FLOWER_KEY_FLAGS_MASK;
> +	}
> +
>  	/* mask is mandatory for flags */
> -	if (!tb[TCA_FLOWER_KEY_FLAGS_MASK]) {
> +	if (NL_REQ_ATTR_CHECK(extack, NULL, tb, fl_mask)) {
>  		NL_SET_ERR_MSG(extack, "Missing flags mask");
>  		return -EINVAL;
>  	}
>  
> -	key = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS]));
> -	mask = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS_MASK]));
> +	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
> +	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));


I think that (at least) the above hunk is wrong - or at least, it is a
functional discontinuity that causes failure in my test. While the
previous bitmask storing tunnel control flags was in host byte ordering,
the information on IP fragmentation are stored in network byte ordering.

So, if we want to use this enum

--- 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),
 };
 
consistently, we should keep using network byte ordering for
TCA_FLOWER_KEY_FLAGS_TUNNEL_* flags (for a reason that I don't understand,
because metadata are not transmitted on wire. But maybe I'm missing something).

Shall I convert iproute2 to flip those bits like it happens for
TCA_FLOWER_KEY_FLAGS ? thanks!

-- 
davide

Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Asbjørn Sloth Tønnesen]

Hi Davide,

On 6/21/24 10:11 AM, Davide Caratti wrote:
> hello Asbjørn,
> 
> some update on this work: I tested your patch after adapting iproute2
> bits (e.g. using TCA_FLOWER_KEY_FLAGS_TUNNEL_<CSUM|DONT_FRAGMENT|OAM|CRIT>

Could you please post your iproute2 code?


> from
> 
> https://lore.kernel.org/netdev/20240611235355.177667-2-ast@fiberby.net/
> 
> Now: functional tests on TCA_FLOWER_KEY_ENC_FLAGS systematically fail. I must
> admit that I didn't complete 100% of the analysis, but IMO there is at least an
> endianness problem here. See below:
> 
> On Tue, Jun 11, 2024 at 11:53:35PM +0000, Asbjørn Sloth Tønnesen wrote:
>> Prepare fl_set_key_flags/fl_dump_key_flags() for use with
>> TCA_FLOWER_KEY_ENC_FLAGS{,_MASK}.
>>
>> This patch adds an encap argument, similar to fl_set_key_ip/
>> fl_dump_key_ip(), and determine the flower keys based on the
>> encap argument, and use them in the rest of the two functions.
>>
>> Since these functions are so far, only called with encap set false,
>> then there is no functional change.
>>
>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>> ---
>>   net/sched/cls_flower.c | 40 ++++++++++++++++++++++++++++++----------
>>   1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index eef570c577ac7..6a5cecfd95619 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -1166,19 +1166,28 @@ static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
>>   	}
>>   }
>>   
>> -static int fl_set_key_flags(struct nlattr **tb, u32 *flags_key,
>> +static int fl_set_key_flags(struct nlattr **tb, bool encap, u32 *flags_key,
>>   			    u32 *flags_mask, struct netlink_ext_ack *extack)
>>   {
>> +	int fl_key, fl_mask;
>>   	u32 key, mask;
>>   
>> +	if (encap) {
>> +		fl_key = TCA_FLOWER_KEY_ENC_FLAGS;
>> +		fl_mask = TCA_FLOWER_KEY_ENC_FLAGS_MASK;
>> +	} else {
>> +		fl_key = TCA_FLOWER_KEY_FLAGS;
>> +		fl_mask = TCA_FLOWER_KEY_FLAGS_MASK;
>> +	}
>> +
>>   	/* mask is mandatory for flags */
>> -	if (!tb[TCA_FLOWER_KEY_FLAGS_MASK]) {
>> +	if (NL_REQ_ATTR_CHECK(extack, NULL, tb, fl_mask)) {
>>   		NL_SET_ERR_MSG(extack, "Missing flags mask");
>>   		return -EINVAL;
>>   	}
>>   
>> -	key = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS]));
>> -	mask = be32_to_cpu(nla_get_be32(tb[TCA_FLOWER_KEY_FLAGS_MASK]));
>> +	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
>> +	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));
> 
> 
> I think that (at least) the above hunk is wrong - or at least, it is a
> functional discontinuity that causes failure in my test. While the
> previous bitmask storing tunnel control flags was in host byte ordering,
> the information on IP fragmentation are stored in network byte ordering.
> 
> So, if we want to use this enum
> 
> --- 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),
>   };
>  
> consistently, we should keep using network byte ordering for
> TCA_FLOWER_KEY_FLAGS_TUNNEL_* flags (for a reason that I don't understand,
> because metadata are not transmitted on wire. But maybe I'm missing something).
> 
> Shall I convert iproute2 to flip those bits like it happens for
> TCA_FLOWER_KEY_FLAGS ? thanks!

It is always preferred to have a well-defined endianness for binary protocols, even
if it might only be used locally for now.

I would base it on flags_str in tc/f_flower.c, so it can reuse
flower_parse_matching_flags() and add a new "enc_flags" argument mirroring "ip_flags".
Then there shouldn't be a difference in endianness. The naming of "ip_flags" is
questionable, since it is only named in an IP-specific way in iproute2.

Most of the patches in this series are just extending and mirroring what is already
done for the existing flags, so I am pretty sure that part should work.

The part I am most unsure about is patch 5, since I don't have a lot of experience
with the bitmap infrastructure, I will make another reply to that patch.

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

Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Davide Caratti]

hello Asbjørn,

thanks for your patience!

On Fri, Jun 21, 2024 at 02:45:28PM +0000, Asbjørn Sloth Tønnesen wrote:
> 
> Could you please post your iproute2 code?

sure, will clean it up and share it today in ML.
 
> > from
> > 
> > https://lore.kernel.org/netdev/20240611235355.177667-2-ast@fiberby.net/
> > 
> > Now: functional tests on TCA_FLOWER_KEY_ENC_FLAGS systematically fail. I must
> > admit that I didn't complete 100% of the analysis, but IMO there is at least an
> > endianness problem here. See below:
> > 
> > On Tue, Jun 11, 2024 at 11:53:35PM +0000, Asbjørn Sloth Tønnesen wrote:

[...]
 
> It is always preferred to have a well-defined endianness for binary protocols, even
> if it might only be used locally for now.

given the implementation of fl_set_key_flags() in patch 2,

	key = be32_to_cpu(nla_get_be32(tb[fl_key]));
	mask = be32_to_cpu(nla_get_be32(tb[fl_mask]));

when fl_key and fl_mask are TCA_FLOWER_KEY_ENC_FLAGS and TCA_FLOWER_KEY_ENC_FLAGS_MASK,
I assume that we want to turn them to network ordering, like it's already being done for
TCA_FLOWER_KEY_FLAGS and TCA_FLOWER_KEY_FLAGS_MASK.

So, we must htonl() the policy mask in the second hunk in patch 7,something like:

@@ -746,9 +746,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_L2_MISS]		= NLA_POLICY_MAX(NLA_U8, 1),
 	[TCA_FLOWER_KEY_CFM]		= { .type = NLA_NESTED },
 	[TCA_FLOWER_KEY_ENC_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  htonl(TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK)),
 	[TCA_FLOWER_KEY_ENC_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  htonl(TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK)),
 };

And for the same reason, the flower code in patch 3 needs to be changed as follows:

@@ -676,8 +680,10 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]	= { .type = NLA_U16 },
-	[TCA_FLOWER_KEY_FLAGS]		= { .type = NLA_U32 },
-	[TCA_FLOWER_KEY_FLAGS_MASK]	= { .type = NLA_U32 },
+	[TCA_FLOWER_KEY_FLAGS]		= NLA_POLICY_MASK(NLA_U32,
+							  ntohl(TCA_FLOWER_KEY_FLAGS_POLICY_MASK)),
+	[TCA_FLOWER_KEY_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
+							  ntohl(TCA_FLOWER_KEY_FLAGS_POLICY_MASK)),
 	[TCA_FLOWER_KEY_ICMPV4_TYPE]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_CODE]	= { .type = NLA_U8 },

Otherwise it will break the following use case (taken from tc_flower.sh kselftest):

# tc qdisc add dev lo clsact
# tc filter add dev lo ingress protocol ip pref 1 handle 101 flower ip_flags frag action continue
RTNETLINK answers: Invalid argument
We have an error talking to the kernel

because TCA_FLOWER_KEY_FLAGS_POLICY_MASK and TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK
are in host byte order _ so netlink policy mask validation will fail unless we turn
the mask to network byte order.

(And I see we don't have a tdc selftest  for 'ip_flags', this might be a
good chance to add it :-) )

-- 
davide

Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Davide Caratti]

On Wed, Jun 26, 2024 at 11:49 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> So, we must htonl() the policy mask in the second hunk in patch 7,something like:
>

or maybe better (but still untested), use NLA_BE32, like netfilter does in [1]

[1] https://elixir.bootlin.com/linux/latest/A/ident/NF_NAT_RANGE_MASK

-- 
davide

Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Asbjørn Sloth Tønnesen]

Hi Davide,

On 6/26/24 10:01 AM, Davide Caratti wrote:
> On Wed, Jun 26, 2024 at 11:49 AM Davide Caratti <dcaratti@redhat.com> wrote:
>>
>> So, we must htonl() the policy mask in the second hunk in patch 7,something like:

Good catch.

> or maybe better (but still untested), use NLA_BE32, like netfilter does in [1]
> 
> [1] https://elixir.bootlin.com/linux/latest/A/ident/NF_NAT_RANGE_MASK

Yes, that is better. It should work, as it triggers a htonl() in nla_validate_mask().

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

Re: [RFC PATCH net-next 2/9] net/sched: cls_flower: prepare fl_{set,dump}_key_flags() for ENC_FLAGS

[Davide Caratti]

hello Asbjørn,

On Wed, Jun 26, 2024 at 11:55:31AM +0000, Asbjørn Sloth Tønnesen wrote:
> Hi Davide,
> 
> On 6/26/24 10:01 AM, Davide Caratti wrote:
> > On Wed, Jun 26, 2024 at 11:49 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > > 
> > > So, we must htonl() the policy mask in the second hunk in patch 7,something like:
> 
> Good catch.
> 
> > or maybe better (but still untested), use NLA_BE32, like netfilter does in [1]
> > 
> > [1] https://elixir.bootlin.com/linux/latest/A/ident/NF_NAT_RANGE_MASK
> 
> Yes, that is better. It should work, as it triggers a htonl() in nla_validate_mask().

NLA_BE32 proved to fix the byte ordering problem:

 - it allows to set TCA_FLOWER_KEY_ENC_FLAGS_MASK and read it back consistently
 - it sets correct FLOW_DIS_F_* bits in 'enc_control'

FTR, I used this hunk on top of your RFC series:

-- >8 --
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -679,9 +679,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
        [TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]  = { .type = NLA_U16 },
        [TCA_FLOWER_KEY_ENC_UDP_DST_PORT]       = { .type = NLA_U16 },
        [TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]  = { .type = NLA_U16 },
-       [TCA_FLOWER_KEY_FLAGS]          = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_FLAGS]          = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_FLAGS_POLICY_MASK),
-       [TCA_FLOWER_KEY_FLAGS_MASK]     = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_FLAGS_MASK]     = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_FLAGS_POLICY_MASK),
        [TCA_FLOWER_KEY_ICMPV4_TYPE]    = { .type = NLA_U8 },
        [TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
@@ -744,9 +744,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
        [TCA_FLOWER_KEY_SPI_MASK]       = { .type = NLA_U32 },
        [TCA_FLOWER_L2_MISS]            = NLA_POLICY_MAX(NLA_U8, 1),
        [TCA_FLOWER_KEY_CFM]            = { .type = NLA_NESTED },
-       [TCA_FLOWER_KEY_ENC_FLAGS]      = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_ENC_FLAGS]      = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK),
-       [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = NLA_POLICY_MASK(NLA_U32,
+       [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = NLA_POLICY_MASK(NLA_BE32,
                                                          TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK),
 };

-- >8 --

but I think I found another small problem. You removed FLOW_DISSECTOR_KEY_ENC_FLAGS
from TC flower, re-using 'enc_control' instead; however, the FLOW_DISSECTOR_KEY_ENC_CONTROL
bit is set only if flower tries to match 'enc_ipv4' or 'enc_ipv6'. We don't notice
the problem with 'ip_flags' because AFAIS flow dissector copies those bits even with
no relevant FLOW_DISSECTOR_KEY* requested. When matching tunnel flags instead, we
will end up in skb_flow_dissect_tunne_info() with 


	/* A quick check to see if there might be something to do. */
	if (!dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_KEYID) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_CONTROL) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_PORTS) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_IP) &&
	    !dissector_uses_key(flow_dissector,
				FLOW_DISSECTOR_KEY_ENC_OPTS))
		return;

 
^^ a kernel that returns without loading tunnel info, because "there is nothing
to do". So, the attempt to put a valid value in patch9 regardless of the address
family is not sufficient. IMO it can be fixed with the following hunk:

-- >8 --
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2199,7 +2199,8 @@ static void fl_init_dissector(struct flow_dissector *dissector,
        FL_KEY_SET_IF_MASKED(mask, keys, cnt,
                             FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS, enc_ipv6);
        if (FL_KEY_IS_MASKED(mask, enc_ipv4) ||
-           FL_KEY_IS_MASKED(mask, enc_ipv6))
+           FL_KEY_IS_MASKED(mask, enc_ipv6) ||
+           FL_KEY_IS_MASKED(mask, enc_control))
                FL_KEY_SET(keys, cnt, FLOW_DISSECTOR_KEY_ENC_CONTROL,
                           enc_control);
        FL_KEY_SET_IF_MASKED(mask, keys, cnt,
-- >8 --

at least it passes my functional test (that I didn't send yet, together with
iproute bits :(  promise will do that now)

-- 
davide

[RFC PATCH net-next 3/9] net/sched: cls_flower: add policy for TCA_FLOWER_KEY_FLAGS

[Asbjørn Sloth Tønnesen]

This policy guards fl_set_key_flags() from seeing flags
not used in the context of TCA_FLOWER_KEY_FLAGS.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/sched/cls_flower.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 6a5cecfd95619..6a2afc31f038b 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -41,6 +41,10 @@
 #define TCA_FLOWER_KEY_CT_FLAGS_MASK \
 		(TCA_FLOWER_KEY_CT_FLAGS_MAX - 1)
 
+#define TCA_FLOWER_KEY_FLAGS_POLICY_MASK \
+		(TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT | \
+		TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST)
+
 #define TUNNEL_FLAGS_PRESENT (\
 	_BITUL(IP_TUNNEL_CSUM_BIT) |		\
 	_BITUL(IP_TUNNEL_DONT_FRAGMENT_BIT) |	\
@@ -676,8 +680,10 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]	= { .type = NLA_U16 },
-	[TCA_FLOWER_KEY_FLAGS]		= { .type = NLA_U32 },
-	[TCA_FLOWER_KEY_FLAGS_MASK]	= { .type = NLA_U32 },
+	[TCA_FLOWER_KEY_FLAGS]		= NLA_POLICY_MASK(NLA_U32,
+							  TCA_FLOWER_KEY_FLAGS_POLICY_MASK),
+	[TCA_FLOWER_KEY_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
+							  TCA_FLOWER_KEY_FLAGS_POLICY_MASK),
 	[TCA_FLOWER_KEY_ICMPV4_TYPE]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_ICMPV4_CODE]	= { .type = NLA_U8 },
-- 
2.45.1

[RFC PATCH net-next 4/9] flow_dissector: prepare for encapsulated control flags

[Asbjørn Sloth Tønnesen]

Rename skb_flow_dissect_set_enc_addr_type() to
skb_flow_dissect_set_enc_control(), and make it set both
addr_type and flags in FLOW_DISSECTOR_KEY_ENC_CONTROL.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/core/flow_dissector.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 59fe46077b3ca..86a11a01445ad 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -299,9 +299,10 @@ void skb_flow_dissect_meta(const struct sk_buff *skb,
 EXPORT_SYMBOL(skb_flow_dissect_meta);
 
 static void
-skb_flow_dissect_set_enc_addr_type(enum flow_dissector_key_id type,
-				   struct flow_dissector *flow_dissector,
-				   void *target_container)
+skb_flow_dissect_set_enc_control(enum flow_dissector_key_id type,
+				 u32 ctrl_flags,
+				 struct flow_dissector *flow_dissector,
+				 void *target_container)
 {
 	struct flow_dissector_key_control *ctrl;
 
@@ -312,6 +313,7 @@ skb_flow_dissect_set_enc_addr_type(enum flow_dissector_key_id type,
 					 FLOW_DISSECTOR_KEY_ENC_CONTROL,
 					 target_container);
 	ctrl->addr_type = type;
+	ctrl->flags = ctrl_flags;
 }
 
 void
@@ -367,6 +369,7 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 {
 	struct ip_tunnel_info *info;
 	struct ip_tunnel_key *key;
+	u32 ctrl_flags = 0;
 
 	/* A quick check to see if there might be something to do. */
 	if (!dissector_uses_key(flow_dissector,
@@ -395,9 +398,9 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 
 	switch (ip_tunnel_info_af(info)) {
 	case AF_INET:
-		skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV4_ADDRS,
-						   flow_dissector,
-						   target_container);
+		skb_flow_dissect_set_enc_control(FLOW_DISSECTOR_KEY_IPV4_ADDRS,
+						 ctrl_flags, flow_dissector,
+						 target_container);
 		if (dissector_uses_key(flow_dissector,
 				       FLOW_DISSECTOR_KEY_ENC_IPV4_ADDRS)) {
 			struct flow_dissector_key_ipv4_addrs *ipv4;
@@ -410,9 +413,9 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 		}
 		break;
 	case AF_INET6:
-		skb_flow_dissect_set_enc_addr_type(FLOW_DISSECTOR_KEY_IPV6_ADDRS,
-						   flow_dissector,
-						   target_container);
+		skb_flow_dissect_set_enc_control(FLOW_DISSECTOR_KEY_IPV6_ADDRS,
+						 ctrl_flags, flow_dissector,
+						 target_container);
 		if (dissector_uses_key(flow_dissector,
 				       FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS)) {
 			struct flow_dissector_key_ipv6_addrs *ipv6;
-- 
2.45.1

[RFC PATCH net-next 5/9] flow_dissector: set encapsulated control flags from tun_flags

[Asbjørn Sloth Tønnesen]

Set the new FLOW_DIS_F_TUNNEL_* encapsulated control flags, based
on if their counter-part is set in tun_flags.

These flags are not userspace visible yet, as the code to dump
encapsulated control flags will first be added, and later activated
in the following patches.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/core/flow_dissector.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 86a11a01445ad..6e9bd4cecab66 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -396,6 +396,15 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 
 	key = &info->key;
 
+	if (test_bit(IP_TUNNEL_CSUM_BIT, key->tun_flags))
+		ctrl_flags |= FLOW_DIS_F_TUNNEL_CSUM;
+	if (test_bit(IP_TUNNEL_DONT_FRAGMENT_BIT, key->tun_flags))
+		ctrl_flags |= FLOW_DIS_F_TUNNEL_DONT_FRAGMENT;
+	if (test_bit(IP_TUNNEL_OAM_BIT, key->tun_flags))
+		ctrl_flags |= FLOW_DIS_F_TUNNEL_OAM;
+	if (test_bit(IP_TUNNEL_CRIT_OPT_BIT, key->tun_flags))
+		ctrl_flags |= FLOW_DIS_F_TUNNEL_CRIT_OPT;
+
 	switch (ip_tunnel_info_af(info)) {
 	case AF_INET:
 		skb_flow_dissect_set_enc_control(FLOW_DISSECTOR_KEY_IPV4_ADDRS,
-- 
2.45.1

Re: [RFC PATCH net-next 5/9] flow_dissector: set encapsulated control flags from tun_flags

[Asbjørn Sloth Tønnesen]

Hi Davide,

On 6/11/24 11:53 PM, Asbjørn Sloth Tønnesen wrote:
> Set the new FLOW_DIS_F_TUNNEL_* encapsulated control flags, based
> on if their counter-part is set in tun_flags.
> 
> These flags are not userspace visible yet, as the code to dump
> encapsulated control flags will first be added, and later activated
> in the following patches.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>   net/core/flow_dissector.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 86a11a01445ad..6e9bd4cecab66 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -396,6 +396,15 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
>   
>   	key = &info->key;
>   
> +	if (test_bit(IP_TUNNEL_CSUM_BIT, key->tun_flags))
> +		ctrl_flags |= FLOW_DIS_F_TUNNEL_CSUM;
> +	if (test_bit(IP_TUNNEL_DONT_FRAGMENT_BIT, key->tun_flags))
> +		ctrl_flags |= FLOW_DIS_F_TUNNEL_DONT_FRAGMENT;
> +	if (test_bit(IP_TUNNEL_OAM_BIT, key->tun_flags))
> +		ctrl_flags |= FLOW_DIS_F_TUNNEL_OAM;
> +	if (test_bit(IP_TUNNEL_CRIT_OPT_BIT, key->tun_flags))
> +		ctrl_flags |= FLOW_DIS_F_TUNNEL_CRIT_OPT;
> +
>   	switch (ip_tunnel_info_af(info)) {
>   	case AF_INET:
>   		skb_flow_dissect_set_enc_control(FLOW_DISSECTOR_KEY_IPV4_ADDRS,

I am not too familiar with the bitmap helpers, so this is the part of this
RFC series that I am most unsure of.

These should maybe have been test_bit(*_BIT, &key->tun_flags), test_bit() in
general is mostly used with a reference, but with tun_flags it's a mixed bag:

git grep 'test_bit(' | grep tun_flags

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

[RFC PATCH net-next 6/9] net/sched: cls_flower: add tunnel flags to fl_{set,dump}_key_flags()

[Asbjørn Sloth Tønnesen]

Prepare to set and dump the tunnel flags.

This code won't see any of these flags yet, as these flags
aren't allowed by the NLA_POLICY_MASK, and the functions
doesn't get called with encap set to true yet.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/sched/cls_flower.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 6a2afc31f038b..61ab336645caa 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1204,6 +1204,21 @@ static int fl_set_key_flags(struct nlattr **tb, bool encap, u32 *flags_key,
 			TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST,
 			FLOW_DIS_FIRST_FRAG);
 
+	fl_set_key_flag(key, mask, flags_key, flags_mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM,
+			FLOW_DIS_F_TUNNEL_CSUM);
+
+	fl_set_key_flag(key, mask, flags_key, flags_mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT,
+			FLOW_DIS_F_TUNNEL_DONT_FRAGMENT);
+
+	fl_set_key_flag(key, mask, flags_key, flags_mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM, FLOW_DIS_F_TUNNEL_OAM);
+
+	fl_set_key_flag(key, mask, flags_key, flags_mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT,
+			FLOW_DIS_F_TUNNEL_CRIT_OPT);
+
 	return 0;
 }
 
@@ -3127,6 +3142,21 @@ static int fl_dump_key_flags(struct sk_buff *skb, bool encap,
 			TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST,
 			FLOW_DIS_FIRST_FRAG);
 
+	fl_get_key_flag(flags_key, flags_mask, &key, &mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM,
+			FLOW_DIS_F_TUNNEL_CSUM);
+
+	fl_get_key_flag(flags_key, flags_mask, &key, &mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT,
+			FLOW_DIS_F_TUNNEL_DONT_FRAGMENT);
+
+	fl_get_key_flag(flags_key, flags_mask, &key, &mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM, FLOW_DIS_F_TUNNEL_OAM);
+
+	fl_get_key_flag(flags_key, flags_mask, &key, &mask,
+			TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT,
+			FLOW_DIS_F_TUNNEL_CRIT_OPT);
+
 	_key = cpu_to_be32(key);
 	_mask = cpu_to_be32(mask);
 
-- 
2.45.1

[RFC PATCH net-next 7/9] net/sched: cls_flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage

[Asbjørn Sloth Tønnesen]

This patch changes how TCA_FLOWER_KEY_ENC_FLAGS is used, so that
it is used with TCA_FLOWER_KEY_FLAGS_* flags, in the same way as
TCA_FLOWER_KEY_FLAGS is currently used.

Where TCA_FLOWER_KEY_FLAGS uses {key,mask}->control.flags, then
TCA_FLOWER_KEY_ENC_FLAGS now uses {key,mask}->enc_control.flags,
therefore {key,mask}->enc_flags is now unused.

As the generic fl_set_key_flags/fl_dump_key_flags() is used with
encap set to true, then fl_{set,dump}_key_enc_flags() is removed.

This breaks unreleased userspace API (net-next since 2024-06-04).

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/sched/cls_flower.c | 52 +++++++++---------------------------------
 1 file changed, 11 insertions(+), 41 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 61ab336645caa..80e5467b8eaee 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -45,11 +45,11 @@
 		(TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT | \
 		TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST)
 
-#define TUNNEL_FLAGS_PRESENT (\
-	_BITUL(IP_TUNNEL_CSUM_BIT) |		\
-	_BITUL(IP_TUNNEL_DONT_FRAGMENT_BIT) |	\
-	_BITUL(IP_TUNNEL_OAM_BIT) |		\
-	_BITUL(IP_TUNNEL_CRIT_OPT_BIT))
+#define TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK \
+		(TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM | \
+		TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT | \
+		TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM | \
+		TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT)
 
 struct fl_flow_key {
 	struct flow_dissector_key_meta meta;
@@ -746,9 +746,9 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_L2_MISS]		= NLA_POLICY_MAX(NLA_U8, 1),
 	[TCA_FLOWER_KEY_CFM]		= { .type = NLA_NESTED },
 	[TCA_FLOWER_KEY_ENC_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK),
 	[TCA_FLOWER_KEY_ENC_FLAGS_MASK]	= NLA_POLICY_MASK(NLA_U32,
-							  TUNNEL_FLAGS_PRESENT),
+							  TCA_FLOWER_KEY_ENC_FLAGS_POLICY_MASK),
 };
 
 static const struct nla_policy
@@ -1866,21 +1866,6 @@ static int fl_set_key_cfm(struct nlattr **tb,
 	return 0;
 }
 
-static int fl_set_key_enc_flags(struct nlattr **tb, u32 *flags_key,
-				u32 *flags_mask, struct netlink_ext_ack *extack)
-{
-	/* mask is mandatory for flags */
-	if (NL_REQ_ATTR_CHECK(extack, NULL, tb, TCA_FLOWER_KEY_ENC_FLAGS_MASK)) {
-		NL_SET_ERR_MSG(extack, "missing enc_flags mask");
-		return -EINVAL;
-	}
-
-	*flags_key = nla_get_u32(tb[TCA_FLOWER_KEY_ENC_FLAGS]);
-	*flags_mask = nla_get_u32(tb[TCA_FLOWER_KEY_ENC_FLAGS_MASK]);
-
-	return 0;
-}
-
 static int fl_set_key(struct net *net, struct nlattr **tb,
 		      struct fl_flow_key *key, struct fl_flow_key *mask,
 		      struct netlink_ext_ack *extack)
@@ -2123,8 +2108,8 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 	}
 
 	if (tb[TCA_FLOWER_KEY_ENC_FLAGS])
-		ret = fl_set_key_enc_flags(tb, &key->enc_flags.flags,
-					   &mask->enc_flags.flags, extack);
+		ret = fl_set_key_flags(tb, true, &key->enc_control.flags,
+				       &mask->enc_control.flags, extack);
 
 	return ret;
 }
@@ -3381,22 +3366,6 @@ static int fl_dump_key_cfm(struct sk_buff *skb,
 	return err;
 }
 
-static int fl_dump_key_enc_flags(struct sk_buff *skb,
-				 struct flow_dissector_key_enc_flags *key,
-				 struct flow_dissector_key_enc_flags *mask)
-{
-	if (!memchr_inv(mask, 0, sizeof(*mask)))
-		return 0;
-
-	if (nla_put_u32(skb, TCA_FLOWER_KEY_ENC_FLAGS, key->flags))
-		return -EMSGSIZE;
-
-	if (nla_put_u32(skb, TCA_FLOWER_KEY_ENC_FLAGS_MASK, mask->flags))
-		return -EMSGSIZE;
-
-	return 0;
-}
-
 static int fl_dump_key_options(struct sk_buff *skb, int enc_opt_type,
 			       struct flow_dissector_key_enc_opts *enc_opts)
 {
@@ -3699,7 +3668,8 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
 	if (fl_dump_key_cfm(skb, &key->cfm, &mask->cfm))
 		goto nla_put_failure;
 
-	if (fl_dump_key_enc_flags(skb, &key->enc_flags, &mask->enc_flags))
+	if (fl_dump_key_flags(skb, true, key->enc_control.flags,
+			      mask->enc_control.flags))
 		goto nla_put_failure;
 
 	return 0;
-- 
2.45.1

[RFC PATCH net-next 8/9] flow_dissector: cleanup FLOW_DISSECTOR_KEY_ENC_FLAGS

[Asbjørn Sloth Tønnesen]

Now that TCA_FLOWER_KEY_ENC_FLAGS is unused, as it's
former data is stored behind TCA_FLOWER_KEY_ENC_CONTROL,
then remove the last bits of FLOW_DISSECTOR_KEY_ENC_FLAGS.

FLOW_DISSECTOR_KEY_ENC_FLAGS is unreleased, and have been
in net-next since 2024-06-04.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 include/net/flow_dissector.h |  9 ---------
 include/net/ip_tunnels.h     | 12 ------------
 net/core/flow_dissector.c    | 16 +---------------
 net/sched/cls_flower.c       |  3 ---
 4 files changed, 1 insertion(+), 39 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 1f0fddb29a0d8..58717bcaf8496 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -338,14 +338,6 @@ struct flow_dissector_key_cfm {
 #define FLOW_DIS_CFM_MDL_MASK GENMASK(7, 5)
 #define FLOW_DIS_CFM_MDL_MAX 7
 
-/**
- * struct flow_dissector_key_enc_flags: tunnel metadata control flags
- * @flags: tunnel control flags
- */
-struct flow_dissector_key_enc_flags {
-	u32 flags;
-};
-
 enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
@@ -380,7 +372,6 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3 */
 	FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */
 	FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */
-	FLOW_DISSECTOR_KEY_ENC_FLAGS, /* struct flow_dissector_key_enc_flags */
 
 	FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 5a530d4fb02c6..9a6a08ec77139 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -247,18 +247,6 @@ static inline bool ip_tunnel_is_options_present(const unsigned long *flags)
 	return ip_tunnel_flags_intersect(flags, present);
 }
 
-static inline void ip_tunnel_set_encflags_present(unsigned long *flags)
-{
-	IP_TUNNEL_DECLARE_FLAGS(present) = { };
-
-	__set_bit(IP_TUNNEL_CSUM_BIT, present);
-	__set_bit(IP_TUNNEL_DONT_FRAGMENT_BIT, present);
-	__set_bit(IP_TUNNEL_OAM_BIT, present);
-	__set_bit(IP_TUNNEL_CRIT_OPT_BIT, present);
-
-	ip_tunnel_flags_or(flags, flags, present);
-}
-
 static inline bool ip_tunnel_flags_is_be16_compat(const unsigned long *flags)
 {
 	IP_TUNNEL_DECLARE_FLAGS(supp) = { };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 6e9bd4cecab66..5fac97dbbd606 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -385,9 +385,7 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 	    !dissector_uses_key(flow_dissector,
 				FLOW_DISSECTOR_KEY_ENC_IP) &&
 	    !dissector_uses_key(flow_dissector,
-				FLOW_DISSECTOR_KEY_ENC_OPTS) &&
-	    !dissector_uses_key(flow_dissector,
-				FLOW_DISSECTOR_KEY_ENC_FLAGS))
+				FLOW_DISSECTOR_KEY_ENC_OPTS))
 		return;
 
 	info = skb_tunnel_info(skb);
@@ -489,18 +487,6 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 				    IP_TUNNEL_GENEVE_OPT_BIT);
 		enc_opt->dst_opt_type = val < __IP_TUNNEL_FLAG_NUM ? val : 0;
 	}
-
-	if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_FLAGS)) {
-		struct flow_dissector_key_enc_flags *enc_flags;
-		IP_TUNNEL_DECLARE_FLAGS(flags) = {};
-
-		enc_flags = skb_flow_dissector_target(flow_dissector,
-						      FLOW_DISSECTOR_KEY_ENC_FLAGS,
-						      target_container);
-		ip_tunnel_set_encflags_present(flags);
-		ip_tunnel_flags_and(flags, flags, info->key.tun_flags);
-		enc_flags->flags = bitmap_read(flags, IP_TUNNEL_CSUM_BIT, 32);
-	}
 }
 EXPORT_SYMBOL(skb_flow_dissect_tunnel_info);
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 80e5467b8eaee..4e8ae4240922e 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -85,7 +85,6 @@ struct fl_flow_key {
 	struct flow_dissector_key_l2tpv3 l2tpv3;
 	struct flow_dissector_key_ipsec ipsec;
 	struct flow_dissector_key_cfm cfm;
-	struct flow_dissector_key_enc_flags enc_flags;
 } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
 
 struct fl_flow_mask_range {
@@ -2223,8 +2222,6 @@ static void fl_init_dissector(struct flow_dissector *dissector,
 			     FLOW_DISSECTOR_KEY_IPSEC, ipsec);
 	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
 			     FLOW_DISSECTOR_KEY_CFM, cfm);
-	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
-			     FLOW_DISSECTOR_KEY_ENC_FLAGS, enc_flags);
 
 	skb_flow_dissector_init(dissector, keys, cnt);
 }
-- 
2.45.1

[RFC PATCH net-next 9/9] flow_dissector: set encapsulation control flags for non-IP

[Asbjørn Sloth Tønnesen]

Make sure to set encapsulated control flags also for non-IP
packets, such that it's possible to allow matching on e.g.
TUNNEL_OAM on a geneve packet carrying a non-IP packet.

Suggested-by: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 net/core/flow_dissector.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 5fac97dbbd606..41311c8b0b2a4 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -434,6 +434,10 @@ skb_flow_dissect_tunnel_info(const struct sk_buff *skb,
 			ipv6->dst = key->u.ipv6.dst;
 		}
 		break;
+	default:
+		skb_flow_dissect_set_enc_control(0, ctrl_flags, flow_dissector,
+						 target_container);
+		break;
 	}
 
 	if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ENC_KEYID)) {
-- 
2.45.1

Re: [RFC PATCH net-next 0/9] flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage

[Davide Caratti]

hi Asbjørn, thanks for the patch! 

On Tue, Jun 11, 2024 at 11:53:33PM +0000, Asbjørn Sloth Tønnesen wrote:
> This series reworks the recently added TCA_FLOWER_KEY_ENC_FLAGS
> attribute, to be more like TCA_FLOWER_KEY_FLAGS, and use
> the unused u32 flags field in TCA_FLOWER_KEY_ENC_CONTROL,
> instead of adding another u32 in FLOW_DISSECTOR_KEY_ENC_FLAGS.
> 
> I have defined the new FLOW_DIS_F_* and TCA_FLOWER_KEY_FLAGS_*
> flags to coexists for now, so the meaning of the flags field
> in struct flow_dissector_key_control is not depending on the
> context that it is used in. If we run out of bits then we can
> always make split them up later, if we really want to.
> 
> Davide and Ilya would this work for you?

If you are ok with this, I can adjust the iproute code I keep locally,
and the kselftest, re-test, and than report back to the series total
reviewed-by.
It's going a take some days though; and of course, those bit will be
upstreamed as well. 

WDYT?

> Currently this series is only compile-tested.
> 

thanks,
-- 
davide

Re: [RFC PATCH net-next 0/9] flower: rework TCA_FLOWER_KEY_ENC_FLAGS usage

[Asbjørn Sloth Tønnesen]

Hi Davide,

On 6/12/24 3:06 PM, Davide Caratti wrote:
> On Tue, Jun 11, 2024 at 11:53:33PM +0000, Asbjørn Sloth Tønnesen wrote:
>> This series reworks the recently added TCA_FLOWER_KEY_ENC_FLAGS
>> attribute, to be more like TCA_FLOWER_KEY_FLAGS, and use
>> the unused u32 flags field in TCA_FLOWER_KEY_ENC_CONTROL,
>> instead of adding another u32 in FLOW_DISSECTOR_KEY_ENC_FLAGS.

s/TCA_FLOWER_KEY_ENC_CONTROL/FLOW_DISSECTOR_KEY_ENC_CONTROL/

>> I have defined the new FLOW_DIS_F_* and TCA_FLOWER_KEY_FLAGS_*
>> flags to coexists for now, so the meaning of the flags field
>> in struct flow_dissector_key_control is not depending on the
>> context that it is used in. If we run out of bits then we can
>> always make split them up later, if we really want to.

s/always make split/always split/

>> Davide and Ilya would this work for you?
> 
> If you are ok with this, I can adjust the iproute code I keep locally,
> and the kselftest, re-test, and than report back to the series total
> reviewed-by.
> It's going a take some days though; and of course, those bit will be
> upstreamed as well.
> 
> WDYT?

That would be great, there is still quite some time left before net-next
closes, I just wanted to get the ball rolling, with some code, so it is
easier to discuss the implementation details.

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