| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201203162123.44927.rjw@sisk.pl>
Date: Fri, 16 Mar 2012 21:23:44 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc: Kay Sievers <kay@...y.org>, Greg KH <gregkh@...uxfoundation.org>,
Christian Lamparter <chunkeey@...glemail.com>,
linux-kernel@...r.kernel.org, alan@...rguk.ukuu.org.uk,
Linus Torvalds <torvalds@...ux-foundation.org>,
Linux PM mailing list <linux-pm@...r.kernel.org>,
skannan@...eaurora.org, Stephen Boyd <sboyd@...eaurora.org>
Subject: Re: [PATCH] firmware loader: don't cancel _nowait requests when helper is not yet available
On Friday, March 16, 2012, Srivatsa S. Bhat wrote:
> On 03/15/2012 04:24 AM, Rafael J. Wysocki wrote:
>
> > On Wednesday, March 14, 2012, Srivatsa S. Bhat wrote:
> >> On 03/14/2012 05:40 AM, Rafael J. Wysocki wrote:
> >>
> >>> On Wednesday, March 14, 2012, Kay Sievers wrote:
> >>>> On Tue, Mar 13, 2012 at 20:42, Rafael J. Wysocki <rjw@...k.pl> wrote:
> >>>>> On Sunday, March 11, 2012, Kay Sievers wrote:
> >>>>>> On Sat, Mar 10, 2012 at 00:36, Greg KH <gregkh@...uxfoundation.org> wrote:
> >>>>
> >>>>>>> What does uevent have to do with things here?
> >>>>>>
> >>>>>> I don't think that the firmware loader should care about the
> >>>>>> usermodehelper at all, and that stuff fiddling should just be removed
> >>>>>> from the firmware class.
> >>>>>
> >>>>> It's there to warn people that their drivers do stupid things like
> >>>>> loading frimware during system resume, which is guaranteed not to work.
> >>>>>
> >>>>> IOW, it's there very much on purpose.
> >>>>
> >>>> Using the /sbin/hotplug is no case that needs any warning. It' such a
> >>>> broken model these days, that firmware loading is the least problem
> >>>> that occurs with it.
> >>>>
> >>>>>> Forking /sbin/hotplug is disabled by default, it is a broken concept,
> >>>>>> and it cannot work reliably on today's systems.
> >>>>>>
> >>>>>> Firmware is not loaded by /sbin/hotplug since many years, but by udev
> >>>>>> or whatever service handles uevents, like ueventd on android.
> >>>>>
> >>>>> Which I'm not sure why is relevant here.
> >>>>
> >>>> It is relevant in the sense that the firmware loader should not even
> >>>> know that a uevent *can* cause a usermodehelper exec() if it runs in
> >>>> legacy mode. The firmware loader just has no business in fiddling with
> >>>> the details of driver core legacy stuff. I don't think his warning
> >>>> makes much sense.
> >>>
> >>> But that warning actually triggers for drivers that attempt to use
> >>> request_firmware() during system resume, even though /sbin/hotplug isn't
> >>> used any more.
> >>>
> >>
> >>
> >> I agree with Rafael about why the warning and the bail out is required,
> >> including the part about the races with freezer which he explained in his
> >> other mail. These problems have already been well documented too.
> >> (See Documentation/power/freezing-of-tasks.txt).
> >>
> >>> usermodehelper_is_disabled() means "we are in the middle of system power
> >>> transition" rather than anything else (I agree it should be called
> >>> suspend_in_progress() or something similar these days).
> >>>
> >>
> >>
> >> How about this patch then?
> >>
> >> ---
> >>
> >> From: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> >> Subject: PM/firmware loader: Use better name for usermodehelper_is_disabled()
> >>
> >> Rafael J. Wysocki <rjw@...k.pl> wrote:
> >>
> >> | usermodehelper_is_disabled() means "we are in the middle of system power
> >> | transition" rather than anything else (I agree it should be called
> >> | suspend_in_progress() or something similar these days).
> >>
> >>
> >> But simply renaming usermodehelper_is_disabled() to suspend_in_progress()
> >> isn't the best thing to do since that would be misleading because suspend
> >> transitions are begun much before usermodehelpers are disabled.
> >>
> >> Apart from that, we don't want people to suddenly start abusing this function
> >> in future in a totally different context to check if suspend is in progress.
> >>
> >> So, add an alias specific to firmware loaders alone, that will internally
> >> call usermodehelpers_is_disabled().
> >>
> >> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> >> ---
> >>
> >> drivers/base/firmware_class.c | 12 +++++++++++-
> >> 1 files changed, 11 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> >> index 6c9387d..9e401e1 100644
> >> --- a/drivers/base/firmware_class.c
> >> +++ b/drivers/base/firmware_class.c
> >> @@ -510,6 +510,8 @@ static void fw_destroy_instance(struct firmware_priv *fw_priv)
> >> device_unregister(f_dev);
> >> }
> >>
> >> +#define suspend_in_progress() usermodehelper_is_disabled()
> >
> > This looks like an overstretch to me. I think a comment would be sufficient.
>
>
> On second thoughts... I agree, a comment is good enough.
>
> >
> >> +
> >> static int _request_firmware(const struct firmware **firmware_p,
> >> const char *name, struct device *device,
> >> bool uevent, bool nowait)
> >> @@ -535,7 +537,15 @@ static int _request_firmware(const struct firmware **firmware_p,
> >>
> >> read_lock_usermodehelper();
> >>
> >> - if (WARN_ON(usermodehelper_is_disabled())) {
> >> + /*
> >> + * It is wrong to request firmware when the system is suspended,
> >> + * because it simply won't work reliably.
> >
> > In fact, it won't work at all.
> >
> >> + Also, it can cause races with
> >> + * the freezer, leading to freezing failures.
> >
> > It actually is worse than that too. It may cause a user space process
> > to run when we think we have frozen user space and _that_ may lead to
> > all kinds of interesting breakage.
> >
>
>
> Oh, yes! That would be really dreadful!
>
> >> * So check if the system is
> >> + * in a state which is unsuitable for requesting firmware (because the
> >> + * system is suspended or not yet fully resumed) and bail out early if
> >> + * needed.
> >
> > And here I'd explain why usermodehelper_is_disabled() is used for that.
> >
>
>
> OK
>
>
> >> + */
> >> + if (WARN_ON(suspend_in_progress())) {
> >> dev_err(device, "firmware: %s will not be loaded\n", name);
> >> retval = -EBUSY;
> >> goto out;
> >
>
>
> So here is the updated patch:
> (I know its a bit verbose, but given that it is causing a considerable amount of
> confusion, may be a proper comment with good explanation is worthwhile).
>
> ---
>
> From: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> Subject: PM/firmware loader: Explain why usermodehelper_is_disabled() check is used
>
>
> Rafael J. Wysocki <rjw@...k.pl> wrote:
>
> | usermodehelper_is_disabled() means "we are in the middle of system power
> | transition" rather than anything else (I agree it should be called
> | suspend_in_progress() or something similar these days).
>
> But instead of renaming usermodehelper_is_disabled(), add a comment
> explaining its importance and also why the warning and bail out at
> _request_firmware() makes sense.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
That's fine by me.
If no one objects, I'll apply it.
Thanks,
Rafael
> ---
>
> drivers/base/firmware_class.c | 16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 6c9387d..9199e3e 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -535,6 +535,22 @@ static int _request_firmware(const struct firmware **firmware_p,
>
> read_lock_usermodehelper();
>
> + /*
> + * It is wrong to request firmware when the system is suspended,
> + * because it simply won't work. Also, it can cause races with
> + * the freezer, leading to freezing failures. Worse than that,
> + * it may even cause a user space process to run when we think
> + * we have frozen the user space! - and that can lead to all kinds
> + * of interesting breakage..
> + *
> + * So check if the system is in a state which is unsuitable for
> + * requesting firmware (because it is suspended or not yet fully
> + * resumed) and bail out early if needed.
> + * Usermodehelpers are disabled at the beginning of suspend, before
> + * freezing tasks and re-enabled only towards the end of resume, after
> + * thawing tasks, when it is safe. So all we need to do here is ensure
> + * that we don't request firmware when usermodehelpers are disabled.
> + */
> if (WARN_ON(usermodehelper_is_disabled())) {
> dev_err(device, "firmware: %s will not be loaded\n", name);
> retval = -EBUSY;
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists