kconfig: Make 'm' safe before modules symbol is defined

PATCH In review Ulf Magnusson 2 participants 5 messages Oct 5, 2017 – Dec 14, 2017

Subsystems (2)

Kbuild Kconfig

lore

Share & tools

lore

Ulf Magnusson

author

2017-10-05 12:02 UTC #0 lore raw

cover kconfig: Make 'm' safe before modules symbol is defined

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(-)

Signature · 1 line

2.7.4

Ulf Magnusson

author

2017-10-05 12:02 UTC #1 lore raw

PATCH 1/3 kconfig: Rename menu_check_dep() to rewrite_m()

More directly describes the only thing it does.

Signed-off-by Ulf Magnusson <[email protected]>

---
 scripts/kconfig/menu.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

scripts/kconfig/menu.c +10 -6

index e935793..8354dfa 100644
@@ -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)) {

Signature · 1 line

2.7.4

Ulf Magnusson

author

2017-10-05 12:02 UTC #2 lore raw

PATCH 2/3 kconfig: Clarify expression rewriting

In reply to 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 <[email protected]>

---
 scripts/kconfig/menu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

scripts/kconfig/menu.c +52 -0

index 8354dfa..94f192de 100644
@@ -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) {

Signature · 1 line

2.7.4

Ulf Magnusson

author

2017-10-05 12:02 UTC #3 lore raw

PATCH 3/3 kconfig: Clean up modules handling and fix crash

In reply to 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 <[email protected]>

---
 scripts/kconfig/menu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

scripts/kconfig/menu.c +6 -4

index 94f192de..a7396e4 100644
@@ -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)

Signature · 1 line

2.7.4

Masahiro Yamada

2017-12-14 23:28 UTC #4 lore raw

In reply to Ulf Magnusson

2017-10-05 21:01 GMT+09:00 Ulf Magnusson <[email protected]>:

Quoted 18 lines

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

Quoted 8 lines

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!

Signature · 2 lines

Best Regards
Masahiro Yamada