All of lore.kernel.org
 help / color / mirror / Atom feed
From: Baoquan He <bhe@redhat.com>
To: Rafael Aquini <aquini@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	kexec@lists.infradead.org, linux-fsdevel@vger.kernel.org,
	dyoung@redhat.com, corbet@lwn.net, mcgrof@kernel.org,
	keescook@chromium.org, akpm@linux-foundation.org, cai@lca.pw,
	rdunlap@infradead.org, tytso@mit.edu, bunk@kernel.org,
	torvalds@linux-foundation.org, gregkh@linuxfoundation.org,
	labbott@redhat.com, jeffm@suse.com, jikos@kernel.org,
	jeyu@suse.de, tiwai@suse.de, AnDavis@suse.com,
	rpalethorpe@suse.de
Subject: Re: [PATCH v3] kernel: add panic_on_taint
Date: Mon, 11 May 2020 09:11:49 +0800	[thread overview]
Message-ID: <20200511011149.GH5029@MiWiFi-R3L-srv> (raw)
In-Reply-To: <20200510182202.GA31704@t490s>

On 05/10/20 at 02:22pm, Rafael Aquini wrote:
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 7bc83f3d9bdf..4a69fe49a70d 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3404,6 +3404,21 @@
> > >  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> > >  			on a WARN().
> > >  
> > > +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> > > +			Format: <str>
> > 			Changed it as 'Format: <string>' to be
> > consistent with the existing other options?
> 
> I can resubmit with the change, if it's a strong req and the surgery
> cannot be done at merge time.

Yeah, maybe maintainer can help adjust this, not sure who will pick it.
No, it's not a strong request, people might get a little bit confusion
about which format should be referred to when a new kernel option is added.

> 
> 
> > > +			Specifies, as a string, the TAINT flag set that will
> > > +			compose a bitmask for calling panic() when the kernel
> > > +			gets tainted.
> > > +			See Documentation/admin-guide/tainted-kernels.rst for
> > > +			details on the taint flags that users can pick to
> > > +			compose the bitmask to assign to panic_on_taint.
> > > +			When the string is prefixed with a '-' the bitmask
> > > +			set in panic_on_taint will be mutually exclusive
> > > +			with the sysctl knob kernel.tainted, and any attempt
> > > +			to write to that sysctl will fail with -EINVAL for
> > > +			any taint value that masks with the flags set for
> > > +			this option.
> > > +
> > >  	crash_kexec_post_notifiers
> > >  			Run kdump after running panic-notifiers and dumping
> > >  			kmsg. This only for the users who doubt kdump always
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index 9b7a8d74a9d6..66bc102cb59a 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -528,6 +528,8 @@ extern int panic_on_oops;
> > >  extern int panic_on_unrecovered_nmi;
> > >  extern int panic_on_io_nmi;
> > >  extern int panic_on_warn;
> > > +extern unsigned long panic_on_taint;
> > > +extern bool panic_on_taint_exclusive;
> > >  extern int sysctl_panic_on_rcu_stall;
> > >  extern int sysctl_panic_on_stackoverflow;
> > >  
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index b69ee9e76cb2..65c62f8a1de8 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/kexec.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/sysrq.h>
> > > +#include <linux/ctype.h>
> > >  #include <linux/init.h>
> > >  #include <linux/nmi.h>
> > >  #include <linux/console.h>
> > > @@ -44,6 +45,8 @@ static int pause_on_oops_flag;
> > >  static DEFINE_SPINLOCK(pause_on_oops_lock);
> > >  bool crash_kexec_post_notifiers;
> > >  int panic_on_warn __read_mostly;
> > > +unsigned long panic_on_taint;
> > > +bool panic_on_taint_exclusive = false;
> > >  
> > >  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> > >  EXPORT_SYMBOL_GPL(panic_timeout);
> > > @@ -434,6 +437,11 @@ void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
> > >  		pr_warn("Disabling lock debugging due to kernel taint\n");
> > >  
> > >  	set_bit(flag, &tainted_mask);
> > > +
> > > +	if (tainted_mask & panic_on_taint) {
> > > +		panic_on_taint = 0;
> > 
> > This panic_on_taint resetting is redundant? It will trigger crash, do we
> > need care if it's 0 or not?
> >
> 
> We might still get more than one CPU hitting a taint adding code path after 
> the one that tripped here called panic. To avoid multiple calls to panic, 
> in that particular scenario, we clear the panic_on_taint bitmask out. 
> Also, albeit non-frequent, we might be tracking TAINT_WARN, and still hit 
> a WARN_ON() in the panic / kdump path, thus incurring in a second 
> (and unwanted) call to panic here.  

Hmm, this cpu will set panic_cpu firstly, all other cpu need stop and
have no chance to execute panic. But yes, clearing panic_on_taint makes
code easier to understand.

> 
>  
> > > +		panic("panic_on_taint set ...");
> > > +	}
> > >  }
> > >  EXPORT_SYMBOL(add_taint);
> > >  
> > > @@ -686,3 +694,35 @@ static int __init oops_setup(char *s)
> > >  	return 0;
> > >  }
> > >  early_param("oops", oops_setup);
> > > +
> > > +static int __init panic_on_taint_setup(char *s)
> > > +{
> > > +	/* we just ignore panic_on_taint if passed without flags */
> > > +	if (!s)
> > > +		goto out;
> > > +
> > > +	for (; *s; s++) {
> > > +		int i;
> > > +
> > > +		if (*s == '-') {
> > > +			panic_on_taint_exclusive = true;
> > > +			continue;
> > > +		}
> > > +
> > > +		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
> > > +			if (toupper(*s) == taint_flags[i].c_true) {
> > > +				set_bit(i, &panic_on_taint);
> > > +				break;
> > > +			}
> > > +		}
> > 
> > Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.
> > If I specify 'panic_on_taint="G"' or 'panic_on_taint="-G"' in cmdline,
> > what is expected for this customer behaviour?
> > 
> 
> This will not panic the system as no taint flag gets actually set in 
> panic_on_taint bitmask for G.
> 
> G is the counterpart of P, and appears on print_tainted() whenever
> TAINT_PROPRIETARY_MODULE is not set. panic_on_taint doesn't set
> anything for G, as it doesn't represent any taint, but the lack
> of one particular taint, instead.
> 
> (apparently, TAINT_PROPRIETARY_MODULE is the only taint flag
> that follows that pattern of having an extra assigned letter 
> that means its absence, and perhaps it should be removed)

Yeah, agree. I will make a draft patch to remove it, see if there's
objection from people.


WARNING: multiple messages have this Message-ID
From: Baoquan He <bhe@redhat.com>
To: Rafael Aquini <aquini@redhat.com>
Cc: linux-doc@vger.kernel.org, tiwai@suse.de, jeffm@suse.com,
	corbet@lwn.net, labbott@redhat.com, dyoung@redhat.com,
	AnDavis@suse.com, rpalethorpe@suse.de, keescook@chromium.org,
	jikos@kernel.org, cai@lca.pw, bunk@kernel.org, tytso@mit.edu,
	jeyu@suse.de, gregkh@linuxfoundation.org, rdunlap@infradead.org,
	kexec@lists.infradead.org, linux-kernel@vger.kernel.org,
	mcgrof@kernel.org, linux-fsdevel@vger.kernel.org,
	akpm@linux-foundation.org, torvalds@linux-foundation.org
Subject: Re: [PATCH v3] kernel: add panic_on_taint
Date: Mon, 11 May 2020 09:11:49 +0800	[thread overview]
Message-ID: <20200511011149.GH5029@MiWiFi-R3L-srv> (raw)
In-Reply-To: <20200510182202.GA31704@t490s>

On 05/10/20 at 02:22pm, Rafael Aquini wrote:
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 7bc83f3d9bdf..4a69fe49a70d 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -3404,6 +3404,21 @@
> > >  	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> > >  			on a WARN().
> > >  
> > > +	panic_on_taint=	[KNL] conditionally panic() in add_taint()
> > > +			Format: <str>
> > 			Changed it as 'Format: <string>' to be
> > consistent with the existing other options?
> 
> I can resubmit with the change, if it's a strong req and the surgery
> cannot be done at merge time.

Yeah, maybe maintainer can help adjust this, not sure who will pick it.
No, it's not a strong request, people might get a little bit confusion
about which format should be referred to when a new kernel option is added.

> 
> 
> > > +			Specifies, as a string, the TAINT flag set that will
> > > +			compose a bitmask for calling panic() when the kernel
> > > +			gets tainted.
> > > +			See Documentation/admin-guide/tainted-kernels.rst for
> > > +			details on the taint flags that users can pick to
> > > +			compose the bitmask to assign to panic_on_taint.
> > > +			When the string is prefixed with a '-' the bitmask
> > > +			set in panic_on_taint will be mutually exclusive
> > > +			with the sysctl knob kernel.tainted, and any attempt
> > > +			to write to that sysctl will fail with -EINVAL for
> > > +			any taint value that masks with the flags set for
> > > +			this option.
> > > +
> > >  	crash_kexec_post_notifiers
> > >  			Run kdump after running panic-notifiers and dumping
> > >  			kmsg. This only for the users who doubt kdump always
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index 9b7a8d74a9d6..66bc102cb59a 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -528,6 +528,8 @@ extern int panic_on_oops;
> > >  extern int panic_on_unrecovered_nmi;
> > >  extern int panic_on_io_nmi;
> > >  extern int panic_on_warn;
> > > +extern unsigned long panic_on_taint;
> > > +extern bool panic_on_taint_exclusive;
> > >  extern int sysctl_panic_on_rcu_stall;
> > >  extern int sysctl_panic_on_stackoverflow;
> > >  
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index b69ee9e76cb2..65c62f8a1de8 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/kexec.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/sysrq.h>
> > > +#include <linux/ctype.h>
> > >  #include <linux/init.h>
> > >  #include <linux/nmi.h>
> > >  #include <linux/console.h>
> > > @@ -44,6 +45,8 @@ static int pause_on_oops_flag;
> > >  static DEFINE_SPINLOCK(pause_on_oops_lock);
> > >  bool crash_kexec_post_notifiers;
> > >  int panic_on_warn __read_mostly;
> > > +unsigned long panic_on_taint;
> > > +bool panic_on_taint_exclusive = false;
> > >  
> > >  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> > >  EXPORT_SYMBOL_GPL(panic_timeout);
> > > @@ -434,6 +437,11 @@ void add_taint(unsigned flag, enum lockdep_ok lockdep_ok)
> > >  		pr_warn("Disabling lock debugging due to kernel taint\n");
> > >  
> > >  	set_bit(flag, &tainted_mask);
> > > +
> > > +	if (tainted_mask & panic_on_taint) {
> > > +		panic_on_taint = 0;
> > 
> > This panic_on_taint resetting is redundant? It will trigger crash, do we
> > need care if it's 0 or not?
> >
> 
> We might still get more than one CPU hitting a taint adding code path after 
> the one that tripped here called panic. To avoid multiple calls to panic, 
> in that particular scenario, we clear the panic_on_taint bitmask out. 
> Also, albeit non-frequent, we might be tracking TAINT_WARN, and still hit 
> a WARN_ON() in the panic / kdump path, thus incurring in a second 
> (and unwanted) call to panic here.  

Hmm, this cpu will set panic_cpu firstly, all other cpu need stop and
have no chance to execute panic. But yes, clearing panic_on_taint makes
code easier to understand.

> 
>  
> > > +		panic("panic_on_taint set ...");
> > > +	}
> > >  }
> > >  EXPORT_SYMBOL(add_taint);
> > >  
> > > @@ -686,3 +694,35 @@ static int __init oops_setup(char *s)
> > >  	return 0;
> > >  }
> > >  early_param("oops", oops_setup);
> > > +
> > > +static int __init panic_on_taint_setup(char *s)
> > > +{
> > > +	/* we just ignore panic_on_taint if passed without flags */
> > > +	if (!s)
> > > +		goto out;
> > > +
> > > +	for (; *s; s++) {
> > > +		int i;
> > > +
> > > +		if (*s == '-') {
> > > +			panic_on_taint_exclusive = true;
> > > +			continue;
> > > +		}
> > > +
> > > +		for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
> > > +			if (toupper(*s) == taint_flags[i].c_true) {
> > > +				set_bit(i, &panic_on_taint);
> > > +				break;
> > > +			}
> > > +		}
> > 
> > Read admin-guide/tainted-kernels.rst, but still do not get what 'G' means.
> > If I specify 'panic_on_taint="G"' or 'panic_on_taint="-G"' in cmdline,
> > what is expected for this customer behaviour?
> > 
> 
> This will not panic the system as no taint flag gets actually set in 
> panic_on_taint bitmask for G.
> 
> G is the counterpart of P, and appears on print_tainted() whenever
> TAINT_PROPRIETARY_MODULE is not set. panic_on_taint doesn't set
> anything for G, as it doesn't represent any taint, but the lack
> of one particular taint, instead.
> 
> (apparently, TAINT_PROPRIETARY_MODULE is the only taint flag
> that follows that pattern of having an extra assigned letter 
> that means its absence, and perhaps it should be removed)

Yeah, agree. I will make a draft patch to remove it, see if there's
objection from people.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

  reply	other threads:[~2020-05-11  1:12 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-09 13:57 [PATCH v3] kernel: add panic_on_taint Rafael Aquini
2020-05-09 13:57 ` Rafael Aquini
2020-05-09 18:59 ` Kees Cook
2020-05-09 18:59   ` Kees Cook
2020-05-10  2:59 ` Baoquan He
2020-05-10  2:59   ` Baoquan He
2020-05-10  4:10   ` Randy Dunlap
2020-05-10  4:10     ` Randy Dunlap
2020-05-10  5:16     ` Baoquan He
2020-05-10  5:16       ` Baoquan He
2020-05-10 18:22   ` Rafael Aquini
2020-05-10 18:22     ` Rafael Aquini
2020-05-11  1:11     ` Baoquan He [this message]
2020-05-11  1:11       ` Baoquan He
2020-05-11 18:24 ` Luis Chamberlain
2020-05-11 18:24   ` Luis Chamberlain
2020-05-11 20:03   ` Rafael Aquini
2020-05-11 20:03     ` Rafael Aquini
2020-05-11 21:05     ` Luis Chamberlain
2020-05-11 21:05       ` Luis Chamberlain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200511011149.GH5029@MiWiFi-R3L-srv \
    --to=bhe@redhat.com \
    --cc=AnDavis@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=aquini@redhat.com \
    --cc=bunk@kernel.org \
    --cc=cai@lca.pw \
    --cc=corbet@lwn.net \
    --cc=dyoung@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jeffm@suse.com \
    --cc=jeyu@suse.de \
    --cc=jikos@kernel.org \
    --cc=keescook@chromium.org \
    --cc=kexec@lists.infradead.org \
    --cc=labbott@redhat.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=rdunlap@infradead.org \
    --cc=rpalethorpe@suse.de \
    --cc=tiwai@suse.de \
    --cc=torvalds@linux-foundation.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.