Now that all drivers properly rejects unsupported flower control flags
used with FLOW_DISSECTOR_KEY_CONTROL, then time has come to add similar
checks to the drivers supporting FLOW_DISSECTOR_KEY_ENC_CONTROL.
There are currently just 4 drivers supporting this key, and
3 of those currently doesn't validate encapsulated control flags.
Encapsulation control flags may currently be unused, but they should
still be validated by the drivers, so that drivers will properly
reject any new flags when they are introduced.
This series adds some helper functions, and implements them in all
4 drivers.
NB: It is currently discussed[1] to use encapsulation control flags
for tunnel flags instead of the new FLOW_DISSECTOR_KEY_ENC_FLAGS.
[1] https://lore.kernel.org/netdev/[email protected]/
Asbjørn Sloth Tønnesen (5):
flow_offload: add encapsulation control flag helpers
sfc: use flow_rule_is_supp_enc_control_flags()
net/mlx5e: flower: validate encapsulation control flags
nfp: flower: validate encapsulation control flags
ice: flower: validate encapsulation control flags
drivers/net/ethernet/intel/ice/ice_tc_lib.c | 4 +++
.../ethernet/mellanox/mlx5/core/en/tc_tun.c | 6 ++++
.../ethernet/netronome/nfp/flower/offload.c | 4 +++
drivers/net/ethernet/sfc/tc.c | 5 +--
include/net/flow_offload.h | 35 +++++++++++++++++++
5 files changed, 50 insertions(+), 4 deletions(-)
--
2.45.1
Encapsulation control flags are currently not used anywhere,
so all flags are currently unsupported by all drivers.
This patch adds validation of this assumption, so that
encapsulation flags may be used in the future.
In case any encapsulation control flags are masked,
flow_rule_match_has_enc_control_flags() sets a NL extended
error message, and we return -EOPNOTSUPP.
Only compile tested.
Signed-off-by: Asbjørn Sloth Tønnesen <[email protected]>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
index 8dfb57f712b0d..721f35e597579 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
@@ -850,6 +850,12 @@ int mlx5e_tc_tun_parse(struct net_device *filter_dev,
flow_rule_match_enc_control(rule, &match);
addr_type = match.key->addr_type;
+ if (flow_rule_has_enc_control_flags(match.mask->flags,
+ extack)) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
+
/* For tunnel addr_type used same key id`s as for non-tunnel */
if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
struct flow_match_ipv4_addrs match;
--
2.45.1
Encapsulation control flags are currently not used anywhere,
so all flags are currently unsupported by all drivers.
This patch adds validation of this assumption, so that
encapsulation flags may be used in the future.
In case any encapsulation control flags are masked,
flow_rule_match_has_enc_control_flags() sets a NL extended
error message, and we return -EOPNOTSUPP.
Only compile tested.
Signed-off-by: Asbjørn Sloth Tønnesen <[email protected]>
---
drivers/net/ethernet/netronome/nfp/flower/offload.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 8e0a890381b60..46ffc2c208930 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -321,6 +321,10 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
flow_rule_match_enc_control(rule, &enc_ctl);
+ if (flow_rule_has_enc_control_flags(enc_ctl.mask->flags,
+ extack))
+ return -EOPNOTSUPP;
+
if (enc_ctl.mask->addr_type != 0xffff) {
NL_SET_ERR_MSG_MOD(extack, "unsupported offload: wildcarded protocols on tunnels are not supported");
return -EOPNOTSUPP;
--
2.45.1
This patch adds two new helper functions:
flow_rule_is_supp_enc_control_flags()
flow_rule_has_enc_control_flags()
They are intended to be used for validating encapsulation control
flags, and compliment the similar helpers without "enc_" in the name.
The only difference is that they have their own error message,
to make it obvious if an unsupported flag error is related to
FLOW_DISSECTOR_KEY_CONTROL or FLOW_DISSECTOR_KEY_ENC_CONTROL.
flow_rule_has_enc_control_flags() is for drivers supporting
FLOW_DISSECTOR_KEY_ENC_CONTROL, but not supporting any
encapsulation control flags.
(Currently all 4 drivers fits this category)
flow_rule_is_supp_enc_control_flags() is currently only used
for the above helper, but should also be used by drivers once
they implement at least one encapsulation control flag.
There is AFAICT currently no need for an "enc_" variant of
flow_rule_match_has_control_flags(), as all drivers currently
supporting FLOW_DISSECTOR_KEY_ENC_CONTROL, are already calling
flow_rule_match_enc_control() directly.
Only compile tested.
Signed-off-by: Asbjørn Sloth Tønnesen <[email protected]>
---
include/net/flow_offload.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index ec9f80509f60c..292cd8f4b762a 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -471,6 +471,28 @@ static inline bool flow_rule_is_supp_control_flags(const u32 supp_flags,
return false;
}
+/**
+ * flow_rule_is_supp_enc_control_flags() - check for supported control flags
+ * @supp_enc_flags: encapsulation control flags supported by driver
+ * @enc_ctrl_flags: encapsulation control flags present in rule
+ * @extack: The netlink extended ACK for reporting errors.
+ *
+ * Return: true if only supported control flags are set, false otherwise.
+ */
+static inline bool flow_rule_is_supp_enc_control_flags(const u32 supp_enc_flags,
+ const u32 enc_ctrl_flags,
+ struct netlink_ext_ack *extack)
+{
+ if (likely((enc_ctrl_flags & ~supp_enc_flags) == 0))
+ return true;
+
+ NL_SET_ERR_MSG_FMT_MOD(extack,
+ "Unsupported match on enc_control.flags %#x",
+ enc_ctrl_flags);
+
+ return false;
+}
+
/**
* flow_rule_has_control_flags() - check for presence of any control flags
* @ctrl_flags: control flags present in rule
@@ -484,6 +506,19 @@ static inline bool flow_rule_has_control_flags(const u32 ctrl_flags,
return !flow_rule_is_supp_control_flags(0, ctrl_flags, extack);
}
+/**
+ * flow_rule_has_enc_control_flags() - check for presence of any control flags
+ * @enc_ctrl_flags: encapsulation control flags present in rule
+ * @extack: The netlink extended ACK for reporting errors.
+ *
+ * Return: true if control flags are set, false otherwise.
+ */
+static inline bool flow_rule_has_enc_control_flags(const u32 enc_ctrl_flags,
+ struct netlink_ext_ack *extack)
+{
+ return !flow_rule_is_supp_enc_control_flags(0, enc_ctrl_flags, extack);
+}
+
/**
* flow_rule_match_has_control_flags() - match and check for any control flags
* @rule: The flow_rule under evaluation.
--
2.45.1
On Sun, Jun 09, 2024 at 05:33:54PM +0000, Asbj?rn Sloth T?nnesen wrote:
> Encapsulation control flags are currently not used anywhere,
> so all flags are currently unsupported by all drivers.
>
> This patch adds validation of this assumption, so that
> encapsulation flags may be used in the future.
>
> In case any encapsulation control flags are masked,
> flow_rule_match_has_enc_control_flags() sets a NL extended
> error message, and we return -EOPNOTSUPP.
>
> Only compile tested.
>
> Signed-off-by: Asbj?rn Sloth T?nnesen <[email protected]>
Looks fair to me, thanks.
Signed-off-by: Louis Peens <[email protected]>
> ---
> drivers/net/ethernet/netronome/nfp/flower/offload.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> index 8e0a890381b60..46ffc2c208930 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
> @@ -321,6 +321,10 @@ nfp_flower_calculate_key_layers(struct nfp_app *app,
>
> flow_rule_match_enc_control(rule, &enc_ctl);
>
> + if (flow_rule_has_enc_control_flags(enc_ctl.mask->flags,
> + extack))
> + return -EOPNOTSUPP;
> +
> if (enc_ctl.mask->addr_type != 0xffff) {
> NL_SET_ERR_MSG_MOD(extack, "unsupported offload: wildcarded protocols on tunnels are not supported");
> return -EOPNOTSUPP;
> --
> 2.45.1
>
On Sun, Jun 09, 2024 at 05:33:50PM +0000, Asbj?rn Sloth T?nnesen wrote:
> Now that all drivers properly rejects unsupported flower control flags
> used with FLOW_DISSECTOR_KEY_CONTROL, then time has come to add similar
> checks to the drivers supporting FLOW_DISSECTOR_KEY_ENC_CONTROL.
>
> There are currently just 4 drivers supporting this key, and
> 3 of those currently doesn't validate encapsulated control flags.
>
> Encapsulation control flags may currently be unused, but they should
> still be validated by the drivers, so that drivers will properly
> reject any new flags when they are introduced.
>
> This series adds some helper functions, and implements them in all
> 4 drivers.
>
Reviewed-by: Davide Caratti <[email protected]>
On Sun, 9 Jun 2024 17:33:50 +0000 Asbjørn Sloth Tønnesen wrote:
> Now that all drivers properly rejects unsupported flower control flags
> used with FLOW_DISSECTOR_KEY_CONTROL, then time has come to add similar
> checks to the drivers supporting FLOW_DISSECTOR_KEY_ENC_CONTROL.
Thanks for doing this work!
Do you have any more of such series left? Could we perhaps consider
recording the driver support somewhere in struct net_device and do
the rejecting in the core? That's much easier to extend when adding
new flags than updating all the drivers.
This series I think may not be a great first candidate as IIUC all
drivers would reject so the flag would be half-dead...
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Sun, 9 Jun 2024 17:33:50 +0000 you wrote:
> Now that all drivers properly rejects unsupported flower control flags
> used with FLOW_DISSECTOR_KEY_CONTROL, then time has come to add similar
> checks to the drivers supporting FLOW_DISSECTOR_KEY_ENC_CONTROL.
>
> There are currently just 4 drivers supporting this key, and
> 3 of those currently doesn't validate encapsulated control flags.
>
> [...]
Here is the summary with links:
- [net-next,1/5] flow_offload: add encapsulation control flag helpers
https://git.kernel.org/netdev/net-next/c/b48a1540b73a
- [net-next,2/5] sfc: use flow_rule_is_supp_enc_control_flags()
https://git.kernel.org/netdev/net-next/c/2ede54f8786f
- [net-next,3/5] net/mlx5e: flower: validate encapsulation control flags
https://git.kernel.org/netdev/net-next/c/28d19ec91755
- [net-next,4/5] nfp: flower: validate encapsulation control flags
https://git.kernel.org/netdev/net-next/c/34cdd9847820
- [net-next,5/5] ice: flower: validate encapsulation control flags
https://git.kernel.org/netdev/net-next/c/5a1b015d521d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hi Jakub,
On 6/13/24 1:04 AM, Jakub Kicinski wrote:
> On Sun, 9 Jun 2024 17:33:50 +0000 Asbjørn Sloth Tønnesen wrote:
>> Now that all drivers properly rejects unsupported flower control flags
>> used with FLOW_DISSECTOR_KEY_CONTROL, then time has come to add similar
>> checks to the drivers supporting FLOW_DISSECTOR_KEY_ENC_CONTROL.
>
> Thanks for doing this work!
Thank you, for maintaining the tree!
> Do you have any more of such series left?
Not at the moment, I only stumbled upon this, because I read the code
with an eye on adding a new IS_JUMBO_FRAME flag (patches for which are
almost ready).
Once this[1] currently RFC patch goes in, I might try to move all
control flags in under FLOW_DIS_F_*, to get rid of the then
FLOW_DIS_(IS_FRAGMENT|FIRST_FRAG|ENCAPSULATION|F_*).
[1] [RFC PATCH net-next 1/9] net/sched: flower: define new tunnel flags
https://lore.kernel.org/netdev/[email protected]/
> Could we perhaps consider
> recording the driver support somewhere in struct net_device and do
> the rejecting in the core?
Sure, it could work for the control flags, and used_keys validation,
but I am not sure if it is worth it, as most of the validation is
very specific to the limitations of the different hardware. An easy
first step in that direction would be to move the used_keys checks
behind a helper, and possibly storing used_keys in struct net_device.
> That's much easier to extend when adding
> new flags than updating all the drivers.
That's how it is now, with the new helpers, as all flags are
unsupported, unless the driver specifically supports it.
Any new flag only needs to be added to the core, and drivers only needs
to be updated when they implement offloading support for a flag.
> This series I think may not be a great first candidate as IIUC all
> drivers would reject so the flag would be half-dead...
Correct. I don't know if there is any hardware support planned for the
tunnel-related encapsulation control flags.
--
Best regards
Asbjørn Sloth Tønnesen
Network Engineer
Fiberby - AS42541