[PATCH net 0/2] net: fix the mirred packet drop due to the incorrect dst

[PATCH net 0/2] net: fix the mirred packet drop due to the incorrect dst

[Xin Long]

This issue was found when using OVS HWOL on OVN-k8s. These packets
dropped on rx path were seen with output dst, which should've been
dropped from the skbs when redirecting them.

The 1st patch is to the fix and the 2nd is a selftest to reproduce
and verify it.

Davide Caratti (1):
  selftests: add a test case for mirred egress to ingress

Xin Long (1):
  net: sched: act_mirred: drop dst for the direction from egress to
    ingress

 net/sched/act_mirred.c                        | 11 +++--
 tools/testing/selftests/net/forwarding/config |  1 +
 .../selftests/net/forwarding/tc_actions.sh    | 47 ++++++++++++++++++-
 3 files changed, 55 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH net 1/2] net: sched: act_mirred: drop dst for the direction from egress to ingress

[Xin Long]

Without dropping dst, the packets sent from local mirred/redirected
to ingress will may still use the old dst. ip_rcv() will drop it as
the old dst is for output and its .input is dst_discard.

This patch is to fix by also dropping dst for those packets that are
mirred or redirected from egress to ingress in act_mirred.

Note that we don't drop it for the direction change from ingress to
egress, as on which there might be a user case attaching a metadata
dst by act_tunnel_key that would be used later.

Fixes: b57dc7c13ea9 ("net/sched: Introduce action ct")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_mirred.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index d64b0eeccbe4..efc963ab995a 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -19,6 +19,7 @@
 #include <linux/if_arp.h>
 #include <net/net_namespace.h>
 #include <net/netlink.h>
+#include <net/dst.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 #include <linux/tc_act/tc_mirred.h>
@@ -228,6 +229,7 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	bool want_ingress;
 	bool is_redirect;
 	bool expects_nh;
+	bool at_ingress;
 	int m_eaction;
 	int mac_len;
 	bool at_nh;
@@ -263,7 +265,8 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	 * ingress - that covers the TC S/W datapath.
 	 */
 	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
-	use_reinsert = skb_at_tc_ingress(skb) && is_redirect &&
+	at_ingress = skb_at_tc_ingress(skb);
+	use_reinsert = at_ingress && is_redirect &&
 		       tcf_mirred_can_reinsert(retval);
 	if (!use_reinsert) {
 		skb2 = skb_clone(skb, GFP_ATOMIC);
@@ -271,10 +274,12 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 			goto out;
 	}
 
+	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
+
 	/* All mirred/redirected skbs should clear previous ct info */
 	nf_reset_ct(skb2);
-
-	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
+	if (want_ingress && !at_ingress) /* drop dst for egress -> ingress */
+		skb_dst_drop(skb2);
 
 	expects_nh = want_ingress || !m_mac_header_xmit;
 	at_nh = skb->data == skb_network_header(skb);
-- 
2.27.0

[PATCH net 2/2] selftests: add a test case for mirred egress to ingress

[Xin Long]

From: Davide Caratti <dcaratti@redhat.com>

add a selftest that verifies the correct behavior of TC act_mirred egress
to ingress: in particular, it checks if the dst_entry is removed from skb
before redirect egress -> ingress. The correct behavior is: an ICMP 'echo
request' generated by ping will be received and generate a reply the same
way as the one generated by mausezahn.

Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 tools/testing/selftests/net/forwarding/config |  1 +
 .../selftests/net/forwarding/tc_actions.sh    | 47 ++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/config b/tools/testing/selftests/net/forwarding/config
index a4bd1b087303..697994a9278b 100644
--- a/tools/testing/selftests/net/forwarding/config
+++ b/tools/testing/selftests/net/forwarding/config
@@ -6,6 +6,7 @@ CONFIG_IPV6_MULTIPLE_TABLES=y
 CONFIG_NET_VRF=m
 CONFIG_BPF_SYSCALL=y
 CONFIG_CGROUP_BPF=y
+CONFIG_NET_ACT_CT=m
 CONFIG_NET_ACT_MIRRED=m
 CONFIG_NET_ACT_MPLS=m
 CONFIG_NET_ACT_VLAN=m
diff --git a/tools/testing/selftests/net/forwarding/tc_actions.sh b/tools/testing/selftests/net/forwarding/tc_actions.sh
index d9eca227136b..de19eb6c38f0 100755
--- a/tools/testing/selftests/net/forwarding/tc_actions.sh
+++ b/tools/testing/selftests/net/forwarding/tc_actions.sh
@@ -3,7 +3,7 @@
 
 ALL_TESTS="gact_drop_and_ok_test mirred_egress_redirect_test \
 	mirred_egress_mirror_test matchall_mirred_egress_mirror_test \
-	gact_trap_test"
+	gact_trap_test mirred_egress_to_ingress_test"
 NUM_NETIFS=4
 source tc_common.sh
 source lib.sh
@@ -13,10 +13,12 @@ tcflags="skip_hw"
 h1_create()
 {
 	simple_if_init $h1 192.0.2.1/24
+	tc qdisc add dev $h1 clsact
 }
 
 h1_destroy()
 {
+	tc qdisc del dev $h1 clsact
 	simple_if_fini $h1 192.0.2.1/24
 }
 
@@ -153,6 +155,49 @@ gact_trap_test()
 	log_test "trap ($tcflags)"
 }
 
+mirred_egress_to_ingress_test()
+{
+	RET=0
+
+	tc filter add dev $h1 protocol ip pref 100 handle 100 egress flower \
+		ip_proto icmp src_ip 192.0.2.1 dst_ip 192.0.2.2 type 8 action \
+			ct commit nat src addr 192.0.2.2 pipe \
+			ct clear pipe \
+			ct commit nat dst addr 192.0.2.1 pipe \
+			mirred ingress redirect dev $h1
+
+	tc filter add dev $swp1 protocol ip pref 11 handle 111 ingress flower \
+		ip_proto icmp src_ip 192.0.2.1 dst_ip 192.0.2.2 type 8 action drop
+	tc filter add dev $swp1 protocol ip pref 12 handle 112 ingress flower \
+		ip_proto icmp src_ip 192.0.2.1 dst_ip 192.0.2.2 type 0 action pass
+
+	$MZ $h1 -c 1 -p 64 -a $h1mac -b $h2mac -A 192.0.2.1 -B 192.0.2.2 \
+		-t icmp "ping,id=42,seq=10" -q
+
+	tc_check_packets "dev $h1 egress" 100 1
+	check_err $? "didn't mirror first packet"
+
+	tc_check_packets "dev $swp1 ingress" 111 1
+	check_fail $? "didn't redirect first packet"
+	tc_check_packets "dev $swp1 ingress" 112 1
+	check_err $? "didn't receive reply to first packet"
+
+	ping 192.0.2.2 -I$h1 -c1 -w1 -q 1>/dev/null 2>&1
+
+	tc_check_packets "dev $h1 egress" 100 2
+	check_err $? "didn't mirror second packet"
+	tc_check_packets "dev $swp1 ingress" 111 1
+	check_fail $? "didn't redirect second packet"
+	tc_check_packets "dev $swp1 ingress" 112 2
+	check_err $? "didn't receive reply to second packet"
+
+	tc filter del dev $h1 egress protocol ip pref 100 handle 100 flower
+	tc filter del dev $swp1 ingress protocol ip pref 11 handle 111 flower
+	tc filter del dev $swp1 ingress protocol ip pref 12 handle 112 flower
+
+	log_test "mirred_egress_to_ingress ($tcflags)"
+}
+
 setup_prepare()
 {
 	h1=${NETIFS[p1]}
-- 
2.27.0

Re: [PATCH net 0/2] net: fix the mirred packet drop due to the incorrect dst

[Xin Long]

+Cc Davide.

On Fri, Nov 12, 2021 at 11:33 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> This issue was found when using OVS HWOL on OVN-k8s. These packets
> dropped on rx path were seen with output dst, which should've been
> dropped from the skbs when redirecting them.
>
> The 1st patch is to the fix and the 2nd is a selftest to reproduce
> and verify it.
>
> Davide Caratti (1):
>   selftests: add a test case for mirred egress to ingress
>
> Xin Long (1):
>   net: sched: act_mirred: drop dst for the direction from egress to
>     ingress
>
>  net/sched/act_mirred.c                        | 11 +++--
>  tools/testing/selftests/net/forwarding/config |  1 +
>  .../selftests/net/forwarding/tc_actions.sh    | 47 ++++++++++++++++++-
>  3 files changed, 55 insertions(+), 4 deletions(-)
>
> --
> 2.27.0
>

Re: [PATCH net 0/2] net: fix the mirred packet drop due to the incorrect dst

[Cong Wang]

On Fri, Nov 12, 2021 at 8:33 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> This issue was found when using OVS HWOL on OVN-k8s. These packets
> dropped on rx path were seen with output dst, which should've been
> dropped from the skbs when redirecting them.
>
> The 1st patch is to the fix and the 2nd is a selftest to reproduce
> and verify it.

Acked-by: Cong Wang <cong.wang@bytedance.com>

Thanks.

Re: [PATCH net 0/2] net: fix the mirred packet drop due to the incorrect dst

[mleitner]

On Fri, Nov 12, 2021 at 11:33:10AM -0500, Xin Long wrote:
> This issue was found when using OVS HWOL on OVN-k8s. These packets
> dropped on rx path were seen with output dst, which should've been
> dropped from the skbs when redirecting them.
>
> The 1st patch is to the fix and the 2nd is a selftest to reproduce
> and verify it.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net 0/2] net: fix the mirred packet drop due to the incorrect dst

[Jakub Kicinski]

On Fri, 12 Nov 2021 11:33:10 -0500 Xin Long wrote:
> This issue was found when using OVS HWOL on OVN-k8s. These packets
> dropped on rx path were seen with output dst, which should've been
> dropped from the skbs when redirecting them.
> 
> The 1st patch is to the fix and the 2nd is a selftest to reproduce
> and verify it.

Applied, thanks!