lkml.org 
[lkml]   [2012]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] firmware loader: Fix the concurrent request_firmware() race for kref_get/put
From
On Fri, Nov 9, 2012 at 8:28 PM, Chuansheng Liu <chuansheng.liu@intel.com> wrote:
>
> There is one race that both request_firmware() with the same
> firmware name.
>
> The race scenerio is as below:
> CPU1 CPU2
> request_firmware() -->
> _request_firmware_load() return err another request_firmware() is coming -->
> _request_firmware_cleanup is called --> _request_firmware_prepare -->
> release_firmware ---> fw_lookup_and_allocate_buf -->
> spin_lock(&fwc->lock)
> ... __fw_lookup_buf() return true
> fw_free_buf() will be called --> ...
> kref_put -->
> decrease the refcount to 0
> kref_get(&tmp->ref) ==> it will trigger warning
> due to refcount == 0
> __fw_free_buf() -->
> ... spin_unlock(&fwc->lock)
> spin_lock(&fwc->lock)
> list_del(&buf->list)
> spin_unlock(&fwc->lock)
> kfree(buf)
> After that, the freed buf will be used.
>
> The key race is decreasing refcount to 0 and list_del is not protected together by
> fwc->lock, and it is possible another thread try to get it between refcount==0
> and list_del.
>
> Fix it here to protect it together.

Good catch, the reference count and the list operation should be atomic.

>
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
> drivers/base/firmware_class.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b44ed35..7df32cd 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -246,7 +246,6 @@ static void __fw_free_buf(struct kref *ref)
> __func__, buf->fw_id, buf, buf->data,
> (unsigned int)buf->size);
>
> - spin_lock(&fwc->lock);
> list_del(&buf->list);
> spin_unlock(&fwc->lock);
>
> @@ -263,7 +262,10 @@ static void __fw_free_buf(struct kref *ref)
>
> static void fw_free_buf(struct firmware_buf *buf)
> {
> - kref_put(&buf->ref, __fw_free_buf);
> + struct firmware_cache *fwc = buf->fwc;
> + spin_lock(&fwc->lock);
> + if(!kref_put(&buf->ref, __fw_free_buf))

$./scripts/checkpatch.pl
ERROR: space required before the open parenthesis '('
#97: FILE: drivers/base/firmware_class.c:267:
+ if(!kref_put(&buf->ref, __fw_free_buf))


> + spin_unlock(&fwc->lock);
> }
>
> /* direct firmware loading support */

After fixing the patch style error, you can add my ack:

Acked-by: Ming Lei <ming.lei@canonical.com>


Thanks,
---
Ming Lei


\
 
 \ /
  Last update: 2012-11-09 10:01    [W:0.519 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site