Joe,
See my comments inline,
Jarno
> On Apr 30, 2015, at 12:38 PM, Joe Stringer <joestrin...@nicira.com> wrote:
>
> When traversing from one bridge to another via a patch port, we should
> drop conntrack metadata from the OpenFlow pipeline. This should only
> affect the subsequent processing on the destination bridge (and not
> continued processing, resubmits, etc. on the original bridge).
>
> If conntrack is executed for the current bridge, then recirculation
> should make the conntrack fields available for matching. In all other
> cases, the conntrack fields should be zeroed during upcall processing.
>
> CC: Jarno Rajahalme <jrajaha...@nicira.com>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> This should be folded into earlier conntrack patches, but I have left it
> as an independent patch for now to aid review.
> ---
> lib/ofp-actions.h | 5 +++
> ofproto/ofproto-dpif-rid.c | 80 +++++++++++++++++++++++-------------------
> ofproto/ofproto-dpif-rid.h | 15 ++++----
> ofproto/ofproto-dpif-xlate.c | 46 ++++++++++++++++++++----
> 4 files changed, 97 insertions(+), 49 deletions(-)
>
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 01958c2..0063b60 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -748,6 +748,11 @@ struct ofpact_unroll_xlate {
> /* Metadata in xlate context, visible to controller via PACKET_INs. */
> uint8_t rule_table_id; /* 0xFF if none. */
> ovs_be64 rule_cookie; /* OVS_BE64_MAX if none. */
> +
> + /* Whether conntrack was executed prior to recirculation. If so, related
> + * fields may be made available post-recirculation, until peer traversal
> + * or subsequent conntrack execution. */
> + bool conntracked;
> };
>
Having ‘conntracked’ in the of ofpact_unroll_xlate would make sense if we both
saved and restored the ‘conntracked’ value across some actions (other than
output to patch ports). Unrolling ‘conntracked’ after a resubmit that did the
conntrack action would reset the bit to 0 for the rest of the pipeline, which
is not what we want. In short, we do not need ‘conntracked’ here.
> /* Converting OpenFlow to ofpacts. */
> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> index f1b3bdc..c539e0a 100644
> --- a/ofproto/ofproto-dpif-rid.c
> +++ b/ofproto/ofproto-dpif-rid.c
> @@ -125,9 +125,9 @@ recirc_id_node_find(uint32_t id)
>
> static uint32_t
> recirc_metadata_hash(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> + bool conntrack, struct recirc_metadata *md,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *ofpacts)
> {
> uint32_t hash;
>
> @@ -135,6 +135,7 @@ recirc_metadata_hash(struct ofproto_dpif *ofproto,
> uint8_t table_id,
>
> hash = hash_pointer(ofproto, 0);
> hash = hash_int(table_id, hash);
> + hash = hash_boolean(conntrack, hash);
> hash = hash_words64((const uint64_t *)md, sizeof *md / sizeof(uint64_t),
> hash);
> if (stack && stack->size != 0) {
> @@ -153,12 +154,13 @@ recirc_metadata_hash(struct ofproto_dpif *ofproto,
> uint8_t table_id,
> static bool
> recirc_metadata_equal(const struct recirc_id_node *node,
> struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> + bool conntrack, struct recirc_metadata *md,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *ofpacts)
> {
> return node->ofproto == ofproto
> && node->table_id == table_id
> + && node->conntrack == conntrack
> && !memcmp(&node->metadata, md, sizeof *md)
> && ((!node->stack && (!stack || stack->size == 0))
> || (node->stack && stack && ofpbuf_equal(node->stack, stack)))
> @@ -171,15 +173,17 @@ recirc_metadata_equal(const struct recirc_id_node *node,
> * state, caller should take a reference. */
> static struct recirc_id_node *
> recirc_find_equal(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts, uint32_t hash)
> + bool conntrack, struct recirc_metadata *md,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *ofpacts,
> + uint32_t hash)
> {
> struct recirc_id_node *node;
>
> CMAP_FOR_EACH_WITH_HASH(node, metadata_node, hash, &metadata_map) {
> - if (recirc_metadata_equal(node, ofproto, table_id, md, stack,
> - action_set_len, ofpacts_len, ofpacts)) {
> + if (recirc_metadata_equal(node, ofproto, table_id, conntrack, md,
> + stack, action_set_len, ofpacts_len,
> + ofpacts)) {
> return node;
> }
> }
> @@ -188,15 +192,16 @@ recirc_find_equal(struct ofproto_dpif *ofproto, uint8_t
> table_id,
>
> static struct recirc_id_node *
> recirc_ref_equal(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts, uint32_t hash)
> + bool conntrack, struct recirc_metadata *md,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *ofpacts,
> + uint32_t hash)
> {
> struct recirc_id_node *node;
>
> do {
> - node = recirc_find_equal(ofproto, table_id, md, stack,
> action_set_len,
> - ofpacts_len, ofpacts, hash);
> + node = recirc_find_equal(ofproto, table_id, conntrack, md, stack,
> + action_set_len, ofpacts_len, ofpacts, hash);
>
> /* Try again if the node was released before we get the reference. */
> } while (node && !ovs_refcount_try_ref_rcu(&node->refcount));
> @@ -210,9 +215,10 @@ recirc_ref_equal(struct ofproto_dpif *ofproto, uint8_t
> table_id,
> * hash is recomputed if it is passed in as 0. */
> static struct recirc_id_node *
> recirc_alloc_id__(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts, uint32_t hash)
> + bool conntrack, struct recirc_metadata *md,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *ofpacts,
> + uint32_t hash)
> {
> struct recirc_id_node *node = xzalloc(sizeof *node +
> OFPACT_ALIGN(ofpacts_len));
> @@ -221,6 +227,7 @@ recirc_alloc_id__(struct ofproto_dpif *ofproto, uint8_t
> table_id,
>
> node->ofproto = ofproto;
> node->table_id = table_id;
> + node->conntrack = conntrack;
> memcpy(&node->metadata, md, sizeof node->metadata);
> node->stack = (stack && stack->size) ? ofpbuf_clone(stack) : NULL;
> node->action_set_len = action_set_len;
> @@ -254,7 +261,7 @@ recirc_alloc_id__(struct ofproto_dpif *ofproto, uint8_t
> table_id,
> /* Look up an existing ID for the given flow's metadata and optional actions.
> */
> uint32_t
> -recirc_find_id(struct ofproto_dpif *ofproto, uint8_t table_id,
> +recirc_find_id(struct ofproto_dpif *ofproto, uint8_t table_id, bool
> conntrack,
> struct recirc_metadata *md, struct ofpbuf *stack,
> uint32_t action_set_len, uint32_t ofpacts_len,
> const struct ofpact *ofpacts)
> @@ -263,10 +270,10 @@ recirc_find_id(struct ofproto_dpif *ofproto, uint8_t
> table_id,
> struct recirc_id_node *node;
> uint32_t hash;
>
> - hash = recirc_metadata_hash(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts);
> - node = recirc_find_equal(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts, hash);
> + hash = recirc_metadata_hash(ofproto, table_id, conntrack, md, stack,
> + action_set_len, ofpacts_len, ofpacts);
> + node = recirc_find_equal(ofproto, table_id, conntrack, md, stack,
> + action_set_len, ofpacts_len, ofpacts, hash);
>
> return node ? node->id : 0;
> }
> @@ -275,25 +282,25 @@ recirc_find_id(struct ofproto_dpif *ofproto, uint8_t
> table_id,
> optional actions. */
> uint32_t
> recirc_alloc_id_ctx(struct ofproto_dpif *ofproto, uint8_t table_id,
> - struct recirc_metadata *md, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *ofpacts)
> + bool conntrack, struct recirc_metadata *md,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *ofpacts)
> {
> struct recirc_id_node *node;
> uint32_t hash;
>
> /* Look up an existing ID. */
> - hash = recirc_metadata_hash(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts);
> - node = recirc_ref_equal(ofproto, table_id, md, stack, action_set_len,
> - ofpacts_len, ofpacts, hash);
> + hash = recirc_metadata_hash(ofproto, table_id, conntrack, md, stack,
> + action_set_len,ofpacts_len, ofpacts);
> + node = recirc_ref_equal(ofproto, table_id, conntrack, md, stack,
> + action_set_len, ofpacts_len, ofpacts, hash);
>
> /* Allocate a new recirc ID if needed. */
> if (!node) {
> ovs_assert(action_set_len <= ofpacts_len);
>
> - node = recirc_alloc_id__(ofproto, table_id, md, stack,
> action_set_len,
> - ofpacts_len, ofpacts, hash);
> + node = recirc_alloc_id__(ofproto, table_id, conntrack, md, stack,
> + action_set_len, ofpacts_len, ofpacts, hash);
> }
>
> return node->id;
> @@ -309,9 +316,10 @@ recirc_alloc_id(struct ofproto_dpif *ofproto)
>
> memset(&md, 0, sizeof md);
> md.in_port = OFPP_NONE;
> - hash = recirc_metadata_hash(ofproto, TBL_INTERNAL, &md, NULL, 0, 0,
> NULL);
> - node = recirc_alloc_id__(ofproto, TBL_INTERNAL, &md, NULL, 0, 0, NULL,
> - hash);
> + hash = recirc_metadata_hash(ofproto, TBL_INTERNAL, false, &md, NULL, 0,
> 0,
> + NULL);
> + node = recirc_alloc_id__(ofproto, TBL_INTERNAL, false, &md, NULL, 0, 0,
> + NULL, hash);
> return node->id;
> }
>
> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> index dc533ce..db77144 100644
> --- a/ofproto/ofproto-dpif-rid.h
> +++ b/ofproto/ofproto-dpif-rid.h
> @@ -138,6 +138,9 @@ struct recirc_id_node {
> /* Initial table for post-recirculation processing. */
> uint8_t table_id;
>
> + /* True if conntracking occurred prior to recirculation. */
> + bool conntrack;
> +
> /* Pipeline context for post-recirculation processing. */
> struct ofproto_dpif *ofproto; /* Post-recirculation bridge. */
> struct recirc_metadata metadata; /* Flow metadata. */
> @@ -157,13 +160,13 @@ void recirc_init(void);
> uint32_t recirc_alloc_id(struct ofproto_dpif *);
>
> uint32_t recirc_alloc_id_ctx(struct ofproto_dpif *, uint8_t table_id,
> - struct recirc_metadata *, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *);
> + bool conntrack, struct recirc_metadata *,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *);
> uint32_t recirc_find_id(struct ofproto_dpif *, uint8_t table_id,
> - struct recirc_metadata *, struct ofpbuf *stack,
> - uint32_t action_set_len, uint32_t ofpacts_len,
> - const struct ofpact *);
> + bool conntrack, struct recirc_metadata *,
> + struct ofpbuf *stack, uint32_t action_set_len,
> + uint32_t ofpacts_len, const struct ofpact *);
> void recirc_free_id(uint32_t recirc_id);
> void recirc_free_ofproto(struct ofproto_dpif *, const char *ofproto_name);
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 5561410..9a9b4a6 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -283,6 +283,11 @@ struct xlate_ctx {
> * the MPLS label stack that was originally present. */
> bool was_mpls;
>
> + /* True if conntrack has been performed on this packet during processing
> + * on the current bridge. This is used to determine whether conntrack
> + * state from the datapath should be honored after recirculation. */
> + bool conntracked;
> +
This should be initialized to false when an xlate_ctx is created.
> /* OpenFlow 1.1+ action set.
> *
> * 'action_set' accumulates "struct ofpact"s added by
> OFPACT_WRITE_ACTIONS.
> @@ -2714,6 +2719,15 @@ build_tunnel_send(const struct xlate_ctx *ctx, const
> struct xport *xport,
> }
>
> static void
> +clear_conntrack(struct flow *flow)
> +{
> + flow->conn_state = 0;
> + flow->conn_zone = 0;
> + flow->conn_mark = 0;
> + memset(&flow->conn_label, 0, sizeof flow->conn_label);
> +}
> +
> +static void
> compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
> const struct xlate_bond_recirc *xr, bool check_stp)
> {
> @@ -2777,6 +2791,7 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> const struct xport *peer = xport->peer;
> struct flow old_flow = ctx->xin->flow;
> bool old_was_mpls = ctx->was_mpls;
> + bool old_conntracked = ctx->conntracked;
> enum slow_path_reason special;
> struct ofpbuf old_stack = ctx->stack;
> union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
> @@ -2792,6 +2807,10 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> memset(flow->regs, 0, sizeof flow->regs);
> flow->actset_output = OFPP_UNSET;
>
> + /* Drop conntrack metadata when traversing a peer. */
> + clear_conntrack(flow);
> + ctx->conntracked = false;
> +
> special = process_special(ctx, &ctx->xin->flow, peer,
> ctx->xin->packet);
> if (special) {
> @@ -2842,6 +2861,8 @@ compose_output_action__(struct xlate_ctx *ctx,
> ofp_port_t ofp_port,
> * bridge. */
> ctx->was_mpls = old_was_mpls;
>
> + ctx->conntracked = old_conntracked;
> +
> /* The fact that the peer bridge exits (for any reason) does not mean
> * that the original bridge should exit. Specifically, if the peer
> * bridge recirculates (which typically modifies the packet), the
> @@ -3483,8 +3504,9 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
> struct ofpbuf *stack,
> /* Allocate a unique recirc id for the given metadata state in the
> * flow. The life-cycle of this recirc id is managed by associating
> it
> * with the udpif key ('ukey') created for each new datapath flow. */
> - id = recirc_alloc_id_ctx(ctx->xbridge->ofproto, 0, &md, stack,
> - action_offset, ofpacts_len, ofpacts);
> + id = recirc_alloc_id_ctx(ctx->xbridge->ofproto, 0, ctx->conntracked,
> + &md, stack, action_offset, ofpacts_len,
> + ofpacts);
> if (!id) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_ERR_RL(&rl, "Failed to allocate recirculation id");
> @@ -3495,8 +3517,8 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
> struct ofpbuf *stack,
> /* Look up an existing recirc id for the given metadata state in the
> * flow. No new reference is taken, as the ID is RCU protected and is
> * only required temporarily for verification. */
> - id = recirc_find_id(ctx->xbridge->ofproto, 0, &md, stack,
> - action_offset, ofpacts_len, ofpacts);
> + id = recirc_find_id(ctx->xbridge->ofproto, 0, ctx->conntracked, &md,
> + stack, action_offset, ofpacts_len, ofpacts);
> /* We let zero 'id' to be used in the RECIRC action below, which will
> * fail all revalidations as zero is not a valid recirculation ID. */
> }
> @@ -3983,14 +4005,16 @@ recirc_put_unroll_xlate(struct xlate_ctx *ctx)
>
> /* Restore the table_id and rule cookie for a potential PACKET
> * IN if needed. */
> - if (!unroll ||
> - (ctx->table_id != unroll->rule_table_id
> - || ctx->rule_cookie != unroll->rule_cookie)) {
> + if (!unroll
> + || ctx->table_id != unroll->rule_table_id
> + || ctx->rule_cookie != unroll->rule_cookie
> + || ctx->conntracked) {
>
> ctx->last_unroll_offset = ctx->action_set.size;
> unroll = ofpact_put_UNROLL_XLATE(&ctx->action_set);
> unroll->rule_table_id = ctx->table_id;
> unroll->rule_cookie = ctx->rule_cookie;
> + unroll->conntracked = ctx->conntracked;
> }
> }
>
> @@ -4113,6 +4137,9 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct
> ofpact_conntrack *ofc)
> put_connhelper(odp_actions, ofc);
> nl_msg_end_nested(odp_actions, ct_offset);
>
> + /* Use conn_* fields from datapath during recirculation upcall. */
> + ctx->conntracked = true;
> +
> if (ofc->flags & NX_CT_F_RECIRC) {
> compose_recirculate_action__(ctx, NULL, 0, 0, NULL);
> }
> @@ -4436,6 +4463,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> /* Restore translation context data that was stored earlier. */
> ctx->table_id = unroll->rule_table_id;
> ctx->rule_cookie = unroll->rule_cookie;
> + ctx->conntracked = unroll->conntracked;
> break;
> }
> case OFPACT_FIN_TIMEOUT:
> @@ -4848,6 +4876,10 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> *xout)
> * only if there are no post-recirculation actions. */
> ctx.table_id = recirc->table_id;
>
> + if (!recirc->conntrack) {
> + clear_conntrack(flow);
> + }
> +
> /* Restore pipeline metadata. May change flow's in_port and other
> * metadata to the values that existed when recirculation was
> * triggered. */
> --
> 1.7.10.4
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
