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

Darrell Ball dball at vmware.com
Thu Nov 30 00:32:42 UTC 2017


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" <dball at vmware.com> wrote:

    
    
    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
        
    
    











More information about the dev mailing list