@@ -1035,6 +1035,7 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
{
struct dpif_flow_stats stats;
struct xlate_in xin;
+ enum xlate_error error;
stats.n_packets = 1;
stats.n_bytes = dp_packet_size(upcall->packet);
@@ -1066,7 +1067,7 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
upcall->dump_seq = seq_read(udpif->dump_seq);
upcall->reval_seq = seq_read(udpif->reval_seq);
- xlate_actions(&xin, &upcall->xout);
+ error = xlate_actions(&xin, &upcall->xout);
upcall->xout_initialized = true;
/* Special case for fail-open mode.
@@ -1094,14 +1095,17 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
ofproto_dpif_send_packet_in(upcall->ofproto, pin);
}
- if (!upcall->xout.slow) {
- ofpbuf_use_const(&upcall->put_actions,
- odp_actions->data, odp_actions->size);
- } else {
- ofpbuf_init(&upcall->put_actions, 0);
- compose_slow_path(udpif, &upcall->xout, upcall->flow,
- upcall->flow->in_port.odp_port,
- &upcall->put_actions);
+ /* Leave put_actions empty to install a drop flow if translation failed. */
+ if (error == XLATE_OK) {
+ if (!upcall->xout.slow) {
+ ofpbuf_use_const(&upcall->put_actions,
+ odp_actions->data, odp_actions->size);
+ } else {
+ /* upcall->put_actions already initialized by upcall_receive(). */
+ compose_slow_path(udpif, &upcall->xout, upcall->flow,
+ upcall->flow->in_port.odp_port,
+ &upcall->put_actions);
+ }
}
/* This function is also called for slow-pathed flows. As we are only
@@ -1859,9 +1863,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
xin.may_learn = true;
}
xin.xcache = ukey->xcache;
- xlate_actions(&xin, &xout);
+ error = xlate_actions(&xin, &xout);
xoutp = &xout;
+ /* Empty actions if translation failed. */
+ if (error) {
+ ofpbuf_clear(odp_actions);
+ }
+
if (!need_revalidate) {
result = UKEY_KEEP;
goto exit;
@@ -313,8 +313,29 @@ struct xlate_ctx {
* datapath actions. */
bool action_set_has_group; /* Action set contains OFPACT_GROUP? */
struct ofpbuf action_set; /* Action set. */
+
+ enum xlate_error error; /* Translation failed. */
};
+const char *xlate_strerror(enum xlate_error error)
+{
+ switch (error) {
+ case XLATE_OK:
+ return "OK";
+ case XLATE_RECURSION_TOO_DEEP:
+ return "Recursion too deep";
+ case XLATE_TOO_MANY_RESUBMITS:
+ return "Too many resubmits";
+ case XLATE_STACK_TOO_DEEP:
+ return "Stack too deep";
+ case XLATE_NO_RECIRCULATION_CONTEXT:
+ return "No recirculation context";
+ case XLATE_TOO_MANY_MPLS_LABELS:
+ return "Too many MPLS labels";
+ }
+ return "Unknown error";
+}
+
static void xlate_action_set(struct xlate_ctx *ctx);
static void xlate_commit_actions(struct xlate_ctx *ctx);
@@ -536,6 +557,17 @@ xlate_report(struct xlate_ctx *ctx, const char *format, ...)
}
}
+static struct vlog_rate_limit error_report_rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
+#define XLATE_REPORT_ERROR(CTX, ...) \
+ do { \
+ if (OVS_UNLIKELY((CTX)->xin->report_hook)) { \
+ xlate_report(CTX, __VA_ARGS__); \
+ } else { \
+ VLOG_ERR_RL(&error_report_rl, __VA_ARGS__); \
+ } \
+ } while (0)
+
static inline void
xlate_report_actions(struct xlate_ctx *ctx, const char *title,
const struct ofpact *ofpacts, size_t ofpacts_len)
@@ -2949,6 +2981,9 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
* recirculated packet! */
ctx->exit = false;
+ /* Peer bridge errors do not propagate back. */
+ ctx->error = 0;
+
if (ctx->xin->resubmit_stats) {
netdev_vport_inc_tx(xport->netdev, ctx->xin->resubmit_stats);
netdev_vport_inc_rx(peer->netdev, ctx->xin->resubmit_stats);
@@ -3126,17 +3161,20 @@ xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
static bool
xlate_resubmit_resource_check(struct xlate_ctx *ctx)
{
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-
if (ctx->recurse >= MAX_RESUBMIT_RECURSION + MAX_INTERNAL_RESUBMITS) {
- VLOG_ERR_RL(&rl, "resubmit actions recursed over %d times",
- MAX_RESUBMIT_RECURSION);
+ XLATE_REPORT_ERROR(ctx, "resubmit actions recursed over %d times",
+ MAX_RESUBMIT_RECURSION);
+ ctx->error = XLATE_RECURSION_TOO_DEEP;
} else if (ctx->resubmits >= MAX_RESUBMITS + MAX_INTERNAL_RESUBMITS) {
- VLOG_ERR_RL(&rl, "over %d resubmit actions", MAX_RESUBMITS);
+ XLATE_REPORT_ERROR(ctx, "over %d resubmit actions", MAX_RESUBMITS);
+ ctx->error = XLATE_TOO_MANY_RESUBMITS;
} else if (ctx->odp_actions->size > UINT16_MAX) {
- VLOG_ERR_RL(&rl, "resubmits yielded over 64 kB of actions");
+ XLATE_REPORT_ERROR(ctx, "resubmits yielded over 64 kB of actions");
+ /* NOT an error, as we'll be slow-pathing the flow in this case? */
+ ctx->exit = true; /* XXX: translation still terminated! */
} else if (ctx->stack.size >= 65536) {
- VLOG_ERR_RL(&rl, "resubmits yielded over 64 kB of stack");
+ XLATE_REPORT_ERROR(ctx, "resubmits yielded over 64 kB of stack");
+ ctx->error = XLATE_STACK_TOO_DEEP;
} else {
return true;
}
@@ -3188,8 +3226,6 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t in_port, uint8_t table_id,
ctx->table_id = old_table_id;
return;
}
-
- ctx->exit = true;
}
static void
@@ -3559,8 +3595,8 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table)
* with the udpif key ('ukey') created for each new datapath flow. */
id = recirc_alloc_id_ctx(&state);
if (!id) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
- VLOG_ERR_RL(&rl, "Failed to allocate recirculation id");
+ XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id");
+ ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
return;
}
xlate_out_add_recirc(ctx->xout, id);
@@ -3568,11 +3604,13 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table)
/* 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.
- *
- * This might fail and return 0. 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. */
+ * If flow tables have changed sufficiently this can fail and we will
+ * delete the old datapath flow. */
id = recirc_find_id(&state);
+ if (!id) {
+ ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
+ return;
+ }
}
nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, id);
@@ -3604,13 +3642,12 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls)
xlate_commit_actions(ctx);
} else if (n >= FLOW_MAX_MPLS_LABELS) {
if (ctx->xin->packet != NULL) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
- VLOG_WARN_RL(&rl, "bridge %s: dropping packet on which an "
+ XLATE_REPORT_ERROR(ctx, "bridge %s: dropping packet on which an "
"MPLS push action can't be performed as it would "
"have more MPLS LSEs than the %d supported.",
ctx->xbridge->name, FLOW_MAX_MPLS_LABELS);
}
- ctx->exit = true;
+ ctx->error = XLATE_TOO_MANY_MPLS_LABELS;
return;
}
@@ -3629,13 +3666,12 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16 eth_type)
}
} else if (n >= FLOW_MAX_MPLS_LABELS) {
if (ctx->xin->packet != NULL) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
- VLOG_WARN_RL(&rl, "bridge %s: dropping packet on which an "
+ XLATE_REPORT_ERROR(ctx, "bridge %s: dropping packet on which an "
"MPLS pop action can't be performed as it has "
"more MPLS LSEs than the %d supported.",
ctx->xbridge->name, FLOW_MAX_MPLS_LABELS);
}
- ctx->exit = true;
+ ctx->error = XLATE_TOO_MANY_MPLS_LABELS;
ofpbuf_clear(ctx->odp_actions);
}
}
@@ -4264,6 +4300,10 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
const struct ofpact_set_field *set_field;
const struct mf_field *mf;
+ if (ctx->error) {
+ break;
+ }
+
if (ctx->exit) {
/* Check if need to store the remaining actions for later
* execution. */
@@ -4622,7 +4662,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
/* Check if need to store this and the remaining actions for later
* execution. */
- if (ctx->exit && ctx_first_recirculation_action(ctx)) {
+ if (!ctx->error && ctx->exit && ctx_first_recirculation_action(ctx)) {
recirc_unroll_actions(a, OFPACT_ALIGN(ofpacts_len -
((uint8_t *)a -
(uint8_t *)ofpacts)),
@@ -4678,8 +4718,15 @@ void
xlate_actions_for_side_effects(struct xlate_in *xin)
{
struct xlate_out xout;
+ int error;
+
+ error = xlate_actions(xin, &xout);
+ if (error) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
+ VLOG_WARN_RL(&rl, "xlate_actions failed (%d)!", error);
+ }
- xlate_actions(xin, &xout);
xlate_out_uninit(&xout);
}
@@ -4869,7 +4916,7 @@ xlate_wc_finish(struct xlate_ctx *ctx)
* 'xout'.
* The caller must take responsibility for eventually freeing 'xout', with
* xlate_out_uninit(). */
-void
+enum xlate_error
xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
{
*xout = (struct xlate_out) {
@@ -4881,7 +4928,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
struct xbridge *xbridge = xbridge_lookup(xcfg, xin->ofproto);
if (!xbridge) {
- return;
+ return ENODEV;
}
struct flow *flow = &xin->flow;
@@ -4914,6 +4961,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
.sflow_odp_port = 0,
.nf_output_iface = NF_OUT_DROP,
.exit = false,
+ .error = 0,
.mirrors = 0,
.recirc_action_offset = -1,
@@ -4966,6 +5014,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
VLOG_WARN_RL(&rl, "Recirculation conflict (%s)!", conflict);
xlate_report(&ctx, "- Recirculation conflict (%s)!", conflict);
+ ctx.error = ENODATA;
goto exit;
}
@@ -4980,6 +5029,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
VLOG_WARN_RL(&rl, "Recirculation bridge no longer exists.");
xlate_report(&ctx, "- Recirculation bridge no longer exists.");
+ ctx.error = ENODATA;
goto exit;
}
ctx.xbridge = new_bridge;
@@ -5039,6 +5089,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
VLOG_WARN_RL(&rl, "Recirculation context not found for ID %"PRIx32,
flow->recirc_id);
+ ctx.error = ENODATA;
goto exit;
}
/* The bridge is now known so obtain its table version. */
@@ -5130,6 +5181,9 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
mirror_ingress_packet(&ctx);
do_xlate_actions(ofpacts, ofpacts_len, &ctx);
+ if (ctx.error) {
+ goto exit;
+ }
/* We've let OFPP_NORMAL and the learning action look at the
* packet, so drop it now if forwarding is disabled. */
@@ -5209,6 +5263,8 @@ exit:
ofpbuf_uninit(&ctx.stack);
ofpbuf_uninit(&ctx.action_set);
ofpbuf_uninit(&scratch_actions);
+
+ return ctx.error;
}
/* Sends 'packet' out 'ofport'.
@@ -239,7 +239,19 @@ int xlate_lookup(const struct dpif_backer *, const struct flow *,
struct dpif_sflow **, struct netflow **,
ofp_port_t *ofp_in_port);
-void xlate_actions(struct xlate_in *, struct xlate_out *);
+enum xlate_error {
+ XLATE_OK = 0,
+ XLATE_RECURSION_TOO_DEEP,
+ XLATE_TOO_MANY_RESUBMITS,
+ XLATE_STACK_TOO_DEEP,
+ XLATE_NO_RECIRCULATION_CONTEXT,
+ XLATE_TOO_MANY_MPLS_LABELS,
+};
+
+const char *xlate_strerror(enum xlate_error error);
+
+enum xlate_error xlate_actions(struct xlate_in *, struct xlate_out *)
+ OVS_WARN_UNUSED_RESULT;
void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
const struct flow *, ofp_port_t in_port, struct rule_dpif *,
uint16_t tcp_flags, const struct dp_packet *packet,
@@ -3730,7 +3730,10 @@ ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto,
xin.resubmit_stats = &stats;
xin.recurse = recurse;
xin.resubmits = resubmits;
- xlate_actions(&xin, &xout);
+ error = xlate_actions(&xin, &xout);
+ if (error) {
+ goto out;
+ }
execute.actions = odp_actions.data;
execute.actions_len = odp_actions.size;
@@ -3749,7 +3752,7 @@ ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto,
execute.packet->md.in_port.odp_port = ofp_port_to_odp_port(ofproto, in_port);
error = dpif_execute(ofproto->backer->dpif, &execute);
-
+out:
xlate_out_uninit(&xout);
ofpbuf_uninit(&odp_actions);
@@ -4988,6 +4991,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
struct ds *ds)
{
struct trace_ctx trace;
+ enum xlate_error error;
ds_put_format(ds, "Bridge: %s\n", ofproto->up.name);
ds_put_cstr(ds, "Flow: ");
@@ -5007,8 +5011,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
trace.xin.resubmit_hook = trace_resubmit;
trace.xin.report_hook = trace_report_valist;
- xlate_actions(&trace.xin, &trace.xout);
-
+ error = xlate_actions(&trace.xin, &trace.xout);
ds_put_char(ds, '\n');
trace_format_flow(ds, 0, "Final flow", &trace);
trace_format_megaflow(ds, 0, "Megaflow", &trace);
@@ -5016,7 +5019,10 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
ds_put_cstr(ds, "Datapath actions: ");
format_odp_actions(ds, trace.odp_actions.data, trace.odp_actions.size);
- if (trace.xout.slow) {
+ if (error != XLATE_OK) {
+ ds_put_format(ds, "\nTranslation failed (%s), packet is dropped.\n",
+ xlate_strerror(error));
+ } else if (trace.xout.slow) {
enum slow_path_reason slow;
ds_put_cstr(ds, "\nThis flow is handled by the userspace "
@@ -6697,9 +6697,10 @@ OVS_VSWITCHD_START
AT_CHECK([ovs-ofctl add-flow br0 actions=resubmit:1,resubmit:2,output:3])
AT_CHECK([ovs-appctl -vpoll_loop:off ofproto/trace br0 'eth_dst=ff:ff:ff:ff:ff:ff'],
[0], [stdout])
-AT_CHECK([tail -1 stdout], [0], [Datapath actions: drop
+AT_CHECK([tail -1 stdout], [0],
+ [Translation failed (Recursion too deep), packet is dropped.
])
-AT_CHECK([grep -c 'resubmit actions recursed over 64 times' ovs-vswitchd.log],
+AT_CHECK([grep -c 'resubmit actions recursed over 64 times' stdout],
[0], [1
])
OVS_VSWITCHD_STOP(["/resubmit actions recursed/d"])
@@ -6715,7 +6716,10 @@ ADD_OF_PORTS([br0], 1)
echo "in_port=65, actions=local") > flows
AT_CHECK([ovs-ofctl add-flows br0 flows])
AT_CHECK([ovs-appctl -vpoll_loop:off ofproto/trace br0 'in_port=1'], [0], [stdout])
-AT_CHECK([grep -c 'over 4096 resubmit actions' ovs-vswitchd.log], [0], [1
+AT_CHECK([tail -1 stdout], [0],
+ [Translation failed (Too many resubmits), packet is dropped.
+])
+AT_CHECK([grep -c 'over 4096 resubmit actions' stdout], [0], [1
])
OVS_VSWITCHD_STOP(["/over.*resubmit actions/d"])
AT_CLEANUP
@@ -6733,7 +6737,7 @@ AT_CHECK([ovs-appctl -vpoll_loop:off ofproto/trace br0 'in_port=1'], [0], [stdou
AT_CHECK([grep -c -e '- Uses action(s) not supported by datapath' stdout],
[0], [1
])
-AT_CHECK([grep -c 'resubmits yielded over 64 kB of actions' ovs-vswitchd.log], [0], [1
+AT_CHECK([grep -c 'resubmits yielded over 64 kB of actions' stdout], [0], [1
])
OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of actions/d"])
AT_CLEANUP
@@ -6749,7 +6753,10 @@ ADD_OF_PORTS([br0], 1)
echo "in_port=13, actions=$push,$push,$push,$push,$push,$push,$push,$push") > flows
AT_CHECK([ovs-ofctl add-flows br0 flows])
AT_CHECK([ovs-appctl -vpoll_loop:off ofproto/trace br0 'in_port=1'], [0], [stdout])
-AT_CHECK([grep -c 'resubmits yielded over 64 kB of stack' ovs-vswitchd.log], [0], [1
+AT_CHECK([tail -1 stdout], [0],
+ [Translation failed (Stack too deep), packet is dropped.
+])
+AT_CHECK([grep -c 'resubmits yielded over 64 kB of stack' stdout], [0], [1
])
OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of stack/d"])
AT_CLEANUP
Sometimes xlate_actions() fails due to too deep recursion, too many MPLS labels, or missing recirculation context. Make xlate_actions() fail in these circumstances, so that we can install a drop flow instead of a flow with partially translated actions. Before this action it was possible that the revalidation installed a flow with a recirculation ID with an invalid recirc ID (== 0), due to the introduction of in-place modification in commit 43b2f131a229 (ofproto: Allow in-place modifications of datapath flows). Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> --- ofproto/ofproto-dpif-upcall.c | 29 ++++++++---- ofproto/ofproto-dpif-xlate.c | 106 ++++++++++++++++++++++++++++++++---------- ofproto/ofproto-dpif-xlate.h | 14 +++++- ofproto/ofproto-dpif.c | 16 +++++-- tests/ofproto-dpif.at | 17 +++++-- 5 files changed, 136 insertions(+), 46 deletions(-)