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

[Darrell Ball]

On 11/20/17, 2:53 PM, "Jan Scheurich" <jan.scheurich at ericsson.com> 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