[ovs-dev] [PATCH v4 2/2] TCP flags matching support.

[Jarno Rajahalme]

Ben,

Thank you for a thorough review, see some comments below:

On Oct 8, 2013, at 2:56 PM, blp at nicira.com wrote:

> On Tue, Oct 08, 2013 at 01:56:55PM -0700, Jarno Rajahalme wrote:
>>    tcp_flags=flags/mask
>>        Bitwise  match on TCP flags.  The flags and mask are 16-bit num???
>>        bers written in decimal or in hexadecimal prefixed by 0x.   Each
>>        1-bit  in  mask requires that the corresponding bit in port must
>>        match.  Each 0-bit in mask causes the corresponding  bit  to  be
>>        ignored.
>> 
>>        TCP  protocol  currently  defines  9 flag bits, and additional 3
>>        bits are reserved (must be transmitted as zero), see  RFCs  793,
>>        3168, and 3540.  The flag bits are, numbering from the least
>> 	significant bit:
>> 
>>        0: FIN No more data from sender.
>> 
>>        1: SYN Synchronize sequence numbers.
>> 
>>        2: RST Reset the connection.
>> 
>>        3: PSH Push function.
>> 
>>        4: ACK Acknowledgement field significant.
>> 
>>        5: URG Urgent pointer field significant.
>> 
>>        6: ECE ECN Echo.
>> 
>>        7: CWR Congestion Windows Reduced.
>> 
>>        8: NS  Nonce Sum.
>> 
>>        9-11:  Reserved.
>> 
>>        12-15: Not matchable, must be zero.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> Since this adds the 'zeros' field back to struct flow, it needs to add
> code to actually zero that field in a few places.  Take a look at the
> corresponding code that was removed in commit b0a17866c3145 (Remove
> mpls_depth field from flow).
> 

Does it really matter what the values of "zeros" are? Maybe it should be renamed as "pad" instead.. After all, we do not seem to care about the implicit padding of 4 bytes in struct flow_tnl. How about removing "zeros" and let the compiler care about the padding, assuming there is a clean fix for the struct size test considering the size difference on 32/64 bit systems?

> match_wc_init() adds a comment that I guess is partly meant as a note
> to the reviewer:
>        if (flow->nw_proto == IPPROTO_TCP &&
>            flow->tcp_flags != 0) { /* XXX: How about matching zero flags? */
>            memset(&wc->masks.tcp_flags, 0xff, sizeof wc->masks.tcp_flags);
>        }
> I think that there is no problem here, because the goal of this
> function (though it is not well documented) is to take a flow that
> represents a packet and construct a match with a mask that reasonably
> represents the packet.  Currently it is, I believe, only used as a
> step in formatting flows for the user.  For that purpose, I think that
> the current strategy is fine.
> 
> In mf_random_value(), I think the mask is reversed, that is, that the
> ~ should be removed.

Will fix.

It seems MFF_IPV6_LABEL has the same problem.

> 
> In parse_odp_key_mask_attr() below, I don't understand why there is a
> test for tcp_flags != 0:
> +        uint16_t tcp_flags, tcp_flags_mask;
> +        int n = -1;
> +
> +        if (mask && sscanf(s, "tcp_flags(%"SCNi16"/%"SCNi16")%n",
> +                   &tcp_flags, &tcp_flags_mask, &n) > 0 && n > 0) {
> +            if (tcp_flags != 0) {
> +                nl_msg_put_be16(key, OVS_KEY_ATTR_TCP_FLAGS, htons(tcp_flags));
> +            }
> +            nl_msg_put_be16(mask, OVS_KEY_ATTR_TCP_FLAGS, htons(tcp_flags_mask));
> +            return n;
> 

I guess there is no reason, other than that I used the same pattern in odp_flow_key_from_flow__(), where it actually makes a difference (avoid creating keys that an older kernel cannot understand). Here we are setting the mask anyway, so this looks weird. However, this should make no functional difference, as missing key attars are generally treated as zeros. Will remove to check to avoid confusion.


> In odp_flow_key_from_flow__(), I think that we need some new logic for
> fragment handling.  The TCP source and dest port are always in the
> first fragment, because they are in the first 8 bytes, but the TCP
> flags start at offset 12, so they cannot be guaranteed to be in the
> first fragment, though of course most of the time they will be.

This due to the minimum Ethernet frame size cutting in between?

My understanding is that TCP tries to use MTU-sized packets, and no-one has 64 byte MTU in their networks, so the only reasonable case where the TCP header would be split would be an attack trying to circumvent TCP flag filtering. To be on the safe side, I think we should drop TCP packets where the whole TCP header is not in the first fragment.

Wouldn't this be even worse problem for IPv6?

Suggestions?

  Jarno