Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Andy Lutomirski]

On Wed, May 24, 2017 at 3:38 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> On Wed, May 24, 2017 at 3:00 PM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>> On Wed, May 24, 2017 at 2:40 PM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
>>> From: Martin Fuzzey <mfuzzey@xxxxxxxxxxx>
>>>
>>> Commit 0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
>>> is interrupted") added via 4.0 added support to abort the fallback mechanism
>>> when a signal was detected and wait_for_completion_interruptible() returned
>>> -ERESTARTSYS. Although the abort was effective we were unfortunately never
>>> really propagating this error though and as such userspace could not know
>>> why the abort happened.
>>
>> Can you give a simple example of what's going on and why it matters?
>>
>> ERESTARTSYS and friends are highly magical, and I'm not convinced that
>> allowing _request_firmware_load to return -ERESTARTSYS is actually a
>> good idea.  What if there are system calls that can't handle this
>> style of restart that start being restarted as a result?
>
> This seems to be a linux-api question, so Cc'ing them and Michael.
>
> For those not familiar it is worth explaining first the user interface.
>
> This describes the fallback mechanism of the Linux firmware API if
> direct filesystem lookup fails.

...

> While we wait we can get a
> -ERESTARTSYS since swait_event_interruptible_timeout() uses
> __swait_event_interruptible_timeout() under the hood and this in turn
> ___swait_event() which can prepare_to_swait_event() which can return
> -ERESTARTSYS on signal_pending_state().

This is too much kernel detail and too little ABI detail.

User code does some syscall.  Kernel requests firmware and that
request gets interrupted.  What syscall is this?  read(2)?  open(2)?
Something else?

mutex_lock_interruptible() returns -EINTR if interrupted.  It seems
odd to be that requesting firmware would be different.

>
> The issue discovered was that Android could issue SIGCHLD and the
> waiter gets a signal but the reason for the exact reason for the
> failure is not propagated. The proposed patch propagates -ERESTARTSYS
> when that is returned on signal_pending_state() as we wait.

Maybe SIGCHLD shouldn't interrupt firmware loading?