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
