* [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests
@ 2021-11-09 16:17 Petr Machata
2021-11-10 14:39 ` Jamal Hadi Salim
2021-11-10 14:40 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Petr Machata @ 2021-11-09 16:17 UTC (permalink / raw)
To: netdev; +Cc: Petr Machata
In commit 6de6e46d27ef ("cls_flower: Fix inability to match GRE/IPIP
packets"), cls_flower was fixed to match an outer packet of a tunneled
packet as would be expected, rather than dissecting to the inner packet and
matching on that.
This fix uncovered several issues in packet matching in mirroring
selftests:
- in mirror_gre_bridge_1d_vlan.sh and mirror_gre_vlan_bridge_1q.sh, the
vlan_ethtype match is copied around as "ip", even as some of the tests
are running over ip6gretap. This is fixed by using an "ipv6" for
vlan_ethtype in the ip6gretap tests.
- in mirror_gre_changes.sh, a filter to count GRE packets is set up to
match TTL of 50. This used to trigger in the offloaded datapath, where
the envelope TTL was matched, but not in the software datapath, which
considered TTL of the inner packet. Now that both match consistently, all
the packets were double-counted. This is fixed by marking the filter as
skip_hw, leaving only the SW datapath component active.
Fixes: 6de6e46d27ef ("cls_flower: Fix inability to match GRE/IPIP packets")
Signed-off-by: Petr Machata <petrm@nvidia.com>
---
.../net/forwarding/mirror_gre_bridge_1d_vlan.sh | 2 +-
.../selftests/net/forwarding/mirror_gre_changes.sh | 2 +-
.../net/forwarding/mirror_gre_vlan_bridge_1q.sh | 13 +++++++------
.../testing/selftests/net/forwarding/mirror_lib.sh | 3 ++-
.../testing/selftests/net/forwarding/mirror_vlan.sh | 4 ++--
5 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh b/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh
index f8cda822c1ce..1b27f2b0f196 100755
--- a/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh
+++ b/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh
@@ -80,7 +80,7 @@ test_gretap()
test_ip6gretap()
{
- test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ip' \
+ test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ipv6' \
"mirror to ip6gretap"
}
diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
index 472bd023e2a5..aff88f78e339 100755
--- a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
+++ b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
@@ -74,7 +74,7 @@ test_span_gre_ttl()
mirror_install $swp1 ingress $tundev "matchall $tcflags"
tc filter add dev $h3 ingress pref 77 prot $prot \
- flower ip_ttl 50 action pass
+ flower skip_hw ip_ttl 50 action pass
mirror_test v$h1 192.0.2.1 192.0.2.2 $h3 77 0
diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh b/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh
index 880e3ab9d088..c8a9b5bd841f 100755
--- a/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh
+++ b/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh
@@ -141,7 +141,7 @@ test_gretap()
test_ip6gretap()
{
- test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ip' \
+ test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ipv6' \
"mirror to ip6gretap"
}
@@ -218,6 +218,7 @@ test_ip6gretap_forbidden_egress()
test_span_gre_untagged_egress()
{
local tundev=$1; shift
+ local ul_proto=$1; shift
local what=$1; shift
RET=0
@@ -225,7 +226,7 @@ test_span_gre_untagged_egress()
mirror_install $swp1 ingress $tundev "matchall $tcflags"
quick_test_span_gre_dir $tundev ingress
- quick_test_span_vlan_dir $h3 555 ingress
+ quick_test_span_vlan_dir $h3 555 ingress "$ul_proto"
h3_addr_add_del del $h3.555
bridge vlan add dev $swp3 vid 555 pvid untagged
@@ -233,7 +234,7 @@ test_span_gre_untagged_egress()
sleep 5
quick_test_span_gre_dir $tundev ingress
- fail_test_span_vlan_dir $h3 555 ingress
+ fail_test_span_vlan_dir $h3 555 ingress "$ul_proto"
h3_addr_add_del del $h3
bridge vlan add dev $swp3 vid 555
@@ -241,7 +242,7 @@ test_span_gre_untagged_egress()
sleep 5
quick_test_span_gre_dir $tundev ingress
- quick_test_span_vlan_dir $h3 555 ingress
+ quick_test_span_vlan_dir $h3 555 ingress "$ul_proto"
mirror_uninstall $swp1 ingress
@@ -250,12 +251,12 @@ test_span_gre_untagged_egress()
test_gretap_untagged_egress()
{
- test_span_gre_untagged_egress gt4 "mirror to gretap"
+ test_span_gre_untagged_egress gt4 ip "mirror to gretap"
}
test_ip6gretap_untagged_egress()
{
- test_span_gre_untagged_egress gt6 "mirror to ip6gretap"
+ test_span_gre_untagged_egress gt6 ipv6 "mirror to ip6gretap"
}
test_span_gre_fdb_roaming()
diff --git a/tools/testing/selftests/net/forwarding/mirror_lib.sh b/tools/testing/selftests/net/forwarding/mirror_lib.sh
index 6406cd76a19d..3e8ebeff3019 100644
--- a/tools/testing/selftests/net/forwarding/mirror_lib.sh
+++ b/tools/testing/selftests/net/forwarding/mirror_lib.sh
@@ -115,13 +115,14 @@ do_test_span_vlan_dir_ips()
local dev=$1; shift
local vid=$1; shift
local direction=$1; shift
+ local ul_proto=$1; shift
local ip1=$1; shift
local ip2=$1; shift
# Install the capture as skip_hw to avoid double-counting of packets.
# The traffic is meant for local box anyway, so will be trapped to
# kernel.
- vlan_capture_install $dev "skip_hw vlan_id $vid vlan_ethtype ip"
+ vlan_capture_install $dev "skip_hw vlan_id $vid vlan_ethtype $ul_proto"
mirror_test v$h1 $ip1 $ip2 $dev 100 $expect
mirror_test v$h2 $ip2 $ip1 $dev 100 $expect
vlan_capture_uninstall $dev
diff --git a/tools/testing/selftests/net/forwarding/mirror_vlan.sh b/tools/testing/selftests/net/forwarding/mirror_vlan.sh
index 9ab2ce77b332..0b44e148235e 100755
--- a/tools/testing/selftests/net/forwarding/mirror_vlan.sh
+++ b/tools/testing/selftests/net/forwarding/mirror_vlan.sh
@@ -85,9 +85,9 @@ test_tagged_vlan_dir()
RET=0
mirror_install $swp1 $direction $swp3.555 "matchall $tcflags"
- do_test_span_vlan_dir_ips 10 "$h3.555" 111 "$direction" \
+ do_test_span_vlan_dir_ips 10 "$h3.555" 111 "$direction" ip \
192.0.2.17 192.0.2.18
- do_test_span_vlan_dir_ips 0 "$h3.555" 555 "$direction" \
+ do_test_span_vlan_dir_ips 0 "$h3.555" 555 "$direction" ip \
192.0.2.17 192.0.2.18
mirror_uninstall $swp1 $direction
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests
2021-11-09 16:17 [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests Petr Machata
@ 2021-11-10 14:39 ` Jamal Hadi Salim
2021-11-10 15:22 ` Petr Machata
2021-11-10 14:40 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 5+ messages in thread
From: Jamal Hadi Salim @ 2021-11-10 14:39 UTC (permalink / raw)
To: Petr Machata, netdev
Cc: Jiri Pirko, Cong Wang, David Miller, Jakub Kicinski,
komachi.yoshiki, Toshiaki Makita
TBH, the bigger question is why that patch was even applied to begin
with and not much of a discussion that happened.
While it makes sense to look at outer header - every other script
out in the wild (including your selftests) assumed inner header.
Now we have to deal with fallout - which is frustrating.
Or the patch could be simply reverted.
IMO: We could have introduced a construct like outer_ip_src/dst/proto
on tc to handle the new semantics while keeping the old assumptions
in place.
cheers,
jamal
On 2021-11-09 11:17, Petr Machata wrote:
> In commit 6de6e46d27ef ("cls_flower: Fix inability to match GRE/IPIP
> packets"), cls_flower was fixed to match an outer packet of a tunneled
> packet as would be expected, rather than dissecting to the inner packet and
> matching on that.
>
> This fix uncovered several issues in packet matching in mirroring
> selftests:
>
> - in mirror_gre_bridge_1d_vlan.sh and mirror_gre_vlan_bridge_1q.sh, the
> vlan_ethtype match is copied around as "ip", even as some of the tests
> are running over ip6gretap. This is fixed by using an "ipv6" for
> vlan_ethtype in the ip6gretap tests.
>
> - in mirror_gre_changes.sh, a filter to count GRE packets is set up to
> match TTL of 50. This used to trigger in the offloaded datapath, where
> the envelope TTL was matched, but not in the software datapath, which
> considered TTL of the inner packet. Now that both match consistently, all
> the packets were double-counted. This is fixed by marking the filter as
> skip_hw, leaving only the SW datapath component active.
>
> Fixes: 6de6e46d27ef ("cls_flower: Fix inability to match GRE/IPIP packets")
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
> .../net/forwarding/mirror_gre_bridge_1d_vlan.sh | 2 +-
> .../selftests/net/forwarding/mirror_gre_changes.sh | 2 +-
> .../net/forwarding/mirror_gre_vlan_bridge_1q.sh | 13 +++++++------
> .../testing/selftests/net/forwarding/mirror_lib.sh | 3 ++-
> .../testing/selftests/net/forwarding/mirror_vlan.sh | 4 ++--
> 5 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh b/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh
> index f8cda822c1ce..1b27f2b0f196 100755
> --- a/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh
> +++ b/tools/testing/selftests/net/forwarding/mirror_gre_bridge_1d_vlan.sh
> @@ -80,7 +80,7 @@ test_gretap()
>
> test_ip6gretap()
> {
> - test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ip' \
> + test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ipv6' \
> "mirror to ip6gretap"
> }
>
> diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
> index 472bd023e2a5..aff88f78e339 100755
> --- a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
> +++ b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
> @@ -74,7 +74,7 @@ test_span_gre_ttl()
>
> mirror_install $swp1 ingress $tundev "matchall $tcflags"
> tc filter add dev $h3 ingress pref 77 prot $prot \
> - flower ip_ttl 50 action pass
> + flower skip_hw ip_ttl 50 action pass
>
> mirror_test v$h1 192.0.2.1 192.0.2.2 $h3 77 0
>
> diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh b/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh
> index 880e3ab9d088..c8a9b5bd841f 100755
> --- a/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh
> +++ b/tools/testing/selftests/net/forwarding/mirror_gre_vlan_bridge_1q.sh
> @@ -141,7 +141,7 @@ test_gretap()
>
> test_ip6gretap()
> {
> - test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ip' \
> + test_vlan_match gt6 'skip_hw vlan_id 555 vlan_ethtype ipv6' \
> "mirror to ip6gretap"
> }
>
> @@ -218,6 +218,7 @@ test_ip6gretap_forbidden_egress()
> test_span_gre_untagged_egress()
> {
> local tundev=$1; shift
> + local ul_proto=$1; shift
> local what=$1; shift
>
> RET=0
> @@ -225,7 +226,7 @@ test_span_gre_untagged_egress()
> mirror_install $swp1 ingress $tundev "matchall $tcflags"
>
> quick_test_span_gre_dir $tundev ingress
> - quick_test_span_vlan_dir $h3 555 ingress
> + quick_test_span_vlan_dir $h3 555 ingress "$ul_proto"
>
> h3_addr_add_del del $h3.555
> bridge vlan add dev $swp3 vid 555 pvid untagged
> @@ -233,7 +234,7 @@ test_span_gre_untagged_egress()
> sleep 5
>
> quick_test_span_gre_dir $tundev ingress
> - fail_test_span_vlan_dir $h3 555 ingress
> + fail_test_span_vlan_dir $h3 555 ingress "$ul_proto"
>
> h3_addr_add_del del $h3
> bridge vlan add dev $swp3 vid 555
> @@ -241,7 +242,7 @@ test_span_gre_untagged_egress()
> sleep 5
>
> quick_test_span_gre_dir $tundev ingress
> - quick_test_span_vlan_dir $h3 555 ingress
> + quick_test_span_vlan_dir $h3 555 ingress "$ul_proto"
>
> mirror_uninstall $swp1 ingress
>
> @@ -250,12 +251,12 @@ test_span_gre_untagged_egress()
>
> test_gretap_untagged_egress()
> {
> - test_span_gre_untagged_egress gt4 "mirror to gretap"
> + test_span_gre_untagged_egress gt4 ip "mirror to gretap"
> }
>
> test_ip6gretap_untagged_egress()
> {
> - test_span_gre_untagged_egress gt6 "mirror to ip6gretap"
> + test_span_gre_untagged_egress gt6 ipv6 "mirror to ip6gretap"
> }
>
> test_span_gre_fdb_roaming()
> diff --git a/tools/testing/selftests/net/forwarding/mirror_lib.sh b/tools/testing/selftests/net/forwarding/mirror_lib.sh
> index 6406cd76a19d..3e8ebeff3019 100644
> --- a/tools/testing/selftests/net/forwarding/mirror_lib.sh
> +++ b/tools/testing/selftests/net/forwarding/mirror_lib.sh
> @@ -115,13 +115,14 @@ do_test_span_vlan_dir_ips()
> local dev=$1; shift
> local vid=$1; shift
> local direction=$1; shift
> + local ul_proto=$1; shift
> local ip1=$1; shift
> local ip2=$1; shift
>
> # Install the capture as skip_hw to avoid double-counting of packets.
> # The traffic is meant for local box anyway, so will be trapped to
> # kernel.
> - vlan_capture_install $dev "skip_hw vlan_id $vid vlan_ethtype ip"
> + vlan_capture_install $dev "skip_hw vlan_id $vid vlan_ethtype $ul_proto"
> mirror_test v$h1 $ip1 $ip2 $dev 100 $expect
> mirror_test v$h2 $ip2 $ip1 $dev 100 $expect
> vlan_capture_uninstall $dev
> diff --git a/tools/testing/selftests/net/forwarding/mirror_vlan.sh b/tools/testing/selftests/net/forwarding/mirror_vlan.sh
> index 9ab2ce77b332..0b44e148235e 100755
> --- a/tools/testing/selftests/net/forwarding/mirror_vlan.sh
> +++ b/tools/testing/selftests/net/forwarding/mirror_vlan.sh
> @@ -85,9 +85,9 @@ test_tagged_vlan_dir()
> RET=0
>
> mirror_install $swp1 $direction $swp3.555 "matchall $tcflags"
> - do_test_span_vlan_dir_ips 10 "$h3.555" 111 "$direction" \
> + do_test_span_vlan_dir_ips 10 "$h3.555" 111 "$direction" ip \
> 192.0.2.17 192.0.2.18
> - do_test_span_vlan_dir_ips 0 "$h3.555" 555 "$direction" \
> + do_test_span_vlan_dir_ips 0 "$h3.555" 555 "$direction" ip \
> 192.0.2.17 192.0.2.18
> mirror_uninstall $swp1 $direction
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests
2021-11-09 16:17 [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests Petr Machata
2021-11-10 14:39 ` Jamal Hadi Salim
@ 2021-11-10 14:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-10 14:40 UTC (permalink / raw)
To: Petr Machata; +Cc: netdev
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Tue, 9 Nov 2021 17:17:34 +0100 you wrote:
> In commit 6de6e46d27ef ("cls_flower: Fix inability to match GRE/IPIP
> packets"), cls_flower was fixed to match an outer packet of a tunneled
> packet as would be expected, rather than dissecting to the inner packet and
> matching on that.
>
> This fix uncovered several issues in packet matching in mirroring
> selftests:
>
> [...]
Here is the summary with links:
- [net] selftests: forwarding: Fix packet matching in mirroring selftests
https://git.kernel.org/netdev/net/c/af0a51113cb7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests
2021-11-10 14:39 ` Jamal Hadi Salim
@ 2021-11-10 15:22 ` Petr Machata
2021-11-12 11:50 ` Jamal Hadi Salim
0 siblings, 1 reply; 5+ messages in thread
From: Petr Machata @ 2021-11-10 15:22 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Petr Machata, netdev, Jiri Pirko, Cong Wang, David Miller,
Jakub Kicinski, komachi.yoshiki, Toshiaki Makita
Jamal Hadi Salim <jhs@mojatatu.com> writes:
> While it makes sense to look at outer header - every other script
> out in the wild (including your selftests) assumed inner header.
It didn't so much make assumptions as happened to work despite being
buggy. But yeah, fair point.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests
2021-11-10 15:22 ` Petr Machata
@ 2021-11-12 11:50 ` Jamal Hadi Salim
0 siblings, 0 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2021-11-12 11:50 UTC (permalink / raw)
To: Petr Machata
Cc: netdev, Jiri Pirko, Cong Wang, David Miller, Jakub Kicinski,
komachi.yoshiki, Toshiaki Makita
On 2021-11-10 10:22, Petr Machata wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
>> While it makes sense to look at outer header - every other script
>> out in the wild (including your selftests) assumed inner header.
>
> It didn't so much make assumptions as happened to work despite being
> buggy. But yeah, fair point.
>
The problem is before this patch when the tc flower command referenced
an IP address it was for the inner header. With this change the same
command now is implying outer header.
cheers,
jamal
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-12 11:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-09 16:17 [PATCH net] selftests: forwarding: Fix packet matching in mirroring selftests Petr Machata
2021-11-10 14:39 ` Jamal Hadi Salim
2021-11-10 15:22 ` Petr Machata
2021-11-12 11:50 ` Jamal Hadi Salim
2021-11-10 14:40 ` patchwork-bot+netdevbpf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.