linux-kernel - Re: [PATCH] kdump: add default crashkernel reserve kernel config options

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]

Message-ID: <20180524014234.GA2031@dhcp-128-65.nay.redhat.com>
Date:   Thu, 24 May 2018 09:42:34 +0800
From:   Dave Young <dyoung@...hat.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
        Cong Wang <xiyou.wangcong@...il.com>,
        Neil Horman <nhorman@...hat.com>,
        Ingo Molnar <mingo@...nel.org>,
        Vivek Goyal <vgoyal@...hat.com>,
        Tony Luck <tony.luck@...el.com>,
        Anton Vorontsov <avorontsov@...mvista.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Hari Bathini <hbathini@...ux.vnet.ibm.com>, dzickus@...hat.com,
        bhe@...hat.com
Subject: Re: [PATCH] kdump: add default crashkernel reserve kernel config
 options

Hi Eric,
On 05/23/18 at 10:53am, Eric W. Biederman wrote:
> Dave Young <dyoung@...hat.com> writes:
> 
> > [snip]
> >
> >> >  
> >> > +config CRASHKERNEL_DEFAULT_THRESHOLD_MB
> >> > +	int "System memory size threshold for kdump memory default reserving"
> >> > +	depends on CRASH_CORE
> >> > +	default 0
> >> > +	help
> >> > +	  CRASHKERNEL_DEFAULT_MB is used as default crashkernel value if
> >> > +	  the system memory size is equal or bigger than the threshold.
> >> 
> >> "the threshold" is rather vague.  Can it be clarified?
> >> 
> >> In fact I'm really struggling to understand the logic here....
> >> 
> >> 
> >> > +config CRASHKERNEL_DEFAULT_MB
> >> > +	int "Default crashkernel memory size reserved for kdump"
> >> > +	depends on CRASH_CORE
> >> > +	default 0
> >> > +	help
> >> > +	  This is used as the default kdump reserved memory size in MB.
> >> > +	  crashkernel=X kernel cmdline can overwrite this value.
> >> > +
> >> >  config HAVE_IMA_KEXEC
> >> >  	bool
> >> >  
> >> > @@ -143,6 +144,24 @@ static int __init parse_crashkernel_simp
> >> >  	return 0;
> >> >  }
> >> >  
> >> > +static int __init get_crashkernel_default(unsigned long long system_ram,
> >> > +					  unsigned long long *size)
> >> > +{
> >> > +	unsigned long long sz = CONFIG_CRASHKERNEL_DEFAULT_MB;
> >> > +	unsigned long long thres = CONFIG_CRASHKERNEL_DEFAULT_THRESHOLD_MB;
> >> > +
> >> > +	thres *= SZ_1M;
> >> > +	sz *= SZ_1M;
> >> > +
> >> > +	if (sz >= system_ram || system_ram < thres) {
> >> > +		pr_debug("crashkernel default size can not be used.\n");
> >> > +		return -EINVAL;
> >> 
> >> In other words,
> >> 
> >> 	if (system_ram <= CONFIG_CRASHKERNEL_DEFAULT_MB ||
> >> 	    system_ram < CONFIG_CRASHKERNEL_DEFAULT_THRESHOLD_MB)
> >> 		fail;
> >> 
> >> yes?
> >> 
> >> How come?  What's happening here?  Perhaps a (good) explanatory comment
> >> is needed.  And clearer Kconfig text.
> >> 
> >> All confused :(
> >
> > Andrew, I tuned it a bit, removed the check of sz >= system_ram, so if
> > the size is too large and kernel can not find enough memory it will
> > still fail in latter code.
> >
> > Is below version looks clearer?
> 
> What is the advantage of providing this in a kconfig option rather
> than on the kernel command line as we can now?

It is not a replacement of the cmdline, this can be a supplement to
the crashkernel command line.  For a lot of common use cases if we have
the auto reservation user just do not need to manually set the cmdline
for example on a virtual machine and usual setup (except of the
comlicate storage and very large machines).  The crashkernel=auto
has been used for long time, Red Hat QE tested it on a lot of different
lab machines and proved it works well.  Kdump usually just works so admin
do little work to enable kdump.

But the crashkernel=auto implementation has some drawbacks that is it
is more like embed policy in the code and it is not flexible like a
config option.

Thanks
Dave