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

[blp at nicira.com]

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).

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.

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;

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.

Thanks,

Ben.