@@ -1029,12 +1029,13 @@ upcall_receive(struct upcall *upcall, const struct dpif_backer *backer,
return 0;
}
-static void
+static int
upcall_xlate(struct udpif *udpif, struct upcall *upcall,
struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
struct dpif_flow_stats stats;
struct xlate_in xin;
+ int error;
stats.n_packets = 1;
stats.n_bytes = dp_packet_size(upcall->packet);
@@ -1066,9 +1067,14 @@ 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;
+ if (error) {
+ ofpbuf_init(&upcall->put_actions, 0);
+ return error;
+ }
+
/* Special case for fail-open mode.
*
* If we are in fail-open mode, but we are connected to a controller too,
@@ -1110,6 +1116,8 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
if (upcall->type == DPIF_UC_MISS) {
upcall->ukey = ukey_create_from_upcall(upcall, wc);
}
+
+ return 0;
}
static void
@@ -1204,8 +1212,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
switch (classify_upcall(upcall->type, userdata)) {
case MISS_UPCALL:
- upcall_xlate(udpif, upcall, odp_actions, wc);
- return 0;
+ return upcall_xlate(udpif, upcall, odp_actions, wc);
case SFLOW_UPCALL:
if (upcall->sflow) {
@@ -1852,9 +1859,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;
+ /* Delete datapath flow if translation failed. */
+ if (error) {
+ goto exit;
+ }
+
if (!need_revalidate) {
result = UKEY_KEEP;
goto exit;
@@ -313,6 +313,8 @@ struct xlate_ctx {
* datapath actions. */
bool action_set_has_group; /* Action set contains OFPACT_GROUP? */
struct ofpbuf action_set; /* Action set. */
+
+ int error; /* Translation failed (errno code). */
};
static void xlate_action_set(struct xlate_ctx *ctx);
@@ -2949,6 +2951,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);
@@ -3131,12 +3136,17 @@ xlate_resubmit_resource_check(struct xlate_ctx *ctx)
if (ctx->recurse >= MAX_RESUBMIT_RECURSION + MAX_INTERNAL_RESUBMITS) {
VLOG_ERR_RL(&rl, "resubmit actions recursed over %d times",
MAX_RESUBMIT_RECURSION);
+ ctx->error = ELOOP;
} else if (ctx->resubmits >= MAX_RESUBMITS + MAX_INTERNAL_RESUBMITS) {
VLOG_ERR_RL(&rl, "over %d resubmit actions", MAX_RESUBMITS);
+ ctx->error = ERANGE;
} else if (ctx->odp_actions->size > UINT16_MAX) {
VLOG_ERR_RL(&rl, "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");
+ ctx->error = EOVERFLOW;
} else {
return true;
}
@@ -3188,8 +3198,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
@@ -3561,6 +3569,7 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table)
if (!id) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_ERR_RL(&rl, "Failed to allocate recirculation id");
+ ctx->error = ENODATA;
return;
}
xlate_out_add_recirc(ctx->xout, id);
@@ -3568,11 +3577,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 = ENODATA;
+ return;
+ }
}
nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, id);
@@ -3610,7 +3621,7 @@ compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls)
"have more MPLS LSEs than the %d supported.",
ctx->xbridge->name, FLOW_MAX_MPLS_LABELS);
}
- ctx->exit = true;
+ ctx->error = ERANGE;
return;
}
@@ -3635,7 +3646,7 @@ compose_mpls_pop_action(struct xlate_ctx *ctx, ovs_be16 eth_type)
"more MPLS LSEs than the %d supported.",
ctx->xbridge->name, FLOW_MAX_MPLS_LABELS);
}
- ctx->exit = true;
+ ctx->error = ERANGE;
ofpbuf_clear(ctx->odp_actions);
}
}
@@ -4264,6 +4275,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 +4637,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 +4693,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 +4891,7 @@ xlate_wc_finish(struct xlate_ctx *ctx)
* 'xout'.
* The caller must take responsibility for eventually freeing 'xout', with
* xlate_out_uninit(). */
-void
+int
xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
{
*xout = (struct xlate_out) {
@@ -4881,7 +4903,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 +4936,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 +4989,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 +5004,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 +5064,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 +5156,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 +5238,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,7 @@ 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 *);
+int 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,7 +4991,8 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow *flow,
struct ds *ds)
{
struct trace_ctx trace;
-
+ int error;
+
ds_put_format(ds, "Bridge: %s\n", ofproto->up.name);
ds_put_cstr(ds, "Flow: ");
flow_format(ds, flow);
@@ -5007,29 +5011,32 @@ 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);
-
- ds_put_char(ds, '\n');
- trace_format_flow(ds, 0, "Final flow", &trace);
- trace_format_megaflow(ds, 0, "Megaflow", &trace);
+ error = xlate_actions(&trace.xin, &trace.xout);
+ if (error) {
+ ds_put_format(ds, "\nTranslation failed with errno %d!", error);
+ } else {
+ ds_put_char(ds, '\n');
+ trace_format_flow(ds, 0, "Final flow", &trace);
+ trace_format_megaflow(ds, 0, "Megaflow", &trace);
- ds_put_cstr(ds, "Datapath actions: ");
- format_odp_actions(ds, trace.odp_actions.data, trace.odp_actions.size);
+ ds_put_cstr(ds, "Datapath actions: ");
+ format_odp_actions(ds, trace.odp_actions.data, trace.odp_actions.size);
- if (trace.xout.slow) {
- enum slow_path_reason slow;
+ if (trace.xout.slow) {
+ enum slow_path_reason slow;
- ds_put_cstr(ds, "\nThis flow is handled by the userspace "
- "slow path because it:");
+ ds_put_cstr(ds, "\nThis flow is handled by the userspace "
+ "slow path because it:");
- slow = trace.xout.slow;
- while (slow) {
- enum slow_path_reason bit = rightmost_1bit(slow);
+ slow = trace.xout.slow;
+ while (slow) {
+ enum slow_path_reason bit = rightmost_1bit(slow);
- ds_put_format(ds, "\n\t- %s.",
- slow_path_reason_to_explanation(bit));
+ ds_put_format(ds, "\n\t- %s.",
+ slow_path_reason_to_explanation(bit));
- slow &= ~bit;
+ slow &= ~bit;
+ }
}
}
@@ -6698,7 +6698,7 @@ 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 with errno 40!
])
AT_CHECK([grep -c 'resubmit actions recursed over 64 times' ovs-vswitchd.log],
[0], [1
@@ -6716,6 +6716,8 @@ 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([tail -1 stdout], [0], [Translation failed with errno 34!
+])
AT_CHECK([grep -c 'over 4096 resubmit actions' ovs-vswitchd.log], [0], [1
])
OVS_VSWITCHD_STOP(["/over.*resubmit actions/d"])
@@ -6750,6 +6752,8 @@ 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([tail -1 stdout], [0], [Translation failed with errno 75!
+])
AT_CHECK([grep -c 'resubmits yielded over 64 kB of stack' ovs-vswitchd.log], [0], [1
])
OVS_VSWITCHD_STOP(["/resubmits yielded over 64 kB of stack/d"])
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: - not install a datapath flow if xlate_actions() fails, and - delete an existing datapath flow, rather than replacing it with an incomplete flow, when the revalidation fails due to failing xlate_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 | 22 +++++++++++++---- ofproto/ofproto-dpif-xlate.c | 55 +++++++++++++++++++++++++++++++++---------- ofproto/ofproto-dpif-xlate.h | 2 +- ofproto/ofproto-dpif.c | 47 ++++++++++++++++++++---------------- tests/ofproto-dpif.at | 6 ++++- 5 files changed, 93 insertions(+), 39 deletions(-)