2012-11-09 03:27:28

by Liu, Chuansheng

Subject: [PATCH] firmware loader: Fix the concurrent request_firmware() race for kref_get/put

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.

Signed-off-by: liu chuansheng <[email protected]>
---
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))
+ spin_unlock(&fwc->lock);
}

/* direct firmware loading support */
--
1.7.0.4

2012-11-09 08:16:05

by Ming Lei

Subject: Re: [PATCH] firmware loader: Fix the concurrent request_firmware() race for kref_get/put

On Fri, Nov 9, 2012 at 8:28 PM, Chuansheng Liu <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>


Thanks,
---
Ming Lei

2012-11-09 08:21:32

by Liu, Chuansheng

Subject: RE: [PATCH] firmware loader: Fix the concurrent request_firmware() race for kref_get/put

> $./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:
>
Thanks your pointing out, fix it soon.

2012-11-09 08:26:59

by Liu, Chuansheng

Subject: [PATCH V2] firmware loader: Fix the concurrent request_firmware() race for kref_get/put

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.

Acked-by: Ming Lei <[email protected]>
Signed-off-by: liu chuansheng <[email protected]>
---
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..be5f7aa 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))
+ spin_unlock(&fwc->lock);
}

/* direct firmware loading support */
--
1.7.0.4