Re: [PATCH] printk/nmi: avoid direct printk()-s from __printk_nmi_flush()

[Petr Mladek]

On Tue 2016-08-30 16:58:34, Sergey Senozhatsky wrote:
> Petr,
> one more question. Not related to the patch, but still related to NMI.
> 
> can NMI nest?

AFAIK, they cannot. NMIs should be disabled until iret is called.
Therefore we should be on the safe side if iret is not called
inside the NMI handler. But this should not happen because
it would cause other problems, like using wrong return address.

Well, x86 nmi code has some hacks to handle exceptions inside
NMI handlers that use iret. But printk_nmi_enter()/printk_nmi_exit()
are never nested there. It is prevented by the nmi_state per-CPU
variable. See do_nmi() in arch/x86/kernel/nmi.c.


> shouldn't we do something like this then? /* not tested */
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> 
> ---
>  kernel/printk/internal.h | 2 ++
>  kernel/printk/nmi.c      | 9 ++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 7fd2838..5b7508f 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -31,6 +31,8 @@ extern raw_spinlock_t logbuf_lock;
>   * via per-CPU variable.
>   */
>  DECLARE_PER_CPU(printk_func_t, printk_func);
> +DECLARE_PER_CPU(printk_func_t, printk_func_saved);
> +
>  static inline __printf(1, 0) int vprintk_func(const char *fmt, va_list args)
>  {
>  	return this_cpu_read(printk_func)(fmt, args);
> diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> index 16bab47..9d83929 100644
> --- a/kernel/printk/nmi.c
> +++ b/kernel/printk/nmi.c
> @@ -39,6 +39,7 @@
>   * were handled or when IRQs are blocked.
>   */
>  DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default;
> +DEFINE_PER_CPU(printk_func_t, printk_func_saved);
>  static int printk_nmi_irq_ready;
>  atomic_t nmi_message_lost;
>  
> @@ -259,10 +260,16 @@ void __init printk_nmi_init(void)
>  
>  void printk_nmi_enter(void)
>  {
> +	printk_func_t func = this_cpu_read(printk_func);
> +
> +	if (func != vprintk_nmi)
> +		this_cpu_write(printk_func_saved, func);
>  	this_cpu_write(printk_func, vprintk_nmi);
>  }
>  
>  void printk_nmi_exit(void)
>  {
> -	this_cpu_write(printk_func, vprintk_default);
> +	printk_func_t func = this_cpu_read(printk_func_saved);
> +
> +	this_cpu_write(printk_func, func);

This would handle only one level of nesting. If nesting was possible
we would probably need something else. Fortunately, I believe that we
do not need this.

Thanks for checking the code.

Best Regards,
Petr