linux-kernel - Re: [RFT v2] tty/sysrq: Make sysrq handler NMI aware

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] [day] [month] [year] [list]

Message-ID: <CAFA6WYN1XOxuVZscd1=oovE7Cf8UZxySYq4Lp=QSsFDndYSNUA@mail.gmail.com>
Date:   Mon, 28 Feb 2022 18:58:03 +0530
From:   Sumit Garg <sumit.garg@...aro.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-serial@...r.kernel.org, hasegawa-hitomi@...itsu.com,
        gregkh@...uxfoundation.org, jirislaby@...nel.org,
        jason.wessel@...driver.com, daniel.thompson@...aro.org,
        dianders@...omium.org, linux-kernel@...r.kernel.org,
        kgdb-bugreport@...ts.sourceforge.net, arnd@...db.de
Subject: Re: [RFT v2] tty/sysrq: Make sysrq handler NMI aware

Hi Peter,

Thanks for your review.

On Mon, 28 Feb 2022 at 17:37, Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Mon, Feb 28, 2022 at 01:23:51PM +0530, Sumit Garg wrote:
> > Allow a magic sysrq to be triggered from an NMI context. This is done
>
> *why* though?
>

I should have copied the reasoning from v1 cover letter [1] to this
single patch as well. Will do it in v3. The basic idea is to enhance
kernel's NMI debuggability for CPUs stuck in hard lockups. As an
example, one should be able to launch kdb as well as other diagnostics
offered by magic sysrq in NMI context.

[1] https://lore.kernel.org/linux-arm-kernel/1595333413-30052-1-git-send-email-sumit.garg@linaro.org/

>
> > +#define SYSRQ_NMI_FIFO_SIZE  2
> > +static DEFINE_KFIFO(sysrq_nmi_fifo, int, SYSRQ_NMI_FIFO_SIZE);
> > +
> > +static void sysrq_do_nmi_work(struct irq_work *work)
>
> That naming don't make sense, it does the !NMI work, from IRQ context.
>

Will rename it to sysrq_do_irq_work().

> > +{
> > +     const struct sysrq_key_op *op_p;
> > +     int orig_suppress_printk;
> > +     int key;
> > +
> > +     orig_suppress_printk = suppress_printk;
> > +     suppress_printk = 0;
> > +
> > +     rcu_sysrq_start();
> > +     rcu_read_lock();
> > +
> > +     if (kfifo_peek(&sysrq_nmi_fifo, &key)) {
> > +             op_p = __sysrq_get_key_op(key);
> > +             if (op_p)
> > +                     op_p->handler(key);
> > +     }
> > +
> > +     rcu_read_unlock();
> > +     rcu_sysrq_end();
> > +
> > +     suppress_printk = orig_suppress_printk;
> > +
> > +     kfifo_reset_out(&sysrq_nmi_fifo);
> > +}
> > +
> > +static DEFINE_IRQ_WORK(sysrq_nmi_work, sysrq_do_nmi_work);
> > +
> >  void __handle_sysrq(int key, bool check_mask)
> >  {
> >       const struct sysrq_key_op *op_p;
> > @@ -573,6 +612,10 @@ void __handle_sysrq(int key, bool check_mask)
> >       int orig_suppress_printk;
> >       int i;
> >
> > +     /* Skip sysrq handling if one already in progress */
> > +     if (!kfifo_is_empty(&sysrq_nmi_fifo))
> > +             return;
> > +
> >       orig_suppress_printk = suppress_printk;
> >       suppress_printk = 0;
> >
> > @@ -596,7 +639,13 @@ void __handle_sysrq(int key, bool check_mask)
> >               if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> >                       pr_info("%s\n", op_p->action_msg);
> >                       console_loglevel = orig_log_level;
> > -                     op_p->handler(key);
> > +
> > +                     if (in_nmi() && !op_p->nmi_safe) {
> > +                             kfifo_put(&sysrq_nmi_fifo, key);
> > +                             irq_work_queue(&sysrq_nmi_work);
> > +                     } else {
> > +                             op_p->handler(key);
> > +                     }
> >               } else {
> >                       pr_info("This sysrq operation is disabled.\n");
> >                       console_loglevel = orig_log_level;
>
> I'm missing the point of that kfifo stuff; afaict it only ever buffers
> _1_ key, might as well use a simple variable, no?

Yeah you are right, using a single key buffer should also suffice. The
original idea was to queue multiple sysrq and handle them one by one
but that turned out to be unsafe.

-Sumit