Message-ID: <202509010949.9A61A98@keescook>
Date: Mon, 1 Sep 2025 09:56:52 -0700
From: Kees Cook <kees@...nel.org>
To: Vegard Nossum <vegard.nossum@...cle.com>
Cc: Nathan Chancellor <nathan@...nel.org>,
	Nicolas Schier <nicolas.schier@...ux.dev>,
	Jonathan Corbet <corbet@....net>,
	Masahiro Yamada <masahiroy@...nel.org>,
	Randy Dunlap <rdunlap@...radead.org>, Arnd Bergmann <arnd@...db.de>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	linux-kbuild@...r.kernel.org, linux-doc@...r.kernel.org,
	Miguel Ojeda <ojeda@...nel.org>,
	Stephen Brennan <stephen.s.brennan@...cle.com>,
	Marco Bonelli <marco@...eim.net>, Petr Vorel <pvorel@...e.cz>,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] kconfig: Add transitional symbol attribute for
 migration support

On Mon, Sep 01, 2025 at 10:34:19AM +0200, Vegard Nossum wrote:
> Drive-by review... consider it more as "here's some stuff that could be
> worth looking at" rather than blocking in any way.

Thanks for looking at it!

> 
> On 30/08/2025 04:01, Kees Cook wrote:
> > During kernel option migrations (e.g. CONFIG_CFI_CLANG to CONFIG_CFI),
> > existing .config files need to maintain backward compatibility while
> > preventing deprecated options from appearing in newly generated
> > configurations. This is challenging with existing Kconfig mechanisms
> > because:
> > 
> > 1. Simply removing old options breaks existing .config files.
> > 2. Manually listing an option as "deprecated" leaves it needlessly
> >     visible and still writes them to new .config files.
> > 3. Using any method to remove visibility (.e.g no 'prompt', 'if n',
> >     etc) prevents the option from being processed at all.
> > 
> > Add a "transitional" attribute that creates symbols which are:
> > - Processed during configuration (can influence other symbols' defaults)
> > - Hidden from user menus (no prompts appear)
> > - Omitted from newly written .config files (gets migrated)
> > - Restricted to only having help sections (no defaults, selects, etc)
> >    making it truly just a "prior value pass-through" option.
> > 
> > The transitional syntax requires a type argument and prevents type
> > redefinition:
> > 
> >      config OLD_OPTION
> >          transitional bool
> >          help
> >            Transitional config for OLD_OPTION migration.
> > 
> >      config NEW_OPTION
> >          bool "New option"
> >          default OLD_OPTION
> 
> Can you add this to scripts/kconfig/tests/ + both positive and negative
> tests? Tests are run with 'make testconfig' but (AFAICT) doesn't
> actually recompile config/mconf/etc. before running the tests, so small
> gotcha there.

Yes, I will get that added if people are generally happy with this
feature idea. :)

> > diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> > index fe2231e0e6a4..be51574d6c77 100644
> > --- a/scripts/kconfig/expr.h
> > +++ b/scripts/kconfig/expr.h
> > @@ -127,6 +127,21 @@ struct symbol {
> >   	/* SYMBOL_* flags */
> >   	int flags;
> > +	/*
> > +	 * Transitional symbol - processed during configuration but hidden from
> > +	 * user in menus and omitted from newly written .config files. Used for
> > +	 * backward compatibility during config option migrations (e.g.,
> > +	 * CFI_CLANG → CFI). Transitional symbols can still influence default
> > +	 * expressions of other symbols.
> > +	 */
> > +	bool transitional:1;
> > +
> > +	/*
> > +	 * Symbol usability - calculated as (visible != no || transitional).
> > +	 * Determines if symbol can be used in expressions.
> > +	 */
> > +	bool usable:1;
> > +
> 
> It's a bit of a "red flag" to see bitfield bools just after an "int
> flags;" member... should these be SYMBOL_ flags?
> 
> Speaking of SYMBOL_ flags, there's apparently one that controls whether
> a given symbol should be written out to .config:

Yeah, I mentioned this in the commit log, and maybe I just have to make
this not as easily readable? But you have a point about "usable" below...

> scripts/kconfig/expr.h:#define SYMBOL_WRITE      0x0200  /* write symbol to
> file (KCONFIG_CONFIG) */
> 
> This seems like something you'd like to use somehow -- maybe simply
> clear it in sym_calc_value() if it's transitional? Similar to how it's
> done for choice values:
> 
>         if (sym_is_choice(sym))
>                 sym->flags &= ~SYMBOL_WRITE;

This is actually handled naturally as part of this logic:

> >   	if (sym->visible != no)
> >   		sym->flags |= SYMBOL_WRITE;

i.e. "usable" doesn't change SYMBOL_WRITE getting set.

> > @@ -205,6 +206,16 @@ config_option: T_PROMPT T_WORD_QUOTE if_expr T_EOL
> >   	printd(DEBUG_PARSE, "%s:%d:prompt\n", cur_filename, cur_lineno);
> >   };
> > +config_option: T_TRANSITIONAL type T_EOL
> > +{
> > +	if (current_entry->sym->type != S_UNKNOWN)
> > +		yyerror("transitional type cannot be set after symbol type is already defined");
> > +	menu_set_type($2);
> > +	current_entry->sym->transitional = true;
> > +	printd(DEBUG_PARSE, "%s:%d:transitional(%u)\n", cur_filename, cur_lineno,
> > +		$2);
> > +};
> 
> You could also consider making this an attribute similar to the
> "modules" flags and simplify:
> 
> config_option: T_TRANSITIONAL T_EOL
> {
>        current_entry->sym->transitional = true;
>        printd(DEBUG_PARSE, "%s:%d:transitional\n", cur_filename,
> cur_lineno);
> };
> 
> ...it would mean the config options look this way:
> 
> config OLD_OPTION
>     bool
>     transitional
> 
> (If not, menu_set_type() does already contain a check for whether the
> type has already been set.)

I went back and forth on how I wanted it to look and ultimately decided
it was awkward to say "use transitional but only with a type that
doesn't have a prompt". Instead it seemed better to have the type
explicitly set.

menu_set_type() does check already, but it's a warning only.

> > @@ -558,6 +606,9 @@ void conf_parse(const char *name)
> >   		if (menu->sym && sym_check_deps(menu->sym))
> >   			yynerrs++;
> > +		if (transitional_check_sanity(menu))
> > +			yynerrs++;
> > +
> >   		if (menu->sym && sym_is_choice(menu->sym)) {
> >   			menu_for_each_sub_entry(child, menu)
> >   				if (child->sym && choice_check_sanity(child))
> > diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> > index 26ab10c0fd76..b822c0c897e5 100644
> > --- a/scripts/kconfig/symbol.c
> > +++ b/scripts/kconfig/symbol.c
> > @@ -447,6 +447,9 @@ void sym_calc_value(struct symbol *sym)
> >   	if (sym->visible != no)
> >   		sym->flags |= SYMBOL_WRITE;
> > +	/* Calculate usable flag */
> > +	sym->usable = (sym->visible != no || sym->transitional);
> > +
> 
> Is this actually ever used outside of this function? (IOW could this
> just be a local variable instead of a sym-> flag/member?) Or do we need
> to set it here because sym_calc_value() calls itself recursively? To me
> it looks like we only ever access sym->usable for the "sym" that was
> passed as an argument to the function.

Ah! It's not any more, no. I had an earlier version where I was
examining it elsewhere, but yeah, this is only needed here.

> 
> >   	/* set default if recursively called */
> >   	sym->curr = newval;
> > @@ -459,13 +462,15 @@ void sym_calc_value(struct symbol *sym)
> >   			sym_calc_choice(choice_menu);
> >   			newval.tri = sym->curr.tri;
> >   		} else {
> > -			if (sym->visible != no) {
> > +			if (sym->usable) {
> >   				/* if the symbol is visible use the user value
> >   				 * if available, otherwise try the default value
> >   				 */
> >   				if (sym_has_value(sym)) {
> > +					tristate value = sym->transitional ?
> > +						sym->def[S_DEF_USER].tri : sym->visible;
> >   					newval.tri = EXPR_AND(sym->def[S_DEF_USER].tri,
> > -							      sym->visible);
> > +							      value);
> 
> This looks a bit odd to me. Just thinking out loud: your new logic is
> there to be able to use a value even though it's not visible. In the
> case where it's transitional you use the .config value instead of the
> condition that makes it visible.
> 
> Could you simply change sym_calc_visibility() instead to always return
> 'yes' when the symbol is transitional? Wouldn't that simplify everything
> in sym_calc_value()?

It's a tristate, so "m" is also possible besides "y". (sym->visible is
also a tristate. :)

I will send a v3 with better bit fields.

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists