[PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined

[PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined

[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

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

[Ulf Magnusson]

More directly describes the only thing it does.

Signed-off-by: Ulf Magnusson <ulfalizer@gmail.com>
---
 scripts/kconfig/menu.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --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

[PATCH 2/3] kconfig: Clarify expression rewriting

[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>
---
 scripts/kconfig/menu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --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

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

[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>
---
 scripts/kconfig/menu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --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

Re: [PATCH 0/3] kconfig: Make 'm' safe before modules symbol is defined

[Masahiro Yamada]

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