[PATCH net-next 0/6] mlxsw: Two sampling fixes

[PATCH net-next 0/6] mlxsw: Two sampling fixes

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This patchset fixes two bugs in recent sampling submissions.

The first fix, in patch #3, prevents matchall rules with sample action
to be added in front of flower rules on egress. Patches #1-#2 are
preparations meant at avoiding similar bugs in the future. Patch #4 is a
selftest.

The second fix, in patch #5, prevents sampling from being enabled on a
port if already enabled. Patch #6 is a selftest.

Ido Schimmel (6):
  mlxsw: spectrum_matchall: Perform protocol check earlier
  mlxsw: spectrum_matchall: Convert if statements to a switch statement
  mlxsw: spectrum_matchall: Perform priority checks earlier
  selftests: mlxsw: Test matchall failure with protocol match
  mlxsw: spectrum: Veto sampling if already enabled on port
  selftests: mlxsw: Test vetoing of double sampling

 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  5 ++
 .../mellanox/mlxsw/spectrum_matchall.c        | 46 ++++++++++---------
 .../drivers/net/mlxsw/tc_restrictions.sh      | 17 +++++++
 .../selftests/drivers/net/mlxsw/tc_sample.sh  | 30 ++++++++++++
 4 files changed, 76 insertions(+), 22 deletions(-)

-- 
2.30.2

[PATCH net-next 1/6] mlxsw: spectrum_matchall: Perform protocol check earlier

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Perform the protocol check earlier in the function instead of repeating
it for every action. Example:

 # tc filter add dev swp1 ingress proto ip matchall skip_sw action sample group 1 rate 100
 Error: matchall rules only supported with 'all' protocol.
 We have an error talking to the kernel

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum_matchall.c    | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
index ce58a795c6fc..9252e23fd082 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
@@ -238,6 +238,11 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 		flower_prio_valid = true;
 	}
 
+	if (protocol != htons(ETH_P_ALL)) {
+		NL_SET_ERR_MSG(f->common.extack, "matchall rules only supported with 'all' protocol");
+		return -EOPNOTSUPP;
+	}
+
 	mall_entry = kzalloc(sizeof(*mall_entry), GFP_KERNEL);
 	if (!mall_entry)
 		return -ENOMEM;
@@ -247,7 +252,7 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 
 	act = &f->rule->action.entries[0];
 
-	if (act->id == FLOW_ACTION_MIRRED && protocol == htons(ETH_P_ALL)) {
+	if (act->id == FLOW_ACTION_MIRRED) {
 		if (flower_prio_valid && mall_entry->ingress &&
 		    mall_entry->priority >= flower_min_prio) {
 			NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
@@ -262,8 +267,7 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 		}
 		mall_entry->type = MLXSW_SP_MALL_ACTION_TYPE_MIRROR;
 		mall_entry->mirror.to_dev = act->dev;
-	} else if (act->id == FLOW_ACTION_SAMPLE &&
-		   protocol == htons(ETH_P_ALL)) {
+	} else if (act->id == FLOW_ACTION_SAMPLE) {
 		if (flower_prio_valid &&
 		    mall_entry->priority >= flower_min_prio) {
 			NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
-- 
2.30.2

[PATCH net-next 2/6] mlxsw: spectrum_matchall: Convert if statements to a switch statement

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Previous patch moved the protocol check out of the action check, so
these if statements can now be converted to a switch statement. Perform
the conversion.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
index 9252e23fd082..af0a20581a37 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
@@ -252,7 +252,8 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 
 	act = &f->rule->action.entries[0];
 
-	if (act->id == FLOW_ACTION_MIRRED) {
+	switch (act->id) {
+	case FLOW_ACTION_MIRRED:
 		if (flower_prio_valid && mall_entry->ingress &&
 		    mall_entry->priority >= flower_min_prio) {
 			NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
@@ -267,7 +268,8 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 		}
 		mall_entry->type = MLXSW_SP_MALL_ACTION_TYPE_MIRROR;
 		mall_entry->mirror.to_dev = act->dev;
-	} else if (act->id == FLOW_ACTION_SAMPLE) {
+		break;
+	case FLOW_ACTION_SAMPLE:
 		if (flower_prio_valid &&
 		    mall_entry->priority >= flower_min_prio) {
 			NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
@@ -279,7 +281,8 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 		mall_entry->sample.params.truncate = act->sample.truncate;
 		mall_entry->sample.params.trunc_size = act->sample.trunc_size;
 		mall_entry->sample.params.rate = act->sample.rate;
-	} else {
+		break;
+	default:
 		err = -EOPNOTSUPP;
 		goto errout;
 	}
-- 
2.30.2

[PATCH net-next 3/6] mlxsw: spectrum_matchall: Perform priority checks earlier

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Perform the priority check earlier in the function instead of repeating
it for every action. This fixes a bug that allowed matchall rules with
sample action to be added in front of flower rules on egress.

Fixes: 54d0e963f683 ("mlxsw: spectrum_matchall: Add support for egress sampling")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 .../mellanox/mlxsw/spectrum_matchall.c        | 31 ++++++++-----------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
index af0a20581a37..07b371cd9818 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_matchall.c
@@ -250,32 +250,27 @@ int mlxsw_sp_mall_replace(struct mlxsw_sp *mlxsw_sp,
 	mall_entry->priority = f->common.prio;
 	mall_entry->ingress = mlxsw_sp_flow_block_is_ingress_bound(block);
 
+	if (flower_prio_valid && mall_entry->ingress &&
+	    mall_entry->priority >= flower_min_prio) {
+		NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
+		err = -EOPNOTSUPP;
+		goto errout;
+	}
+	if (flower_prio_valid && !mall_entry->ingress &&
+	    mall_entry->priority <= flower_max_prio) {
+		NL_SET_ERR_MSG(f->common.extack, "Failed to add in front of existing flower rules");
+		err = -EOPNOTSUPP;
+		goto errout;
+	}
+
 	act = &f->rule->action.entries[0];
 
 	switch (act->id) {
 	case FLOW_ACTION_MIRRED:
-		if (flower_prio_valid && mall_entry->ingress &&
-		    mall_entry->priority >= flower_min_prio) {
-			NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
-			err = -EOPNOTSUPP;
-			goto errout;
-		}
-		if (flower_prio_valid && !mall_entry->ingress &&
-		    mall_entry->priority <= flower_max_prio) {
-			NL_SET_ERR_MSG(f->common.extack, "Failed to add in front of existing flower rules");
-			err = -EOPNOTSUPP;
-			goto errout;
-		}
 		mall_entry->type = MLXSW_SP_MALL_ACTION_TYPE_MIRROR;
 		mall_entry->mirror.to_dev = act->dev;
 		break;
 	case FLOW_ACTION_SAMPLE:
-		if (flower_prio_valid &&
-		    mall_entry->priority >= flower_min_prio) {
-			NL_SET_ERR_MSG(f->common.extack, "Failed to add behind existing flower rules");
-			err = -EOPNOTSUPP;
-			goto errout;
-		}
 		mall_entry->type = MLXSW_SP_MALL_ACTION_TYPE_SAMPLE;
 		mall_entry->sample.params.psample_group = act->sample.psample_group;
 		mall_entry->sample.params.truncate = act->sample.truncate;
-- 
2.30.2

[PATCH net-next 4/6] selftests: mlxsw: Test matchall failure with protocol match

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The driver can only offload matchall rules that do not match on a
protocol. Test that matchall rules that match on a protocol are vetoed.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 .../drivers/net/mlxsw/tc_restrictions.sh        | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/tc_restrictions.sh b/tools/testing/selftests/drivers/net/mlxsw/tc_restrictions.sh
index b4dbda706c4d..5ec3beb637c8 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/tc_restrictions.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/tc_restrictions.sh
@@ -11,6 +11,7 @@ ALL_TESTS="
 	matchall_mirror_behind_flower_ingress_test
 	matchall_sample_behind_flower_ingress_test
 	matchall_mirror_behind_flower_egress_test
+	matchall_proto_match_test
 	police_limits_test
 	multi_police_test
 "
@@ -291,6 +292,22 @@ matchall_mirror_behind_flower_egress_test()
 	matchall_behind_flower_egress_test "mirror" "mirred egress mirror dev $swp2"
 }
 
+matchall_proto_match_test()
+{
+	RET=0
+
+	tc qdisc add dev $swp1 clsact
+
+	tc filter add dev $swp1 ingress pref 1 proto ip handle 101 \
+		matchall skip_sw \
+		action sample group 1 rate 100
+	check_fail $? "Incorrect success to add matchall rule with protocol match"
+
+	tc qdisc del dev $swp1 clsact
+
+	log_test "matchall protocol match"
+}
+
 police_limits_test()
 {
 	RET=0
-- 
2.30.2

[PATCH net-next 5/6] mlxsw: spectrum: Veto sampling if already enabled on port

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The per-port sampling triggers (i.e., ingress / egress) cannot be
enabled twice. Meaning, the below configuration will not result in
packets being sampled twice:

 # tc filter add dev swp1 ingress matchall skip_sw action sample rate 100 group 1
 # tc filter add dev swp1 ingress matchall skip_sw action sample rate 100 group 1

Therefore, reject such configurations.

Fixes: 90f53c53ec4a ("mlxsw: spectrum: Start using sampling triggers hash table")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index efc7acb4842c..bca0354482cb 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -2668,6 +2668,11 @@ mlxsw_sp_sample_trigger_params_set(struct mlxsw_sp *mlxsw_sp,
 		return mlxsw_sp_sample_trigger_node_init(mlxsw_sp, &key,
 							 params);
 
+	if (trigger_node->trigger.local_port) {
+		NL_SET_ERR_MSG_MOD(extack, "Sampling already enabled on port");
+		return -EINVAL;
+	}
+
 	if (trigger_node->params.psample_group != params->psample_group ||
 	    trigger_node->params.truncate != params->truncate ||
 	    trigger_node->params.rate != params->rate ||
-- 
2.30.2

[PATCH net-next 6/6] selftests: mlxsw: Test vetoing of double sampling

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Test that two sampling rules cannot be configured on the same port with
the same trigger.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 .../selftests/drivers/net/mlxsw/tc_sample.sh  | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/mlxsw/tc_sample.sh b/tools/testing/selftests/drivers/net/mlxsw/tc_sample.sh
index 57b05f042787..093bed088ad0 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/tc_sample.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/tc_sample.sh
@@ -34,6 +34,7 @@ lib_dir=$(dirname $0)/../../../net/forwarding
 ALL_TESTS="
 	tc_sample_rate_test
 	tc_sample_max_rate_test
+	tc_sample_conflict_test
 	tc_sample_group_conflict_test
 	tc_sample_md_iif_test
 	tc_sample_md_lag_iif_test
@@ -272,6 +273,35 @@ tc_sample_max_rate_test()
 	log_test "tc sample maximum rate"
 }
 
+tc_sample_conflict_test()
+{
+	RET=0
+
+	# Test that two sampling rules cannot be configured on the same port,
+	# even when they share the same parameters.
+
+	tc filter add dev $rp1 ingress protocol all pref 1 handle 101 matchall \
+		skip_sw action sample rate 1024 group 1
+	check_err $? "Failed to configure sampling rule"
+
+	tc filter add dev $rp1 ingress protocol all pref 2 handle 102 matchall \
+		skip_sw action sample rate 1024 group 1 &> /dev/null
+	check_fail $? "Managed to configure second sampling rule"
+
+	# Delete the first rule and make sure the second rule can now be
+	# configured.
+
+	tc filter del dev $rp1 ingress protocol all pref 1 handle 101 matchall
+
+	tc filter add dev $rp1 ingress protocol all pref 2 handle 102 matchall \
+		skip_sw action sample rate 1024 group 1
+	check_err $? "Failed to configure sampling rule after deletion"
+
+	log_test "tc sample conflict test"
+
+	tc filter del dev $rp1 ingress protocol all pref 2 handle 102 matchall
+}
+
 tc_sample_group_conflict_test()
 {
 	RET=0
-- 
2.30.2

Re: [PATCH net-next 0/6] mlxsw: Two sampling fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Mon, 29 Mar 2021 13:09:42 +0300 you wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This patchset fixes two bugs in recent sampling submissions.
> 
> The first fix, in patch #3, prevents matchall rules with sample action
> to be added in front of flower rules on egress. Patches #1-#2 are
> preparations meant at avoiding similar bugs in the future. Patch #4 is a
> selftest.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] mlxsw: spectrum_matchall: Perform protocol check earlier
    https://git.kernel.org/netdev/net-next/c/4947e7309a31
  - [net-next,2/6] mlxsw: spectrum_matchall: Convert if statements to a switch statement
    https://git.kernel.org/netdev/net-next/c/50401f292434
  - [net-next,3/6] mlxsw: spectrum_matchall: Perform priority checks earlier
    https://git.kernel.org/netdev/net-next/c/b24303048a6b
  - [net-next,4/6] selftests: mlxsw: Test matchall failure with protocol match
    https://git.kernel.org/netdev/net-next/c/c3572a0b731f
  - [net-next,5/6] mlxsw: spectrum: Veto sampling if already enabled on port
    https://git.kernel.org/netdev/net-next/c/17b96a5cbe3d
  - [net-next,6/6] selftests: mlxsw: Test vetoing of double sampling
    https://git.kernel.org/netdev/net-next/c/7ede22e65832

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