[ovs-dev] [PATCHv2 2/6] Add connection tracking mark support.

[Ben Pfaff]

On Thu, Sep 17, 2015 at 04:04:24PM -0700, Joe Stringer wrote:
> This patch adds a new 32-bit metadata field to the connection tracking
> interface. When a mark is specified as part of the ct action and the
> connection is committed, the value is saved with the current connection.
> Subsequent ct lookups with the table specified will expose this metadata
> as the "ct_mark" field in the flow.
> 
> For example, to allow new connections from port 1->2 and only allow
> established connections from port 2->1, and to associate a mark with those
> connections:
> 
>     priority=1,action=drop
>     priority=10,arp,action=normal
>     priority=10,icmp,action=normal
>     in_port=1,tcp,action=ct(commit,exec(set_field:1->ct_mark)),2
>     in_port=2,ct_state=-trk,tcp,action=ct(table=1)
>     table=1,in_port=2,ct_state=+trk,ct_mark=1,tcp,action=1
> 
> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

Thanks a lot for writing this!

Without the explanation and the example in the commit log above, the
power and the restrictions and the intent are hard to understand.  Do
you think you could add any or all of the above to the documentation
somewhere?

decode_NXAST_RAW_CT() assumes that the nested actions are always
OpenFlow 1.0 actions.  I think that's a bad idea--I think that the
nested actions should use the same OpenFlow version as NXAST_RAW_CT
itself.  I don't think that information is readily available to decode
functions, so you might have to add a new parameter to all the decode
functions (in a separate patch please to avoid making this one hard to
review).

It seems slightly inconsistent for decode_NXAST_RAW_CT() to have just
one case where it returns without pushing the actions back on.  I'd
change the
        return OFPERR_OFPBAC_BAD_ARGUMENT;
to
        error = OFPERR_OFPBAC_BAD_ARGUMENT;
unless there's some reason for the difference that I didn't spot.

ofpacts_verify_nested() seems odd.  First it seems strange to verify
'outer_action' at all like this:
+    if (outer_action != OFPACT_CT && outer_action != OFPACT_WRITE_ACTIONS) {
+        return unsupported_nesting(a->type, outer_action);
+    }
because aren't the possibilities known at compile time?  If I'm right
about that, then if we need to check 'outer_action' then I'd guess an
ovs_assert would be the right thing since we'd be checking an invariant.

Second, the check at the end of ofpacts_verify_nested() seems like
something we'd want to check for actions that aren't inside an outer
action at all, not just something to check inside OFPACT_WRITE_ACTIONS:
+    if (field && outer_action != OFPACT_CT && field->id == MFF_CT_MARK) {
+        VLOG_WARN("cannot set CT fields outside of \"ct\" action");
+        return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
     }

This comment on CT_MEMBERS strikes me a bit oddly.  There's ordinarily
no need to worry about alignment in a struct unless you want tight
packing or to exactly match some protocol definition.  Neither one comes
up here as far as I can tell.  Also I don't know why ct_pad is there
when there's an automatically sized pad[] member in struct
ofpact_conntrack:
+/* Even though zone_src contains a pointer, it should be 64-bit aligned due
+ * to the initial ofpact,flags,zone_imm members in struct ofpact_conntrack. */
 #define CT_MEMBERS                      \
     uint16_t flags;                     \
     uint16_t zone_imm;                  \
     struct mf_subfield zone_src;        \
-    uint8_t recirc_table;
+    uint8_t recirc_table;               \
+    uint16_t ct_pad;

The documentation seems a little incomplete, because it says that
certain actions can only be taken inside ct(), but it leaves out the
severe restrictions on what is allowed to be done inside ct().

I'm not certain that verification actually recurses into OFPACT_CT
nested actions.  I don't see a test case that provokes a nested action
error, which would probably be easy to write.

Thanks,

Ben.