[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACVXFVN7RVzQB5X8h=XazQm+vwwwKzGyAm5jNhiYbdqTJeR_TQ@mail.gmail.com>
Date: Fri, 9 Nov 2012 16:16:00 +0800
From: Ming Lei <ming.lei@...onical.com>
To: Chuansheng Liu <chuansheng.liu@...el.com>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org
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 <chuansheng.liu@...el.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@...el.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@...onical.com>
Thanks,
---
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists