Thanks for the patch, Yi-Hung, and the review, Darrell. I pushed this to master.
--Justin > On Sep 29, 2019, at 10:09 AM, Darrell Ball <[email protected]> wrote: > > Thanks for the patch > > Looks good and matches the upstream version, including the rcu deference > fixup. > Thanks for remembering to add the requested test incremental, post fix. > > Darrell > > On Fri, Sep 27, 2019 at 2:14 PM Yi-Hung Wei <[email protected]> wrote: > >> This patch is from the following upstream net-next commit along with >> an updated system traffic test to avoid regression. >> >> Upstream commit: >> commit 7177895154e6a35179d332f4a584d396c50d0612 >> Author: Yi-Hung Wei <[email protected]> >> Date: Thu Aug 22 13:17:50 2019 -0700 >> >> openvswitch: Fix conntrack cache with timeout >> >> This patch addresses a conntrack cache issue with timeout policy. >> Currently, we do not check if the timeout extension is set >> properly in the >> cached conntrack entry. Thus, after packet recirculate from >> conntrack >> action, the timeout policy is not applied properly. This patch >> fixes the >> aforementioned issue. >> >> Fixes: 06bd2bdf19d2 ("openvswitch: Add timeout support to ct >> action") >> Reported-by: kbuild test robot <[email protected]> >> Signed-off-by: Yi-Hung Wei <[email protected]> >> Acked-by: Pravin B Shelar <[email protected]> >> Signed-off-by: David S. Miller <[email protected]> >> >> Signed-off-by: Yi-Hung Wei <[email protected]> >> --- >> datapath/conntrack.c | 13 +++++++++++++ >> tests/system-traffic.at | 18 ++++++++++++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/datapath/conntrack.c b/datapath/conntrack.c >> index 35a183aeb33a..c6d523758ff1 100644 >> --- a/datapath/conntrack.c >> +++ b/datapath/conntrack.c >> @@ -88,6 +88,7 @@ struct ovs_conntrack_info { >> struct md_mark mark; >> struct md_labels labels; >> char timeout[CTNL_TIMEOUT_NAME_MAX]; >> + struct nf_ct_timeout *nf_ct_timeout; >> #ifdef CONFIG_NF_NAT_NEEDED >> struct nf_nat_range2 range; /* Only present for SRC NAT and DST >> NAT. */ >> #endif >> @@ -750,6 +751,14 @@ static bool skb_nfct_cached(struct net *net, >> if (help && rcu_access_pointer(help->helper) != >> info->helper) >> return false; >> } >> + if (info->nf_ct_timeout) { >> + struct nf_conn_timeout *timeout_ext; >> + >> + timeout_ext = nf_ct_timeout_find(ct); >> + if (!timeout_ext || info->nf_ct_timeout != >> + rcu_dereference(timeout_ext->timeout)) >> + return false; >> + } >> /* Force conntrack entry direction to the current packet? */ >> if (info->force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) { >> /* Delete the conntrack entry if confirmed, else just >> release >> @@ -1709,6 +1718,10 @@ int ovs_ct_copy_action(struct net *net, const >> struct nlattr *attr, >> ct_info.timeout)) >> pr_info_ratelimited("Failed to associated timeout " >> "policy `%s'\n", >> ct_info.timeout); >> + else >> + ct_info.nf_ct_timeout = rcu_dereference( >> + nf_ct_timeout_find(ct_info.ct)->timeout); >> + >> } >> >> if (helper) { >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index bfc6bb5b47c7..3d4e365764b5 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -3242,6 +3242,24 @@ sleep 4 >> AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], >> [dnl >> ]) >> >> +dnl Re-send ICMP and UDP traffic to test conntrack cache >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | >> FORMAT_PING], [0], [dnl >> +3 packets transmitted, 3 received, 0% packet loss, time 0ms >> +]) >> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 >> packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 >> actions=resubmit(,0)"]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sort], >> [0], [dnl >> >> +icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=<cleared>,type=0,code=0),zone=5 >> >> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),zone=5 >> +]) >> + >> +dnl Wait until the timeout expire. >> +dnl We intend to wait a bit longer, because conntrack does not recycle >> the entry right after it is expired. >> +sleep 4 >> + >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], >> [dnl >> +]) >> + >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
