Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Mar 2022 14:23:56 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     linux-serial@vger.kernel.org, hasegawa-hitomi@fujitsu.com,
        dianders@chromium.org, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, jason.wessel@windriver.com,
        linux-kernel@vger.kernel.org, kgdb-bugreport@lists.sourceforge.net,
        arnd@arndb.de, peterz@infradead.org
Subject: Re: [RFT v4] tty/sysrq: Make sysrq handler NMI aware
Message-ID: <20220307142356.ksx7k5xalqlsxnqk@maple.lan>
References: <20220307110328.2557655-1-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220307110328.2557655-1-sumit.garg@linaro.org>
Precedence: bulk

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?

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 think the logic to prohibht multiple deferred sysrqs should only
be present on code paths where we are actually going to defer the sysrq.


Daniel.