[PATCH v2] init: Do not select DEBUG_KERNEL by default

[PATCH v2] init: Do not select DEBUG_KERNEL by default

[Sinan Kaya]

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.

It looks like CONFIG_KALLSYMS_ALL is the only feature that
requires CONFIG_DEBUG_KERNEL.

Select CONFIG_EXPERT when CONFIG_DEBUG is chosen but you can
still choose CONFIG_EXPERT without CONFIG_DEBUG.

Signed-off-by: Sinan Kaya <okaya@kernel.org>
---
 init/Kconfig      | 2 --
 lib/Kconfig.debug | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..37e10a8391a3 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
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 0d9e81779e37..9fbf3499ec8d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -434,6 +434,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.
-- 
2.21.0

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Kees Cook]

On Wed, Apr 10, 2019 at 3:42 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.
>
> It looks like CONFIG_KALLSYMS_ALL is the only feature that
> requires CONFIG_DEBUG_KERNEL.
>
> Select CONFIG_EXPERT when CONFIG_DEBUG is chosen but you can

Typo: CONFIG_DEBUG_KERNEL

> still choose CONFIG_EXPERT without CONFIG_DEBUG.

same.

>
> Signed-off-by: Sinan Kaya <okaya@kernel.org>

But with those fixed, looks good to me. Adding Josh (and others) to CC
since he originally added the linkage to EXPERT in commit
f505c553dbe2.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  init/Kconfig      | 2 --
>  lib/Kconfig.debug | 1 +
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 4592bf7997c0..37e10a8391a3 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
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 0d9e81779e37..9fbf3499ec8d 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -434,6 +434,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.
> --
> 2.21.0
>


-- 
Kees Cook

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Josh Triplett]

On April 10, 2019 3:58:55 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>On Wed, Apr 10, 2019 at 3:42 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.
>>
>> It looks like CONFIG_KALLSYMS_ALL is the only feature that
>> requires CONFIG_DEBUG_KERNEL.
>>
>> Select CONFIG_EXPERT when CONFIG_DEBUG is chosen but you can
>
>Typo: CONFIG_DEBUG_KERNEL
>
>> still choose CONFIG_EXPERT without CONFIG_DEBUG.
>
>same.
>
>>
>> Signed-off-by: Sinan Kaya <okaya@kernel.org>
>
>But with those fixed, looks good to me. Adding Josh (and others) to CC
>since he originally added the linkage to EXPERT in commit
>f505c553dbe2.

CONFIG_DEBUG_KERNEL shouldn't affect code generation in any way; it should only make more options appear in kconfig. I originally added this to ensure that features you might want to *disable* aren't hidden, as part of the tinification effort.

What specific problem does having CONFIG_DEBUG_KERNEL enabled cause for you? I'd still prefer to have a single switch for "don't hide things I might want to disable", rather than several.

This would also need checking to make sure it doesn't grow tinyconfig.

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Kees Cook]

On Wed, Apr 10, 2019 at 4:22 PM Josh Triplett <josh@joshtriplett.org> wrote:
>
> On April 10, 2019 3:58:55 PM PDT, Kees Cook <keescook@chromium.org> wrote:
> >On Wed, Apr 10, 2019 at 3:42 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.
> >>
> >> It looks like CONFIG_KALLSYMS_ALL is the only feature that
> >> requires CONFIG_DEBUG_KERNEL.
> >>
> >> Select CONFIG_EXPERT when CONFIG_DEBUG is chosen but you can
> >
> >Typo: CONFIG_DEBUG_KERNEL
> >
> >> still choose CONFIG_EXPERT without CONFIG_DEBUG.
> >
> >same.
> >
> >>
> >> Signed-off-by: Sinan Kaya <okaya@kernel.org>
> >
> >But with those fixed, looks good to me. Adding Josh (and others) to CC
> >since he originally added the linkage to EXPERT in commit
> >f505c553dbe2.
>
> CONFIG_DEBUG_KERNEL shouldn't affect code generation in any way; it should only make more options appear in kconfig. I originally added this to ensure that features you might want to *disable* aren't hidden, as part of the tinification effort.
>
> What specific problem does having CONFIG_DEBUG_KERNEL enabled cause for you? I'd still prefer to have a single switch for "don't hide things I might want to disable", rather than several.

See earlier in the thread: code generation depends on
CONFIG_DEBUG_KERNEL now unfortunately.

-- 
Kees Cook

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Josh Triplett]

On April 10, 2019 4:24:18 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>On Wed, Apr 10, 2019 at 4:22 PM Josh Triplett <josh@joshtriplett.org>
>wrote:
>>
>> On April 10, 2019 3:58:55 PM PDT, Kees Cook <keescook@chromium.org>
>wrote:
>> >On Wed, Apr 10, 2019 at 3:42 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.
>> >>
>> >> It looks like CONFIG_KALLSYMS_ALL is the only feature that
>> >> requires CONFIG_DEBUG_KERNEL.
>> >>
>> >> Select CONFIG_EXPERT when CONFIG_DEBUG is chosen but you can
>> >
>> >Typo: CONFIG_DEBUG_KERNEL
>> >
>> >> still choose CONFIG_EXPERT without CONFIG_DEBUG.
>> >
>> >same.
>> >
>> >>
>> >> Signed-off-by: Sinan Kaya <okaya@kernel.org>
>> >
>> >But with those fixed, looks good to me. Adding Josh (and others) to
>CC
>> >since he originally added the linkage to EXPERT in commit
>> >f505c553dbe2.
>>
>> CONFIG_DEBUG_KERNEL shouldn't affect code generation in any way; it
>should only make more options appear in kconfig. I originally added
>this to ensure that features you might want to *disable* aren't hidden,
>as part of the tinification effort.
>>
>> What specific problem does having CONFIG_DEBUG_KERNEL enabled cause
>for you? I'd still prefer to have a single switch for "don't hide
>things I might want to disable", rather than several.
>
>See earlier in the thread: code generation depends on
>CONFIG_DEBUG_KERNEL now unfortunately.

Then let's fix *that*, and get checkpatch to help enforce it in the future. EXPERT doesn't affect code generation, and neither should this.

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Sinan Kaya]

On 4/10/2019 11:02 PM, Josh Triplett wrote:
> Then let's fix*that*, and get checkpatch to help enforce it in the future. EXPERT doesn't affect code generation, and neither should this.

I think we have to do both. We need to go after the users as well as
solve the immediate problem per this patch.

As Mathieu identified, CONFIG_DEBUG_KERNEL is being used all over the
place and getting subsystem owners to remove let alone add a check
to checkpatch is just going to take time.

Please let us know if you are OK with this plan.

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Josh Triplett]

On Wed, Apr 10, 2019 at 11:13:52PM -0400, Sinan Kaya wrote:
> On 4/10/2019 11:02 PM, Josh Triplett wrote:
> > Then let's fix*that*, and get checkpatch to help enforce it in the future. EXPERT doesn't affect code generation, and neither should this.
> 
> I think we have to do both. We need to go after the users as well as
> solve the immediate problem per this patch.
> 
> As Mathieu identified, CONFIG_DEBUG_KERNEL is being used all over the
> place and getting subsystem owners to remove let alone add a check
> to checkpatch is just going to take time.
> 
> Please let us know if you are OK with this plan.

I'm not OK with this plan. Turning on EXPERT should make the options
under DEBUG_KERNEL visible; it's a bug that DEBUG_KERNEL affects code
generation as well.

Proposed alternative plan: let's add a new symbol, something like
DEBUG_MISC ("Miscellaneous debug code that should be under a more
specific debug option but isn't"), make it depend on DEBUG_KERNEL and be
"default DEBUG_KERNEL" but allow itself to be turned off, and then
mechanically change the small handful of "#ifdef CONFIG_DEBUG_KERNEL" to
"#ifdef CONFIG_DEBUG_MISC".

Does that sound like an appropriately rapid solution for this bug?

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Kees Cook]

On Thu, Apr 11, 2019 at 3:16 PM Josh Triplett <josh@joshtriplett.org> wrote:
>
> On Wed, Apr 10, 2019 at 11:13:52PM -0400, Sinan Kaya wrote:
> > On 4/10/2019 11:02 PM, Josh Triplett wrote:
> > > Then let's fix*that*, and get checkpatch to help enforce it in the future. EXPERT doesn't affect code generation, and neither should this.
> >
> > I think we have to do both. We need to go after the users as well as
> > solve the immediate problem per this patch.
> >
> > As Mathieu identified, CONFIG_DEBUG_KERNEL is being used all over the
> > place and getting subsystem owners to remove let alone add a check
> > to checkpatch is just going to take time.
> >
> > Please let us know if you are OK with this plan.
>
> I'm not OK with this plan. Turning on EXPERT should make the options
> under DEBUG_KERNEL visible; it's a bug that DEBUG_KERNEL affects code
> generation as well.
>
> Proposed alternative plan: let's add a new symbol, something like
> DEBUG_MISC ("Miscellaneous debug code that should be under a more
> specific debug option but isn't"), make it depend on DEBUG_KERNEL and be
> "default DEBUG_KERNEL" but allow itself to be turned off, and then
> mechanically change the small handful of "#ifdef CONFIG_DEBUG_KERNEL" to
> "#ifdef CONFIG_DEBUG_MISC".
>
> Does that sound like an appropriately rapid solution for this bug?

Sure, that sounds fine to me. Sinan can you take care of that for v4?

-- 
Kees Cook

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Sinan Kaya]

On 4/11/2019 6:21 PM, Kees Cook wrote:
>> Proposed alternative plan: let's add a new symbol, something like
>> DEBUG_MISC ("Miscellaneous debug code that should be under a more
>> specific debug option but isn't"), make it depend on DEBUG_KERNEL and be
>> "default DEBUG_KERNEL" but allow itself to be turned off, and then
>> mechanically change the small handful of "#ifdef CONFIG_DEBUG_KERNEL" to
>> "#ifdef CONFIG_DEBUG_MISC".
>>
>> Does that sound like an appropriately rapid solution for this bug?
> Sure, that sounds fine to me. Sinan can you take care of that for v4?

Sure, let me work on this.

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Josh Triplett]

On Thu, Apr 11, 2019 at 06:27:04PM -0400, Sinan Kaya wrote:
> On 4/11/2019 6:21 PM, Kees Cook wrote:
> > > Proposed alternative plan: let's add a new symbol, something like
> > > DEBUG_MISC ("Miscellaneous debug code that should be under a more
> > > specific debug option but isn't"), make it depend on DEBUG_KERNEL and be
> > > "default DEBUG_KERNEL" but allow itself to be turned off, and then
> > > mechanically change the small handful of "#ifdef CONFIG_DEBUG_KERNEL" to
> > > "#ifdef CONFIG_DEBUG_MISC".
> > > 
> > > Does that sound like an appropriately rapid solution for this bug?
> > Sure, that sounds fine to me. Sinan can you take care of that for v4?
> 
> Sure, let me work on this.

Thank you both! I really appreciate that.

Re: [PATCH v2] init: Do not select DEBUG_KERNEL by default

[Randy Dunlap]

On 4/10/19 8:02 PM, Josh Triplett wrote:
> On April 10, 2019 4:24:18 PM PDT, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Apr 10, 2019 at 4:22 PM Josh Triplett <josh@joshtriplett.org>
>> wrote:
>>>
>>> On April 10, 2019 3:58:55 PM PDT, Kees Cook <keescook@chromium.org>
>> wrote:
>>>> On Wed, Apr 10, 2019 at 3:42 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.
>>>>>
>>>>> It looks like CONFIG_KALLSYMS_ALL is the only feature that
>>>>> requires CONFIG_DEBUG_KERNEL.
>>>>>
>>>>> Select CONFIG_EXPERT when CONFIG_DEBUG is chosen but you can
>>>>
>>>> Typo: CONFIG_DEBUG_KERNEL
>>>>
>>>>> still choose CONFIG_EXPERT without CONFIG_DEBUG.
>>>>
>>>> same.
>>>>
>>>>>
>>>>> Signed-off-by: Sinan Kaya <okaya@kernel.org>
>>>>
>>>> But with those fixed, looks good to me. Adding Josh (and others) to
>> CC
>>>> since he originally added the linkage to EXPERT in commit
>>>> f505c553dbe2.
>>>
>>> CONFIG_DEBUG_KERNEL shouldn't affect code generation in any way; it
>> should only make more options appear in kconfig. I originally added
>> this to ensure that features you might want to *disable* aren't hidden,
>> as part of the tinification effort.
>>>
>>> What specific problem does having CONFIG_DEBUG_KERNEL enabled cause
>> for you? I'd still prefer to have a single switch for "don't hide
>> things I might want to disable", rather than several.
>>
>> See earlier in the thread: code generation depends on
>> CONFIG_DEBUG_KERNEL now unfortunately.
> 
> Then let's fix *that*, and get checkpatch to help enforce it in the future. EXPERT doesn't affect code generation, and neither should this.
> 

checkpatch is not an enforcer.  It takes maintainers to do that.


-- 
~Randy