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

Commit Message

Ram Pai Aug. 29, 2012, 10:14 a.m. UTC

On Tue, Aug 28, 2012 at 05:10:43PM -0700, Linus Torvalds wrote:
> On Tue, Aug 28, 2012 at 10:05 AM, Linus Torvalds
> <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yinghai Lu Aug. 29, 2012, 4:02 p.m. UTC | #1

On Wed, Aug 29, 2012 at 3:14 AM, Ram Pai <[e-mail]> wrote:
> @@ -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;

yes, in the v7 of probe_resource() patch that  I sent before,
introduced _adjust_resource solved the problem.


@@ -741,14 +741,13 @@ void insert_resource_expand_to_fit(struc
  * arguments.  Returns 0 on success, -EBUSY if it can't fit.
  * Existing children of the resource are assumed to be immutable.
  */
-int adjust_resource(struct resource *res, resource_size_t start,
resource_size_t size)
+static int __adjust_resource(struct resource *res, resource_size_t start,
+                            resource_size_t size)
 {
        struct resource *tmp, *parent = res->parent;
        resource_size_t end = start + size - 1;
        int result = -EBUSY;

-       write_lock(&resource_lock);
-
        if (!parent)
                goto skip;

@@ -776,9 +775,19 @@ skip:
        result = 0;

  out:
-       write_unlock(&resource_lock);
        return result;
 }
+int adjust_resource(struct resource *res, resource_size_t start,
+                   resource_size_t size)
+{
+       int ret;
+
+       write_lock(&resource_lock);
+       ret = __adjust_resource(res, start, size);
+       write_unlock(&resource_lock);
+
+       return ret;
+}
 EXPORT_SYMBOL(adjust_resource);

 static void __init __reserve_region_with_split(struct resource *root,
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;