* [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined
@ 2017-10-05 12:01 Ulf Magnusson
2017-10-05 12:01 ` [PATCH 1/3] kconfig: Rename menu_check_dep() to rewrite_m() Ulf Magnusson
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Ulf Magnusson @ 2017-10-05 12:01 UTC (permalink / raw)
To: yann.morin.1998, linux-kbuild
Cc: sam, zippel, nicolas.pitre, mmarek, dirk, yamada.masahiro,
lacombar, JBeulich, linux-kernel, Ulf Magnusson
Hello,
This patchset fixes a segfault that occurs if 'm' appears in certain
expressions before the modules symbol is defined. The problem is that m is
rewritten to m && <modules symbol> already during parsing. Doing it in
menu_finalize(), which runs after parsing, fixes the problem.
To aid the review and people trying to understand menu_finalize() in the future
(including me), it also renames menu_check_dep() to rewrite_m() and adds
comments to clarify the existing expression rewriting and dependency
propagation logic.
The changes have been tested for regressions using zconfdump(), Kconfiglib, and
Valgrind.
Cheers,
Ulf
Ulf Magnusson (3):
kconfig: Rename menu_check_dep() to rewrite_m()
kconfig: Clarify expression rewriting
kconfig: Clean up modules handling and fix crash
scripts/kconfig/menu.c | 74 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 66 insertions(+), 8 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/3] kconfig: Rename menu_check_dep() to rewrite_m()
2017-10-05 12:01 [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Ulf Magnusson
@ 2017-10-05 12:01 ` Ulf Magnusson
2017-10-05 12:01 ` [PATCH 2/3] kconfig: Clarify expression rewriting Ulf Magnusson
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Ulf Magnusson @ 2017-10-05 12:01 UTC (permalink / raw)
To: yann.morin.1998, linux-kbuild
Cc: sam, zippel, nicolas.pitre, mmarek, dirk, yamada.masahiro,
lacombar, JBeulich, linux-kernel, Ulf Magnusson
More directly describes the only thing it does.
Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
---
| 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
--git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index e935793..8354dfa 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -79,19 +79,23 @@ void menu_end_menu(void)
current_menu = current_menu->parent;
}
-static struct expr *menu_check_dep(struct expr *e)
+/*
+ * Rewrites 'm' to 'm' && MODULES, so that it evaluates to 'n' when running
+ * without modules
+ */
+static struct expr *rewrite_m(struct expr *e)
{
if (!e)
return e;
switch (e->type) {
case E_NOT:
- e->left.expr = menu_check_dep(e->left.expr);
+ e->left.expr = rewrite_m(e->left.expr);
break;
case E_OR:
case E_AND:
- e->left.expr = menu_check_dep(e->left.expr);
- e->right.expr = menu_check_dep(e->right.expr);
+ e->left.expr = rewrite_m(e->left.expr);
+ e->right.expr = rewrite_m(e->right.expr);
break;
case E_SYMBOL:
/* change 'm' into 'm' && MODULES */
@@ -106,7 +110,7 @@ static struct expr *menu_check_dep(struct expr *e)
void menu_add_dep(struct expr *dep)
{
- current_entry->dep = expr_alloc_and(current_entry->dep, menu_check_dep(dep));
+ current_entry->dep = expr_alloc_and(current_entry->dep, rewrite_m(dep));
}
void menu_set_type(int type)
@@ -131,7 +135,7 @@ static struct property *menu_add_prop(enum prop_type type, char *prompt, struct
prop->menu = current_entry;
prop->expr = expr;
- prop->visible.expr = menu_check_dep(dep);
+ prop->visible.expr = rewrite_m(dep);
if (prompt) {
if (isspace(*prompt)) {
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] kconfig: Clarify expression rewriting
2017-10-05 12:01 [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Ulf Magnusson
2017-10-05 12:01 ` [PATCH 1/3] kconfig: Rename menu_check_dep() to rewrite_m() Ulf Magnusson
@ 2017-10-05 12:01 ` Ulf Magnusson
2017-10-05 12:01 ` [PATCH 3/3] kconfig: Clean up modules handling and fix crash Ulf Magnusson
2017-12-14 23:27 ` [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Masahiro Yamada
3 siblings, 0 replies; 5+ messages in thread
From: Ulf Magnusson @ 2017-10-05 12:01 UTC (permalink / raw)
To: yann.morin.1998, linux-kbuild
Cc: sam, zippel, nicolas.pitre, mmarek, dirk, yamada.masahiro,
lacombar, JBeulich, linux-kernel, Ulf Magnusson
menu_finalize() is one of the more opaque parts of Kconfig, and I need
to make some changes to it to fix an issue related to modules. Add some
comments related to expression rewriting and dependency propagation as a
review aid. They will also help other people trying to understand the
code.
Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
---
| 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
--git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 8354dfa..94f192de 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -296,6 +296,11 @@ void menu_finalize(struct menu *parent)
sym = parent->sym;
if (parent->list) {
+ /*
+ * This menu node has children. We (recursively) process them
+ * and propagate parent dependencies before moving on.
+ */
+
if (sym && sym_is_choice(sym)) {
if (sym->type == S_UNKNOWN) {
/* find the first choice value to find out choice type */
@@ -319,24 +324,66 @@ void menu_finalize(struct menu *parent)
else
parentdep = parent->dep;
+ /* For each child menu node... */
for (menu = parent->list; menu; menu = menu->next) {
+ /*
+ * Propagate parent dependencies to the child menu
+ * node, also rewriting and simplifying expressions
+ */
basedep = expr_transform(menu->dep);
basedep = expr_alloc_and(expr_copy(parentdep), basedep);
basedep = expr_eliminate_dups(basedep);
menu->dep = basedep;
+
if (menu->sym)
+ /*
+ * Note: For symbols, all prompts are included
+ * too in the symbol's own property list
+ */
prop = menu->sym->prop;
else
+ /*
+ * For non-symbol menu nodes, we just need to
+ * handle the prompt
+ */
prop = menu->prompt;
+
+ /* For each property... */
for (; prop; prop = prop->next) {
if (prop->menu != menu)
+ /*
+ * Two possibilities:
+ *
+ * 1. The property lacks dependencies
+ * and so isn't location-specific,
+ * e.g. an 'option'
+ *
+ * 2. The property belongs to a symbol
+ * defined in multiple locations and
+ * is from some other location. It
+ * will be handled there in that
+ * case.
+ *
+ * Skip the property.
+ */
continue;
+
+ /*
+ * Propagate parent dependencies to the
+ * property's condition, rewriting and
+ * simplifying expressions at the same time
+ */
dep = expr_transform(prop->visible.expr);
dep = expr_alloc_and(expr_copy(basedep), dep);
dep = expr_eliminate_dups(dep);
if (menu->sym && menu->sym->type != S_TRISTATE)
dep = expr_trans_bool(dep);
prop->visible.expr = dep;
+
+ /*
+ * Handle selects and implies, which modify the
+ * dependencies of the selected/implied symbol
+ */
if (prop->type == P_SELECT) {
struct symbol *es = prop_get_symbol(prop);
es->rev_dep.expr = expr_alloc_or(es->rev_dep.expr,
@@ -348,6 +395,11 @@ void menu_finalize(struct menu *parent)
}
}
}
+
+ /*
+ * Recursively process children in the same fashion before
+ * moving on
+ */
for (menu = parent->list; menu; menu = menu->next)
menu_finalize(menu);
} else if (sym) {
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] kconfig: Clean up modules handling and fix crash
2017-10-05 12:01 [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Ulf Magnusson
2017-10-05 12:01 ` [PATCH 1/3] kconfig: Rename menu_check_dep() to rewrite_m() Ulf Magnusson
2017-10-05 12:01 ` [PATCH 2/3] kconfig: Clarify expression rewriting Ulf Magnusson
@ 2017-10-05 12:01 ` Ulf Magnusson
2017-12-14 23:27 ` [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Masahiro Yamada
3 siblings, 0 replies; 5+ messages in thread
From: Ulf Magnusson @ 2017-10-05 12:01 UTC (permalink / raw)
To: yann.morin.1998, linux-kbuild
Cc: sam, zippel, nicolas.pitre, mmarek, dirk, yamada.masahiro,
lacombar, JBeulich, linux-kernel, Ulf Magnusson
Kconfig currently doesn't handle 'm' appearing in a Kconfig file before
the modules symbol is defined (the symbol with 'option modules'). The
problem is the following code, which runs during parsing:
/* change 'm' into 'm' && MODULES */
if (e->left.sym == &symbol_mod)
return expr_alloc_and(e, expr_alloc_symbol(modules_sym));
If the modules symbol has not yet been defined, modules_sym is NULL,
giving an invalid expression.
Here is a test file where both BEFORE_1 and BEFORE_2 trigger a segfault.
If the modules symbol is removed, all symbols trigger segfaults.
config BEFORE_1
def_tristate y if m
if m
config BEFORE_2
def_tristate y
endif
config MODULES
def_bool y
option modules
config AFTER_1
def_tristate y if m
if m
config AFTER_2
def_tristate y
endif
Fix the issue by rewriting 'm' in menu_finalize() instead. This function
runs after parsing and is the proper place to do it. The following
existing code in conf_parse() in zconf.y ensures that the modules symbol
exists at that point:
if (!modules_sym)
modules_sym = sym_find( "n" );
...
menu_finalize(&rootmenu);
The following tests were done to ensure no functional changes for
configurations that don't reference 'm' before the modules symbol:
- zconfdump(stdout) was run with ARCH=x86 and ARCH=arm before
and after the change and verified to produce identical output.
This function prints all symbols, choices, and menus together
with their properties and their dependency expressions. A
rewritten 'm' appears as 'm && MODULES'.
A small annoyance is that the assert(len != 0) in xfwrite()
needs to be disabled in order to use zconfdump(), because it
chokes on e.g. 'default ""'.
- The Kconfiglib test suite was run to indirectly verify that
alldefconfig, allyesconfig, allnoconfig, and all defconfigs in
the kernel still generate the same final .config.
- Valgrind was used to check for memory errors and (new) memory
leaks.
Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
---
| 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
--git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 94f192de..a7396e4 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -110,7 +110,7 @@ static struct expr *rewrite_m(struct expr *e)
void menu_add_dep(struct expr *dep)
{
- current_entry->dep = expr_alloc_and(current_entry->dep, rewrite_m(dep));
+ current_entry->dep = expr_alloc_and(current_entry->dep, dep);
}
void menu_set_type(int type)
@@ -135,7 +135,7 @@ static struct property *menu_add_prop(enum prop_type type, char *prompt, struct
prop->menu = current_entry;
prop->expr = expr;
- prop->visible.expr = rewrite_m(dep);
+ prop->visible.expr = dep;
if (prompt) {
if (isspace(*prompt)) {
@@ -330,7 +330,8 @@ void menu_finalize(struct menu *parent)
* Propagate parent dependencies to the child menu
* node, also rewriting and simplifying expressions
*/
- basedep = expr_transform(menu->dep);
+ basedep = rewrite_m(menu->dep);
+ basedep = expr_transform(basedep);
basedep = expr_alloc_and(expr_copy(parentdep), basedep);
basedep = expr_eliminate_dups(basedep);
menu->dep = basedep;
@@ -373,7 +374,8 @@ void menu_finalize(struct menu *parent)
* property's condition, rewriting and
* simplifying expressions at the same time
*/
- dep = expr_transform(prop->visible.expr);
+ dep = rewrite_m(prop->visible.expr);
+ dep = expr_transform(dep);
dep = expr_alloc_and(expr_copy(basedep), dep);
dep = expr_eliminate_dups(dep);
if (menu->sym && menu->sym->type != S_TRISTATE)
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined
2017-10-05 12:01 [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Ulf Magnusson
` (2 preceding siblings ...)
2017-10-05 12:01 ` [PATCH 3/3] kconfig: Clean up modules handling and fix crash Ulf Magnusson
@ 2017-12-14 23:27 ` Masahiro Yamada
3 siblings, 0 replies; 5+ messages in thread
From: Masahiro Yamada @ 2017-12-14 23:27 UTC (permalink / raw)
To: Ulf Magnusson
Cc: Yann E. MORIN, Linux Kbuild mailing list, Sam Ravnborg, zippel,
Nicolas Pitre, Michal Marek, dirk, Arnaud Lacombe, Jan Beulich,
Linux Kernel Mailing List
2017-10-05 21:01 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> Hello,
>
> This patchset fixes a segfault that occurs if 'm' appears in certain
> expressions before the modules symbol is defined. The problem is that m is
> rewritten to m && <modules symbol> already during parsing. Doing it in
> menu_finalize(), which runs after parsing, fixes the problem.
>
> To aid the review and people trying to understand menu_finalize() in the future
> (including me), it also renames menu_check_dep() to rewrite_m() and adds
> comments to clarify the existing expression rewriting and dependency
> propagation logic.
>
> The changes have been tested for regressions using zconfdump(), Kconfiglib, and
> Valgrind.
>
> Cheers,
> Ulf
>
> Ulf Magnusson (3):
> kconfig: Rename menu_check_dep() to rewrite_m()
> kconfig: Clarify expression rewriting
> kconfig: Clean up modules handling and fix crash
>
> scripts/kconfig/menu.c | 74 ++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 66 insertions(+), 8 deletions(-)
>
Series, applied to linux-kbuild/kconfig. Thanks!
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-14 23:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-05 12:01 [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Ulf Magnusson
2017-10-05 12:01 ` [PATCH 1/3] kconfig: Rename menu_check_dep() to rewrite_m() Ulf Magnusson
2017-10-05 12:01 ` [PATCH 2/3] kconfig: Clarify expression rewriting Ulf Magnusson
2017-10-05 12:01 ` [PATCH 3/3] kconfig: Clean up modules handling and fix crash Ulf Magnusson
2017-12-14 23:27 ` [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined Masahiro Yamada
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).