[PATCH] omap: resource: Fix race in register_resource()

[PATCH] omap: resource: Fix race in register_resource()

[Mike Chan]

Checking if the resource is already registered and adding to the list
must be atomic or bad things can happen.

Signed-off-by: Mike Chan <mike@android.com>
---
 arch/arm/plat-omap/resource.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
index e9ace13..9d20249 100644
--- a/arch/arm/plat-omap/resource.c
+++ b/arch/arm/plat-omap/resource.c
@@ -253,6 +253,7 @@ int resource_refresh(void)
  */
 int resource_register(struct shared_resource *resp)
 {
+	int ret = 0;
 	if (!resp)
 		return -EINVAL;
 
@@ -260,13 +261,15 @@ int resource_register(struct shared_resource *resp)
 		return -EINVAL;
 
 	/* Verify that the resource is not already registered */
-	if (resource_lookup(resp->name))
-		return -EEXIST;
+	down(&res_mutex);
+	if (_resource_lookup(resp->name)) {
+		ret = -EEXIST;
+		goto out;
+	}
 
 	INIT_LIST_HEAD(&resp->users_list);
 	mutex_init(&resp->resource_mutex);
 
-	down(&res_mutex);
 	/* Add the resource to the resource list */
 	list_add(&resp->node, &res_list);
 
@@ -274,10 +277,11 @@ int resource_register(struct shared_resource *resp)
 	if (resp->ops->init)
 		resp->ops->init(resp);
 
-	up(&res_mutex);
 	pr_debug("resource: registered %s\n", resp->name);
 
-	return 0;
+out:
+	up(&res_mutex);
+	return ret;
 }
 EXPORT_SYMBOL(resource_register);
 
-- 
1.5.4.5

Re: [PATCH] omap: resource: Fix race in register_resource()

[Kevin Hilman]

Mike Chan <mike@android.com> writes:

> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>

Functionally, this looks fine.  Some procedural details still to work
out:

Are you the original author of this?  or was it Chunqiu or Limei?  The
changelog should be prefixed with a From: line of the original author
(handled by git-format-patch) if it was not you.  Also add the other's
signoffs if appropriate.

What tree does this apply to?  It's not specified, and does not apply
to either current PM branch or pm-2.6.29. 

Kevin

> ---
>  arch/arm/plat-omap/resource.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index e9ace13..9d20249 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -253,6 +253,7 @@ int resource_refresh(void)
>   */
>  int resource_register(struct shared_resource *resp)
>  {
> +	int ret = 0;
>  	if (!resp)
>  		return -EINVAL;
>  
> @@ -260,13 +261,15 @@ int resource_register(struct shared_resource *resp)
>  		return -EINVAL;
>  
>  	/* Verify that the resource is not already registered */
> -	if (resource_lookup(resp->name))
> -		return -EEXIST;
> +	down(&res_mutex);
> +	if (_resource_lookup(resp->name)) {
> +		ret = -EEXIST;
> +		goto out;
> +	}
>  
>  	INIT_LIST_HEAD(&resp->users_list);
>  	mutex_init(&resp->resource_mutex);
>  
> -	down(&res_mutex);
>  	/* Add the resource to the resource list */
>  	list_add(&resp->node, &res_list);
>  
> @@ -274,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>  	if (resp->ops->init)
>  		resp->ops->init(resp);
>  
> -	up(&res_mutex);
>  	pr_debug("resource: registered %s\n", resp->name);
>  
> -	return 0;
> +out:
> +	up(&res_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL(resource_register);
>  
> -- 
> 1.5.4.5

Re: [PATCH] omap: resource: Fix race in register_resource()

[Mike Chan]

On Tue, Sep 8, 2009 at 3:39 PM, Kevin Hilman<khilman@deeprootsystems.com> wrote:
> Mike Chan <mike@android.com> writes:
>
>> Checking if the resource is already registered and adding to the list
>> must be atomic or bad things can happen.
>>
>> Signed-off-by: Mike Chan <mike@android.com>
>
> Functionally, this looks fine.  Some procedural details still to work
> out:
>
> Are you the original author of this?  or was it Chunqiu or Limei?  The
> changelog should be prefixed with a From: line of the original author
> (handled by git-format-patch) if it was not you.  Also add the other's
> signoffs if appropriate.
>

I am the original author of this patch. Chunqiu sent two patches out
1) a per-resource level mutex (vs global).
2) Make update_resource_level() thread safe.

I've been meaning to send them out to l-o on behalf of Chunqiu.

> What tree does this apply to?  It's not specified, and does not apply
> to either current PM branch or pm-2.6.29.
>

I thought I rebased this off of pm but I may have goofed. I can send a new one.

-- Mike

> Kevin
>
>> ---
>>  arch/arm/plat-omap/resource.c |   14 +++++++++-----
>>  1 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>> index e9ace13..9d20249 100644
>> --- a/arch/arm/plat-omap/resource.c
>> +++ b/arch/arm/plat-omap/resource.c
>> @@ -253,6 +253,7 @@ int resource_refresh(void)
>>   */
>>  int resource_register(struct shared_resource *resp)
>>  {
>> +     int ret = 0;
>>       if (!resp)
>>               return -EINVAL;
>>
>> @@ -260,13 +261,15 @@ int resource_register(struct shared_resource *resp)
>>               return -EINVAL;
>>
>>       /* Verify that the resource is not already registered */
>> -     if (resource_lookup(resp->name))
>> -             return -EEXIST;
>> +     down(&res_mutex);
>> +     if (_resource_lookup(resp->name)) {
>> +             ret = -EEXIST;
>> +             goto out;
>> +     }
>>
>>       INIT_LIST_HEAD(&resp->users_list);
>>       mutex_init(&resp->resource_mutex);
>>
>> -     down(&res_mutex);
>>       /* Add the resource to the resource list */
>>       list_add(&resp->node, &res_list);
>>
>> @@ -274,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>>       if (resp->ops->init)
>>               resp->ops->init(resp);
>>
>> -     up(&res_mutex);
>>       pr_debug("resource: registered %s\n", resp->name);
>>
>> -     return 0;
>> +out:
>> +     up(&res_mutex);
>> +     return ret;
>>  }
>>  EXPORT_SYMBOL(resource_register);
>>
>> --
>> 1.5.4.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap: resource: Fix race in register_resource()

[Mike Chan]

On Wed, Sep 30, 2009 at 4:49 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Mike Chan <mike@android.com> writes:
>
>> Checking if the resource is already registered and adding to the list
>> must be atomic or bad things can happen.
>>
>> Signed-off-by: Mike Chan <mike@android.com>
>> ---
>>  arch/arm/plat-omap/resource.c |   13 ++++++++-----
>>  1 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
>> index 111020a..4631912 100644
>> --- a/arch/arm/plat-omap/resource.c
>> +++ b/arch/arm/plat-omap/resource.c
>> @@ -255,6 +255,7 @@ int resource_refresh(void)
>>   */
>>  int resource_register(struct shared_resource *resp)
>>  {
>> +     int ret = 0;
>>       if (!resp)
>>               return -EINVAL;
>>
>> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
>>               return -EINVAL;
>>
>>       /* Verify that the resource is not already registered */
>> -     if (resource_lookup(resp->name))
>> -             return -EEXIST;
>> +     down(&res_mutex);
>> +     if (_resource_lookup(resp->name))
>> +             ret = -EEXIST;
>> +             goto out;
>
> Ahem, you're rebased version has a little problem.  Missing {} block
> mans this goto is always executed and no resources are ever added.
>
> I'll fix this up manually before applying, but some more testing next
> time would be helpful
>

Opps, sorry about that :/ and thanks for cleaning it up.

-- Mike

> Thanks,
>
> Kevin
>
>
>>       INIT_LIST_HEAD(&resp->users_list);
>>
>> -     down(&res_mutex);
>>       /* Add the resource to the resource list */
>>       list_add(&resp->node, &res_list);
>>
>> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>>       if (resp->ops->init)
>>               resp->ops->init(resp);
>>
>> -     up(&res_mutex);
>>       pr_debug("resource: registered %s\n", resp->name);
>>
>> -     return 0;
>> +out:
>> +     up(&res_mutex);
>> +     return ret;
>>  }
>>  EXPORT_SYMBOL(resource_register);
>>
>> --
>> 1.5.4.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] omap: resource: Fix race in register_resource()

[Kevin Hilman]

Mike Chan <mike@android.com> writes:

> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>
> ---
>  arch/arm/plat-omap/resource.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 111020a..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -255,6 +255,7 @@ int resource_refresh(void)
>   */
>  int resource_register(struct shared_resource *resp)
>  {
> +	int ret = 0;
>  	if (!resp)
>  		return -EINVAL;
>  
> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
>  		return -EINVAL;
>  
>  	/* Verify that the resource is not already registered */
> -	if (resource_lookup(resp->name))
> -		return -EEXIST;
> +	down(&res_mutex);
> +	if (_resource_lookup(resp->name))
> +		ret = -EEXIST;
> +		goto out;

Ahem, you're rebased version has a little problem.  Missing {} block
mans this goto is always executed and no resources are ever added.

I'll fix this up manually before applying, but some more testing next
time would be helpful

Thanks,

Kevin


>  	INIT_LIST_HEAD(&resp->users_list);
>  
> -	down(&res_mutex);
>  	/* Add the resource to the resource list */
>  	list_add(&resp->node, &res_list);
>  
> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>  	if (resp->ops->init)
>  		resp->ops->init(resp);
>  
> -	up(&res_mutex);
>  	pr_debug("resource: registered %s\n", resp->name);
>  
> -	return 0;
> +out:
> +	up(&res_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL(resource_register);
>  
> -- 
> 1.5.4.5

Re: [PATCH] omap: resource: Fix race in register_resource()

[Kevin Hilman]

Mike Chan <mike@android.com> writes:

> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>

Thanks, this one applies.  Pushing to PM branch and pm-2.6.29.

Kevin

> ---
>  arch/arm/plat-omap/resource.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 111020a..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -255,6 +255,7 @@ int resource_refresh(void)
>   */
>  int resource_register(struct shared_resource *resp)
>  {
> +	int ret = 0;
>  	if (!resp)
>  		return -EINVAL;
>  
> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
>  		return -EINVAL;
>  
>  	/* Verify that the resource is not already registered */
> -	if (resource_lookup(resp->name))
> -		return -EEXIST;
> +	down(&res_mutex);
> +	if (_resource_lookup(resp->name))
> +		ret = -EEXIST;
> +		goto out;
>  
>  	INIT_LIST_HEAD(&resp->users_list);
>  
> -	down(&res_mutex);
>  	/* Add the resource to the resource list */
>  	list_add(&resp->node, &res_list);
>  
> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>  	if (resp->ops->init)
>  		resp->ops->init(resp);
>  
> -	up(&res_mutex);
>  	pr_debug("resource: registered %s\n", resp->name);
>  
> -	return 0;
> +out:
> +	up(&res_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL(resource_register);
>  
> -- 
> 1.5.4.5

Re: [PATCH] omap: resource: Fix race in register_resource()

[Mike Chan]

On Thu, Sep 17, 2009 at 4:32 PM, Mike Chan <mike@android.com> wrote:
> Checking if the resource is already registered and adding to the list
> must be atomic or bad things can happen.
>
> Signed-off-by: Mike Chan <mike@android.com>
> ---
>  arch/arm/plat-omap/resource.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
> index 111020a..4631912 100644
> --- a/arch/arm/plat-omap/resource.c
> +++ b/arch/arm/plat-omap/resource.c
> @@ -255,6 +255,7 @@ int resource_refresh(void)
>  */
>  int resource_register(struct shared_resource *resp)
>  {
> +       int ret = 0;
>        if (!resp)
>                return -EINVAL;
>
> @@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
>                return -EINVAL;
>
>        /* Verify that the resource is not already registered */
> -       if (resource_lookup(resp->name))
> -               return -EEXIST;
> +       down(&res_mutex);
> +       if (_resource_lookup(resp->name))
> +               ret = -EEXIST;
> +               goto out;
>
>        INIT_LIST_HEAD(&resp->users_list);
>
> -       down(&res_mutex);
>        /* Add the resource to the resource list */
>        list_add(&resp->node, &res_list);
>
> @@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
>        if (resp->ops->init)
>                resp->ops->init(resp);
>
> -       up(&res_mutex);
>        pr_debug("resource: registered %s\n", resp->name);
>
> -       return 0;
> +out:
> +       up(&res_mutex);
> +       return ret;
>  }
>  EXPORT_SYMBOL(resource_register);
>
> --
> 1.5.4.5
>
>

Kevin, this should be based off of your pm branch (2.6.30?)

-- Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] omap: resource: Fix race in register_resource()

[Mike Chan]

Checking if the resource is already registered and adding to the list
must be atomic or bad things can happen.

Signed-off-by: Mike Chan <mike@android.com>
---
 arch/arm/plat-omap/resource.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c
index 111020a..4631912 100644
--- a/arch/arm/plat-omap/resource.c
+++ b/arch/arm/plat-omap/resource.c
@@ -255,6 +255,7 @@ int resource_refresh(void)
  */
 int resource_register(struct shared_resource *resp)
 {
+	int ret = 0;
 	if (!resp)
 		return -EINVAL;
 
@@ -262,12 +263,13 @@ int resource_register(struct shared_resource *resp)
 		return -EINVAL;
 
 	/* Verify that the resource is not already registered */
-	if (resource_lookup(resp->name))
-		return -EEXIST;
+	down(&res_mutex);
+	if (_resource_lookup(resp->name))
+		ret = -EEXIST;
+		goto out;
 
 	INIT_LIST_HEAD(&resp->users_list);
 
-	down(&res_mutex);
 	/* Add the resource to the resource list */
 	list_add(&resp->node, &res_list);
 
@@ -275,10 +277,11 @@ int resource_register(struct shared_resource *resp)
 	if (resp->ops->init)
 		resp->ops->init(resp);
 
-	up(&res_mutex);
 	pr_debug("resource: registered %s\n", resp->name);
 
-	return 0;
+out:
+	up(&res_mutex);
+	return ret;
 }
 EXPORT_SYMBOL(resource_register);
 
-- 
1.5.4.5