All of lore.kernel.org
 help / color / mirror / Atom feed
From: Luis Chamberlain <mcgrof@kernel.org>
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, bhe@redhat.com, corbet@lwn.net,
	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 21:05:13 +0000	[thread overview]
Message-ID: <20200511210513.GU11244@42.do-not-panic.com> (raw)
In-Reply-To: <20200511200325.GE367616@optiplex-lnx>

On Mon, May 11, 2020 at 04:03:25PM -0400, Rafael Aquini wrote:
> On Mon, May 11, 2020 at 06:24:55PM +0000, Luis Chamberlain wrote:
> > On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote:
> > > +Trigger Kdump on add_taint()
> > > +============================
> > > +
> > > +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> > > +whenever the value set in this bitmask matches with the bit flag being set
> > > +by add_taint(). This will cause a kdump to occur at the panic() call.
> > > +In cases where a user wants to specify this during runtime,
> > > +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> > > +to achieve the same behaviour.
> > > +
> > >  Contact
> > >  =======
> > >  
> > > 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>
> > > +			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.
> > 
> > This talks about using a string, but that it sets a bitmask. Its not
> > very clear that one must use the string representation from each taint
> > flag. Also, I don't think to use the character representation as we
> > limit ourselves to the alphabet and quirky what-should-be-arbitrary
> > characters that represent the taint flags. The taint flag character
> > representation is juse useful for human reading of a panic, but I think
> > because of the limitation of the mask with the alphabet this was not
> > such a great idea long term.
> >
> 
> I respectfully disagree with you on this one, but that might be just
> because I'm a non-native English speaker and the cause of confusion is 
> not very clear to me. Would you mind providing a blurb with a text that
> you think it would be clearer on this regard?

A simple example of what can be used and what it would mean would
suffice.

> Also, making the input of the option to match with something one
> is used to see in taint reports make it easier to use. It would
> be still a human setting the boot option, so it makes sense to
> utilize a known/meaningful representation for panic_on_taint input.

Yes, however I still believe that what we are printing is only doing
a disservice to limiting the size of our bitmask.

> > So, I don't think we should keep on extending the alphabet use case, a
> > simple digit representation would suffice. I think this means we'd need
> > two params one for exclusive and one for the value of the taint.
> > 
> > Using a hex value or number also lets us make the input value shorter.
> >
> 
> Albeit you're right on the limitation of an alphabetical representation, 
> the truth is that taint flags are not introduced that frequently.
> Considering how many times these flags were introduced in the past,
> one could infer we probably will not run out of alphabet in the next 20 
> years (a couple of new flags gets introduced every 2 years, or so, in
> average), and the rate of change in these seems to be slowing down
> considerably, as in the past 5 years, we've seen only 2 new flags.
> 
> I'm not against your suggestion, though, but I think it makes
> clumsier to utilize the feature as you now require 2 kernel
> cmdline options, instead of just one, and a less intuitive
> way of setting it via base16 values. All at the expense of 
> a theoretical shortage of taint flags. 
> 
> If the others that were already OK with the simple string interface 
> don't object your change suggestions in that regard, I'll refactor
> the parser to meet them. 

It is just silly for us to restrict our bitmask to the alphabet. The
alphabetic restrictions were useful for print until we started running
out of meaningful characters, moving forward they'll just be a nuisance.

I don't think its wise to encourage their use now for input, now that
we have realized that their use will be pointless soon.

> > If a kernel boots with panic-on-taint flag not yet supported, we don't
> > complain, therefore getting a false sense of security that we will panic
> > with a not yet supported taint flag. I think we should pr_warn() or
> > fail to boot when that happens.
> >
> 
> Bear in mind that these very early print outs (like the ones eventually
> produced by setting panic_on_taint) to the log buffer might get lost in 
> verbose-logging long-running time systems, but apart from that, I see no 
> problems in being a little bit more verbose in that parser. I'll make
> the changes for a next revision, if no one else objects them.

See mitigations_parse_cmdline(), we pr_crit() there as well if we enter
a wrong value. I think that's the best we can do.

  Luis

WARNING: multiple messages have this Message-ID
From: Luis Chamberlain <mcgrof@kernel.org>
To: Rafael Aquini <aquini@redhat.com>
Cc: linux-doc@vger.kernel.org, tiwai@suse.de, jeffm@suse.com,
	bhe@redhat.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,
	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 21:05:13 +0000	[thread overview]
Message-ID: <20200511210513.GU11244@42.do-not-panic.com> (raw)
In-Reply-To: <20200511200325.GE367616@optiplex-lnx>

On Mon, May 11, 2020 at 04:03:25PM -0400, Rafael Aquini wrote:
> On Mon, May 11, 2020 at 06:24:55PM +0000, Luis Chamberlain wrote:
> > On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote:
> > > +Trigger Kdump on add_taint()
> > > +============================
> > > +
> > > +The kernel parameter, panic_on_taint, calls panic() from within add_taint(),
> > > +whenever the value set in this bitmask matches with the bit flag being set
> > > +by add_taint(). This will cause a kdump to occur at the panic() call.
> > > +In cases where a user wants to specify this during runtime,
> > > +/proc/sys/kernel/panic_on_taint can be set to a respective bitmask value
> > > +to achieve the same behaviour.
> > > +
> > >  Contact
> > >  =======
> > >  
> > > 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>
> > > +			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.
> > 
> > This talks about using a string, but that it sets a bitmask. Its not
> > very clear that one must use the string representation from each taint
> > flag. Also, I don't think to use the character representation as we
> > limit ourselves to the alphabet and quirky what-should-be-arbitrary
> > characters that represent the taint flags. The taint flag character
> > representation is juse useful for human reading of a panic, but I think
> > because of the limitation of the mask with the alphabet this was not
> > such a great idea long term.
> >
> 
> I respectfully disagree with you on this one, but that might be just
> because I'm a non-native English speaker and the cause of confusion is 
> not very clear to me. Would you mind providing a blurb with a text that
> you think it would be clearer on this regard?

A simple example of what can be used and what it would mean would
suffice.

> Also, making the input of the option to match with something one
> is used to see in taint reports make it easier to use. It would
> be still a human setting the boot option, so it makes sense to
> utilize a known/meaningful representation for panic_on_taint input.

Yes, however I still believe that what we are printing is only doing
a disservice to limiting the size of our bitmask.

> > So, I don't think we should keep on extending the alphabet use case, a
> > simple digit representation would suffice. I think this means we'd need
> > two params one for exclusive and one for the value of the taint.
> > 
> > Using a hex value or number also lets us make the input value shorter.
> >
> 
> Albeit you're right on the limitation of an alphabetical representation, 
> the truth is that taint flags are not introduced that frequently.
> Considering how many times these flags were introduced in the past,
> one could infer we probably will not run out of alphabet in the next 20 
> years (a couple of new flags gets introduced every 2 years, or so, in
> average), and the rate of change in these seems to be slowing down
> considerably, as in the past 5 years, we've seen only 2 new flags.
> 
> I'm not against your suggestion, though, but I think it makes
> clumsier to utilize the feature as you now require 2 kernel
> cmdline options, instead of just one, and a less intuitive
> way of setting it via base16 values. All at the expense of 
> a theoretical shortage of taint flags. 
> 
> If the others that were already OK with the simple string interface 
> don't object your change suggestions in that regard, I'll refactor
> the parser to meet them. 

It is just silly for us to restrict our bitmask to the alphabet. The
alphabetic restrictions were useful for print until we started running
out of meaningful characters, moving forward they'll just be a nuisance.

I don't think its wise to encourage their use now for input, now that
we have realized that their use will be pointless soon.

> > If a kernel boots with panic-on-taint flag not yet supported, we don't
> > complain, therefore getting a false sense of security that we will panic
> > with a not yet supported taint flag. I think we should pr_warn() or
> > fail to boot when that happens.
> >
> 
> Bear in mind that these very early print outs (like the ones eventually
> produced by setting panic_on_taint) to the log buffer might get lost in 
> verbose-logging long-running time systems, but apart from that, I see no 
> problems in being a little bit more verbose in that parser. I'll make
> the changes for a next revision, if no one else objects them.

See mitigations_parse_cmdline(), we pr_crit() there as well if we enter
a wrong value. I think that's the best we can do.

  Luis

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

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

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-09 13:57 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
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 [this message]
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=20200511210513.GU11244@42.do-not-panic.com \
    --to=mcgrof@kernel.org \
    --cc=AnDavis@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=aquini@redhat.com \
    --cc=bhe@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=rdunlap@infradead.org \
    --cc=rpalethorpe@suse.de \
    --cc=tiwai@suse.de \
    --cc=torvalds@linux-foundation.org \
    --cc=tytso@mit.edu \
    --subject='Re: [PATCH v3] kernel: add panic_on_taint' \
    /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

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.