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.
