Re: [RFT v4] tty/sysrq: Make sysrq handler NMI aware

[Sumit Garg]

Hi Daniel,

On Mon, 7 Mar 2022 at 19:53, Daniel Thompson <daniel.thompson@xxxxxxxxxx> wrote:
>
> On Mon, Mar 07, 2022 at 04:33:28PM +0530, Sumit Garg wrote:
> > Allow a magic sysrq to be triggered from an NMI context. This is done
> > via marking some sysrq actions as NMI safe. Safe actions will be allowed
> > to run from NMI context whilst that cannot run from an NMI will be queued
> > as irq_work for later processing.
> >
> > <snip>
> >
> > @@ -566,12 +573,46 @@ static void __sysrq_put_key_op(int key, const struct sysrq_key_op *op_p)
> >               sysrq_key_table[i] = op_p;
> >  }
> >
> > +static atomic_t sysrq_key = ATOMIC_INIT(-1);
> > +
> > +static void sysrq_do_irq_work(struct irq_work *work)
> > +{
> > +     const struct sysrq_key_op *op_p;
> > +     int orig_suppress_printk;
> > +     int key = atomic_read(&sysrq_key);
> > +
> > +     orig_suppress_printk = suppress_printk;
> > +     suppress_printk = 0;
> > +
> > +     rcu_sysrq_start();
> > +     rcu_read_lock();
> > +
> > +     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;
> > +     atomic_set(&sysrq_key, -1);
> > +}
> > +
> > +static DEFINE_IRQ_WORK(sysrq_irq_work, sysrq_do_irq_work);
> > +
> >  void __handle_sysrq(int key, bool check_mask)
> >  {
> >       const struct sysrq_key_op *op_p;
> >       int orig_log_level;
> >       int orig_suppress_printk;
> >       int i;
> > +     bool irq_work = false;
> > +
> > +     /* Skip sysrq handling if one already in progress */
> > +     if (atomic_cmpxchg(&sysrq_key, -1, key) != -1) {
> > +             pr_warn("Skip sysrq key: %i as one already in progress\n", key);
> > +             return;
> > +     }
>
> Doesn't this logic needlessly jam sysrq handling if the irq_work cannot
> be undertaken?
>

Here this is done purposefully to ensure synchronisation of three
contexts while handling sysrq:
1. Thread context
2. IRQ context
3. NMI context

> A console user could unwittingly attempt an !nmi_safe SysRq action on
> a damaged system that cannot service interrupts. Logic that prevents
> things like backtrace, ftrace dump, kgdb or reboot is actively harmful
> to that user's capability to figure out why their original sysrq doesn't
> work.

I see your point.

>
> I think the logic to prohibht multiple deferred sysrqs should only
> be present on code paths where we are actually going to defer the sysrq.
>

It's not only there to prohibit multiple deferred sysrq (as that alone
could be handled by irq_work_queue()) but rather to avoid parallelism
scenarios that Doug mentioned on prior versions.

How about the following add-on change to allow passthrough for broken
irq_work systems?

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 005c9f9e0004..0a91d3ccf862 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -608,6 +608,15 @@ void __handle_sysrq(int key, bool check_mask)
        int i;
        bool irq_work = false;

+       /*
+        * Handle a case if irq_work cannot be undertaken on a damaged
+        * system stuck in hard lockup and cannot service interrupts.
+        * In such cases we shouldn't atleast block NMI safe handlers
+        * that doesn't depend on irq_work.
+        */
+       if (irq_work_is_pending(&sysrq_irq_work))
+               atomic_set(&sysrq_key, -1);
+
        /* Skip sysrq handling if one already in progress */
        if (atomic_cmpxchg(&sysrq_key, -1, key) != -1) {
                pr_warn("Skip sysrq key: %i as one already in progress\n", key);

-Sumit

>
> Daniel.