Re: [PATCH -v12 02/15] resources: Add probe_resource()

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Yinghai Lu <yinghai@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Tony Luck <tony.luck@intel.com>,
	David Miller <davem@davemloft.net>, x86 <x86@kernel.org>,
	Dominik Brodowski <linux@dominikbrodowski.net>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	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()
Date: Tue, 28 Aug 2012 17:10:43 -0700	[thread overview]
Message-ID: <CA+55aFyb-g77CQNoR72J0pjw76ZCy_nufP=VavT_X3xBg6=G-Q@mail.gmail.com> (raw)
In-Reply-To: <CA+55aFxLUExCZE1=gQpDT0ip9s+iMRYEVK2QjdCGNk9iympfYA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On Tue, Aug 28, 2012 at 10:05 AM, Linus Torvalds
<torvalds@linux-foundation.org> 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.

I don't really know if we care. Anybody who does a
"allocate_resource()" on an existing resource that might already be in
the resource tree hopefully does *not* do that in parallel with
another user trying to change the resource allocation, so maybe the
right thing to do is to just say "whatever, if there is a race with
two threads reallocating the same resource at the same time, the bug
is a much more serious one at a higher level".

So this may well be a non-issue. It was introduced some time ago, in
commit 23c570a67448e ("resource: ability to resize an allocated
resource"), since before that we never even allowed re-allocation of
an already allocated resource in the first place, and everything
happened under the lock.

I dunno. Here's a (UNTESTED!) patch that should fix it. Because it's
extremely doubtful whether this is a real problem, I'm certainly not
going to commit it now, much less mark it for stable. But I'm throwing
it out as an RFC. Technically, if people rely on any races being
handled by the resource subsystem, this *could* trigger. But I'm
hoping that the PCI layer has some higher-level locking for
"reallocate the resources of this PCI device". I did *not* check the
callers.

Btw, reallocate_resource() isn't actually used anywhere in the tree
that I can see, so maybe we should remove it and the export, and just
have the __reallocate_resource() that is static to resource.c and is
to be called only with the lock held.

                         Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 3383 bytes --]

 kernel/resource.c | 75 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 34d45886ee84..829443690497 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;
 
+	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;
 }
