linux-kernel - Re: [PATCH V3] kernel, add bug_on

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:	Thu, 23 Oct 2014 12:40:34 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Prarit Bhargava <prarit@...hat.com>
Cc:	linux-kernel@...r.kernel.org, Jonathan Corbet <corbet@....net>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"H. Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Fabian Frederick <fabf@...net.be>, vgoyal@...hat.com,
	isimatu.yasuaki@...fujitsu.com, linux-doc@...r.kernel.org,
	kexec@...ts.infradead.org, linux-api@...r.kernel.org
Subject: Re: [PATCH V3] kernel, add bug_on_warn

On Thu, 23 Oct 2014 08:53:14 -0400 Prarit Bhargava <prarit@...hat.com> wrote:

> There have been several times where I have had to rebuild a kernel to
> cause a panic when hitting a WARN() in the code in order to get a crash
> dump from a system.  Sometimes this is easy to do, other times (such as
> in the case of a remote admin) it is not trivial to send new images to the
> user.
> 
> A much easier method would be a switch to change the WARN() over to a
> BUG().  This makes debugging easier in that I can now test the actual
> image the WARN() was seen on and I do not have to engage in remote
> debugging.
> 
> This patch adds a bug_on_warn kernel parameter, which calls BUG() in the
> warn_slowpath_common() path.  The function will still print out the
> location of the warning.

The changelog doesn't mention /proc/sys/kernel/bug_on_warn?

Why do we need both the sysctl and the kernel parameter?  Only to
trigger BUG for warnings which occur prior to initscripts.  Is there a
legitimate case for this?  Is kdump even usable at this time?

> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -75,10 +75,18 @@ extern void warn_slowpath_null(const char *file, const int line);
>  #define __WARN_printf_taint(taint, arg...)				\
>  	warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
>  #else
> -#define __WARN()		__WARN_TAINT(TAINT_WARN)
> +#define check_bug_on_warn()						\
> +	do {								\
> +		if (bug_on_warn)					\
> +			BUG();						\
> +	} while (0)

#define check_bug_on_warn() BUG_ON(bug_on_warn)

would suffice?

> +#define __WARN()							\
> +	do { __WARN_TAINT(TAINT_WARN); check_bug_on_warn(); } while (0)
> +
>  #define __WARN_printf(arg...)	do { printk(arg); __WARN(); } while (0)
>  #define __WARN_printf_taint(taint, arg...)				\
> -	do { printk(arg); __WARN_TAINT(taint); } while (0)
> +	do { printk(arg); __WARN_TAINT(taint); check_bug_on_warn(); } while (0)
>  #endif

What's this code here for anyway?  The changes to
warn_slowpath_common() aren't sufficient?

>  #ifndef WARN_ON
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 40728cf..4094a60 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -422,6 +422,7 @@ extern int panic_on_oops;
>  extern int panic_on_unrecovered_nmi;
>  extern int panic_on_io_nmi;
>  extern int sysctl_panic_on_stackoverflow;
> +extern int bug_on_warn;
>  /*
>   * Only to be used by arch init code. If the user over-wrote the default
>   * CONFIG_PANIC_TIMEOUT, honor it.
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 43aaba1..2ba0a58 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -153,6 +153,7 @@ enum
>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
> +	KERN_BUG_ON_WARN=77, /* int: call BUG() in WARN() functions */
>  };
>  
>  
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d09dc5c..a6d2e2f 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -33,6 +33,7 @@ static int pause_on_oops;
>  static int pause_on_oops_flag;
>  static DEFINE_SPINLOCK(pause_on_oops_lock);
>  static bool crash_kexec_post_notifiers;
> +int bug_on_warn;

I suppose this should be __read_mostly.  Assuming __read_mostly is
useful :(

>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/