From patchwork Thu Dec 7 23:54:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 845908 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3ytC4n5lxkz9s74 for ; Fri, 8 Dec 2017 10:54:17 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 54826BF0; Thu, 7 Dec 2017 23:54:14 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id A0F75BD0 for ; Thu, 7 Dec 2017 23:54:13 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 04008F6 for ; Thu, 7 Dec 2017 23:54:12 +0000 (UTC) X-Originating-IP: 208.91.3.26 Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 9E074A80C0; Fri, 8 Dec 2017 00:54:10 +0100 (CET) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 7 Dec 2017 15:54:03 -0800 Message-Id: <20171207235403.27248-1-blp@ovn.org> X-Mailer: git-send-email 2.10.2 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff , Kevin Lin Subject: [ovs-dev] [PATCH] ofproto-dpif-xlate: Correctly decide whether truncating. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org xlate_output_action() must tell some of the functions it calls whether the packet is being truncated. Until now, it has inferred that based on whether its max_len argument is nonzero. Unfortunately, max_len conflates two different purposes. Historically it was used only to limit the number of bytes of packets sent to an OpenFlow controller in packet_in messages. When packet truncation was introduced, it was then also used to specify the truncation length. This meant that, for example, when xlate_output_reg_action() called into xlate_output_action() passing along for max_len an OpenFlow controller byte limit (which ovs-ofctl by default sets to 65535), xlate_output_action() interpreted that as a truncation request and told the functions it called that the packet was being truncated, which in the worst case led to assertion failures. This commit disentangles these two meaning of max_len, separating them into two separate parameters, and updates the callers. Reported-by: Kevin Lin Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045841.html Signed-off-by: Ben Pfaff --- ofproto/ofproto-dpif-xlate.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index fcced344ed8a..83e2b7a0891c 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4825,13 +4825,27 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx) return true; } +/* Emits an action that outputs to 'port', within 'ctx'. + * + * 'controller_len' affects only packets sent to an OpenFlow controller. It + * is the maximum number of bytes of the packet to send. UINT16_MAX means to + * send the whole packet (and 0 means to omit the packet entirely). + * + * 'may_packet_in' determines whether the packet may be sent to an OpenFlow + * controller. If it is false, then the packet is never sent to the OpenFlow + * controller. + * + * 'is_last_action' should be true if this output is the last OpenFlow action + * to be processed, which enables certain optimizations. + * + * 'truncate' should be true if the packet to be output is being truncated, + * which suppresses certain optimizations. */ static void -xlate_output_action(struct xlate_ctx *ctx, - ofp_port_t port, uint16_t max_len, bool may_packet_in, - bool is_last_action) +xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port, + uint16_t controller_len, bool may_packet_in, + bool is_last_action, bool truncate) { ofp_port_t prev_nf_output_iface = ctx->nf_output_iface; - bool truncate = max_len != 0; ctx->nf_output_iface = NF_OUT_DROP; @@ -4855,7 +4869,7 @@ xlate_output_action(struct xlate_ctx *ctx, flood_packets(ctx, true, is_last_action); break; case OFPP_CONTROLLER: - execute_controller_action(ctx, max_len, + execute_controller_action(ctx, controller_len, (ctx->in_packet_out ? OFPR_PACKET_OUT : ctx->in_group ? OFPR_GROUP : ctx->in_action_set ? OFPR_ACTION_SET @@ -4897,8 +4911,8 @@ xlate_output_reg_action(struct xlate_ctx *ctx, memset(&value, 0xff, sizeof value); mf_write_subfield_flow(&or->src, &value, &ctx->wc->masks); - xlate_output_action(ctx, u16_to_ofp(port), or->max_len, false, - is_last_action); + xlate_output_action(ctx, u16_to_ofp(port), or->max_len, + false, is_last_action, false); } else { xlate_report(ctx, OFT_WARN, "output port %"PRIu64" is out of range", port); @@ -4945,7 +4959,7 @@ xlate_output_trunc_action(struct xlate_ctx *ctx, OVS_ACTION_ATTR_TRUNC, sizeof *trunc); trunc->max_len = max_len; - xlate_output_action(ctx, port, max_len, false, is_last_action); + xlate_output_action(ctx, port, 0, false, is_last_action, true); if (!support_trunc) { ctx->xout->slow |= SLOW_ACTION; } @@ -4970,7 +4984,8 @@ xlate_enqueue_action(struct xlate_ctx *ctx, error = dpif_queue_to_priority(ctx->xbridge->dpif, queue_id, &priority); if (error) { /* Fall back to ordinary output action. */ - xlate_output_action(ctx, enqueue->port, 0, false, is_last_action); + xlate_output_action(ctx, enqueue->port, 0, false, + is_last_action, false); return; } @@ -5043,7 +5058,7 @@ xlate_bundle_action(struct xlate_ctx *ctx, nxm_reg_load(&bundle->dst, ofp_to_u16(port), &ctx->xin->flow, ctx->wc); xlate_report_subfield(ctx, &bundle->dst); } else { - xlate_output_action(ctx, port, 0, false, is_last_action); + xlate_output_action(ctx, port, 0, false, is_last_action, false); } } @@ -6202,7 +6217,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, switch (a->type) { case OFPACT_OUTPUT: xlate_output_action(ctx, ofpact_get_OUTPUT(a)->port, - ofpact_get_OUTPUT(a)->max_len, true, last); + ofpact_get_OUTPUT(a)->max_len, true, last, + false); break; case OFPACT_GROUP: