Re: [PATCH] firmware class: remove from pending list on load failure

[Ming Lei]

On Thu, Jan 8, 2015 at 11:06 AM, Ming Lei <ming.lei@xxxxxxxxxxxxx> wrote:
> On Thu, Jan 8, 2015 at 10:37 AM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote:
>> On 01/07/2015 09:15 PM, Ming Lei wrote:
>>> On Thu, Jan 8, 2015 at 12:39 AM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote:
>>>> On 01/06/2015 11:52 PM, Ming Lei wrote:
>>>>> On Mon, Jan 5, 2015 at 11:41 PM, Sasha Levin <sasha.levin@xxxxxxxxxx> wrote:
>>>>>>> If we failed loading the firmware we have to make sure it leaves the pending
>>>>>>> list if abort wasn't executed for it.
>>>>>>>
>>>>>>> Otherwise we'd free an object still on the pending list and corrupt it.
>>>>>>>
>>>>>>> Signed-off-by: Sasha Levin <sasha.levin@xxxxxxxxxx>
>>>>>>> ---
>>>>>>>  drivers/base/firmware_class.c | 11 ++++++++++-
>>>>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>>>>>>> index 58470c3..8ccf6cf4 100644
>>>>>>> --- a/drivers/base/firmware_class.c
>>>>>>> +++ b/drivers/base/firmware_class.c
>>>>>>> @@ -929,9 +929,18 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>>>>>>>         cancel_delayed_work_sync(&fw_priv->timeout_work);
>>>>>>>         if (is_fw_load_aborted(buf))
>>>>>>>                 retval = -EAGAIN;
>>>>>>> -       else if (!buf->data)
>>>>>>> +       else if (!buf->data) {
>>>>>>>                 retval = -ENOMEM;
>>>>>>>
>>>>>>> +               /*
>>>>>>> +                * We failed loading, but abort was never done so we
>>>>>>> +                * need to remove it from the pending list ourselves.
>>>>>>> +                */
>>>>>>> +               mutex_lock(&fw_lock);
>>>>>>> +               list_del_init(&buf->pending_list);
>>>>>>> +               mutex_unlock(&fw_lock);
>>>>> The buf is always removed before the complete_all(), isn't it? Or did
>>>>> you observe the issue?
>>>>
>>>> Not in the case where userspacehelper fails. Yes, this is not a theoretical
>>>> thing and can be easily reproduced.
>>>
>>> OK, I am just curious how it is triggered.
>>>
>>> One situation I thought of is that the request context is interrupted
>>> by signal after wait_for_completion() becomes interruptable, and the
>>> fix should abort the request if retval is -ERESTARTSYS.
>>>
>>> Or could you share how to reproduce if it isn't above case?
>>
>> It's pretty simple, just abort it once with Ctrl-C:
>
> Looks it is just the case I said, :-)
>
> Then I suggest you to remove the buf from list in case of
> 'retval == -ERESTARTSYS', and the return value should be
> changed to -EINTR.

Thought of the case further, the request still need to be
aborted in the situation since it doesn't make sense for
requests on same fw in other contexts to wait any more.


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/