[PATCH net-next 0/6] flower: validate control flags

[PATCH net-next 0/6] flower: validate control flags

[Asbjørn Sloth Tønnesen]

I have reviewed the flower control flags code.
In all, but one (sfc), the flags field wasn't
checked properly for unsupported flags.

In this series I have only included a single example
user for each helper function. Once the helpers are in,
I will submit patches for all other drivers implementing
flower.

After which there will be:
- 6 drivers using flow_rule_no_unsupp_control_flags
- 8 drivers using flow_rule_no_control_flags
- 11 drivers using flow_rule_match_no_control_flags

Asbjørn Sloth Tønnesen (6):
  flow_offload: add flow_rule_no_unsupp_control_flags()
  nfp: flower: fix check for unsupported control flags
  flow_offload: add flow_rule_no_control_flags()
  net: prestera: flower: validate control flags
  flow_offload: add flow_rule_match_no_control_flags()
  net: dsa: microchip: ksz9477: flower: validate control flags

 drivers/net/dsa/microchip/ksz9477_tc_flower.c |  3 +
 .../marvell/prestera/prestera_flower.c        |  4 ++
 .../ethernet/netronome/nfp/flower/offload.c   |  6 +-
 include/net/flow_offload.h                    | 55 +++++++++++++++++++
 4 files changed, 65 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Asbjørn Sloth Tønnesen]

This helper can be used by drivers to check for the
presence of unsupported control flags.

It mirrors the existing check done in sfc:
  drivers/net/ethernet/sfc/tc.c +276

This is aimed at drivers, which implements some control flags.

This should also be used by drivers that implement all
current flags, so that future flags will be unsupported
by default.

Only compile-tested.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 include/net/flow_offload.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 314087a5e1818..c1317b14da08c 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
 	return dissector_uses_key(rule->match.dissector, key);
 }
 
+/**
+ * flow_rule_no_unsupp_control_flags() - check for unsupported control flags
+ * @supp_flags: flags supported by driver
+ * @flags: flags present in rule
+ * @extack: The netlink extended ACK for reporting errors.
+ *
+ * Returns true if only supported control flags are set, false otherwise.
+ */
+static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
+						     const u32 flags,
+						     struct netlink_ext_ack *extack)
+{
+	if (likely((flags & ~supp_flags) == 0))
+		return true;
+
+	NL_SET_ERR_MSG_FMT_MOD(extack,
+			       "Unsupported match on control.flags %#x",
+			       flags);
+
+	return false;
+}
+
 struct flow_stats {
 	u64	pkts;
 	u64	bytes;
-- 
2.43.0

[PATCH net-next 2/6] nfp: flower: fix check for unsupported control flags

[Asbjørn Sloth Tønnesen]

Use flow_rule_no_unsupp_control_flags()

Check the mask, not the key, for unsupported control flags.

Only compile-tested, no access to HW

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 0aceef9fe5826..bc7d7c7e68efb 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -527,10 +527,10 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
 		struct flow_match_control ctl;
 
 		flow_rule_match_control(rule, &ctl);
-		if (ctl.key->flags & ~NFP_FLOWER_SUPPORTED_CTLFLAGS) {
-			NL_SET_ERR_MSG_MOD(extack, "unsupported offload: match on unknown control flag");
+
+		if (!flow_rule_no_unsupp_control_flags(NFP_FLOWER_SUPPORTED_CTLFLAGS,
+						       ctl.mask->flags, extack))
 			return -EOPNOTSUPP;
-		}
 	}
 
 	ret_key_ls->key_layer = key_layer;
-- 
2.43.0

[PATCH net-next 3/6] flow_offload: add flow_rule_no_control_flags()

[Asbjørn Sloth Tønnesen]

This helper can be used by drivers, that doesn't support
any control flags, to reject any attempt to install rules
with control flags.

This is aimed at drivers, which uses flow_rule_match_control(),
but doesn't implement any control flags.

Only compile-tested.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 include/net/flow_offload.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index c1317b14da08c..415d225204a1f 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -471,6 +471,19 @@ static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
 	return false;
 }
 
+/**
+ * flow_rule_no_control_flags() - check for presence of any control flags
+ * @flags: flags present in rule
+ * @extack: The netlink extended ACK for reporting errors.
+ *
+ * Returns true if no control flags are set, false otherwise.
+ */
+static inline bool flow_rule_no_control_flags(const u32 flags,
+					      struct netlink_ext_ack *extack)
+{
+	return flow_rule_no_unsupp_control_flags(0, flags, extack);
+}
+
 struct flow_stats {
 	u64	pkts;
 	u64	bytes;
-- 
2.43.0

[PATCH net-next 4/6] net: prestera: flower: validate control flags

[Asbjørn Sloth Tønnesen]

Add check for unsupported control flags.

Only compile-tested, no access to HW.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/ethernet/marvell/prestera/prestera_flower.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_flower.c b/drivers/net/ethernet/marvell/prestera/prestera_flower.c
index 8b9455d8a4f7a..075aed847913d 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_flower.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_flower.c
@@ -229,6 +229,10 @@ static int prestera_flower_parse(struct prestera_flow_block *block,
 
 		flow_rule_match_control(f_rule, &match);
 		addr_type = match.key->addr_type;
+
+		if (!flow_rule_no_control_flags(match.mask->flags,
+						f->common.extack))
+			return -EOPNOTSUPP;
 	}
 
 	if (flow_rule_match_key(f_rule, FLOW_DISSECTOR_KEY_BASIC)) {
-- 
2.43.0

[PATCH net-next 5/6] flow_offload: add flow_rule_match_no_control_flags()

[Asbjørn Sloth Tønnesen]

This helper can by used by drivers, that doesn't look
into FLOW_DISSECTOR_KEY_CONTROL at all.

This is aimed at drivers, which doesn't call flow_rule_match_control()
directly, and therefore doesn't support any control flags.

Only compile-tested.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 include/net/flow_offload.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 415d225204a1f..b427b93d151a9 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -484,6 +484,26 @@ static inline bool flow_rule_no_control_flags(const u32 flags,
 	return flow_rule_no_unsupp_control_flags(0, flags, extack);
 }
 
+/**
+ * flow_rule_match_no_control_flags() - match and check for any control flags
+ * @rule: The flow_rule under evaluation.
+ * @extack: The netlink extended ACK for reporting errors.
+ *
+ * Returns true if no control flags are set, false otherwise.
+ */
+static inline bool flow_rule_match_no_control_flags(struct flow_rule *rule,
+						    struct netlink_ext_ack *extack)
+{
+	struct flow_match_control match;
+
+	if (!flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL))
+		return true;
+
+	flow_rule_match_control(rule, &match);
+
+	return flow_rule_no_control_flags(match.mask->flags, extack);
+}
+
 struct flow_stats {
 	u64	pkts;
 	u64	bytes;
-- 
2.43.0

[PATCH net-next 6/6] net: dsa: microchip: ksz9477: flower: validate control flags

[Asbjørn Sloth Tønnesen]

Add check for unsupported control flags.

Only compile-tested, no access to HW.

Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
 drivers/net/dsa/microchip/ksz9477_tc_flower.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477_tc_flower.c b/drivers/net/dsa/microchip/ksz9477_tc_flower.c
index 8b2f5be667e01..4823a876ad8ab 100644
--- a/drivers/net/dsa/microchip/ksz9477_tc_flower.c
+++ b/drivers/net/dsa/microchip/ksz9477_tc_flower.c
@@ -124,6 +124,9 @@ static int ksz9477_flower_parse_key(struct ksz_device *dev, int port,
 		return -EOPNOTSUPP;
 	}
 
+	if (!flow_rule_match_no_control_flags(rule, extack))
+		return -EOPNOTSUPP;
+
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC) ||
 	    flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
 		ret = ksz9477_flower_parse_key_l2(dev, port, extack, rule,
-- 
2.43.0

RE: [PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Baowen Zheng]

On April 8, 2024 9:09 PM,  Asbjørn wrote:

>This helper can be used by drivers to check for the presence of unsupported
>control flags.
>
>It mirrors the existing check done in sfc:
>  drivers/net/ethernet/sfc/tc.c +276
>
>This is aimed at drivers, which implements some control flags.
>
>This should also be used by drivers that implement all current flags, so that
>future flags will be unsupported by default.
>
>Only compile-tested.
>
>Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>---
> include/net/flow_offload.h | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>314087a5e1818..c1317b14da08c 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct
>flow_rule *rule,
>        return dissector_uses_key(rule->match.dissector, key);  }
>
>+/**
>+ * flow_rule_no_unsupp_control_flags() - check for unsupported control
>+flags
>+ * @supp_flags: flags supported by driver
>+ * @flags: flags present in rule
>+ * @extack: The netlink extended ACK for reporting errors.
>+ *
>+ * Returns true if only supported control flags are set, false otherwise.
>+ */
>+static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
>+                                                    const u32 flags,
>+ 
Hi Asbjørn, thanks for your work, it makes sense for driver check. Will it better to name flags as "ctrl_flags" to make it more clear since it indicates the ctrl_flags in rule and you name it as control.flags in the following print message.
                                                   struct
>+netlink_ext_ack *extack) {
>+       if (likely((flags & ~supp_flags) == 0))
>+               return true;
>+
>+       NL_SET_ERR_MSG_FMT_MOD(extack,
>+                              "Unsupported match on control.flags %#x",
>+                              flags);
>+
>+       return false;
>+}
>+
> struct flow_stats {
>        u64     pkts;
>        u64     bytes;
>--
This should not be included in this patch.
>2.43.0

RE: [PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Baowen Zheng]

On April 8, 2024 9:09 PM, Asbjørn wrote:

>flow_rule_no_unsupp_control_flags()
>
>[Some people who received this message don't often get email from
>ast@fiberby.net. Learn why this is important at
>https://aka.ms/LearnAboutSenderIdentification ]
>
>This helper can be used by drivers to check for the presence of unsupported
>control flags.
>
>It mirrors the existing check done in sfc:
>  drivers/net/ethernet/sfc/tc.c +276
>
>This is aimed at drivers, which implements some control flags.
>
>This should also be used by drivers that implement all current flags, so that
>future flags will be unsupported by default.
>
>Only compile-tested.
>
>Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>---
> include/net/flow_offload.h | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>314087a5e1818..c1317b14da08c 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct
>flow_rule *rule,
>        return dissector_uses_key(rule->match.dissector, key);  }
>
>+/**
>+ * flow_rule_no_unsupp_control_flags() - check for unsupported control
>+flags
>+ * @supp_flags: flags supported by driver
>+ * @flags: flags present in rule
>+ * @extack: The netlink extended ACK for reporting errors.
>+ *
>+ * Returns true if only supported control flags are set, false otherwise.
>+ */
>+static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
>+                                                    const u32 flags,
>+                                                    struct
>+netlink_ext_ack *extack) {
>+       if (likely((flags & ~supp_flags) == 0))
>+               return true;
>+
>+       NL_SET_ERR_MSG_FMT_MOD(extack,
>+                              "Unsupported match on control.flags %#x",
>+                              flags);
>+
>+       return false;
>+}
How about to squash this change with series I patch since they have similar functions for driver to use.
>+
> struct flow_stats {
>        u64     pkts;
>        u64     bytes;
>--
>2.43.0

RE: [PATCH net-next 3/6] flow_offload: add flow_rule_no_control_flags()

[Baowen Zheng]

On April 8, 2024 9:09 PM, Asbjørn wrote:

>This helper can be used by drivers, that doesn't support any control flags, to
>reject any attempt to install rules with control flags.
>
>This is aimed at drivers, which uses flow_rule_match_control(), but doesn't
>implement any control flags.
>
>Only compile-tested.
>
>Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>---
> include/net/flow_offload.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>c1317b14da08c..415d225204a1f 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -471,6 +471,19 @@ static inline bool
>flow_rule_no_unsupp_control_flags(const u32 supp_flags,
> 	return false;
> }
>
>+/**
>+ * flow_rule_no_control_flags() - check for presence of any control
>+flags
>+ * @flags: flags present in rule
>+ * @extack: The netlink extended ACK for reporting errors.
>+ *
>+ * Returns true if no control flags are set, false otherwise.
>+ */
>+static inline bool flow_rule_no_control_flags(const u32 flags,
>+					      struct netlink_ext_ack *extack) {
>+	return flow_rule_no_unsupp_control_flags(0, flags, extack); }
>+
How about to squash this change with series I patch since they have similar functions for driver to use.
> struct flow_stats {
> 	u64	pkts;
> 	u64	bytes;
>--
>2.43.0

Re: [PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Louis Peens]

On Mon, Apr 08, 2024 at 01:09:19PM +0000, Asbjørn Sloth Tønnesen wrote:
> [Some people who received this message don't often get email from ast@fiberby.net. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> This helper can be used by drivers to check for the
> presence of unsupported control flags.
> 
> It mirrors the existing check done in sfc:
>   drivers/net/ethernet/sfc/tc.c +276
> 
> This is aimed at drivers, which implements some control flags.
> 
> This should also be used by drivers that implement all
> current flags, so that future flags will be unsupported
> by default.
> 
> Only compile-tested.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  include/net/flow_offload.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 314087a5e1818..c1317b14da08c 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
>         return dissector_uses_key(rule->match.dissector, key);
>  }
> 
> +/**
> + * flow_rule_no_unsupp_control_flags() - check for unsupported control flags
> + * @supp_flags: flags supported by driver
> + * @flags: flags present in rule
> + * @extack: The netlink extended ACK for reporting errors.
> + *
> + * Returns true if only supported control flags are set, false otherwise.
> + */
> +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
> +                                                    const u32 flags,
> +                                                    struct netlink_ext_ack *extack)
Thanks for the change Asbjørn, I like the series in general. I do have
some nitpicking with the naming of this function, the double negative
makes it a bit hard to read. Especially where it gets used, where it
then reads as:
    'if not no unsupported'

I think something like:
    'if not supported'
or
    'if unsupported'

will read much better - personally I think the first option is the best,
otherwise you might end up with 'if not unsupported', which is also
weird.

Some possible suggestions I can think of:
    flow_rule_control_flags_is_supp()
    flow_rule_is_supp_control_flags()
    flow_rule_check_supp_control_flags()

or perhaps some even better variant of this. I hope it's not just me, if
that's the case please feel free to ignore.

Re: [PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Asbjørn Sloth Tønnesen]

Hi Louis,

On 4/9/24 8:40 AM, Louis Peens wrote:
> On Mon, Apr 08, 2024 at 01:09:19PM +0000, Asbjørn Sloth Tønnesen wrote:
>> [Some people who received this message don't often get email from ast@fiberby.net. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> This helper can be used by drivers to check for the
>> presence of unsupported control flags.
>>
>> It mirrors the existing check done in sfc:
>>    drivers/net/ethernet/sfc/tc.c +276
>>
>> This is aimed at drivers, which implements some control flags.
>>
>> This should also be used by drivers that implement all
>> current flags, so that future flags will be unsupported
>> by default.
>>
>> Only compile-tested.
>>
>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>> ---
>>   include/net/flow_offload.h | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index 314087a5e1818..c1317b14da08c 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
>>          return dissector_uses_key(rule->match.dissector, key);
>>   }
>>
>> +/**
>> + * flow_rule_no_unsupp_control_flags() - check for unsupported control flags
>> + * @supp_flags: flags supported by driver
>> + * @flags: flags present in rule
>> + * @extack: The netlink extended ACK for reporting errors.
>> + *
>> + * Returns true if only supported control flags are set, false otherwise.
>> + */
>> +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
>> +                                                    const u32 flags,
>> +                                                    struct netlink_ext_ack *extack)
> Thanks for the change Asbjørn, I like the series in general. I do have
> some nitpicking with the naming of this function, the double negative
> makes it a bit hard to read. Especially where it gets used, where it
> then reads as:
>      'if not no unsupported'
> 
> I think something like:
>      'if not supported'
> or
>      'if unsupported'
> 
> will read much better - personally I think the first option is the best,
> otherwise you might end up with 'if not unsupported', which is also
> weird.
> 
> Some possible suggestions I can think of:
>      flow_rule_control_flags_is_supp()
>      flow_rule_is_supp_control_flags()
>      flow_rule_check_supp_control_flags()
> 
> or perhaps some even better variant of this. I hope it's not just me, if
> that's the case please feel free to ignore.
I agree, I will update the naming in v2:

flow_rule_no_unsupp_control_flags             => flow_rule_is_supp_control_flags
flow_rule_no_control_flags        + s/no/has/ => flow_rule_has_control_flags
flow_rule_match_no_control_flags  + s/no/has/ => flow_rule_match_has_control_flags

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

Re: [PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Asbjørn Sloth Tønnesen]

Hi Baowen,

On 4/9/24 1:56 AM, Baowen Zheng wrote:
> On April 8, 2024 9:09 PM,  Asbjørn wrote:
> 
>> This helper can be used by drivers to check for the presence of unsupported
>> control flags.
>>
>> It mirrors the existing check done in sfc:
>>   drivers/net/ethernet/sfc/tc.c +276
>>
>> This is aimed at drivers, which implements some control flags.
>>
>> This should also be used by drivers that implement all current flags, so that
>> future flags will be unsupported by default.
>>
>> Only compile-tested.
>>
>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>> ---
>> include/net/flow_offload.h | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>> 314087a5e1818..c1317b14da08c 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -449,6 +449,28 @@ static inline bool flow_rule_match_key(const struct
>> flow_rule *rule,
>>         return dissector_uses_key(rule->match.dissector, key);  }
>>
>> +/**
>> + * flow_rule_no_unsupp_control_flags() - check for unsupported control
>> +flags
>> + * @supp_flags: flags supported by driver
>> + * @flags: flags present in rule
>> + * @extack: The netlink extended ACK for reporting errors.
>> + *
>> + * Returns true if only supported control flags are set, false otherwise.
>> + */
>> +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
>> +                                                    const u32 flags,
>> +
> Hi Asbjørn, thanks for your work, it makes sense for driver check. Will it better to name flags as "ctrl_flags" to make it more clear since it indicates the ctrl_flags in rule and you name it as control.flags in the following print message.

Good point, I will rename that argument in v2.

I copied the error message verbatim from sfc.

>                                                     struct
>> +netlink_ext_ack *extack) {
>> +       if (likely((flags & ~supp_flags) == 0))
>> +               return true;
>> +
>> +       NL_SET_ERR_MSG_FMT_MOD(extack,
>> +                              "Unsupported match on control.flags %#x",
>> +                              flags);
>> +
>> +       return false;
>> +}
>> +
>> struct flow_stats {
>>         u64     pkts;
>>         u64     bytes;
>> --
> This should not be included in this patch.

It's the default 3 lines of context, as is default in git format-patch.

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

Re: [PATCH net-next 3/6] flow_offload: add flow_rule_no_control_flags()

[Asbjørn Sloth Tønnesen]

Hi Baowen,

On 4/9/24 2:09 AM, Baowen Zheng wrote:
> On April 8, 2024 9:09 PM, Asbjørn wrote:
> 
>> This helper can be used by drivers, that doesn't support any control flags, to
>> reject any attempt to install rules with control flags.
>>
>> This is aimed at drivers, which uses flow_rule_match_control(), but doesn't
>> implement any control flags.
>>
>> Only compile-tested.
>>
>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>> ---
>> include/net/flow_offload.h | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index
>> c1317b14da08c..415d225204a1f 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -471,6 +471,19 @@ static inline bool
>> flow_rule_no_unsupp_control_flags(const u32 supp_flags,
>> 	return false;
>> }
>>
>> +/**
>> + * flow_rule_no_control_flags() - check for presence of any control
>> +flags
>> + * @flags: flags present in rule
>> + * @extack: The netlink extended ACK for reporting errors.
>> + *
>> + * Returns true if no control flags are set, false otherwise.
>> + */
>> +static inline bool flow_rule_no_control_flags(const u32 flags,
>> +					      struct netlink_ext_ack *extack) {
>> +	return flow_rule_no_unsupp_control_flags(0, flags, extack); }
>> +
> How about to squash this change with series I patch since they have similar functions for driver to use.

Do you have a link to the series, couldn't find it on the netdev list.

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

RE: [PATCH net-next 3/6] flow_offload: add flow_rule_no_control_flags()

[Baowen Zheng]

>> On April 8, 2024 9:09 PM, Asbjørn wrote:
>>
>>> This helper can be used by drivers, that doesn't support any control
>>> flags, to reject any attempt to install rules with control flags.
>>>
>>> This is aimed at drivers, which uses flow_rule_match_control(), but
>>> doesn't implement any control flags.
>>>
>>> Only compile-tested.
>>>
>>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>>> ---
>>> include/net/flow_offload.h | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>>> index c1317b14da08c..415d225204a1f 100644
>>> --- a/include/net/flow_offload.h
>>> +++ b/include/net/flow_offload.h
>>> @@ -471,6 +471,19 @@ static inline bool
>>> flow_rule_no_unsupp_control_flags(const u32 supp_flags,
>>> 	return false;
>>> }
>>>
>>> +/**
>>> + * flow_rule_no_control_flags() - check for presence of any control
>>> +flags
>>> + * @flags: flags present in rule
>>> + * @extack: The netlink extended ACK for reporting errors.
>>> + *
>>> + * Returns true if no control flags are set, false otherwise.
>>> + */
>>> +static inline bool flow_rule_no_control_flags(const u32 flags,
>>> +					      struct netlink_ext_ack *extack) {
>>> +	return flow_rule_no_unsupp_control_flags(0, flags, extack); }
>>> +
>> How about to squash this change with series I patch since they have similar
>functions for driver to use.
>
>Do you have a link to the series, couldn't find it on the netdev list.
Sorry for confusing you. I mean if you can squash this change with your first patch which introduce the function of flow_rule_no_unsupp_control_flags.
>
>--
>Best regards
>Asbjørn Sloth Tønnesen
>Network Engineer
>Fiberby - AS42541

Re: [PATCH net-next 1/6] flow_offload: add flow_rule_no_unsupp_control_flags()

[Louis Peens]

On Tue, Apr 09, 2024 at 11:13:22AM +0000, Asbjørn Sloth Tønnesen wrote:
> [Some people who received this message don't often get email from ast@fiberby.net. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > > +static inline bool flow_rule_no_unsupp_control_flags(const u32 supp_flags,
> > > +                                                    const u32 flags,
> > > +                                                    struct netlink_ext_ack *extack)
> > Thanks for the change Asbjørn, I like the series in general. I do have
> > some nitpicking with the naming of this function, the double negative
> > makes it a bit hard to read. Especially where it gets used, where it
> > then reads as:
> >      'if not no unsupported'
> > 
> > I think something like:
> >      'if not supported'
> > or
> >      'if unsupported'
> > 
> > will read much better - personally I think the first option is the best,
> > otherwise you might end up with 'if not unsupported', which is also
> > weird.
> > 
> > Some possible suggestions I can think of:
> >      flow_rule_control_flags_is_supp()
> >      flow_rule_is_supp_control_flags()
> >      flow_rule_check_supp_control_flags()
> > 
> > or perhaps some even better variant of this. I hope it's not just me, if
> > that's the case please feel free to ignore.
> I agree, I will update the naming in v2:
> 
> flow_rule_no_unsupp_control_flags             => flow_rule_is_supp_control_flags
> flow_rule_no_control_flags        + s/no/has/ => flow_rule_has_control_flags
> flow_rule_match_no_control_flags  + s/no/has/ => flow_rule_match_has_control_flags
Hi Asbjørn. I like these, I think it will follow much easier, thanks.

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