From patchwork Wed Nov 25 19:23:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jarno Rajahalme X-Patchwork-Id: 548724 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (li376-54.members.linode.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3577C1402D4 for ; Thu, 26 Nov 2015 06:23:49 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id D3DE110C2A; Wed, 25 Nov 2015 11:23:48 -0800 (PST) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 4651C10C29 for ; Wed, 25 Nov 2015 11:23:47 -0800 (PST) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 6A15B420043 for ; Wed, 25 Nov 2015 12:23:46 -0700 (MST) X-ASG-Debug-ID: 1448479425-09eadd53d13f8350001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar5.cudamail.com with ESMTP id ffXllAL8Ndh7FrzG (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 25 Nov 2015 12:23:45 -0700 (MST) X-Barracuda-Envelope-From: jarno@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO relay2-d.mail.gandi.net) (217.70.183.194) by mx1-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 25 Nov 2015 19:23:44 -0000 Received-SPF: pass (mx1-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.194 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.194 X-Barracuda-RBL-IP: 217.70.183.194 Received: from mfilter22-d.gandi.net (mfilter22-d.gandi.net [217.70.178.150]) by relay2-d.mail.gandi.net (Postfix) with ESMTP id 6B9EFC5A5A; Wed, 25 Nov 2015 20:23:39 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter22-d.gandi.net Received: from relay2-d.mail.gandi.net ([IPv6:::ffff:217.70.183.194]) by mfilter22-d.gandi.net (mfilter22-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id 93ggML87Slz0; Wed, 25 Nov 2015 20:23:38 +0100 (CET) X-Originating-IP: 208.91.1.34 Received: from htb-1n-eng-dhcp122.eng.vmware.com (unknown [208.91.1.34]) (Authenticated sender: jarno@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B82ACC5A5D; Wed, 25 Nov 2015 20:23:36 +0100 (CET) Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2104\)) X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-1124067442 X-CudaMail-DTE: 112515 X-CudaMail-Originating-IP: 217.70.183.194 X-CudaMail-Envelope-Sender: jarno@ovn.org X-ASG-Orig-Subj: [##CM-E2-1124067442##]Re: [ovs-dev] [PATCH v3 1/2] ofproto: Allow xlate_actions() to fail. From: Jarno Rajahalme In-Reply-To: Date: Wed, 25 Nov 2015 11:23:35 -0800 Message-Id: References: <1448401261-33942-1-git-send-email-jarno@ovn.org> To: Joe Stringer X-Mailer: Apple Mail (2.2104) X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1448479425 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-Content-Filtered-By: Mailman/MimeDel 2.1.16 Cc: "dev@openvswitch.org" Subject: Re: [ovs-dev] [PATCH v3 1/2] ofproto: Allow xlate_actions() to fail. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" > On Nov 25, 2015, at 11:11 AM, Jarno Rajahalme wrote: > > >> On Nov 25, 2015, at 10:52 AM, Joe Stringer > wrote: >> >> On 25 November 2015 at 10:31, Jarno Rajahalme > wrote: >>> >>>> On Nov 24, 2015, at 5:02 PM, Joe Stringer > wrote: >>>> >>>> On 24 November 2015 at 13:41, Jarno Rajahalme > wrote: >>>>> Sometimes xlate_actions() fails due to too deep recursion, too many >>>>> MPLS labels, or missing recirculation context. Make xlate_actions() >>>>> clear out the produced odp actions in these cases to make it easy for >>>>> the caller to install a drop flow (instead or installing a flow with >>>>> partially translated actions). Also, return a specific error code, so >>>>> that the error can be properly propagated where meaningful. >>>>> >>>>> Before this patch 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 > >>>> >>>> Should this also set the error when receiving packets on a mirror port >>>> in xlate_actions()? Or when receiving tagged VLAN traffic that doesn't >>>> correspond to the port's vlan tag? Or when a group has no live bucket? >>>> Are there any other cases that should also be covered? (I just scanned >>>> across ofproto/ofproto-dpif-xlate.c looking for cases where we're >>>> already logging that we drop the packet, but maybe there's a reasoning >>>> behind not including these - if so, please enlighten me) >>> >>> No reasoning for missing those, I just did not notice them. Thanks for pointing them out. >> >> OK, I thought it may have been something like "expected errors" vs. >> "unexpected errors". > > Looking into these I noticed this to be the case. Must discern whether to fail just the individual action v.s. the whole pipeline. > How about this incremental to cover two cases here (rest are “expected errors” IMO): The last one is debatable, as setting the error fails the whole translation rather than just the normal action. But this is most likely an configuration error, so maybe failing the whole pipeline (and installing a drop flow) is the right thing to do here? > Jarno > Acked-by: Joe Stringer Acked-by: Joe Stringer diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 36a6fbc..2908339 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -336,6 +336,10 @@ const char *xlate_strerror(enum xlate_error error) return "Recirculation conflict"; case XLATE_TOO_MANY_MPLS_LABELS: return "Too many MPLS labels"; + case XLATE_BUCKET_CHAINING_TOO_DEEP: + return "Bucket chaining too deep"; + case XLATE_NO_INPUT_BUNDLE: + return "No input bundle"; } return "Unknown error"; } @@ -1444,10 +1448,9 @@ bucket_is_alive(const struct xlate_ctx *ctx, struct ofputil_bucket *bucket, int depth) { if (depth >= MAX_LIVENESS_RECURSION) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - - VLOG_WARN_RL(&rl, "bucket chaining exceeded %d links", - MAX_LIVENESS_RECURSION); + XLATE_REPORT_ERROR(ctx, "bucket chaining exceeded %d links", + MAX_LIVENESS_RECURSION); + ctx->error = XLATE_BUCKET_CHAINING_TOO_DEEP; return false; } @@ -2323,7 +2326,8 @@ xlate_normal(struct xlate_ctx *ctx) in_xbundle = lookup_input_bundle(ctx->xbridge, flow->in_port.ofp_port, ctx->xin->packet != NULL, &in_port); if (!in_xbundle) { - xlate_report(ctx, "no input bundle, dropping"); + XLATE_REPORT_ERROR(ctx, "no input bundle, dropping"); + ctx->error = XLATE_NO_INPUT_BUNDLE; return; }