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

[PATCH net-next v2 0/4] 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_is_supp_control_flags()
- 8 drivers using flow_rule_has_control_flags()
- 11 drivers using flow_rule_match_has_control_flags()

v2:
- squashed the 3 helper functions to one commmit (requested by Baowen Zheng)
- renamed helper functions to avoid double negatives (suggested by Louis Peens)
- reverse booleans in some functions and callsites to align with new names
- fix autodoc format

v1: https://lore.kernel.org/netdev/20240408130927.78594-1-ast@fiberby.net/

Asbjørn Sloth Tønnesen (4):
  flow_offload: add control flag checking helpers
  nfp: flower: fix check for unsupported control flags
  net: prestera: flower: validate 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 v2 1/4] flow_offload: add control flag checking helpers

[Asbjørn Sloth Tønnesen]

These helpers aim to help drivers, with checking
for the presence of unsupported control flags.

For drivers supporting at least one control flag:
  flow_rule_is_supp_control_flags()

For drivers using flow_rule_match_control(), but not using flags:
  flow_rule_has_control_flags()

For drivers not using flow_rule_match_control():
  flow_rule_match_has_control_flags()

While primarily aimed at FLOW_DISSECTOR_KEY_CONTROL
and flow_rule_match_control(), then the first two
can also be used with FLOW_DISSECTOR_KEY_ENC_CONTROL
and flow_rule_match_enc_control().

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

Only compile-tested.

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

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 314087a5e1818..9ee3ad4a308a8 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -449,6 +449,61 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
 	return dissector_uses_key(rule->match.dissector, key);
 }
 
+/**
+ * flow_rule_is_supp_control_flags() - check for supported control flags
+ * @supp_flags: control flags supported by driver
+ * @ctrl_flags: control 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_is_supp_control_flags(const u32 supp_flags,
+						   const u32 ctrl_flags,
+						   struct netlink_ext_ack *extack)
+{
+	if (likely((ctrl_flags & ~supp_flags) == 0))
+		return true;
+
+	NL_SET_ERR_MSG_FMT_MOD(extack,
+			       "Unsupported match on control.flags %#x",
+			       ctrl_flags);
+
+	return false;
+}
+
+/**
+ * flow_rule_has_control_flags() - check for presence of any control flags
+ * @ctrl_flags: control flags present in rule
+ * @extack: The netlink extended ACK for reporting errors.
+ *
+ * @returns true if control flags are set, false otherwise.
+ */
+static inline bool flow_rule_has_control_flags(const u32 ctrl_flags,
+					       struct netlink_ext_ack *extack)
+{
+	return !flow_rule_is_supp_control_flags(0, ctrl_flags, extack);
+}
+
+/**
+ * flow_rule_match_has_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 control flags are set, false otherwise.
+ */
+static inline bool flow_rule_match_has_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 false;
+
+	flow_rule_match_control(rule, &match);
+
+	return flow_rule_has_control_flags(match.mask->flags, extack);
+}
+
 struct flow_stats {
 	u64	pkts;
 	u64	bytes;
-- 
2.43.0

Re: [PATCH net-next v2 1/4] flow_offload: add control flag checking helpers

[Asbjørn Sloth Tønnesen]

On 4/10/24 9:32 AM, Asbjørn Sloth Tønnesen wrote:
> These helpers aim to help drivers, with checking
> for the presence of unsupported control flags.
> 
> For drivers supporting at least one control flag:
>    flow_rule_is_supp_control_flags()
> 
> For drivers using flow_rule_match_control(), but not using flags:
>    flow_rule_has_control_flags()
> 
> For drivers not using flow_rule_match_control():
>    flow_rule_match_has_control_flags()
> 
> While primarily aimed at FLOW_DISSECTOR_KEY_CONTROL
> and flow_rule_match_control(), then the first two
> can also be used with FLOW_DISSECTOR_KEY_ENC_CONTROL
> and flow_rule_match_enc_control().
> 
> These helpers mirrors the existing check done in sfc:
>    drivers/net/ethernet/sfc/tc.c +276
> 
> Only compile-tested.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>   include/net/flow_offload.h | 55 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 314087a5e1818..9ee3ad4a308a8 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -449,6 +449,61 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
>   	return dissector_uses_key(rule->match.dissector, key);
>   }
>   
> +/**
> + * flow_rule_is_supp_control_flags() - check for supported control flags
> + * @supp_flags: control flags supported by driver
> + * @ctrl_flags: control flags present in rule
> + * @extack: The netlink extended ACK for reporting errors.
> + *
> + * @returns true if only supported control flags are set, false otherwise.
> + */


The kdoc test failed:
 > include/net/flow_offload.h:463: warning: No description found for return value of 'flow_rule_is_supp_control_flags'

For some reason I didn't find kernel-doc.rst, because I searched for autodoc sphinx stuff.

Will do proper "Return:" in v3.

I wasn't able to reproduce the kdoc failure[1] locally:

$ ./scripts/kernel-doc -none include/net/flow_offload.h
$ ./scripts/kernel-doc -none -v include/net/flow_offload.h
include/net/flow_offload.h:345: info: Scanning doc for function flow_offload_has_one_action
include/net/flow_offload.h:453: info: Scanning doc for function flow_rule_is_supp_control_flags
include/net/flow_offload.h:475: info: Scanning doc for function flow_rule_has_control_flags
include/net/flow_offload.h:488: info: Scanning doc for function flow_rule_match_has_control_flags

[1] https://netdev.bots.linux.dev/static/nipa/843159/13623977/kdoc/

pw-bot: changes-requested


> +static inline bool flow_rule_is_supp_control_flags(const u32 supp_flags,
> +						   const u32 ctrl_flags,
> +						   struct netlink_ext_ack *extack)
> +{
> +	if (likely((ctrl_flags & ~supp_flags) == 0))
> +		return true;
> +
> +	NL_SET_ERR_MSG_FMT_MOD(extack,
> +			       "Unsupported match on control.flags %#x",
> +			       ctrl_flags);
> +
> +	return false;
> +}
> +
> +/**
> + * flow_rule_has_control_flags() - check for presence of any control flags
> + * @ctrl_flags: control flags present in rule
> + * @extack: The netlink extended ACK for reporting errors.
> + *
> + * @returns true if control flags are set, false otherwise.
> + */
> +static inline bool flow_rule_has_control_flags(const u32 ctrl_flags,
> +					       struct netlink_ext_ack *extack)
> +{
> +	return !flow_rule_is_supp_control_flags(0, ctrl_flags, extack);
> +}
> +
> +/**
> + * flow_rule_match_has_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 control flags are set, false otherwise.
> + */
> +static inline bool flow_rule_match_has_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 false;
> +
> +	flow_rule_match_control(rule, &match);
> +
> +	return flow_rule_has_control_flags(match.mask->flags, extack);
> +}
> +
>   struct flow_stats {
>   	u64	pkts;
>   	u64	bytes;

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

Re: [PATCH net-next v2 1/4] flow_offload: add control flag checking helpers

[Louis Peens]

On Wed, Apr 10, 2024 at 09:32:22AM +0000, Asbjørn Sloth Tønnesen wrote:
> These helpers aim to help drivers, with checking
> for the presence of unsupported control flags.
> 
> For drivers supporting at least one control flag:
>   flow_rule_is_supp_control_flags()
> 
> For drivers using flow_rule_match_control(), but not using flags:
>   flow_rule_has_control_flags()
> 
> For drivers not using flow_rule_match_control():
>   flow_rule_match_has_control_flags()
> 
> While primarily aimed at FLOW_DISSECTOR_KEY_CONTROL
> and flow_rule_match_control(), then the first two
> can also be used with FLOW_DISSECTOR_KEY_ENC_CONTROL
> and flow_rule_match_enc_control().
> 
> These helpers mirrors the existing check done in sfc:
>   drivers/net/ethernet/sfc/tc.c +276
> 
> Only compile-tested.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---

Looks good from my perspective, thanks for the naming updates:
Reviewed-by: Louis Peens <louis.peens@corigine.com>

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

[Asbjørn Sloth Tønnesen]

Use flow_rule_is_supp_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..8e0a890381b60 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_is_supp_control_flags(NFP_FLOWER_SUPPORTED_CTLFLAGS,
+						     ctl.mask->flags, extack))
 			return -EOPNOTSUPP;
-		}
 	}
 
 	ret_key_ls->key_layer = key_layer;
-- 
2.43.0

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

[Louis Peens]

On Wed, Apr 10, 2024 at 09:32:23AM +0000, Asbjørn Sloth Tønnesen wrote:
> Use flow_rule_is_supp_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(-)

Thanks for updating:
Reviewed-by: Louis Peens <louis.peens@corigine.com>

[PATCH net-next v2 3/4] 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..418101a931490 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_has_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 v2 4/4] 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..ca7830ab168ac 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_has_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