[ovs-dev] datapath: Fix conntrack cache with timeout
diff mbox series

Message ID 1569618857-80225-1-git-send-email-yihung.wei@gmail.com
State New
Headers show
Series
  • [ovs-dev] datapath: Fix conntrack cache with timeout
Related show

Commit Message

Yi-Hung Wei Sept. 27, 2019, 9:14 p.m. UTC
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 <yihung.wei@gmail.com>
    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 <lkp@intel.com>
        Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
        Acked-by: Pravin B Shelar <pshelar@ovn.org>
        Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 datapath/conntrack.c    | 13 +++++++++++++
 tests/system-traffic.at | 18 ++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

Darrell Ball Sept. 29, 2019, 5:09 p.m. UTC | #1
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 <yihung.wei@gmail.com> 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 <yihung.wei@gmail.com>
>     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 <lkp@intel.com>
>         Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>         Acked-by: Pravin B Shelar <pshelar@ovn.org>
>         Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
>  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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Justin Pettit Sept. 30, 2019, 8:19 p.m. UTC | #2
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 <dlu998@gmail.com> 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 <yihung.wei@gmail.com> 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 <yihung.wei@gmail.com>
>>    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 <lkp@intel.com>
>>        Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>>        Acked-by: Pravin B Shelar <pshelar@ovn.org>
>>        Signed-off-by: David S. Miller <davem@davemloft.net>
>> 
>> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>> ---
>> 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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

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