On 11/20/17, 10:02 AM, "Aaron Conole" <acon...@redhat.com> wrote:
Darrell Ball <db...@vmware.com> writes: > On 11/20/17, 9:43 AM, "Aaron Conole" <acon...@redhat.com> wrote: > > Darrell Ball <db...@vmware.com> writes: > > > On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of > > Aaron Conole" <ovs-dev-boun...@openvswitch.org on behalf of > > acon...@redhat.com> wrote: > > > > Darrell Ball <dlu...@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 <dlu...@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. > See comment at the block in question. > > > > }; > > > > > > 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; > > > + } > > > +} This code doesn't care across packets. It simply always sets CS_ESTABLISHED and CS_REPLY_DIR when ctx->reply is true. Did I misunderstand something? There are Two separate ‘if’ conditions, ( The second ‘if’ condition gets executed whether the present packet is a reply or not. Think about it. > > > + > > > 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 > > d...@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= > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev