> On Mar 3, 2017, at 10:35 AM, Joe Stringer <[email protected]> wrote:
>
> On 28 February 2017 at 17:17, Jarno Rajahalme <[email protected]
> <mailto:[email protected]>> wrote:
>> Add resubmit option to use the Conntrack original direction tuple
>> swapped with the corresponding packet header fields during the lookup.
>> This could allow the same ACL table be used for admitting return
>> and/or related traffic as is used for admitting the original direction
>> traffic.
>>
>> Signed-off-by: Jarno Rajahalme <[email protected]>
>> ---
>> include/openvswitch/ofp-actions.h | 4 +-
>> lib/ofp-actions.c | 82 +++++++++++++++++++------
>> ofproto/ofproto-dpif-xlate.c | 68 ++++++++++++++++++---
>> tests/ofp-actions.at | 6 ++
>> tests/ofproto-dpif.at | 89 +++++++++++++++++++++------
>> tests/system-traffic.at | 122
>> ++++++++++++++++++++++++++++----------
>> utilities/ovs-ofctl.8.in | 19 +++++-
>> 7 files changed, 310 insertions(+), 80 deletions(-)
>>
>> diff --git a/include/openvswitch/ofp-actions.h
>> b/include/openvswitch/ofp-actions.h
>> index 53d6b44..5ea0763 100644
>> --- a/include/openvswitch/ofp-actions.h
>> +++ b/include/openvswitch/ofp-actions.h
>> @@ -640,11 +640,13 @@ struct ofpact_nat {
>>
>> /* OFPACT_RESUBMIT.
>> *
>> - * Used for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE. */
>> + * Used for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE, NXAST_RESUBMIT_TABLE_CT.
>> */
>> struct ofpact_resubmit {
>> struct ofpact ofpact;
>> ofp_port_t in_port;
>> uint8_t table_id;
>> + bool with_ct_orig; /* Resubmit with Conntrack original direction tuple
>> + * fields in place of IP header fields. */
>> };
>>
>> /* Bits for 'flags' in struct nx_action_learn.
>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
>> index 2869e0f..4d35a77 100644
>> --- a/lib/ofp-actions.c
>> +++ b/lib/ofp-actions.c
>> @@ -265,6 +265,8 @@ enum ofp_raw_action_type {
>> NXAST_RAW_RESUBMIT,
>> /* NX1.0+(14): struct nx_action_resubmit. */
>> NXAST_RAW_RESUBMIT_TABLE,
>> + /* NX1.0+(44): struct nx_action_resubmit. */
>> + NXAST_RAW_RESUBMIT_TABLE_CT,
>>
>> /* NX1.0+(2): uint32_t. */
>> NXAST_RAW_SET_TUNNEL,
>> @@ -3850,19 +3852,20 @@ format_FIN_TIMEOUT(const struct ofpact_fin_timeout
>> *a, struct ds *s)
>> ds_put_format(s, "%s)%s", colors.paren, colors.end);
>> }
>>
>> -/* Action structures for NXAST_RESUBMIT and NXAST_RESUBMIT_TABLE.
>> +/* Action structures for NXAST_RESUBMIT, NXAST_RESUBMIT_TABLE, and
>> + * NXAST_RESUBMIT_TABLE_CT.
>> *
>> * These actions search one of the switch's flow tables:
>> *
>> - * - For NXAST_RESUBMIT_TABLE only, if the 'table' member is not 255,
>> then
>> - * it specifies the table to search.
>> + * - For NXAST_RESUBMIT_TABLE and NXAST_RESUBMIT_TABLE_CT only, if the
>> + * 'table' member is not 255, then it specifies the table to search.
>
> 'only' is a bit superfluous - it's now for 2 of the 3 cases.
removed.
>
>> *
>> - * - Otherwise (for NXAST_RESUBMIT_TABLE with a 'table' of 255, or for
>> - * NXAST_RESUBMIT regardless of 'table'), it searches the current flow
>> - * table, that is, the OpenFlow flow table that contains the flow from
>> - * which this action was obtained. If this action did not come from a
>> - * flow table (e.g. it came from an OFPT_PACKET_OUT message), then
>> table 0
>> - * is the current table.
>> + * - Otherwise (for NXAST_RESUBMIT_TABLE or NXAST_RESUBMIT_TABLE_CT with
>> a
>> + * 'table' of 255, or for NXAST_RESUBMIT regardless of 'table'), it
>> + * searches the current flow table, that is, the OpenFlow flow table
>> that
>> + * contains the flow from which this action was obtained. If this
>> action
>> + * did not come from a flow table (e.g. it came from an OFPT_PACKET_OUT
>> + * message), then table 0 is the current table.
>> *
>> * The flow table lookup uses a flow that may be slightly modified from the
>> * original lookup:
>> @@ -3870,9 +3873,12 @@ format_FIN_TIMEOUT(const struct ofpact_fin_timeout
>> *a, struct ds *s)
>> * - For NXAST_RESUBMIT, the 'in_port' member of struct nx_action_resubmit
>> * is used as the flow's in_port.
>> *
>> - * - For NXAST_RESUBMIT_TABLE, if the 'in_port' member is not
>> OFPP_IN_PORT,
>> - * then its value is used as the flow's in_port. Otherwise, the
>> original
>> - * in_port is used.
>> + * - For NXAST_RESUBMIT_TABLE and NXAST_RESUBMIT_TABLE_CT, if the
>> 'in_port'
>> + * member is not OFPP_IN_PORT, then its value is used as the flow's
>> + * in_port. Otherwise, the original in_port is used.
>> + *
>> + * - For NXAST_RESUBMIT_TABLE_CT the Conntrack 5-tuple fields are used as
>> + * the packets IP header fields during the lookup.
>> *
>> * - If actions that modify the flow (e.g. OFPAT_SET_VLAN_VID) precede the
>> * resubmit action, then the flow is updated with the new values.
>> @@ -3905,11 +3911,12 @@ format_FIN_TIMEOUT(const struct ofpact_fin_timeout
>> *a, struct ds *s)
>> * a total limit of 4,096 resubmits per flow translation (earlier
>> versions
>> * did not impose any total limit).
>> *
>> - * NXAST_RESUBMIT ignores 'table' and 'pad'. NXAST_RESUBMIT_TABLE requires
>> - * 'pad' to be all-bits-zero.
>> + * NXAST_RESUBMIT ignores 'table' and 'pad'. NXAST_RESUBMIT_TABLE and
>> + * NXAST_RESUBMIT_TABLE_CT require 'pad' to be all-bits-zero.
>> *
>> * Open vSwitch 1.0.1 and earlier did not support recursion. Open vSwitch
>> - * before 1.2.90 did not support NXAST_RESUBMIT_TABLE.
>> + * before 1.2.90 did not support NXAST_RESUBMIT_TABLE. Open vSwitch before
>> + * 2.7.0 did not support NXAST_RESUBMIT_TABLE_CT.
>
> 2.8.0.
>
OK.
>> */
>> struct nx_action_resubmit {
>> ovs_be16 type; /* OFPAT_VENDOR. */
>> @@ -3954,6 +3961,21 @@ decode_NXAST_RAW_RESUBMIT_TABLE(const struct
>> nx_action_resubmit *nar,
>> return 0;
>> }
>>
>> +static enum ofperr
>> +decode_NXAST_RAW_RESUBMIT_TABLE_CT(const struct nx_action_resubmit *nar,
>> + enum ofp_version ofp_version OVS_UNUSED,
>> + struct ofpbuf *out)
>> +{
>> + enum ofperr error = decode_NXAST_RAW_RESUBMIT_TABLE(nar, ofp_version,
>> out);
>> + if (error) {
>> + return error;
>> + }
>> + struct ofpact_resubmit *resubmit = out->header;
>> + resubmit->ofpact.raw = NXAST_RAW_RESUBMIT_TABLE_CT;
>> + resubmit->with_ct_orig = true;
>> + return 0;
>> +}
>> +
>> static void
>> encode_RESUBMIT(const struct ofpact_resubmit *resubmit,
>> enum ofp_version ofp_version OVS_UNUSED, struct ofpbuf *out)
>> @@ -3961,10 +3983,12 @@ encode_RESUBMIT(const struct ofpact_resubmit
>> *resubmit,
>> uint16_t in_port = ofp_to_u16(resubmit->in_port);
>>
>> if (resubmit->table_id == 0xff
>> - && resubmit->ofpact.raw != NXAST_RAW_RESUBMIT_TABLE) {
>> + && resubmit->ofpact.raw == NXAST_RAW_RESUBMIT) {
>> put_NXAST_RESUBMIT(out, in_port);
>> } else {
>> - struct nx_action_resubmit *nar = put_NXAST_RESUBMIT_TABLE(out);
>> + struct nx_action_resubmit *nar;
>> + nar = resubmit->with_ct_orig
>> + ? put_NXAST_RESUBMIT_TABLE_CT(out) :
>> put_NXAST_RESUBMIT_TABLE(out);
>> nar->table = resubmit->table_id;
>> nar->in_port = htons(in_port);
>> }
>> @@ -3975,7 +3999,7 @@ parse_RESUBMIT(char *arg, struct ofpbuf *ofpacts,
>> enum ofputil_protocol *usable_protocols OVS_UNUSED)
>> {
>> struct ofpact_resubmit *resubmit;
>> - char *in_port_s, *table_s;
>> + char *in_port_s, *table_s, *ct_s;
>>
>> resubmit = ofpact_put_RESUBMIT(ofpacts);
>>
>> @@ -4002,6 +4026,16 @@ parse_RESUBMIT(char *arg, struct ofpbuf *ofpacts,
>> resubmit->table_id = 255;
>> }
>>
>> + ct_s = strsep(&arg, ",");
>> + if (ct_s && ct_s[0]) {
>> + if (strcmp(ct_s, "ct")) {
>> + return xasprintf("%s: unknown parameter", ct_s);
>> + }
>> + resubmit->with_ct_orig = true;
>> + } else {
>> + resubmit->with_ct_orig = false;
>> + }
>> +
>> if (resubmit->in_port == OFPP_IN_PORT && resubmit->table_id == 255) {
>> return xstrdup("at least one \"in_port\" or \"table\" must be "
>> "specified on resubmit");
>> @@ -4024,6 +4058,9 @@ format_RESUBMIT(const struct ofpact_resubmit *a,
>> struct ds *s)
>> if (a->table_id != 255) {
>> ds_put_format(s, "%"PRIu8, a->table_id);
>> }
>> + if (a->with_ct_orig) {
>> + ds_put_cstr(s, ",ct");
>> + }
>
> If no table is used, this could serialize two commas.
>
We discussed this offline, the parameters can be seen as positional, so we
leave this as it is. Besides, resubmits without a table are very rare (test
suite has none).
>> ds_put_format(s, "%s)%s", colors.paren, colors.end);
>> }
>> }
>> @@ -7220,9 +7257,16 @@ ofpact_check__(enum ofputil_protocol
>> *usable_protocols, struct ofpact *a,
>> case OFPACT_SET_TUNNEL:
>> case OFPACT_SET_QUEUE:
>> case OFPACT_POP_QUEUE:
>> - case OFPACT_RESUBMIT:
>> return 0;
>>
>> + case OFPACT_RESUBMIT: {
>> + struct ofpact_resubmit *resubmit = ofpact_get_RESUBMIT(a);
>> +
>> + if (resubmit->with_ct_orig && !is_ct_valid(flow, &match->wc, NULL))
>> {
>> + return OFPERR_OFPBAC_MATCH_INCONSISTENT;
>> + }
>> + return 0;
>> + }
>> case OFPACT_FIN_TIMEOUT:
>> if (flow->nw_proto != IPPROTO_TCP) {
>> inconsistent_match(usable_protocols);
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 0a6b730..257b736 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -483,7 +483,7 @@ static void do_xlate_actions(const struct ofpact *,
>> size_t ofpacts_len,
>> static void xlate_normal(struct xlate_ctx *);
>> static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
>> uint8_t table_id, bool may_packet_in,
>> - bool honor_table_miss);
>> + bool honor_table_miss, bool with_ct_orig);
>> static bool input_vid_is_valid(const struct xlate_ctx *,
>> uint16_t vid, struct xbundle *);
>> static uint16_t input_vid_to_vlan(const struct xbundle *, uint16_t vid);
>> @@ -3204,7 +3204,8 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>>
>> if (!process_special(ctx, peer) && may_receive(peer, ctx)) {
>> if (xport_stp_forward_state(peer) &&
>> xport_rstp_forward_state(peer)) {
>> - xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
>> true);
>> + xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
>> true,
>> + false);
>> if (!ctx->freezing) {
>> xlate_action_set(ctx);
>> }
>> @@ -3218,7 +3219,8 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> size_t old_size = ctx->odp_actions->size;
>> mirror_mask_t old_mirrors2 = ctx->mirrors;
>>
>> - xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
>> true);
>> + xlate_table_action(ctx, flow->in_port.ofp_port, 0, true,
>> true,
>> + false);
>> ctx->mirrors = old_mirrors2;
>> ctx->base_flow = old_base_flow;
>> ctx->odp_actions->size = old_size;
>> @@ -3473,8 +3475,50 @@ xlate_resubmit_resource_check(struct xlate_ctx *ctx)
>> }
>>
>> static void
>> +tuple_swap(struct flow *flow, const struct flow *key)
>> +{
>> + /* Do not swap if there is no CT tuple. */
>> + if (flow == key && flow->ct_nw_proto == 0) {
>> + OVS_NOT_REACHED(); /* Prerequisite check should take care of
>> this! */
>
> I'm not sure I follow the reasoning behind the flow / key pointer check here.
>
> It seems to me like this function is always called if someone writes
> the resubmit(...,ct) parameter, is there a guarantee that they've run
> the connection through ct() action before running this action?
As commented, prerequisites checking should take care of that, but I agree, I’m
making these more robust and issue a warning instead of crashing OVS if
something unexpected happens here.
>
>> + }
>> +
>> + uint8_t nw_proto = flow->nw_proto;
>> + flow->nw_proto = flow->ct_nw_proto;
>> + flow->ct_nw_proto = nw_proto;
>> +
>> + if (key->dl_type == htons(ETH_TYPE_IP)) {
>> + ovs_be32 nw_src = flow->nw_src;
>> + flow->nw_src = flow->ct_nw_src;
>> + flow->ct_nw_src = nw_src;
>> +
>> + ovs_be32 nw_dst = flow->nw_dst;
>> + flow->nw_dst = flow->ct_nw_dst;
>> + flow->ct_nw_dst = nw_dst;
>> + } else if (key->dl_type == htons(ETH_TYPE_IPV6)) {
>> + struct in6_addr ipv6_src = flow->ipv6_src;
>> + flow->ipv6_src = flow->ct_ipv6_src;
>> + flow->ct_ipv6_src = ipv6_src;
>> +
>> + struct in6_addr ipv6_dst = flow->ipv6_dst;
>> + flow->ipv6_dst = flow->ct_ipv6_dst;
>> + flow->ct_ipv6_dst = ipv6_dst;
>> + } else {
>> + OVS_NOT_REACHED();
>
> Remind me, does prerequisite check ensure the ethertype will be
> IP/IPV6? - I recall CT having something like this, but this is the
> resubmit action so I'm not sure we have as many guarantees on our
> prerequisites.
Prerequisites checking ensures CT state indicates packet has been through
conntrack and is trackable, which implies IPv4 or IPv6. I’ll make this more
obviously robust as well.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
