From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org, stable@vger.kernel.org Cc: Greg Kroah-Hartman , alan@lxorguk.ukuu.org.uk, liu chuansheng , Ming Lei Subject: [ 002/123] firmware loader: Fix the race FW_STATUS_DONE is followed by class_timeout Date: Wed, 9 Jan 2013 12:34:01 -0800 Message-Id: <20130109201458.708495445@linuxfoundation.org> In-Reply-To: <20130109201458.392601412@linuxfoundation.org> References: <20130109201458.392601412@linuxfoundation.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: 3.7-stable review patch. If anyone has any objections, please let me know. ------------------ From: Chuansheng Liu commit ce2fcbd99cef580623116bb33531dbc3e6f690b0 upstream. There is a race as below when calling request_firmware(): CPU1 CPU2 write 0 > loading mutex_lock(&fw_lock) ... set_bit FW_STATUS_DONE class_timeout is coming set_bit FW_STATUS_ABORT complete_all &completion ... mutex_unlock(&fw_lock) In this time, the bit FW_STATUS_DONE and FW_STATUS_ABORT are set, and request_firmware() will return failure due to condition in _request_firmware_load(): if (!buf->size || test_bit(FW_STATUS_ABORT, &buf->status)) retval = -ENOENT; But from the above scenerio, it should be a successful requesting. So we need judge if the bit FW_STATUS_DONE is already set before calling fw_load_abort() in timeout function. As Ming's proposal, we need change the timer into sched_work to benefit from using &fw_lock mutex also. Signed-off-by: liu chuansheng Acked-by: Ming Lei Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_class.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -143,7 +143,7 @@ struct fw_cache_entry { }; struct firmware_priv { - struct timer_list timeout; + struct delayed_work timeout_work; bool nowait; struct device dev; struct firmware_buf *buf; @@ -667,11 +667,18 @@ static struct bin_attribute firmware_att .write = firmware_data_write, }; -static void firmware_class_timeout(u_long data) +static void firmware_class_timeout_work(struct work_struct *work) { - struct firmware_priv *fw_priv = (struct firmware_priv *) data; + struct firmware_priv *fw_priv = container_of(work, + struct firmware_priv, timeout_work.work); + mutex_lock(&fw_lock); + if (test_bit(FW_STATUS_DONE, &(fw_priv->buf->status))) { + mutex_unlock(&fw_lock); + return; + } fw_load_abort(fw_priv); + mutex_unlock(&fw_lock); } static struct firmware_priv * @@ -690,8 +697,8 @@ fw_create_instance(struct firmware *firm fw_priv->nowait = nowait; fw_priv->fw = firmware; - setup_timer(&fw_priv->timeout, - firmware_class_timeout, (u_long) fw_priv); + INIT_DELAYED_WORK(&fw_priv->timeout_work, + firmware_class_timeout_work); f_dev = &fw_priv->dev; @@ -858,7 +865,9 @@ static int _request_firmware_load(struct dev_dbg(f_dev->parent, "firmware: direct-loading" " firmware %s\n", buf->fw_id); + mutex_lock(&fw_lock); set_bit(FW_STATUS_DONE, &buf->status); + mutex_unlock(&fw_lock); complete_all(&buf->completion); direct_load = 1; goto handle_fw; @@ -894,15 +903,14 @@ static int _request_firmware_load(struct dev_set_uevent_suppress(f_dev, false); dev_dbg(f_dev, "firmware: requesting %s\n", buf->fw_id); if (timeout != MAX_SCHEDULE_TIMEOUT) - mod_timer(&fw_priv->timeout, - round_jiffies_up(jiffies + timeout)); + schedule_delayed_work(&fw_priv->timeout_work, timeout); kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD); } wait_for_completion(&buf->completion); - del_timer_sync(&fw_priv->timeout); + cancel_delayed_work_sync(&fw_priv->timeout_work); handle_fw: mutex_lock(&fw_lock);