[ovs-dev] [patch v1 4/4] conntrack: Change established state to match kernel.

Darrell Ball dball at vmware.com
Mon Nov 20 17:44:51 UTC 2017



On 11/20/17, 9:43 AM, "Aaron Conole" <aconole at redhat.com> wrote:

    Darrell Ball <dball at vmware.com> writes:
    
    > On 11/20/17, 7:46 AM, "ovs-dev-bounces at openvswitch.org on behalf of
    > Aaron Conole" <ovs-dev-bounces at openvswitch.org on behalf of
    > aconole at redhat.com> wrote:
    >
    >     Darrell Ball <dlu998 at gmail.com> writes:
    >     
    >     > Presently, the userpace connection tracker 'established' packet
    >     > state diverges from the kernel and this patch brings them in line.
    >     > The behavior is now that 'established' is only possible after a
    >     > reply packet is seen.
    >     > The previous behavior is hard to notice when rules are written to
    >     > commit a connection in the trusted direction, which is recommended.
    >     >
    >     > A test is added to verify this.
    >     >
    >     > The documentation is updated to describe the new behavior of
    >     > 'established' and also clarify 'new'.
    >     >
    >     > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
    >     > ---
    >     >  lib/conntrack-private.h |  1 +
    >     >  lib/conntrack.c         | 21 ++++++++++++++++-----
    >     >  lib/meta-flow.xml       | 10 +++++++---
    >     >  tests/system-traffic.at | 35 +++++++++++++++++++++++++++++++++++
    >     >  4 files changed, 59 insertions(+), 8 deletions(-)
    >     >
    >     > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
    >     > index ac0198f..1f6a107 100644
    >     > --- a/lib/conntrack-private.h
    >     > +++ b/lib/conntrack-private.h
    >     > @@ -107,6 +107,7 @@ struct conn {
    >     >      uint8_t seq_skew_dir;
    >     >      /* True if alg data connection. */
    >     >      uint8_t alg_related;
    >     > +    uint8_t reply_seen;
    >     
    >     Curious - do you envision using this in the future in other areas of the
    >     code?  Just wondering why add it to the conn struct now.
    >
    > It is needed for this change; specifically, this check
    >> +    if ((*conn)->reply_seen) {
    > The flag is added to keep context across different packets. For example, a packet in
    > the forward direction needs to know that a reply packet has been seen
    > for that connection.
    >
    > I have patch to convert flags to a bitarray but that is for later and
    > there are other considerations,
    > so I deferred it.
    
    I'm still confused, though.  In the code, it looks that having ctx->reply
    will be good enough to set state to established.  This field is only
    set in that condition.  That's why I ask.  I don't understand if there's
    a plan to use this field in the future.

Reply is per packet and not saved across packets.


    
    >     >  };
    >     >  
    >     >  enum ct_update_res {
    >     > diff --git a/lib/conntrack.c b/lib/conntrack.c
    >     > index dea2fed..323114a 100644
    >     > --- a/lib/conntrack.c
    >     > +++ b/lib/conntrack.c
    >     > @@ -928,6 +928,21 @@ nat_res_exhaustion:
    >     >      return NULL;
    >     >  }
    >     >  
    >     > +/* This function is called with the bucket lock held. */
    >     > +static void
    >     > +conn_handle_reply(struct dp_packet *pkt, const struct conn_lookup_ctx *ctx,
    >     > +                  struct conn **conn)
    >     > +{
    >     > +    if (ctx->reply) {
    >     > +        pkt->md.ct_state |= CS_REPLY_DIR;
    >     > +        (*conn)->reply_seen = true;
    >     > +    }
    >     > +    if ((*conn)->reply_seen) {
    >     > +         pkt->md.ct_state |= CS_ESTABLISHED;
    >     > +         pkt->md.ct_state &= ~CS_NEW;
    >     > +    }
    >     > +}
    >     > +
    >     >  static bool
    >     >  conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
    >     >                    struct conn_lookup_ctx *ctx, struct conn **conn,
    >     > @@ -950,11 +965,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
    >     >  
    >     >          switch (res) {
    >     >          case CT_UPDATE_VALID:
    >     > -            pkt->md.ct_state |= CS_ESTABLISHED;
    >     > -            pkt->md.ct_state &= ~CS_NEW;
    >     > -            if (ctx->reply) {
    >     > -                pkt->md.ct_state |= CS_REPLY_DIR;
    >     > -            }
    >     > +            conn_handle_reply(pkt, ctx, conn);
    >     >              break;
    >     >          case CT_UPDATE_INVALID:
    >     >              pkt->md.ct_state = CS_INVALID;
    >     > diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
    >     > index 08ee0ec..33a2ad6 100644
    >     > --- a/lib/meta-flow.xml
    >     > +++ b/lib/meta-flow.xml
    >     > @@ -2493,13 +2493,17 @@ actions=clone(load:0->NXM_OF_IN_PORT[],output:123)
    >     >        <dl>
    >     >          <dt><code>new</code> (0x01)</dt>
    >     >          <dd>
    >     > -          A new connection.  Set to 1 if this is an uncommitted connection.
    >     > +          A new connection.  Set to 1 if this is an uncommitted connection
    >     > +          or a committed connection that has not seen a reply yet.
    >     >          </dd>
    >     >  
    >     >          <dt><code>est</code> (0x02)</dt>
    >     >          <dd>
    >     > -          Part of an existing connection.  Set to 1 if this is a committed
    >     > -          connection.
    >     > +          There are two requirements for a packet to be established, namely,
    >     > +          the associated connection must be committed and a reply must have
    >     > +          been seen.  The reply packet that creates this condition will be
    >     > +          marked as established as well as subsequent packets in either
    >     > +          direction that are associated with the same conntrack entry.
    >     >          </dd>
    >     >  
    >     >          <dt><code>rel</code> (0x04)</dt>
    >     > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
    >     > index fd7b612..cd55406 100644
    >     > --- a/tests/system-traffic.at
    >     > +++ b/tests/system-traffic.at
    >     > @@ -871,6 +871,41 @@ NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.1 | FORMAT_PING], [0],
    >     >  OVS_TRAFFIC_VSWITCHD_STOP
    >     >  AT_CLEANUP
    >     >  
    >     > +AT_SETUP([conntrack - IPv4 ping Check Est state])
    >     > +CHECK_CONNTRACK()
    >     > +OVS_TRAFFIC_VSWITCHD_START()
    >     > +
    >     > +ADD_NAMESPACES(at_ns0, at_ns1)
    >     > +
    >     > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
    >     > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
    >     > +
    >     > +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
    >     > +dnl Check that packet is not established before a reply.
    >     > +AT_DATA([flows.txt], [dnl
    >     > +priority=1,action=drop
    >     > +priority=10,arp,action=normal
    >     > +table=0,priority=10,in_port=2,icmp,ct_state=-trk,action=ct(table=0)
    >     > +table=0,priority=10,in_port=2,icmp,ct_state=+trk+est actions=output:1
    >     > +table=0,priority=10,in_port=1,icmp actions=ct(commit,table=1)
    >     > +table=1,priority=10,in_port=1,icmp,ct_state=+trk+new actions=ct(table=2)
    >     > +table=2,priority=10,in_port=1,icmp,ct_state=+trk+new actions=output:2
    >     > +])
    >     > +
    >     > +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
    >     > +
    >     > +dnl Pings from ns0->ns1 should work fine.
    >     > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl
    >     > +1 packets transmitted, 1 received, 0% packet loss, time 0ms
    >     > +])
    >     > +
    >     > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [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)
    >     > +])
    >     > +
    >     > +OVS_TRAFFIC_VSWITCHD_STOP
    >     > +AT_CLEANUP
    >     > +
    >     >  AT_SETUP([conntrack - IPv6 ping])
    >     >  CHECK_CONNTRACK()
    >     >  OVS_TRAFFIC_VSWITCHD_START()
    >     _______________________________________________
    >     dev mailing list
    >     dev at openvswitch.org
    >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=87H5HK6RLf5U06uQVBVU0PtlxYH_raGxgr49EgY0iZY&s=1bVHxIV3LB7P0GL5vtjPjaUgbYy31jQ2M5zPZxZQSZI&e=
    >     
    



More information about the dev mailing list