netdev - Re: [PATCH 13/30] s390/consoles: Improve panic notifiers reliability

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: <YmwyjMtT7QTZiHaa@osiris>
Date:   Fri, 29 Apr 2022 20:46:36 +0200
From:   Heiko Carstens <hca@...ux.ibm.com>
To:     "Guilherme G. Piccoli" <gpiccoli@...lia.com>
Cc:     akpm@...ux-foundation.org, bhe@...hat.com, pmladek@...e.com,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com, coresight@...ts.linaro.org,
        linuxppc-dev@...ts.ozlabs.org, linux-alpha@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-edac@...r.kernel.org,
        linux-hyperv@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-mips@...r.kernel.org, linux-parisc@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
        linux-s390@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-um@...ts.infradead.org, linux-xtensa@...ux-xtensa.org,
        netdev@...r.kernel.org, openipmi-developer@...ts.sourceforge.net,
        rcu@...r.kernel.org, sparclinux@...r.kernel.org,
        xen-devel@...ts.xenproject.org, x86@...nel.org,
        kernel-dev@...lia.com, kernel@...ccoli.net, halves@...onical.com,
        fabiomirmar@...il.com, alejandro.j.jimenez@...cle.com,
        andriy.shevchenko@...ux.intel.com, arnd@...db.de, bp@...en8.de,
        corbet@....net, d.hatayama@...fujitsu.com,
        dave.hansen@...ux.intel.com, dyoung@...hat.com,
        feng.tang@...el.com, gregkh@...uxfoundation.org,
        mikelley@...rosoft.com, hidehiro.kawai.ez@...achi.com,
        jgross@...e.com, john.ogness@...utronix.de, keescook@...omium.org,
        luto@...nel.org, mhiramat@...nel.org, mingo@...hat.com,
        paulmck@...nel.org, peterz@...radead.org, rostedt@...dmis.org,
        senozhatsky@...omium.org, stern@...land.harvard.edu,
        tglx@...utronix.de, vgoyal@...hat.com, vkuznets@...hat.com,
        will@...nel.org, Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>
Subject: Re: [PATCH 13/30] s390/consoles: Improve panic notifiers reliability

On Wed, Apr 27, 2022 at 07:49:07PM -0300, Guilherme G. Piccoli wrote:
> Currently many console drivers for s390 rely on panic/reboot notifiers
> to invoke callbacks on these events. The panic() function disables local
> IRQs, secondary CPUs and preemption, so callbacks invoked on panic are
> effectively running in atomic context.
> 
> Happens that most of these console callbacks from s390 doesn't take the
> proper care with regards to atomic context, like taking spinlocks that
> might be taken in other function/CPU and hence will cause a lockup
> situation.
> 
> The goal for this patch is to improve the notifiers reliability, acting
> on 4 console drivers, as detailed below:
> 
> (1) con3215: changed a regular spinlock to the trylock alternative.
> 
> (2) con3270: also changed a regular spinlock to its trylock counterpart,
> but here we also have another problem: raw3270_activate_view() takes a
> different spinlock. So, we worked a helper to validate if this other lock
> is safe to acquire, and if so, raw3270_activate_view() should be safe.
> 
> Notice though that there is a functional change here: it's now possible
> to continue the notifier code [reaching con3270_wait_write() and
> con3270_rebuild_update()] without executing raw3270_activate_view().
> 
> (3) sclp: a global lock is used heavily in the functions called from
> the notifier, so we added a check here - if the lock is taken already,
> we just bail-out, preventing the lockup.
> 
> (4) sclp_vt220: same as (3), a lock validation was added to prevent the
> potential lockup problem.
> 
> Besides (1)-(4), we also removed useless void functions, adding the
> code called from the notifier inside its own body, and changed the
> priority of such notifiers to execute late, since they are "heavyweight"
> for the panic environment, so we aim to reduce risks here.
> Changed return values to NOTIFY_DONE as well, the standard one.
> 
> Cc: Alexander Gordeev <agordeev@...ux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@...ux.ibm.com>
> Cc: Heiko Carstens <hca@...ux.ibm.com>
> Cc: Sven Schnelle <svens@...ux.ibm.com>
> Cc: Vasily Gorbik <gor@...ux.ibm.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@...lia.com>
> ---
> 
> As a design choice, the option used here to verify a given spinlock is taken
> was the function "spin_is_locked()" - but we noticed that it is not often used.
> An alternative would to take the lock with a spin_trylock() and if it succeeds,
> just release the spinlock and continue the code. But that seemed weird...
> 
> Also, we'd like to ask a good validation of case (2) potential functionality
> change from the s390 console experts - far from expert here, and in our naive
> code observation, that seems fine, but that analysis might be missing some
> corner case.
> 
> Thanks in advance!
> 
>  drivers/s390/char/con3215.c    | 36 +++++++++++++++--------------
>  drivers/s390/char/con3270.c    | 34 +++++++++++++++------------
>  drivers/s390/char/raw3270.c    | 18 +++++++++++++++
>  drivers/s390/char/raw3270.h    |  1 +
>  drivers/s390/char/sclp_con.c   | 28 +++++++++++++----------
>  drivers/s390/char/sclp_vt220.c | 42 +++++++++++++++++++---------------
>  6 files changed, 96 insertions(+), 63 deletions(-)

Code looks good, and everything still seems to work. I applied this
internally for the time being, and if it passes testing, I'll schedule
it for the next merge window.

Thanks!