linux-kernel - Re: [PATCH V3] firmware: google: Test spinlock on panic path to avoid lockups

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: <YxDX9+p+58q2sip2@kroah.com>
Date:   Thu, 1 Sep 2022 18:04:07 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Guilherme G. Piccoli" <gpiccoli@...lia.com>
Cc:     evgreen@...omium.org, arnd@...db.de, linux-efi@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...ccoli.net, ardb@...nel.org,
        davidgow@...gle.com, jwerner@...omium.org,
        Petr Mladek <pmladek@...e.com>
Subject: Re: [PATCH V3] firmware: google: Test spinlock on panic path to
 avoid lockups

On Thu, Sep 01, 2022 at 12:59:36PM -0300, Guilherme G. Piccoli wrote:
> + Petr, since this was extensively discussed in the original thread [0]
> and maybe he can help with the argument.
> 
> [0]
> https://lore.kernel.org/lkml/20220427224924.592546-1-gpiccoli@igalia.com/
> 
> 
> On 01/09/2022 12:52, Greg KH wrote:
> > [...]
> 
> >> +	 * Perform the lock check before effectively trying
> >> +	 * to acquire it on gsmi_shutdown_reason() to avoid
> >> +	 * potential lockups in atomic context.
> >> +	 */
> >> +	if (spin_is_locked(&gsmi_dev.lock))
> >> +		return NOTIFY_DONE;
> >> +
> > 
> > What happens if the lock is grabbed right after testing for it?
> > Shouldn't you use lockdep_assert_held() instead as the documentation
> > says to?
> 
> How, if in this point only a single CPU (this one, executing the code)
> is running?

How are we supposed to know this here?

> Remember this is the panic path - before this point we disabled all
> other CPUs, except this one executing the code. So, either the lock was
> taken (and we bail), or it wasn't and it's safe to continue.

Then who else could have taken the lock?  And if all other CPUs are
stopped, who cares about the lock at all?  Just don't grab it (you
should check for that when you want to grab it) and then you can work
properly at that point in time.

thanks,

greg k-h