Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1661849 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback Andy Lutomirski <luto@kernel.org> - 2017-06-09 03:20 +0200
    Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-09 03:40 +0200
      Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-06-09 23:30 +0200

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

On Tue, Jun 6, 2017 at 11:25 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Jun 06, 2017 at 09:56:47PM -0700, Andy Lutomirski wrote:
>> On Tue, Jun 6, 2017 at 5:22 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > On Tue, Jun 06, 2017 at 06:11:51PM -0400, Theodore Ts'o wrote:
>> >> On Tue, Jun 06, 2017 at 06:47:34PM +0200, Luis R. Rodriguez wrote:
>> >> > On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote:
>> >
>> > We rely on swait, and swait right now only uses -ERESTARTSYS. Are
>> > you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR
>> > or -ERESTARTNOHAND if we see fit for some future functionality / need ?
>>
>> I think that has essentially nothing to do with swait.  User code does
>> some syscall.  That syscall triggers a firmware load.  The caller gets
>> a signal.  If you're going to let firmware load get interrupted, you
>> need to consider what the syscall is.
>
> I think it is way too complicated and I do not think driver writers will
> stand a chance of implementing this correctly, given that often firmware
> load might be triggered indirectly and by multitude of syscalls.
>

That's what I meant, but I said it unclearly.  I meant that, if we're
going to start allowing interruption, we would need to audit all the
callers.  Ugh.

I suppose we could have request_firmware_interruptable(), but that
seems like it's barely worth it.

--Andy

[toc] | [next] | [standalone]

#1661853

On Thu, Jun 8, 2017 at 6:14 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Jun 6, 2017 at 11:25 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Tue, Jun 06, 2017 at 09:56:47PM -0700, Andy Lutomirski wrote:
>>> On Tue, Jun 6, 2017 at 5:22 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> > On Tue, Jun 06, 2017 at 06:11:51PM -0400, Theodore Ts'o wrote:
>>> >> On Tue, Jun 06, 2017 at 06:47:34PM +0200, Luis R. Rodriguez wrote:
>>> >> > On Tue, Jun 06, 2017 at 03:53:16PM +0100, Alan Cox wrote:
>>> >
>>> > We rely on swait, and swait right now only uses -ERESTARTSYS. Are
>>> > you saying we could mask out -ERESTARTSYS and map it to -ERESTARTNOINTR
>>> > or -ERESTARTNOHAND if we see fit for some future functionality / need ?
>>>
>>> I think that has essentially nothing to do with swait.  User code does
>>> some syscall.  That syscall triggers a firmware load.  The caller gets
>>> a signal.  If you're going to let firmware load get interrupted, you
>>> need to consider what the syscall is.
>>
>> I think it is way too complicated and I do not think driver writers will
>> stand a chance of implementing this correctly, given that often firmware
>> load might be triggered indirectly and by multitude of syscalls.
>>
>
> That's what I meant, but I said it unclearly.  I meant that, if we're
> going to start allowing interruption, we would need to audit all the
> callers.  Ugh.

There are actually two audits worth evaluating if what we've concluded
is fair game:

  a) firmware sync calls on interruptible paths
  b) use of swait / old interruptible waits on sysfs paths

As for a) we drove tons of code away from using sync, request
firmware, and on async firmware the return value is lost, we only can
currently know if a failure of some sort occurred. If that push to
async failed we also scared away tons of drivers to use
request_firmware() calls on init and later incorrectly on probe due to
serialized init + probe delay on boot. From what I recall my last
Coccinelle evil-monster-hunt, I did indeed find quite a bit of drivers
still still relying on sync firmware which ultimately revealed use on
a probe path. The signal however was only effective as of commit
0cb64249ca500 ("firmware_loader: abort request if wait_for_completion
is interrupted") merged as of v4.0. Creating awareness of the issue
seems fair, but I don't think its worth a huge fly swatter.

I have no information to contribute for b) other than I was reluctant
to even consider it.

> I suppose we could have request_firmware_interruptable(), but that
> seems like it's barely worth it.

I don't think that's worth it, given the signal was effective only as
of v4.0, we already had a big push away from sync requests, and also
had the "don't use request firmware" on init scare which also
propagated to probe later.

  Luis

[toc] | [prev] | [next] | [standalone]

#1662759

On Thu, Jun 8, 2017 at 6:33 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Jun 8, 2017 at 6:14 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> That's what I meant, but I said it unclearly.  I meant that, if we're
>> going to start allowing interruption, we would need to audit all the
>> callers.  Ugh.
>
> There are actually two audits worth evaluating if what we've concluded
> is fair game:
>
>   a) firmware sync calls on interruptible paths
>   b) use of swait / old interruptible waits on sysfs paths

And as I noted in the other thread -- another possible issue could be
any swait / interruptable wait on init or probe. Provided any child
completes and the kernel code for wait handler does abort, that
request would be terminated. This could for instance happen at bootup
as modules load and any child from the loader terminates.

We already have Coccinelle grammar to hunt for "though shall not
request firmware on init or probe", such SmPL grammar could be in turn
be repruposed to hunt for these types of conditions.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web