From patchwork Tue Jan 19 07:26:48 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 569826 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (unknown [IPv6:2600:3c00::f03c:91ff:fe6e:bdf7]) by ozlabs.org (Postfix) with ESMTP id 6EC901402E2 for ; Tue, 19 Jan 2016 18:27:47 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 08466102A7; Mon, 18 Jan 2016 23:27:40 -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 5B5B410287 for ; Mon, 18 Jan 2016 23:27:38 -0800 (PST) Received: from bar2.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 80CEC420671 for ; Tue, 19 Jan 2016 00:27:37 -0700 (MST) X-ASG-Debug-ID: 1453188456-03dc5377946f500001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar2.cudamail.com with ESMTP id 9Z81e2MUEVVQ2cFn (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Tue, 19 Jan 2016 00:27:36 -0700 (MST) X-Barracuda-Envelope-From: blp@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO relay3-d.mail.gandi.net) (217.70.183.195) by mx1-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 19 Jan 2016 07:27:36 -0000 Received-SPF: pass (mx1-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.195 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.195 X-Barracuda-RBL-IP: 217.70.183.195 Received: from mfilter27-d.gandi.net (mfilter27-d.gandi.net [217.70.178.155]) by relay3-d.mail.gandi.net (Postfix) with ESMTP id 47CA9A80C7; Tue, 19 Jan 2016 08:27:35 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter27-d.gandi.net Received: from relay3-d.mail.gandi.net ([IPv6:::ffff:217.70.183.195]) by mfilter27-d.gandi.net (mfilter27-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id oJ8UAa9TJXsK; Tue, 19 Jan 2016 08:27:33 +0100 (CET) X-Originating-IP: 173.228.112.189 Received: from sigabrt.gateway.sonic.net (173-228-112-189.dsl.dynamic.fusionbroadband.com [173.228.112.189]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 27470A80E4; Tue, 19 Jan 2016 08:27:32 +0100 (CET) X-CudaMail-Envelope-Sender: blp@ovn.org From: Ben Pfaff To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-118000555 X-CudaMail-DTE: 011916 X-CudaMail-Originating-IP: 217.70.183.195 Date: Mon, 18 Jan 2016 23:26:48 -0800 X-ASG-Orig-Subj: [##CM-E2-118000555##][PATCH 01/41] ofproto: Fix memory leak and memory exhaustion bugs in group_mod. Message-Id: <1453188448-2437-2-git-send-email-blp@ovn.org> X-Mailer: git-send-email 2.1.3 In-Reply-To: <1453188448-2437-1-git-send-email-blp@ovn.org> References: <1453188448-2437-1-git-send-email-blp@ovn.org> X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1453188456 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 Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 01/41] ofproto: Fix memory leak and memory exhaustion bugs in group_mod. 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: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" In handle_group_mod() cases where adding a group failed, nothing freed the list of buckets, causing a leak. The same was true in every case of modifying a group. This commit fixes the problem by changing add_group() to never steal or free the buckets (modify_group() already acted this way) and then making handle_group_mod() always free the buckets when it's done. This approach might at first raise objections, because it makes add_group() copy the buckets instead of just take the existing ones. But it actually fixes a worse problem too: when OF1.4+ REQUESTFORWARD is enabled, the group_mod is reused for the request forwarding. Until now, for a group_mod that adds a new group and that has some buckets, the previous stealing of buckets in add_group() meant that the group_mod's buckets were no longer valid; in practice, the list of buckets became linked in a way that iteration never terminated, which caused memory to be exhausted while composing the requestforward message. By making add_group() no longer modify the group_mod, we also fix this problem. The requestforward test in the testsuite did not find the latter problem because it only added a group without any buckets. This commit also updates the testsuite to include a bucket in its group_mod, which would have found the problem. Found by pain and suffering. Signed-off-by: Ben Pfaff Acked-by: Jarno Rajahalme --- ofproto/ofproto.c | 17 +++++++++++------ tests/ofproto.at | 16 ++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 528d5ac..386009e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -297,7 +297,8 @@ static bool ofproto_group_exists__(const struct ofproto *ofproto, static bool ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) OVS_EXCLUDED(ofproto->groups_rwlock); -static enum ofperr add_group(struct ofproto *, struct ofputil_group_mod *); +static enum ofperr add_group(struct ofproto *, + const struct ofputil_group_mod *); static void handle_openflow(struct ofconn *, const struct ofpbuf *); static enum ofperr ofproto_flow_mod_start(struct ofproto *, struct ofproto_flow_mod *) @@ -6280,7 +6281,7 @@ handle_queue_get_config_request(struct ofconn *ofconn, } static enum ofperr -init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, +init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, struct ofgroup **ofgroup) { enum ofperr error; @@ -6306,7 +6307,9 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, *CONST_CAST(long long int *, &((*ofgroup)->modified)) = now; ovs_refcount_init(&(*ofgroup)->ref_count); - list_move(&(*ofgroup)->buckets, &gm->buckets); + list_init(&(*ofgroup)->buckets); + ofputil_bucket_clone_list(&(*ofgroup)->buckets, &gm->buckets, NULL); + *CONST_CAST(uint32_t *, &(*ofgroup)->n_buckets) = list_size(&(*ofgroup)->buckets); @@ -6326,7 +6329,7 @@ init_group(struct ofproto *ofproto, struct ofputil_group_mod *gm, * 'ofproto''s group table. Returns 0 on success or an OpenFlow error code on * failure. */ static enum ofperr -add_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) +add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) { struct ofgroup *ofgroup; enum ofperr error; @@ -6474,7 +6477,7 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup, * ofproto's ofgroup hash map. Thus, the group is never altered while users of * the xlate module hold a pointer to the group. */ static enum ofperr -modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm) +modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) { struct ofgroup *ofgroup, *new_ofgroup, *retiring; enum ofperr error; @@ -6643,7 +6646,7 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d", ofproto->name, gm.command); } - return OFPERR_OFPGMFC_BAD_COMMAND; + error = OFPERR_OFPGMFC_BAD_COMMAND; } if (!error) { @@ -6653,6 +6656,8 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) rf.group_mod = &gm; connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf); } + ofputil_bucket_list_destroy(&gm.buckets); + return error; } diff --git a/tests/ofproto.at b/tests/ofproto.at index b3b8a0f..787def1 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -3144,25 +3144,25 @@ check_async () { shift # OFPGC_ADD - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020000000000000001 + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020000000000000001 00100000 ffffffffffffffff 00000000" if test X"$1" = X"OFPGC_ADD"; then shift; echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): - ADD group_id=1,type=all" + ADD group_id=1,type=all,bucket=actions=drop" echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod - ADD group_id=1,type=all" + ADD group_id=1,type=all,bucket=bucket_id:0,actions=drop" echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod - ADD group_id=1,type=all" + ADD group_id=1,type=all,bucket=actions=drop" fi # OFPGC_MODIFY - ovs-appctl -t `pwd`/c2 ofctl/send 050f0010000000020001010000000001 + ovs-appctl -t `pwd`/c2 ofctl/send "050f0020000000020001010000000001 00100000 ffffffffffffffff 00000000" if test X"$1" = X"OFPGC_MODIFY"; then shift; echo >>expout2 "send: OFPT_GROUP_MOD (OF1.4): - MOD group_id=1,type=select" + MOD group_id=1,type=select,bucket=weight:0,actions=drop" echo >>expout1 "OFPT_REQUESTFORWARD (OF1.5): reason=group_mod - MOD group_id=1,type=select" + MOD group_id=1,type=select,bucket=bucket_id:0,weight:0,actions=drop" echo >>expout3 "OFPT_REQUESTFORWARD (OF1.4): reason=group_mod - MOD group_id=1,type=select" + MOD group_id=1,type=select,bucket=weight:0,actions=drop" fi ovs-appctl -t `pwd`/c1 ofctl/barrier