[ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

Darrell Ball dlu998 at gmail.com
Tue Aug 20 20:24:50 UTC 2019


On Mon, Aug 12, 2019 at 5:22 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

> On Sun, Aug 11, 2019 at 12:30 PM Darrell Ball <dlu998 at gmail.com> wrote:
> >
> > I did some further testing and ran into another issue; in this case,
> one, I did not expect.
> >
> > I added an additional sending of packets at the end of the test after
> this check:
> >
> > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0],
> [dnl
> > ])
> >
> > Below is new code
> >
> > dnl Do it again
> > dnl Send ICMP and UDP traffic
> > 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 5
> >
> > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0],
> [dnl
> > ])
> >
> > The test fails bcoz the second time with short timeouts, the conntrack
> entries are not cleanup up quickly
> >
> > @@ -0,0 +1,2 @@
> >
> +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
>
>
> Thanks for testing!   This test actually catch a kernel bug when ovs
> kernel handles conntrack cache.  It works for me on my ubuntu xenial
> VM with 4.4 kernel.
>
> Since this requires upstream kernel change, it will be backported to
> OVS once the fix gets upstream.
>
> Thanks,
>
> -Yi-Hung
>
> diff --git a/datapath/conntrack.c b/datapath/conntrack.c
> index f85d0a2572f6..ad48b559bcde 100644
> --- a/datapath/conntrack.c
> +++ b/datapath/conntrack.c
> @@ -76,6 +76,7 @@ enum ovs_ct_nat {
>  /* Conntrack action context for execution. */
>  struct ovs_conntrack_info {
>         struct nf_conntrack_helper *helper;
> +       struct nf_ct_timeout *nf_ct_timeout;
>         struct nf_conntrack_zone zone;
>         struct nf_conn *ct;
>         u8 commit : 1;
> @@ -745,6 +746,13 @@ 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 !=
> 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
> @@ -1704,6 +1712,8 @@ 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 =
> nf_ct_timeout_find(ct_info.ct)->timeout;
>         }
>

Forgot to respond to this one earlier.
I did review, unit test and system test these changes and they are fine.

Thanks Darrell



>
>         if (helper) {
>


More information about the dev mailing list