linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] init: Do not select DEBUG_KERNEL by default
@ 2019-04-10 21:26 Sinan Kaya
  2019-04-10 21:45 ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: Sinan Kaya @ 2019-04-10 21:26 UTC (permalink / raw)
  To: linux-kernel
  Cc: Sinan Kaya, Masahiro Yamada, Andrew Morton, Kees Cook,
	Johannes Weiner, Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

We can't seem to have a kernel with CONFIG_EXPERT set but
CONFIG_DEBUG_KERNEL unset these days.

While some of the features under the CONFIG_EXPERT require
CONFIG_DEBUG_KERNEL, it doesn't apply for all features.

The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
mixed here.

It looks like CONFIG_KALLSYMS_ALL is the only feature that
requires CONFIG_DEBUG_KERNEL. Move the CONFIG_DEBUG_KERNEL
selection to where it really is needed.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 init/Kconfig | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..0a6346feae8d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1206,8 +1206,6 @@ config BPF
 
 menuconfig EXPERT
 	bool "Configure standard kernel features (expert users)"
-	# Unhide debug options, to make the on-by-default options visible
-	select DEBUG_KERNEL
 	help
 	  This option allows certain base kernel options and settings
           to be disabled or tweaked. This is for specialized
@@ -1473,7 +1471,8 @@ config KALLSYMS
 
 config KALLSYMS_ALL
 	bool "Include all symbols in kallsyms"
-	depends on DEBUG_KERNEL && KALLSYMS
+	select DEBUG_KERNEL
+	depends on KALLSYMS
 	help
 	   Normally kallsyms only contains the symbols of functions for nicer
 	   OOPS messages and backtraces (i.e., symbols from the text and inittext
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 21:26 [PATCH v1] init: Do not select DEBUG_KERNEL by default Sinan Kaya
@ 2019-04-10 21:45 ` Kees Cook
  2019-04-10 21:53   ` Sinan Kaya
  0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2019-04-10 21:45 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: LKML, Masahiro Yamada, Andrew Morton, Kees Cook, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On Wed, Apr 10, 2019 at 2:26 PM Sinan Kaya <okaya@kernel.org> wrote:
>
> We can't seem to have a kernel with CONFIG_EXPERT set but
> CONFIG_DEBUG_KERNEL unset these days.
>
> While some of the features under the CONFIG_EXPERT require
> CONFIG_DEBUG_KERNEL, it doesn't apply for all features.
>
> The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
> mixed here.

I don't agree: the point of EXPERT is to show _everything_, which
means DEBUG_KERNEL should be selected to show those options as well. I
think this is fine as-is. What is the problem you want to solve?

I think of it as low (nothing selected) medium (DEBUG_KERNEL) and high
(EXPERT and DEBUG_KERNEL). So EXPERT enables DEBUG_KERNEL too.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 21:45 ` Kees Cook
@ 2019-04-10 21:53   ` Sinan Kaya
  2019-04-10 22:04     ` Mathieu Desnoyers
  2019-04-10 22:04     ` Kees Cook
  0 siblings, 2 replies; 11+ messages in thread
From: Sinan Kaya @ 2019-04-10 21:53 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On 4/10/2019 5:45 PM, Kees Cook wrote:
> On Wed, Apr 10, 2019 at 2:26 PM Sinan Kaya <okaya@kernel.org> wrote:
>>
>> We can't seem to have a kernel with CONFIG_EXPERT set but
>> CONFIG_DEBUG_KERNEL unset these days.
>>
>> While some of the features under the CONFIG_EXPERT require
>> CONFIG_DEBUG_KERNEL, it doesn't apply for all features.
>>
>> The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
>> mixed here.
> 
> I don't agree: the point of EXPERT is to show _everything_, which
> means DEBUG_KERNEL should be selected to show those options as well. I
> think this is fine as-is. What is the problem you want to solve?
> 
> I think of it as low (nothing selected) medium (DEBUG_KERNEL) and high
> (EXPERT and DEBUG_KERNEL). So EXPERT enables DEBUG_KERNEL too.
> 

Sure, let's see if there is a better option.

I don't want any of the debug features in my kernel but still
need all the expert features. My kernel is considered a production
kernel. I don't really want to ship all the good debug enables.

On the other hand, I need the features under CONFIG_EXPERT to have
a functional system.

Let's take "multiple users" as an example.

What's the point of having a kernel without multiple users? :)

I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
as none of the features except KALLSYMS depend on it. If there was
a compile time dependency, I'd say move it to the things that need
it as this patch suggests.

P.S. I found a circular dependency now. I can respin the patch based
on feedback.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 21:53   ` Sinan Kaya
@ 2019-04-10 22:04     ` Mathieu Desnoyers
  2019-04-10 22:07       ` Kees Cook
  2019-04-10 22:04     ` Kees Cook
  1 sibling, 1 reply; 11+ messages in thread
From: Mathieu Desnoyers @ 2019-04-10 22:04 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: Kees Cook, linux-kernel, Masahiro Yamada, Andrew Morton,
	Johannes Weiner, Peter Zijlstra, Nicholas Piggin, gor,
	Adrian Reber, Richard Guy Briggs

----- On Apr 10, 2019, at 5:53 PM, Sinan Kaya Okaya@kernel.org wrote:

> On 4/10/2019 5:45 PM, Kees Cook wrote:
>> On Wed, Apr 10, 2019 at 2:26 PM Sinan Kaya <okaya@kernel.org> wrote:
>>>
>>> We can't seem to have a kernel with CONFIG_EXPERT set but
>>> CONFIG_DEBUG_KERNEL unset these days.
>>>
>>> While some of the features under the CONFIG_EXPERT require
>>> CONFIG_DEBUG_KERNEL, it doesn't apply for all features.
>>>
>>> The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
>>> mixed here.
>> 
>> I don't agree: the point of EXPERT is to show _everything_, which
>> means DEBUG_KERNEL should be selected to show those options as well. I
>> think this is fine as-is. What is the problem you want to solve?
>> 
>> I think of it as low (nothing selected) medium (DEBUG_KERNEL) and high
>> (EXPERT and DEBUG_KERNEL). So EXPERT enables DEBUG_KERNEL too.
>> 
> 
> Sure, let's see if there is a better option.
> 
> I don't want any of the debug features in my kernel but still
> need all the expert features. My kernel is considered a production
> kernel. I don't really want to ship all the good debug enables.
> 
> On the other hand, I need the features under CONFIG_EXPERT to have
> a functional system.
> 
> Let's take "multiple users" as an example.
> 
> What's the point of having a kernel without multiple users? :)
> 
> I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
> as none of the features except KALLSYMS depend on it. If there was
> a compile time dependency, I'd say move it to the things that need
> it as this patch suggests.
> 
> P.S. I found a circular dependency now. I can respin the patch based
> on feedback.

I think part of the issue here is that a few .c/.S files use CONFIG_DEBUG_KERNEL
as #ifdef directly, which I'm not sure was meant to be. For instance:

arch/powerpc/kernel/sysfs.c:

#ifdef CONFIG_DEBUG_KERNEL
SYSFS_SPRSETUP(hid0, SPRN_HID0);
SYSFS_SPRSETUP(hid1, SPRN_HID1);
SYSFS_SPRSETUP(hid4, SPRN_HID4);
SYSFS_SPRSETUP(hid5, SPRN_HID5);
SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
#endif /* CONFIG_DEBUG_KERNEL */


arch/mips/kernel/setup.c:

#if defined(CONFIG_DEBUG_KERNEL) && defined(CONFIG_DEBUG_INFO)
                /*
                 * This information is necessary when debugging the kernel
                 * But is a security vulnerability otherwise!
                 */
                show_kernel_relocation(KERN_INFO);
#endif

net/netfilter/core.c:

static void hooks_validate(const struct nf_hook_entries *hooks)
{
#ifdef CONFIG_DEBUG_KERNEL
        struct nf_hook_ops **orig_ops;
        int prio = INT_MIN;
        size_t i = 0;

        orig_ops = nf_hook_entries_get_hook_ops(hooks);

        for (i = 0; i < hooks->num_hook_entries; i++) {
                if (orig_ops[i] == &dummy_ops)
                        continue;

                WARN_ON(orig_ops[i]->priority < prio);

                if (orig_ops[i]->priority > prio)
                        prio = orig_ops[i]->priority;
        }
#endif
}

and also:
arch/xtensa/kernel/smp.c
arch/xtensa/kernel/entry.S

I was under the impression that config DEBUG_KERNEL was only making a
"group" of menu entries visible without any direct impact on the code,
but it does not appear to be the case for a few exceptions. Perhaps this
is the actual issue ? (and lack of documentation of this Kconfig entry)

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 21:53   ` Sinan Kaya
  2019-04-10 22:04     ` Mathieu Desnoyers
@ 2019-04-10 22:04     ` Kees Cook
  2019-04-10 22:18       ` Sinan Kaya
  1 sibling, 1 reply; 11+ messages in thread
From: Kees Cook @ 2019-04-10 22:04 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On Wed, Apr 10, 2019 at 2:54 PM Sinan Kaya <Okaya@kernel.org> wrote:
>
> On 4/10/2019 5:45 PM, Kees Cook wrote:
> > On Wed, Apr 10, 2019 at 2:26 PM Sinan Kaya <okaya@kernel.org> wrote:
> >>
> >> We can't seem to have a kernel with CONFIG_EXPERT set but
> >> CONFIG_DEBUG_KERNEL unset these days.
> >>
> >> While some of the features under the CONFIG_EXPERT require
> >> CONFIG_DEBUG_KERNEL, it doesn't apply for all features.
> >>
> >> The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
> >> mixed here.
> >
> > I don't agree: the point of EXPERT is to show _everything_, which
> > means DEBUG_KERNEL should be selected to show those options as well. I
> > think this is fine as-is. What is the problem you want to solve?
> >
> > I think of it as low (nothing selected) medium (DEBUG_KERNEL) and high
> > (EXPERT and DEBUG_KERNEL). So EXPERT enables DEBUG_KERNEL too.
> >
>
> Sure, let's see if there is a better option.
>
> I don't want any of the debug features in my kernel but still
> need all the expert features. My kernel is considered a production
> kernel. I don't really want to ship all the good debug enables.

Production kernels enable it. e.g. Ubuntu:
$ grep '\bCONFIG_DEBUG_KERNEL\b' /boot/config-$(uname -r)
CONFIG_DEBUG_KERNEL=y

> On the other hand, I need the features under CONFIG_EXPERT to have
> a functional system.
>
> Let's take "multiple users" as an example.
>
> What's the point of having a kernel without multiple users? :)
>
> I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
> as none of the features except KALLSYMS depend on it. If there was
> a compile time dependency, I'd say move it to the things that need
> it as this patch suggests.

CONFIG_DEBUG_KERNEL mainly only enables the visibility of various
other options. I can only find two instances of it controlling a
"default", and one is overridden by CONFIG_SMP on alpha. :)

$ git grep -B2 'default.*DEBUG_KERNEL'
arch/alpha/Kconfig.debug-config MATHEMU
arch/alpha/Kconfig.debug-       tristate "Kernel FP software
completion" if DEBUG_KERNEL && !SMP
arch/alpha/Kconfig.debug:       default y if !DEBUG_KERNEL || SMP
--
kernel/trace/Kconfig-menuconfig FTRACE
kernel/trace/Kconfig-   bool "Tracers"
kernel/trace/Kconfig:   default y if DEBUG_KERNEL

What do you see enabled that CONFIG_DEBUG_KERNEL enables that you don't want?

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 22:04     ` Mathieu Desnoyers
@ 2019-04-10 22:07       ` Kees Cook
  0 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2019-04-10 22:07 UTC (permalink / raw)
  To: Mathieu Desnoyers
  Cc: Sinan Kaya, Kees Cook, linux-kernel, Masahiro Yamada,
	Andrew Morton, Johannes Weiner, Peter Zijlstra, Nicholas Piggin,
	gor, Adrian Reber, Richard Guy Briggs

On Wed, Apr 10, 2019 at 3:04 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Apr 10, 2019, at 5:53 PM, Sinan Kaya Okaya@kernel.org wrote:
>
> > On 4/10/2019 5:45 PM, Kees Cook wrote:
> >> On Wed, Apr 10, 2019 at 2:26 PM Sinan Kaya <okaya@kernel.org> wrote:
> >>>
> >>> We can't seem to have a kernel with CONFIG_EXPERT set but
> >>> CONFIG_DEBUG_KERNEL unset these days.
> >>>
> >>> While some of the features under the CONFIG_EXPERT require
> >>> CONFIG_DEBUG_KERNEL, it doesn't apply for all features.
> >>>
> >>> The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
> >>> mixed here.
> >>
> >> I don't agree: the point of EXPERT is to show _everything_, which
> >> means DEBUG_KERNEL should be selected to show those options as well. I
> >> think this is fine as-is. What is the problem you want to solve?
> >>
> >> I think of it as low (nothing selected) medium (DEBUG_KERNEL) and high
> >> (EXPERT and DEBUG_KERNEL). So EXPERT enables DEBUG_KERNEL too.
> >>
> >
> > Sure, let's see if there is a better option.
> >
> > I don't want any of the debug features in my kernel but still
> > need all the expert features. My kernel is considered a production
> > kernel. I don't really want to ship all the good debug enables.
> >
> > On the other hand, I need the features under CONFIG_EXPERT to have
> > a functional system.
> >
> > Let's take "multiple users" as an example.
> >
> > What's the point of having a kernel without multiple users? :)
> >
> > I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
> > as none of the features except KALLSYMS depend on it. If there was
> > a compile time dependency, I'd say move it to the things that need
> > it as this patch suggests.
> >
> > P.S. I found a circular dependency now. I can respin the patch based
> > on feedback.
>
> I think part of the issue here is that a few .c/.S files use CONFIG_DEBUG_KERNEL
> as #ifdef directly, which I'm not sure was meant to be. For instance:
>
> arch/powerpc/kernel/sysfs.c:
>
> #ifdef CONFIG_DEBUG_KERNEL
> SYSFS_SPRSETUP(hid0, SPRN_HID0);
> SYSFS_SPRSETUP(hid1, SPRN_HID1);
> SYSFS_SPRSETUP(hid4, SPRN_HID4);
> SYSFS_SPRSETUP(hid5, SPRN_HID5);
> SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
> SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
> SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
> SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
> SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
> SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
> SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
> SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
> SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
> SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
> SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
> SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
> SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
> SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
> SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
> SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
> SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
> SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
> SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
> SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
> SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
> SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
> SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
> SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
> #endif /* CONFIG_DEBUG_KERNEL */
>
>
> arch/mips/kernel/setup.c:
>
> #if defined(CONFIG_DEBUG_KERNEL) && defined(CONFIG_DEBUG_INFO)
>                 /*
>                  * This information is necessary when debugging the kernel
>                  * But is a security vulnerability otherwise!
>                  */
>                 show_kernel_relocation(KERN_INFO);
> #endif

This one is unfortunate for sure. :P

> net/netfilter/core.c:
>
> static void hooks_validate(const struct nf_hook_entries *hooks)
> {
> #ifdef CONFIG_DEBUG_KERNEL
>         struct nf_hook_ops **orig_ops;
>         int prio = INT_MIN;
>         size_t i = 0;
>
>         orig_ops = nf_hook_entries_get_hook_ops(hooks);
>
>         for (i = 0; i < hooks->num_hook_entries; i++) {
>                 if (orig_ops[i] == &dummy_ops)
>                         continue;
>
>                 WARN_ON(orig_ops[i]->priority < prio);
>
>                 if (orig_ops[i]->priority > prio)
>                         prio = orig_ops[i]->priority;
>         }
> #endif
> }

This seems best to just always enable, neither caller appears to be fast-path.

>
> and also:
> arch/xtensa/kernel/smp.c
> arch/xtensa/kernel/entry.S
>
> I was under the impression that config DEBUG_KERNEL was only making a
> "group" of menu entries visible without any direct impact on the code,
> but it does not appear to be the case for a few exceptions. Perhaps this
> is the actual issue ? (and lack of documentation of this Kconfig entry)

Yeah, that's certainly not how it was intended. But under EXPERT, I
think there is still an argument to made that it's the right thing to
do.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 22:04     ` Kees Cook
@ 2019-04-10 22:18       ` Sinan Kaya
  2019-04-10 22:21         ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: Sinan Kaya @ 2019-04-10 22:18 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On 4/10/2019 6:04 PM, Kees Cook wrote:

>> I don't want any of the debug features in my kernel but still
>> need all the expert features. My kernel is considered a production
>> kernel. I don't really want to ship all the good debug enables.
> 
> Production kernels enable it. e.g. Ubuntu:
> $ grep '\bCONFIG_DEBUG_KERNEL\b' /boot/config-$(uname -r)
> CONFIG_DEBUG_KERNEL=y
> 

It makes sense for a general purpose operating system. It doesn't apply
to my limited case where I'm very concerned about image size.

>> I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
>> as none of the features except KALLSYMS depend on it. If there was
>> a compile time dependency, I'd say move it to the things that need
>> it as this patch suggests.
> 
> CONFIG_DEBUG_KERNEL mainly only enables the visibility of various
> other options. I can only find two instances of it controlling a
> "default", and one is overridden by CONFIG_SMP on alpha. :)
> 
> $ git grep -B2 'default.*DEBUG_KERNEL'
> arch/alpha/Kconfig.debug-config MATHEMU
> arch/alpha/Kconfig.debug-       tristate "Kernel FP software
> completion" if DEBUG_KERNEL && !SMP
> arch/alpha/Kconfig.debug:       default y if !DEBUG_KERNEL || SMP
> --
> kernel/trace/Kconfig-menuconfig FTRACE
> kernel/trace/Kconfig-   bool "Tracers"
> kernel/trace/Kconfig:   default y if DEBUG_KERNEL

If the idea is to just show, nothing should happen based on
DEBUG_KERNEL, right?

No default selection as in FTRACE, no c/S file changes in the code
path as Mathieu identified.

I can go after individual enables if you agree assuming Mathieu will
go after the changes in the other email. Let me know otherwise.

> 
> What do you see enabled that CONFIG_DEBUG_KERNEL enables that you don't want?
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 22:18       ` Sinan Kaya
@ 2019-04-10 22:21         ` Kees Cook
  2019-04-10 22:25           ` Sinan Kaya
  0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2019-04-10 22:21 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On Wed, Apr 10, 2019 at 3:18 PM Sinan Kaya <Okaya@kernel.org> wrote:
>
> On 4/10/2019 6:04 PM, Kees Cook wrote:
>
> >> I don't want any of the debug features in my kernel but still
> >> need all the expert features. My kernel is considered a production
> >> kernel. I don't really want to ship all the good debug enables.
> >
> > Production kernels enable it. e.g. Ubuntu:
> > $ grep '\bCONFIG_DEBUG_KERNEL\b' /boot/config-$(uname -r)
> > CONFIG_DEBUG_KERNEL=y
> >
>
> It makes sense for a general purpose operating system. It doesn't apply
> to my limited case where I'm very concerned about image size.

Gotcha.

>
> >> I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
> >> as none of the features except KALLSYMS depend on it. If there was
> >> a compile time dependency, I'd say move it to the things that need
> >> it as this patch suggests.
> >
> > CONFIG_DEBUG_KERNEL mainly only enables the visibility of various
> > other options. I can only find two instances of it controlling a
> > "default", and one is overridden by CONFIG_SMP on alpha. :)
> >
> > $ git grep -B2 'default.*DEBUG_KERNEL'
> > arch/alpha/Kconfig.debug-config MATHEMU
> > arch/alpha/Kconfig.debug-       tristate "Kernel FP software
> > completion" if DEBUG_KERNEL && !SMP
> > arch/alpha/Kconfig.debug:       default y if !DEBUG_KERNEL || SMP
> > --
> > kernel/trace/Kconfig-menuconfig FTRACE
> > kernel/trace/Kconfig-   bool "Tracers"
> > kernel/trace/Kconfig:   default y if DEBUG_KERNEL
>
> If the idea is to just show, nothing should happen based on
> DEBUG_KERNEL, right?
>
> No default selection as in FTRACE, no c/S file changes in the code
> path as Mathieu identified.

Yeah, this is clearly a problem. I hadn't looked for code #ifdefs :(

> I can go after individual enables if you agree assuming Mathieu will
> go after the changes in the other email. Let me know otherwise.

How about you split it, but make DEBUG_KERNEL be "default EXPERT" that
way enabling EXPERT will enable DEBUG_KERNEL still in the default
case?

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 22:21         ` Kees Cook
@ 2019-04-10 22:25           ` Sinan Kaya
  2019-04-10 22:28             ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: Sinan Kaya @ 2019-04-10 22:25 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On 4/10/2019 6:21 PM, Kees Cook wrote:
>> I can go after individual enables if you agree assuming Mathieu will
>> go after the changes in the other email. Let me know otherwise.
> How about you split it, but make DEBUG_KERNEL be "default EXPERT" that
> way enabling EXPERT will enable DEBUG_KERNEL still in the default
> case?

Sorry, can you explain what you mean by split?

Do you mean move the things I need out of EXPERT? or something else?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 22:25           ` Sinan Kaya
@ 2019-04-10 22:28             ` Kees Cook
  2019-04-10 22:29               ` Sinan Kaya
  0 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2019-04-10 22:28 UTC (permalink / raw)
  To: Sinan Kaya
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On Wed, Apr 10, 2019 at 3:25 PM Sinan Kaya <Okaya@kernel.org> wrote:
>
> On 4/10/2019 6:21 PM, Kees Cook wrote:
> >> I can go after individual enables if you agree assuming Mathieu will
> >> go after the changes in the other email. Let me know otherwise.
> > How about you split it, but make DEBUG_KERNEL be "default EXPERT" that
> > way enabling EXPERT will enable DEBUG_KERNEL still in the default
> > case?
>
> Sorry, can you explain what you mean by split?

I mean when you disconnect them (i.e. remove the "select" in EXPERT)

> Do you mean move the things I need out of EXPERT? or something else?

So, for example, this:

diff --git a/init/Kconfig b/init/Kconfig
index c9386a365eea..7ce4a60ab3e9 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1188,8 +1188,6 @@ config BPF

 menuconfig EXPERT
        bool "Configure standard kernel features (expert users)"
-       # Unhide debug options, to make the on-by-default options visible
-       select DEBUG_KERNEL
        help
          This option allows certain base kernel options and settings
           to be disabled or tweaked. This is for specialized
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d4df5b24d75e..6a9bc118b64a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -433,6 +433,7 @@ config MAGIC_SYSRQ_SERIAL

 config DEBUG_KERNEL
        bool "Kernel debugging"
+       default EXPERT
        help
          Say Y here if you are developing drivers or trying to debug and
          identify kernel problems.

Then you shouldn't need the KALLSYMS_ALL change, yes? But maybe I
misunderstood that.

-- 
Kees Cook

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default
  2019-04-10 22:28             ` Kees Cook
@ 2019-04-10 22:29               ` Sinan Kaya
  0 siblings, 0 replies; 11+ messages in thread
From: Sinan Kaya @ 2019-04-10 22:29 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Masahiro Yamada, Andrew Morton, Johannes Weiner,
	Peter Zijlstra (Intel),
	Nicholas Piggin, Mathieu Desnoyers, Vasily Gorbik, Adrian Reber,
	Richard Guy Briggs

On 4/10/2019 6:28 PM, Kees Cook wrote:
> diff --git a/init/Kconfig b/init/Kconfig
> index c9386a365eea..7ce4a60ab3e9 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1188,8 +1188,6 @@ config BPF
> 
>   menuconfig EXPERT
>          bool "Configure standard kernel features (expert users)"
> -       # Unhide debug options, to make the on-by-default options visible
> -       select DEBUG_KERNEL
>          help
>            This option allows certain base kernel options and settings
>             to be disabled or tweaked. This is for specialized
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d4df5b24d75e..6a9bc118b64a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -433,6 +433,7 @@ config MAGIC_SYSRQ_SERIAL
> 
>   config DEBUG_KERNEL
>          bool "Kernel debugging"
> +       default EXPERT
>          help
>            Say Y here if you are developing drivers or trying to debug and
>            identify kernel problems.

Got it. Thanks. I'll test and respin v2.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-04-10 22:29 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10 21:26 [PATCH v1] init: Do not select DEBUG_KERNEL by default Sinan Kaya
2019-04-10 21:45 ` Kees Cook
2019-04-10 21:53   ` Sinan Kaya
2019-04-10 22:04     ` Mathieu Desnoyers
2019-04-10 22:07       ` Kees Cook
2019-04-10 22:04     ` Kees Cook
2019-04-10 22:18       ` Sinan Kaya
2019-04-10 22:21         ` Kees Cook
2019-04-10 22:25           ` Sinan Kaya
2019-04-10 22:28             ` Kees Cook
2019-04-10 22:29               ` Sinan Kaya

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).