On Tue, Aug 28, 2012 at 05:10:43PM -0700, Linus Torvalds wrote:
> On Tue, Aug 28, 2012 at 10:05 AM, Linus Torvalds
> <torva...@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.
>
:( 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(-)
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
