[PATCH net 0/2] Fix police 'continue' action offload

[PATCH net 0/2] Fix police 'continue' action offload

[Vlad Buslov]

TC act_police with 'continue' action had been supported by mlx5 matchall
classifier offload implementation for some time. However, 'continue' was
assumed implicitly and recently got broken in multiple places. Fix it in
both TC hardware offload validation code and mlx5 driver.

Vlad Buslov (2):
  net/sched: act_police: allow 'continue' action offload
  net/mlx5e: Fix matchall police parameters validation

 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 13 ++++++-------
 include/net/flow_offload.h                      |  1 +
 net/sched/act_police.c                          |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.36.1

[PATCH net 1/2] net/sched: act_police: allow 'continue' action offload

[Vlad Buslov]

Offloading police with action TC_ACT_UNSPEC was erroneously disabled even
though it was supported by mlx5 matchall offload implementation, which
didn't verify the action type but instead assumed that any single police
action attached to matchall classifier is a 'continue' action. Lack of
action type check made it non-obvious what mlx5 matchall implementation
actually supports and caused implementers and reviewers of referenced
commits to disallow it as a part of improved validation code.

Fixes: b8cd5831c61c ("net: flow_offload: add tc police action parameters")
Fixes: b50e462bc22d ("net/sched: act_police: Add extack messages for offload failure")
Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>
---
 include/net/flow_offload.h | 1 +
 net/sched/act_police.c     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 6484095a8c01..7ac313858037 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -152,6 +152,7 @@ enum flow_action_id {
 	FLOW_ACTION_PIPE,
 	FLOW_ACTION_VLAN_PUSH_ETH,
 	FLOW_ACTION_VLAN_POP_ETH,
+	FLOW_ACTION_CONTINUE,
 	NUM_FLOW_ACTIONS,
 };
 
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 79c8901f66ab..b759628a47c2 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -442,7 +442,7 @@ static int tcf_police_act_to_flow_act(int tc_act, u32 *extval,
 		act_id = FLOW_ACTION_JUMP;
 		*extval = tc_act & TC_ACT_EXT_VAL_MASK;
 	} else if (tc_act == TC_ACT_UNSPEC) {
-		NL_SET_ERR_MSG_MOD(extack, "Offload not supported when conform/exceed action is \"continue\"");
+		act_id = FLOW_ACTION_CONTINUE;
 	} else {
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported conform/exceed action offload");
 	}
-- 
2.36.1

[PATCH net 2/2] net/mlx5e: Fix matchall police parameters validation

[Vlad Buslov]

Referenced commit prepared the code for upcoming extension that allows mlx5
to offload police action attached to flower classifier. However, with
regard to existing matchall classifier offload validation should be
reversed as FLOW_ACTION_CONTINUE is the only supported notexceed police
action type. Fix the problem by allowing FLOW_ACTION_CONTINUE for police
action and extend scan_tc_matchall_fdb_actions() to only allow such actions
with matchall classifier.

Fixes: d97b4b105ce7 ("flow_offload: reject offload for all drivers with invalid police parameters")
Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 34bf11cdf90f..3a39a50146dd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4529,13 +4529,6 @@ static int mlx5e_policer_validate(const struct flow_action *action,
 		return -EOPNOTSUPP;
 	}
 
-	if (act->police.notexceed.act_id != FLOW_ACTION_PIPE &&
-	    act->police.notexceed.act_id != FLOW_ACTION_ACCEPT) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Offload not supported when conform action is not pipe or ok");
-		return -EOPNOTSUPP;
-	}
-
 	if (act->police.notexceed.act_id == FLOW_ACTION_ACCEPT &&
 	    !flow_action_is_last_entry(action, act)) {
 		NL_SET_ERR_MSG_MOD(extack,
@@ -4586,6 +4579,12 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_POLICE:
+			if (act->police.notexceed.act_id != FLOW_ACTION_CONTINUE) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Offload not supported when conform action is not continue");
+				return -EOPNOTSUPP;
+			}
+
 			err = mlx5e_policer_validate(flow_action, act, extack);
 			if (err)
 				return err;
-- 
2.36.1

Re: [PATCH net 2/2] net/mlx5e: Fix matchall police parameters validation

[Saeed Mahameed]

On 04 Jul 22:44, Vlad Buslov wrote:
>Referenced commit prepared the code for upcoming extension that allows mlx5
>to offload police action attached to flower classifier. However, with
>regard to existing matchall classifier offload validation should be
>reversed as FLOW_ACTION_CONTINUE is the only supported notexceed police
>action type. Fix the problem by allowing FLOW_ACTION_CONTINUE for police
>action and extend scan_tc_matchall_fdb_actions() to only allow such actions
>with matchall classifier.
>
>Fixes: d97b4b105ce7 ("flow_offload: reject offload for all drivers with invalid police parameters")
>Signed-off-by: Vlad Buslov <vladbu@nvidia.com>

Acked-by: Saeed Mahameed <saeedm@nvidia.com>

Re: [PATCH net 0/2] Fix police 'continue' action offload

[Jakub Kicinski]

On Mon, 4 Jul 2022 22:44:03 +0200 Vlad Buslov wrote:
> TC act_police with 'continue' action had been supported by mlx5 matchall
> classifier offload implementation for some time. However, 'continue' was
> assumed implicitly and recently got broken in multiple places. Fix it in
> both TC hardware offload validation code and mlx5 driver.

Saeed, ack or should they go via your tree? 

The signals are a little mixed since the tree in the subject is "net"
which makes sense but there's no ack / sob from Saeed.

Re: [PATCH net 0/2] Fix police 'continue' action offload

[Saeed Mahameed]

On 05 Jul 17:11, Jakub Kicinski wrote:
>On Mon, 4 Jul 2022 22:44:03 +0200 Vlad Buslov wrote:
>> TC act_police with 'continue' action had been supported by mlx5 matchall
>> classifier offload implementation for some time. However, 'continue' was
>> assumed implicitly and recently got broken in multiple places. Fix it in
>> both TC hardware offload validation code and mlx5 driver.
>
>Saeed, ack or should they go via your tree?
>
>The signals are a little mixed since the tree in the subject is "net"
>which makes sense but there's no ack / sob from Saeed.

Acked, vlad was under pressure to submit this I told him to go ahead and I
will ack on the list.

Re: [PATCH net 0/2] Fix police 'continue' action offload

[Vlad Buslov]

Adding maintainers of other drivers that might have been impacted by the
validation change. If your driver supports 'continue' police action
offload with matchall classifier, then you may want to also relax the
validation restrictions in similar manner as I do in patch 2 for mlx5,
as I also submitted OvS fix to restore matchall police notexceed action
type to 'continue' here:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395561.html

On Mon 04 Jul 2022 at 22:44, Vlad Buslov <vladbu@nvidia.com> wrote:
> TC act_police with 'continue' action had been supported by mlx5 matchall
> classifier offload implementation for some time. However, 'continue' was
> assumed implicitly and recently got broken in multiple places. Fix it in
> both TC hardware offload validation code and mlx5 driver.
>
> Vlad Buslov (2):
>   net/sched: act_police: allow 'continue' action offload
>   net/mlx5e: Fix matchall police parameters validation
>
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 13 ++++++-------
>  include/net/flow_offload.h                      |  1 +
>  net/sched/act_police.c                          |  2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)

Re: [PATCH net 0/2] Fix police 'continue' action offload

[Simon Horman]

+ oss-drivers@corigine.com

On Wed, Jul 06, 2022 at 12:57:48PM +0300, Vlad Buslov wrote:
> On Mon 04 Jul 2022 at 22:44, Vlad Buslov <vladbu@nvidia.com> wrote:
> > TC act_police with 'continue' action had been supported by mlx5 matchall
> > classifier offload implementation for some time. However, 'continue' was
> > assumed implicitly and recently got broken in multiple places. Fix it in
> > both TC hardware offload validation code and mlx5 driver.
> >
> > Vlad Buslov (2):
> >   net/sched: act_police: allow 'continue' action offload
> >   net/mlx5e: Fix matchall police parameters validation
> >
> >  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 13 ++++++-------
> >  include/net/flow_offload.h                      |  1 +
> >  net/sched/act_police.c                          |  2 +-
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> Adding maintainers of other drivers that might have been impacted by the
> validation change. If your driver supports 'continue' police action
> offload with matchall classifier, then you may want to also relax the
> validation restrictions in similar manner as I do in patch 2 for mlx5,
> as I also submitted OvS fix to restore matchall police notexceed action
> type to 'continue' here:
> https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395561.html

Thanks Vlad,

I've looked through the nfp driver and it appears that
nfp_policer_validate() rejects policer offload if the
conform/exceed action (act_id) is not FLOW_ACTION_DROP.

So I think it will not be affected by any new valid values for act_id.
And thus this patchset, which adds FLOW_ACTION_CONTINUE as a valid
value for act_id should not affect the nfp driver.

For patch 1/2:

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net 0/2] Fix police 'continue' action offload

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 4 Jul 2022 22:44:03 +0200 you wrote:
> TC act_police with 'continue' action had been supported by mlx5 matchall
> classifier offload implementation for some time. However, 'continue' was
> assumed implicitly and recently got broken in multiple places. Fix it in
> both TC hardware offload validation code and mlx5 driver.
> 
> Vlad Buslov (2):
>   net/sched: act_police: allow 'continue' action offload
>   net/mlx5e: Fix matchall police parameters validation
> 
> [...]

Here is the summary with links:
  - [net,1/2] net/sched: act_police: allow 'continue' action offload
    https://git.kernel.org/netdev/net/c/052f744f4446
  - [net,2/2] net/mlx5e: Fix matchall police parameters validation
    https://git.kernel.org/netdev/net/c/4d1e07d83ccc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html