linux-kernel - Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFxU_RqskECWOCSY-3jCU-aYxD_hO08HJvhZ3OC+22_jJw@mail.gmail.com>
Date:   Wed, 13 Sep 2017 12:30:44 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Marcel Holtmann <marcel@...tmann.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        Gabriel C <nix.or.die@...il.com>,
        Arend Van Spriel <arend.vanspriel@...adcom.com>
Subject: Re: [PATCH 4.13 20/27] Revert "firmware: add sanity check on shutdown/suspend"

On Wed, Sep 13, 2017 at 11:38 AM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
>> Commit 06a45a93e7d34a seems to be a cleanup. The arguments in
>> 06a45a93e7d3 ("firmware: move umh try locks into the umh code") seem
>> valid, and there's no real reason to worry about that FW_OPT_NOWAIT
>> etc for the direct-from-filesystem loading. That's simply not
>> sensible.
>
> Indeed! That stupid UMH lock *seems* wrong on the direct filesystem path!
>
> Hence these changes.
>
> The devil is in the details though. That UMH lock however carried an implicit
> suspend guard, the "cleanup" actually then has a functional change. The commit
> which was reverted provided the safe guard in generic form, in case we already
> had become dependent on the suspend guard. This UMH lock on the direct FS path
> then added an implicit "arbitrary rule", as you put it, on the firmware API.

I still refuse to revert a commit "just because".

It improved the code.

There is no actual sign that it causes problems.

Yes, moving the lock may change behavior, but nothing you say actually
makes me think it regressed anything.

I refuse to do this "let's just go back to what we used to have,
without an actual _reason_ to go back to it".

The old user-mode-helper crap was crap that had its own totally
separate issues. Even without the problems that we had with incredibly
bad maintainership of the user mode side, the usermode helper had
serious issues with just the fact that user mode itself is disabled by
the suspend/resume process.

So getting rid of the locking that was added for usermode helper ion
the direct file loading path makes a lot of sense.

Just saying that "we tried to re-introduce that locking in another
form, and that was a mistake, and got reverted" is *not* a reason to
then revert the movement.

Yes, the movement of the locking might need to be reverted too - but
only if it actually shows problems.

We should *not* go back to old code "just because".

            Linus