Skip to content

Commit

Permalink
Merge branch 'net-fix-the-mirred-packet-drop-due-to-the-incorrect-dst'
Browse files Browse the repository at this point in the history
Xin Long says:

====================
net: fix the mirred packet drop due to the incorrect dst

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.
====================

Link: https://lore.kernel.org/r/cover.1636734751.git.lucien.xin@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
  • Loading branch information
Jakub Kicinski committed Nov 17, 2021
2 parents b0024a0 + 1d127ef commit e4ca782
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 4 deletions.
11 changes: 8 additions & 3 deletions net/sched/act_mirred.c
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -263,18 +265,21 @@ 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);
if (!skb2)
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);
Expand Down
1 change: 1 addition & 0 deletions tools/testing/selftests/net/forwarding/config
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 46 additions & 1 deletion tools/testing/selftests/net/forwarding/tc_actions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand Down Expand Up @@ -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]}
Expand Down

0 comments on commit e4ca782

Please sign in to comment.