On Sat, Sep 24, 2016 at 10:41:46AM -0700, Dmitry Torokhov wrote:
> On Fri, Sep 23, 2016 at 6:37 PM, Herbert, Marc <[email protected]> wrote:
> > On 03/09/2016 11:10, Dmitry Torokhov wrote:
> >> I was thinking if we kernel could post
> >> "conditions" (maybe simple stings) that it waits for, and userspace
> >> could unlock these "conditions". One of them might be "firmware
> >> available".
> >
> > On idea offered by Josh Triplett that seems to overlap with this one
> > is to have something similar to the (deprecated) userhelper with
> > *per-blob* requests and notifications except for one major difference:
> > userspace would not anymore be in charge of *providing* the blob but
> > would instead only *signal* when a given blob becomes available and is
> > either found or found missing. Then the kernel loads the blob _by
> > itself_; unlike the userhelper. No new “critical filesystem” concept
> > and a *per-blob basis*, allowing any variation of blob locations
> > across any number of initramfs and filesystems.
> >
>
> Really, I do not quite understand why people have issues with usermode
> helper/uevents.
One reason is you'd have to implement your own cache for suspend/resume.
> It used to work reasonably well (if you were using
> request_firmware_nowait()), as the kernel would post the request and
> then, when userspace was ready[^Hier], uevents would be processed and
> firmware would be loaded. We had a timeout of 60(?) seconds by
> default, but that would be adjusted as systems needed.
The issue with the timeout was kernel developers *assumed* module init
and probe were detached, and saying 'thou shall not load firmware on
probe' seems actually like a more radical change than just saying
'thou shall load firmware on init'. I'll note that as it stands
its the right thing to complain about these users only because we
lack the semantics to ensure correctness if used on init or probe.
The timeout incurred huge latencies for optional firmwares, and
while we had a new API added to avoid the wait on optional firmware,
that obviously still leaved the races as possible. We now have async
probe which *does* enable some original misconceptions by kernel
developers, but by now other issues have also been found on the
usermode helper, the cache was one, another one was a recent discusion
over the user of the UMH lock with the assumption this was providing
a sort of safeguard on early boot use -- it does not, for the same
exact reasons why a UMH lock does not suffice to avoid all possible
rootfs races. For this later issue refer to a recent discussion in
review with Daniel Wagner's patches.
> Unfortunately it all broke when udev started insisting [1] on
> servicing some uevents in strict sequence, which resulted in boot
> stalls.
That was not the only issue... another implicit issue was that
you are reducing the number of possible supported number of
devices Linux supports per module by the timeout, it would
depend on the combine time it takes to both init and probe.
Some drivers are super complex and even if you *don't* have
firmware requirements and say burn the firmware onto a device
we found that *probe* alone was taking a long long time on some
device drivers -- check out cxgb4 driver, where one device actually
ends up loading about 4 subdevices underneath it. Yes that's a mess
and the driver needs a major rewrite to address this in a clean way
but that takes time. Its no trivial pursuit. The umh timeout then
would not be implicated anymore *but* since systemd implemented the
timeout in general for kmod loading it did mean system was limiting
them Linux drivers and how much devices a driver can support
depending on this timeout value. At SUSE we solved this by lifting
this timeout for kmod workers for now. A long term goal here, which
could help, is also to just detach init and probes, so we give to
system what it originally thought. Summary of this all is here:
http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html
I have some code that starts to enable some of this on systemd/kmod
but it still needs some more testing before I post.
> Maybe the ultimate answer is to write a firmware loading
> daemon that would also listen to netlink events and do properly what
> udev refused to be doing?
Meh, in the wireless subsystem we devised our own file loader,
check CRDA. That worked for us since we needed to optionally
enable digital RSA signed file checking, but long term our
experience is that this is pointless. So we're going to phase
that out in favor of using the firmware API for the file loading
of this file, and support then digital signatures on the firmware.
I am not sure how/why a firmware loading daemon would be a better
idea now. What Marc describes that Josh proposed with signals for
userspcae seems more aligned with what we likely need -- but note
that since we now use a shared common API for kernel reads from a
path via kernel_read_file_from_path() we'd probably want something
like a notifier for any kernel_read_file_from_path() user. The ability
for the kernel to register a generic userspace notifier seems worthy
of consideration, but I'd be surprised if we don't already have
something quite like this already?
> The distribution would know when it is ready
> to service firmware requests (and thus when to start this daemon), and
> we would have the freedom of having drivers both built-in and as
> modules and bulding firmware into kernel, intiramfs or keep on a
> "real" fs available at later time.
What difference would there be if we just used notifications to
guarantee to the kernel the file in question is now available?
Luis
On Tue, Oct 4, 2016 at 5:00 PM, Luis R. Rodriguez <[email protected]> wrote:
>
> I am not sure how/why a firmware loading daemon would be a better
> idea now. What Marc describes that Josh proposed with signals for
> userspcae seems more aligned with what we likely need
Quite frankly, I doubt you want a signal.
You will want to have some way to specify where the firmware files
are. Right now we have "fw_path[]" which is hardcoded except for the
first entry that can be set as a module parameter. But you'd probably
want to expand on that, which implies some /sys or /proc interface.
And once you do that, wouldn't it make more sense to just make the
"update the firmware path /proc/sys/kernel/fw_path file" make things
re-search for firmware?
In other words, the interface has to be something *sensible*. Not some
idiotic ad-hoc "send a signal" (of which that stupid original patch
was just a very odd example).
Linus
On Tue, Oct 4, 2016 at 5:12 PM, Linus Torvalds
<[email protected]> wrote:
> On Tue, Oct 4, 2016 at 5:00 PM, Luis R. Rodriguez <[email protected]> wrote:
>>
>> I am not sure how/why a firmware loading daemon would be a better
>> idea now. What Marc describes that Josh proposed with signals for
>> userspcae seems more aligned with what we likely need
>
> Quite frankly, I doubt you want a signal.
>
> You will want to have some way to specify where the firmware files
> are. Right now we have "fw_path[]" which is hardcoded except for the
> first entry that can be set as a module parameter. But you'd probably
> want to expand on that, which implies some /sys or /proc interface.
>
> And once you do that, wouldn't it make more sense to just make the
> "update the firmware path /proc/sys/kernel/fw_path file" make things
> re-search for firmware?
We can, but re-searching for firmware assumes we cache pending
firmware, we currently don't, we just either process sync or async
firmware requests.
> In other words, the interface has to be something *sensible*. Not some
> idiotic ad-hoc "send a signal" (of which that stupid original patch
> was just a very odd example).
Note that the races are beyond firmware, so all
kernel_read_file_from_path() users, as such re-using such old /sys/
interafeces for firmware will not suffice to cover all ground now for
the same race for other possible users.
Luis
On Tue, Oct 4, 2016 at 5:24 PM, Luis R. Rodriguez <[email protected]> wrote:
>
> Note that the races are beyond firmware, so all
> kernel_read_file_from_path() users, as such re-using such old /sys/
> interafeces for firmware will not suffice to cover all ground now for
> the same race for other possible users.
Blah blah blah.
The reason I've hated this whole discussion is that it's full of
"let's re-architect everything", and then it has these horribly warty
interfaces. It's classic second-system syndrome.
Just do *one* thing, and do it well. Don't change anything else. Don't
force existign drivers to use new interfaces. Don't over-architect,
and don't do stupid interfaces.
If user-space mounts a new filesystem (or just unpacks files from a
tar-file that has firmware images in it, for chissake), that is not
some magical "critical mount event". The whole concept is just stupid.
Is it a "mount event" when the user downloads a new firmware image
from the internet?
HELL NO.
But what is equally stupid is to then dismiss simple models because
some totally unrelated "beyond firmware" issue.
Anything that is "beyond firmware" shouldn't even be discussed, for
chrissake! It has nothing what-so-ever to do with firmware loading. If
there ends up being some common helper functions, and shared code,
that *still* doesn't make it so.
Basic rules of thumb:
(a) don't over-design
(b) don't have stupid illogical interfaces
(c) don't conflate different issues just because you think they may
have shared code.
(4) be consistent. Don't make up new interfaces, and most certainly
do *NOT* dismiss something just because it's what we have done before.
That's it.
Linus
On Tue, Oct 04, 2016 at 05:12:58PM -0700, Linus Torvalds wrote:
> On Tue, Oct 4, 2016 at 5:00 PM, Luis R. Rodriguez <[email protected]> wrote:
> > I am not sure how/why a firmware loading daemon would be a better
> > idea now. What Marc describes that Josh proposed with signals for
> > userspcae seems more aligned with what we likely need
>
> Quite frankly, I doubt you want a signal.
>
> You will want to have some way to specify where the firmware files
> are. Right now we have "fw_path[]" which is hardcoded except for the
> first entry that can be set as a module parameter. But you'd probably
> want to expand on that, which implies some /sys or /proc interface.
>
> And once you do that, wouldn't it make more sense to just make the
> "update the firmware path /proc/sys/kernel/fw_path file" make things
> re-search for firmware?
That could work, but it seems like overkill to allow changing the path,
rather than the simpler interface of just telling the one driver "go
ahead and direct-load your firmware now". I definitely don't think it
should be a system-wide "mount event"; it should be a per-device "go
direct-load your firmware" poke from userspace. That would solve the
"build-in the driver so it can start waking up slow monitors, but wait
to load the firmware until you have a filesystem" problem. (And it
would avoid creating some unusual driver-specific late-firmware-load
mechanism.)
That said, the Chrome OS folks apparently have some mechanism where they
mount a tmpfs over /lib/firmware to let userspace choose firmware at
runtime, so perhaps the path-changing mechanism would help there. Kees?
On Tue, Oct 4, 2016 at 6:48 PM, Josh Triplett <[email protected]> wrote:
>
> I definitely don't think it
> should be a system-wide "mount event"; it should be a per-device "go
> direct-load your firmware" poke from userspace.
I don't disagree with that kind of interface. We already have things
like "rescan" for PCI bus devices to force a bus rescan. Iit's a
simple device attribute. Having a similar thing to trigger firmware
reload for a driver sounds entirely sane.
Linus
On Tue, Oct 04, 2016 at 05:32:22PM -0700, Linus Torvalds wrote:
> On Tue, Oct 4, 2016 at 5:24 PM, Luis R. Rodriguez <[email protected]> wrote:
> >
> > Note that the races are beyond firmware, so all
> > kernel_read_file_from_path() users, as such re-using such old /sys/
> > interafeces for firmware will not suffice to cover all ground now for
> > the same race for other possible users.
>
> Blah blah blah.
>
> The reason I've hated this whole discussion is that it's full of
> "let's re-architect everything", and then it has these horribly warty
> interfaces.
To be clear, kernel_read_file_from_path() was an agreed upon strategy
about 1 year ago at the Linux Security summit as we found different
kernel implementations for the same exact task, reading files from
the filesystem -- my point here was simply that acknowledging that the
race on early init and driver's init / probe for firmware is implicating
that the race is *also* possible for the other kernel-read-from-fs points.
Its not clear to me what your grudge here is other than the proposal
for a solution in this patch is not what we want.
> It's classic second-system syndrome.
>
> Just do *one* thing, and do it well. Don't change anything else. Don't
> force existign drivers to use new interfaces. Don't over-architect,
> and don't do stupid interfaces.
If there is a race for the other users and we want to avoid wrapping
a solution for it to the other callers without doing any vetting for
correctness then so be it, but to disregard completely seems error-prone.
I accept that thinking about such other users may complicate a solution
for firmware and if you prefer we just separate the race solution for
both that's fine.
> If user-space mounts a new filesystem (or just unpacks files from a
> tar-file that has firmware images in it, for chissake), that is not
> some magical "critical mount event". The whole concept is just stupid.
> Is it a "mount event" when the user downloads a new firmware image
> from the internet?
>
> HELL NO.
We've gotten passed that the original implementation proposed is not what we
want, let's move on.
> But what is equally stupid is to then dismiss simple models because
> some totally unrelated "beyond firmware" issue.
I have not heard back from the other stakeholders using
kernel_read_file_from_path() and possible races for them. You seem to suggest
to ignore those possible theoretical races in the name of a simple solution for
firmware. Fine.
> Anything that is "beyond firmware" shouldn't even be discussed, for
> chrissake! It has nothing what-so-ever to do with firmware loading. If
> there ends up being some common helper functions, and shared code,
> that *still* doesn't make it so.
My point was to raise the flag of the possible races on the other call sites
where we read files directly from the kernel, that's all, if we agree we really
don't care for that fine.
> Basic rules of thumb:
>
> (a) don't over-design
>
> (b) don't have stupid illogical interfaces
>
> (c) don't conflate different issues just because you think they may
> have shared code.
>
> (4) be consistent. Don't make up new interfaces, and most certainly
> do *NOT* dismiss something just because it's what we have done before.
>
> That's it.
OK..
Luis