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

Date:   Mon, 28 Feb 2022 13:07:30 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Sumit Garg <sumit.garg@...aro.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

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?


> +#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.

> +{
> +	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?