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

Joe Stringer joestringer at nicira.com
Tue Sep 22 18:03:28 UTC 2015


On 18 September 2015 at 09:58, Ben Pfaff <blp at nicira.com> wrote:
> 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?

A fair request. I see that the match fields for
ct_zone/ct_mark/ct_label are a bit lacking in ovs-ofctl, and I'll do
another iteration across the ovs-ofctl documentation in general to try
to make it flow better. I can also add a basic example of using
connection tracking after the action documentation.

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

Agree, next series will have this patch.

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

Nope, that was unintended. Good catch.

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

OK.

> 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;
>      }

True, the function originally handled both cases but this behaviour
was dropped by mistake. I'll update it so that ofpacts_verify_nested()
handles all constraints related to nesting, and is called
unconditionally. I should be able to write a simple test to ensure
this behaviour, too.

> 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;

This is one of the pieces that changed between the version I sent to
you separately vs. the email above, sorry about that. The ct_pad
member is no longer present, CT_MEMBERS still exists but there's a
different explanation:

/* We want to determine the size of these elements at compile time to ensure
 * actions alignment, but we also want to allow ofpact_conntrack to have
 * basic _put(), _get(), etc accessors defined below which access these
 * members directly from ofpact_conntrack. An anonymous struct will serve
 * both of these purposes. */
#define CT_MEMBERS                      \
struct {                                \
    struct ofpact ofpact;               \
    uint16_t flags;                     \
    uint16_t zone_imm;                  \
    struct mf_subfield zone_src;        \
    uint16_t alg;                       \
    uint8_t recirc_table;               \
}

/* This is (and should only be) used for calculating action alignment. */
struct ofpact_ct_pad {
    CT_MEMBERS;
};

/* OFPACT_CT.
 *
 * Used for NXAST_CT. */
struct ofpact_conntrack {
    union {
        CT_MEMBERS;
        uint8_t pad[OFPACT_ALIGN(sizeof(struct ofpact_ct_pad))];
    };
    struct ofpact actions[];
};
BUILD_ASSERT_DECL(offsetof(struct ofpact_conntrack, actions)
                  % OFPACT_ALIGNTO == 0);
BUILD_ASSERT_DECL(offsetof(struct ofpact_conntrack, actions)
                  == sizeof(struct ofpact_conntrack));

> 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'll update it.

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

Sure, I'll add it.



More information about the dev mailing list