From patchwork Wed Aug 29 10:14:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ram Pai X-Patchwork-Id: 1384411 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 4150FDF215 for ; Wed, 29 Aug 2012 10:15:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752589Ab2H2KOo (ORCPT ); Wed, 29 Aug 2012 06:14:44 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:39246 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752543Ab2H2KOn (ORCPT ); Wed, 29 Aug 2012 06:14:43 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 29 Aug 2012 04:14:42 -0600 Received: from d03dlp03.boulder.ibm.com (9.17.202.179) by e36.co.us.ibm.com (192.168.1.136) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 29 Aug 2012 04:14:32 -0600 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id A285B19D803E; Wed, 29 Aug 2012 04:14:31 -0600 (MDT) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7TAEUlA182886; Wed, 29 Aug 2012 04:14:30 -0600 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7TAESkJ027747; Wed, 29 Aug 2012 04:14:30 -0600 Received: from us.ibm.com ([9.123.236.179]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id q7TAEN7t027362; Wed, 29 Aug 2012 04:14:24 -0600 Received: by us.ibm.com (sSMTP sendmail emulation); Wed, 29 Aug 2012 18:14:23 +0800 Date: Wed, 29 Aug 2012 18:14:23 +0800 From: Ram Pai To: Linus Torvalds Cc: Yinghai Lu , Bjorn Helgaas , Benjamin Herrenschmidt , Tony Luck , David Miller , x86 , Dominik Brodowski , Andrew Morton , Greg Kroah-Hartman , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org Subject: Re: [PATCH -v12 02/15] resources: Add probe_resource() Message-ID: <20120829101423.GA18856@ram-ThinkPad-T61> Reply-To: Ram Pai References: <1340736849-14875-1-git-send-email-yinghai@kernel.org> <1340736849-14875-3-git-send-email-yinghai@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Content-Scanned: Fidelis XPS MAILER x-cbid: 12082910-7606-0000-0000-000003294CFE Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Aug 28, 2012 at 05:10:43PM -0700, Linus Torvalds wrote: > On Tue, Aug 28, 2012 at 10:05 AM, Linus Torvalds > wrote: > > > > Ugh. Ok, looking closer at this, > > Btw, looking at that code, I also found what looks like a potential > locking bug in allocate_resource(). > > The code does > > if (new->parent) > .. reallocate .. > > to check whether a resource was already allocated. HOWEVER, it does so > without actually holding the resource lock. Which means that > "new->parent" might in theory change. > :( it was my mistake. BTW: adjust_resource() also has the same problem. It is also accessing res->parent without holding the lock. The following patch enhances your patch to fix that potential race too. kernel/resource.c | 81 +++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 45 insertions(+), 36 deletions(-) --- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/kernel/resource.c b/kernel/resource.c index 34d4588..427ed48 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -462,50 +462,59 @@ static int find_resource(struct resource *root, struct resource *new, return __find_resource(root, NULL, new, size, constraint); } -/** - * reallocate_resource - allocate a slot in the resource tree given range & alignment. - * The resource will be relocated if the new size cannot be reallocated in the - * current location. - * - * @root: root resource descriptor - * @old: resource descriptor desired by caller - * @newsize: new size of the resource descriptor - * @constraint: the size and alignment constraints to be met. - */ -int reallocate_resource(struct resource *root, struct resource *old, + +static int __reallocate_resource(struct resource *root, struct resource *old, resource_size_t newsize, struct resource_constraint *constraint) { - int err=0; + int err; struct resource new = *old; struct resource *conflict; - write_lock(&resource_lock); - - if ((err = __find_resource(root, old, &new, newsize, constraint))) - goto out; + err = __find_resource(root, old, &new, newsize, constraint); + if (err) + return err; if (resource_contains(&new, old)) { old->start = new.start; old->end = new.end; - goto out; + return 0; } - if (old->child) { - err = -EBUSY; - goto out; - } + if (old->child) + return -EBUSY; if (resource_contains(old, &new)) { old->start = new.start; old->end = new.end; - } else { - __release_resource(old); - *old = new; - conflict = __request_resource(root, old); - BUG_ON(conflict); + return 0; } -out: + + __release_resource(old); + *old = new; + conflict = __request_resource(root, old); + BUG_ON(conflict); + return 0; +} + +/** + * reallocate_resource - allocate a slot in the resource tree given range + * & alignment. The resource will be relocated if the new size cannot be + * reallocated in the current location. + * + * @root: root resource descriptor + * @old: resource descriptor desired by caller + * @newsize: new size of the resource descriptor + * @constraint: the size and alignment constraints to be met. + */ +int reallocate_resource(struct resource *root, struct resource *old, + resource_size_t newsize, + struct resource_constraint *constraint) +{ + int err; + + write_lock(&resource_lock); + err = __reallocate_resource(root, old, newsize, constraint); write_unlock(&resource_lock); return err; } @@ -544,16 +553,16 @@ int allocate_resource(struct resource *root, struct resource *new, constraint.alignf = alignf; constraint.alignf_data = alignf_data; - if ( new->parent ) { + write_lock(&resource_lock); + if (new->parent) { /* resource is already allocated, try reallocating with the new constraints */ - return reallocate_resource(root, new, size, &constraint); + err = __reallocate_resource(root, new, size, &constraint); + } else { + err = find_resource(root, new, size, &constraint); + if (err >= 0 && __request_resource(root, new)) + err = -EBUSY; } - - write_lock(&resource_lock); - err = find_resource(root, new, size, &constraint); - if (err >= 0 && __request_resource(root, new)) - err = -EBUSY; write_unlock(&resource_lock); return err; } @@ -718,12 +727,12 @@ void insert_resource_expand_to_fit(struct resource *root, struct resource *new) */ int adjust_resource(struct resource *res, resource_size_t start, resource_size_t size) { - struct resource *tmp, *parent = res->parent; + struct resource *tmp, *parent; resource_size_t end = start + size - 1; int result = -EBUSY; write_lock(&resource_lock); - + parent = res->parent; if (!parent) goto skip;