On 28 February 2017 at 17:17, Jarno Rajahalme <ja...@ovn.org> 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 <ja...@ovn.org>
> ---
> 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.
> *
> - * - 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.
> */
> 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.
> 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?
> + }
> +
> + 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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
