Re: [PATCH] firmware loader: fix pending_fw_head list corruption

[Ming Lei]

On Fri, Aug 30, 2013 at 2:28 AM, Maxime Bizon <mbizon@xxxxxxxxxx> wrote:
> Got the following oops just before reboot:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [<8028d300>] (__list_del_entry+0x44/0xac)
> [<802e3320>] (__fw_load_abort.part.13+0x1c/0x50)
> [<802e337c>] (fw_shutdown_notify+0x28/0x50)
> [<80034f80>] (notifier_call_chain.isra.1+0x5c/0x9c)
> [<800350ec>] (__blocking_notifier_call_chain+0x44/0x58)
> [<80035114>] (blocking_notifier_call_chain+0x14/0x18)
> [<80035d64>] (kernel_restart_prepare+0x14/0x38)
> [<80035d94>] (kernel_restart+0xc/0x50)
>
> The following race condition triggers here:
>
>   _request_firmware_load()
>   device_create_file(...)
>   kobject_uevent(...)
>   (schedule)
>                                        (resume)
>                                        firmware_loading_store(1)
>                                        firmware_loading_store(0)
>                                        list_del_init(&buf->pending_list)
>                                        (schedule)
>   (resume)
>   list_add(&buf->pending_list, &pending_fw_head);
>   wait_for_completion(&buf->completion);
>
> causing an oops later when walking pending_list after the firmware has
> been released.
>
> The proposed fix is to move the list_add() before sysfs attribute
> creation.
>
> Signed-off-by: Maxime Bizon <mbizon@xxxxxxxxxx>
> ---
>  drivers/base/firmware_class.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index a439602..c8dac74 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -868,8 +868,15 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                 goto err_del_dev;
>         }
>
> +       mutex_lock(&fw_lock);
> +       list_add(&buf->pending_list, &pending_fw_head);
> +       mutex_unlock(&fw_lock);
> +
>         retval = device_create_file(f_dev, &dev_attr_loading);
>         if (retval) {
> +               mutex_lock(&fw_lock);
> +               list_del_init(&buf->pending_list);
> +               mutex_unlock(&fw_lock);
>                 dev_err(f_dev, "%s: device_create_file failed\n", __func__);
>                 goto err_del_bin_attr;
>         }
> @@ -884,10 +891,6 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
>                 kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
>         }
>
> -       mutex_lock(&fw_lock);
> -       list_add(&buf->pending_list, &pending_fw_head);
> -       mutex_unlock(&fw_lock);
> -
>         wait_for_completion(&buf->completion);
>
>         cancel_delayed_work_sync(&fw_priv->timeout_work);

Good catch, thanks.

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

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/