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

[Aaron Conole]

Lots to digest - responses below

Jan Scheurich  writes:

> Hi Darrel,
> Let me try respond to your points below.
> Regards, Jan
>> -Original Message-
>> From: Darrell Ball [mailto:db...@vmware.com]
>> Sent: Thursday, 30 November, 2017 01:33
>> 
>> The idea of creating an “Conntrack Established state” specific to
>> each protocol layer, as you propose, does not adhere to any protocol
>> specifications that I am aware of.
>> 
>> 1/ UDP and ICMP do not even have such a concept as “Established”
>> connection, so having those specific protocols track “Conntrack
>> Established state” has no technical basis.
>> 
>> 2/ Even if we somehow ignored UDP and ICMP, which we cannot, TCP end
>> to end connection established state is based on a 3-WAY handshake.
>> You propose to have a “Conntrack Established state” based on a 2-way
>> handshake and build knowledge of the kernel derived Conntrack
>> Established state into the TCP state machine itself.
>> I think that is wrong as the TCP state machine should not know
>> anything about the arbitrary “kernel defined established conntrack
>> state”, based on a 2-way handshake or any other Conntrack
>> state. These are different layers.
>> Moreover, it is not even necessary, since the TCP state machine
>> generically tracks packets as valid (as does UDP and ICMP) including
>> replies.
>> So (valid && reply) tells us if we have a valid reply, in a protocol 
>> agnostic manner.
> After having re-read the kernel conntrack specification for userland
> states in http://www.iptables.info/en/connection-state.html (table
> 7-1), I tend to agree with you that the generic definition of
> "established" in the main conntrack module as "valid reply packet
> seen" would be in line with kernel semantics. And we would acknowledge
> your corresponding patch.
>> 3/ This brings us back to the question of bringing userspace
>> conntrack established state in line with the kernel conntrack
>> definition of established.
>> 
>> We don’t see a requirement of blindly copying the kernel; proper
>> modelling and technical merit is more important.
>> The discussion around points “1” and “2” above make this all the more clear.
>> 
>> The kernel uses an arbitrary definition of “conntrack established”
>> based on having received a reply packet and at a midpoint b/w the 2
>> endpoints.
> The Linux kernel definition of ESTABLISHED may be arbitrary and not
> entirely without problems in some corner cases, but we have to admit
> that it is well known and has proven to be useful in very many
> deployments of Linux conntrack at the heart of a stateful firewall
> within an OVS context, but more generally outside OVS. So it has set a
> de-facto standard.
>> The userspace connection tracker defines Established as any packet
>> that hits an existing conntrack entry.
>> Everyone thinks this definition is semantically correct and
>> intuitive. I also think that even you agree with this, based on your
>> previous e-mails and discussions.
>> The existence of a conntrack entry vs none is the key difference
>> that should be tracked for EST and this is exactly what the
>> userspace connection tracker does.
> The userspace conntrack definition of ESTABLISHED would be simple and
> logical, if we were starting from a clean slate. Unfortunately we
> aren't. The new semantics clashes with the Linux de-facto standard and
> the OVS legacy. In my eyes that is where simplicity ends and we create
> problems for users and developers.
> I would also argue that the userspace definition is just as arbitrary
> as the Linux kernel. It is different, perhaps slightly simpler, but I
> wouldn't say it is more correct than the kernel.
>> 
>> Furthermore, as discussed on another thread, any pipeline written
>> properly, where the commit of a conntrack entry happens in the
>> trusted direction, will never observe a difference b/w kernel and
>> userspace EST, since forward direction packets in the trusted
>> direction are always trusted and should not be subject to drop
>> whether NEW or EST.
>> 
>> Hence, I think we should keep userspace conntrack EST as is for now.
>> We can always revisit if we find differences w.r.t. real/proper conntrack 
>> pipelines.
>> Don’t be surprised, if I look at a pipeline and say it is not proper
>> with a reason provided, as I have done earlier. I might even say
>> something like “since your VM accepts all incoming traffic without
>> filtering, why do you even use conntrack rules for that VM”, for
>> example.
>> I know there is an infinite number of broken conntrack pipelines that can be 
>> written.
> I don't think we should lead this discussion based on whether a
> specific example conntrack pipeline is considered "proper" or
> not. Features that OVS offers to its users should have a well-defined
> semantics, no matter what datapath is present and how users choose to
> use them.
> If OVS cannot guarantee well-defined semantics across datapaths, it
> should warn users so that 

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

[Jan Scheurich]

Hi Darrel,

Let me try respond to your points below.

Regards, Jan

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Thursday, 30 November, 2017 01:33
> 
> The idea of creating an “Conntrack Established state” specific to each 
> protocol layer, as you propose, does not adhere to any protocol 
> specifications that I am aware of.
> 
> 1/ UDP and ICMP do not even have such a concept as “Established” connection, 
> so having those specific protocols track “Conntrack Established state” has no 
>  technical basis.
> 
> 2/ Even if we somehow ignored UDP and ICMP, which we cannot, TCP end to end 
> connection established state is based on a 3-WAY handshake.
> You propose to have a “Conntrack Established state” based on a 2-way 
> handshake and build knowledge of the kernel derived Conntrack Established 
> state into the TCP state machine itself.
> I think that is wrong as the TCP state machine should not know anything about 
> the arbitrary “kernel defined established conntrack state”, based on a 2-way 
> handshake or any other Conntrack state. These are different layers.
> Moreover, it is not even necessary, since the TCP state machine generically 
> tracks packets as valid (as does UDP and ICMP) including replies.
> So (valid && reply) tells us if we have a valid reply, in a protocol agnostic 
> manner.

After having re-read the kernel conntrack specification for userland states in 
http://www.iptables.info/en/connection-state.html (table 7-1), I tend to agree 
with you that the generic definition of "established" in the main conntrack 
module as "valid reply packet seen" would be in line with kernel semantics. And 
we would acknowledge your corresponding patch.

> 3/ This brings us back to the question of bringing userspace conntrack 
> established state in line with the kernel conntrack definition of established.
> 
> We don’t see a requirement of blindly copying the kernel; proper modelling 
> and technical merit is more important.
> The discussion around points “1” and “2” above make this all the more clear.
> 
> The kernel uses an arbitrary definition of “conntrack established” based on 
> having received a reply packet and at a midpoint b/w the 2 endpoints.

The Linux kernel definition of ESTABLISHED may be arbitrary and not entirely 
without problems in some corner cases, but we have to admit that it is well 
known and has proven to be useful in very many deployments of Linux conntrack 
at the heart of a stateful firewall within an OVS context, but more generally 
outside OVS. So it has set a de-facto standard.

> The userspace connection tracker defines Established as any packet that hits 
> an existing conntrack entry.
> Everyone thinks this definition is semantically correct and intuitive. I also 
> think that even you agree with this, based on your previous e-mails and 
> discussions.
> The existence of a conntrack entry vs none is the key difference that should 
> be tracked for EST and this is exactly what the userspace connection tracker 
> does.

The userspace conntrack definition of ESTABLISHED would be simple and logical, 
if we were starting from a clean slate. Unfortunately we aren't. The new 
semantics clashes with the Linux de-facto standard and the OVS legacy. In my 
eyes that is where simplicity ends and we create problems for users and 
developers.

I would also argue that the userspace definition is just as arbitrary as the 
Linux kernel. It is different, perhaps slightly simpler, but I wouldn't say it 
is more correct than the kernel.
 
> 
> Furthermore, as discussed on another thread, any pipeline written properly, 
> where the commit of a conntrack entry happens in the trusted direction, will 
> never observe a difference b/w kernel and userspace EST, since forward 
> direction packets in the trusted direction are always trusted and should not 
> be subject to drop whether NEW or EST.
> 
> Hence, I think we should keep userspace conntrack EST as is for now.
> We can always revisit if we find differences w.r.t. real/proper conntrack 
> pipelines.
> Don’t be surprised, if I look at a pipeline and say it is not proper with a 
> reason provided, as I have done earlier. I might even say something like 
> “since your VM accepts all incoming traffic without filtering, why do you 
> even use conntrack rules for that VM”, for example.
> I know there is an infinite number of broken conntrack pipelines that can be 
> written.

I don't think we should lead this discussion based on whether a specific 
example conntrack pipeline is considered "proper" or not. Features that OVS 
offers to its users should have a well-defined semantics, no matter what 
datapath is present and how users choose to use them.

If OVS cannot guarantee well-defined semantics across datapaths, it should warn 
users so that they can avoid pitfalls and limit themselves to an uncritical 
subset.

With the current mismatch between kernel and userspace conntrack, the 
documentation would 

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

[Darrell Ball]

The idea of creating an “Conntrack Established state” specific to each protocol 
layer, as you propose, does not adhere to any protocol specifications that
I am aware of.

1/ UDP and ICMP do not even have such a concept as “Established” connection, so 
having those specific protocols track “Conntrack Established state” has no
 technical basis.

2/ Even if we somehow ignored UDP and ICMP, which we cannot, TCP end to end 
connection established state is based on a 3-WAY handshake.
You propose to have a “Conntrack Established state” based on a 2-way handshake 
and build knowledge of the kernel derived Conntrack Established state
into the TCP state machine itself.
I think that is wrong as the TCP state machine should not know anything about 
the arbitrary “kernel defined established conntrack state”,
based on a 2-way handshake or any other Conntrack state. These are different 
layers.
Moreover, it is not even necessary, since the TCP state machine generically 
tracks packets as valid (as does UDP and ICMP) including replies.
So (valid && reply) tells us if we have a valid reply, in a protocol agnostic 
manner. 

3/ This brings us back to the question of bringing userspace conntrack 
established state in line with the kernel conntrack definition of established.

We don’t see a requirement of blindly copying the kernel; proper modelling and 
technical merit is more important. 
The discussion around points “1” and “2” above make this all the more clear.

The kernel uses an arbitrary definition of “conntrack established” based on 
having received a reply packet and at a midpoint b/w the 2 endpoints.

The userspace connection tracker defines Established as any packet that hits an 
existing conntrack entry.
Everyone thinks this definition is semantically correct and intuitive. I also 
think that even you agree with this, based on your previous e-mails and 
discussions.
The existence of a conntrack entry vs none is the key difference that should be 
tracked for EST and this is exactly what the userspace connection tracker does.

Furthermore, as discussed on another thread, any pipeline written properly, 
where the commit of a conntrack entry happens in the trusted
direction, will never observe a difference b/w kernel and userspace EST, since 
forward direction packets in the trusted direction are always trusted and
should not be subject to drop whether NEW or EST.

Hence, I think we should keep userspace conntrack EST as is for now.
We can always revisit if we find differences w.r.t. real/proper conntrack 
pipelines.
Don’t be surprised, if I look at a pipeline and say it is not proper with a 
reason provided, as I have done earlier. I might even say something like
“since your VM accepts all incoming traffic without filtering, why do you even 
use conntrack rules for that VM”, for example.
I know there is an infinite number of broken conntrack pipelines that can be 
written.

Thanks Darrell
 

On 11/20/17, 6:13 PM, "Darrell Ball"  wrote:



On 11/20/17, 2:53 PM, "Jan Scheurich"  wrote:

Thanks, Darrel, for the quick patch. 
I have one major concern (see below).

> > 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.
> 
> I guess I confused myself.  My original point was unclear.  I was
> considering that you would add something like ->status, which was a 
bitset
> for the connection, and then be able to track many different 
conditions
> (not just reply_seen, but for possible future expansion).
> 
> I think your answer...
> 
> > I have patch to convert flags to a bitarray but that is for later 
and
> > there are other considerations,
> > so I deferred it.
> 
> ...does address that.  If that's the case, I'm okay.

I'm not convinced the generic struct conn should have a reply_seen 
state that governs whether the connection is considered established or not, no 
matter if encoded as boolean or bit in a bitarray.

I my view this is protocol specific and should be derived from 
connection state already maintained in the specific connection structs 
conn_tcp, conn_icmp and conn_other. Why should we duplicate this state? 

The simple reply_seen logic is OK for icmp and udp (other) but only a 
crude approximation of the actual conn_tcp behavior. It might treat a tcp 
connection as established at the first "reply" packet even if that was not a 
valid Syn-Ack packet according to conn_tcp. 

[Darrell] Please state the problem case 

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

[Darrell Ball]

On 11/20/17, 2:53 PM, "Jan Scheurich"  wrote:

Thanks, Darrel, for the quick patch. 
I have one major concern (see below).

> > 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.
> 
> I guess I confused myself.  My original point was unclear.  I was
> considering that you would add something like ->status, which was a bitset
> for the connection, and then be able to track many different conditions
> (not just reply_seen, but for possible future expansion).
> 
> I think your answer...
> 
> > I have patch to convert flags to a bitarray but that is for later and
> > there are other considerations,
> > so I deferred it.
> 
> ...does address that.  If that's the case, I'm okay.

I'm not convinced the generic struct conn should have a reply_seen state 
that governs whether the connection is considered established or not, no matter 
if encoded as boolean or bit in a bitarray.

I my view this is protocol specific and should be derived from connection 
state already maintained in the specific connection structs conn_tcp, conn_icmp 
and conn_other. Why should we duplicate this state? 

The simple reply_seen logic is OK for icmp and udp (other) but only a crude 
approximation of the actual conn_tcp behavior. It might treat a tcp connection 
as established at the first "reply" packet even if that was not a valid Syn-Ack 
packet according to conn_tcp. 

[Darrell] Please state the problem case specifically in terms of packet 1 and 
packet 2.

And future protocols like sctp might have yet other rules for treating a 
connection as established.

[Darrell] The TCP state tracking module we have determines whether a packet is 
valid and this includes 3-way handshake.
   This also includes valid reply packets and also should handle 
race conditions with crossover.
   Only if a packet is valid according to the state machine does it 
get considered as established setting.
   Now when the packet is flagged as valid, the caller determines 
the established state by noting the reply state.

   Now, of course, if you think there is a bug in the TCP state 
tracker or if there is a hole, please state the specific case as I mentioned 
above.

   On a related note, I am now starting to have some second 
thoughts about bringing EST in line with the kernel.
   I have since had some more discussions and would like to have 
further discussions after this holiday week with more people.

   Thanks again
   Darrell 






Please consider to change this and base the packet's established state on 
the state of the actual connection after the xxx_conn_update(). The 
xxx_conn_update() functions could return not only if the packet belongs to a 
valid connection but also if the connection is "established" or not.

BR, Jan


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Jan Scheurich]

Thanks, Darrel, for the quick patch. 
I have one major concern (see below).

> > 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.
> 
> I guess I confused myself.  My original point was unclear.  I was
> considering that you would add something like ->status, which was a bitset
> for the connection, and then be able to track many different conditions
> (not just reply_seen, but for possible future expansion).
> 
> I think your answer...
> 
> > I have patch to convert flags to a bitarray but that is for later and
> > there are other considerations,
> > so I deferred it.
> 
> ...does address that.  If that's the case, I'm okay.

I'm not convinced the generic struct conn should have a reply_seen state that 
governs whether the connection is considered established or not, no matter if 
encoded as boolean or bit in a bitarray.

I my view this is protocol specific and should be derived from connection state 
already maintained in the specific connection structs conn_tcp, conn_icmp and 
conn_other. Why should we duplicate this state? 

The simple reply_seen logic is OK for icmp and udp (other) but only a crude 
approximation of the actual conn_tcp behavior. It might treat a tcp connection 
as established at the first "reply" packet even if that was not a valid Syn-Ack 
packet according to conn_tcp. And future protocols like sctp might have yet 
other rules for treating a connection as established.

Please consider to change this and base the packet's established state on the 
state of the actual connection after the xxx_conn_update(). The 
xxx_conn_update() functions could return not only if the packet belongs to a 
valid connection but also if the connection is "established" or not.

BR, Jan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Aaron Conole]

Darrell Ball  writes:

> On 11/20/17, 10:02 AM, "Aaron Conole"  wrote:
>
> Darrell Ball  writes:
> 
> > On 11/20/17, 9:43 AM, "Aaron Conole"  wrote:
> >
> > Darrell Ball  writes:
> > 
> > > On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf 
> of
> > > Aaron Conole"  > > acon...@redhat.com> wrote:
> > >
> > > Darrell Ball  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 
> > > > ---
> > > >  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.

I guess I confused 

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

[Darrell Ball]

On 11/20/17, 10:02 AM, "Aaron Conole"  wrote:

Darrell Ball  writes:

> On 11/20/17, 9:43 AM, "Aaron Conole"  wrote:
>
> Darrell Ball  writes:
> 
> > On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> > Aaron Conole"  > acon...@redhat.com> wrote:
> >
> > Darrell Ball  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 
> > > ---
> > >  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, 

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

[Darrell Ball]

On 11/20/17, 9:43 AM, "Aaron Conole"  wrote:

Darrell Ball  writes:

> On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> Aaron Conole"  acon...@redhat.com> wrote:
>
> Darrell Ball  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 
> > ---
> >  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)
> >
> >  new (0x01)
> >  
> > -  A new connection.  Set to 1 if this is an uncommitted 
connection.
> > +  A new connection.  Set to 1 if this is an 

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

[Aaron Conole]

Darrell Ball  writes:

> On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> Aaron Conole"  acon...@redhat.com> wrote:
>
> Darrell Ball  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 
> > ---
> >  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.

> >  };
> >  
> >  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)
> >
> >  new (0x01)
> >  
> > -  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.
> >  
> >  
> >  est (0x02)
> >  
> > -  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 

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

[Darrell Ball]

On 11/20/17, 7:46 AM, "ovs-dev-boun...@openvswitch.org on behalf of Aaron 
Conole"  wrote:

Darrell Ball  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 
> ---
>  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.


>  };
>  
>  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)
>
>  new (0x01)
>  
> -  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.
>  
>  
>  est (0x02)
>  
> -  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.
>  
>  
>  rel (0x04)
> 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-dev] [patch v1 4/4] conntrack: Change established state to match kernel.

[Darrell Ball]

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 
---
 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;
 };
 
 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)
   
 new (0x01)
 
-  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.
 
 
 est (0x02)
 
-  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.
 
 
 rel (0x04)
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=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - IPv6 ping])
 CHECK_CONNTRACK()