[RFC,4/7] kconfig: support new special property shell=

Commit Message

Masahiro Yamada Feb. 8, 2018, 4:19 p.m. UTC

This works with bool, int, hex, string types.

For bool, the symbol is set to 'y' or 'n' depending on the exit value
of the command.

For int, hex, string, the symbol is set to the value to the stdout
of the command. (only the first line of the stdout)

The following shows how to write this and how it works.

--------------------(example Kconfig)------------------
config srctree
        string
        option env="srctree"

config CC
        string
        option env="CC"

config CC_HAS_STACKPROTECTOR
        bool
        option shell="$CC -Werror -fstack-protector -c -x c /dev/null"

config CC_HAS_STACKPROTECTOR_STRONG
        bool
        option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"

config CC_VERSION
        int
        option shell="$srctree/scripts/gcc-version.sh $CC | sed 's/^0*//'"
        help
          gcc-version.sh returns 4 digits number. Unfortunately, the preceding
          zero would cause 'number is invalid'.  Cut it off.

config CC_IS_CLANG
        bool
        option shell="$CC --version | grep -q clang"

config CC_IS_GCC
        bool
        option shell="$CC --version | grep -q gcc"
-----------------------------------------------------

  $ make alldefconfig
  scripts/kconfig/conf  --alldefconfig Kconfig
  #
  # configuration written to .config
  #
  $ cat .config
  #
  # Automatically generated file; DO NOT EDIT.
  # Linux Kernel Configuration
  #
  CONFIG_CC_HAS_STACKPROTECTOR=y
  CONFIG_CC_HAS_STACKPROTECTOR_STRONG=y
  CONFIG_CC_VERSION=504
  # CONFIG_CC_IS_CLANG is not set
  CONFIG_CC_IS_GCC=y

Suggested-by: Linus Torvalds <[e-mail]>
Signed-off-by: Masahiro Yamada <[e-mail]>
---

 scripts/kconfig/expr.h     |  1 +
 scripts/kconfig/kconf_id.c |  1 +
 scripts/kconfig/lkc.h      |  1 +
 scripts/kconfig/menu.c     |  3 ++
 scripts/kconfig/symbol.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 80 insertions(+)

Comments

Ulf Magnusson Feb. 9, 2018, 5:30 a.m. UTC | #1

On Fri, Feb 09, 2018 at 01:19:09AM +0900, Masahiro Yamada wrote:
> This works with bool, int, hex, string types.
> 
> For bool, the symbol is set to 'y' or 'n' depending on the exit value
> of the command.
> 
> For int, hex, string, the symbol is set to the value to the stdout
> of the command. (only the first line of the stdout)
> 
> The following shows how to write this and how it works.
> 
> --------------------(example Kconfig)------------------
> config srctree
>         string
>         option env="srctree"
> 
> config CC
>         string
>         option env="CC"
> 
> config CC_HAS_STACKPROTECTOR
>         bool
>         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> 
> config CC_HAS_STACKPROTECTOR_STRONG
>         bool
>         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> 
> config CC_VERSION
>         int
>         option shell="$srctree/scripts/gcc-version.sh $CC | sed 's/^0*//'"
>         help
>           gcc-version.sh returns 4 digits number. Unfortunately, the preceding
>           zero would cause 'number is invalid'.  Cut it off.
> 
> config CC_IS_CLANG
>         bool
>         option shell="$CC --version | grep -q clang"
> 
> config CC_IS_GCC
>         bool
>         option shell="$CC --version | grep -q gcc"
> -----------------------------------------------------
> 
>   $ make alldefconfig
>   scripts/kconfig/conf  --alldefconfig Kconfig
>   #
>   # configuration written to .config
>   #
>   $ cat .config
>   #
>   # Automatically generated file; DO NOT EDIT.
>   # Linux Kernel Configuration
>   #
>   CONFIG_CC_HAS_STACKPROTECTOR=y
>   CONFIG_CC_HAS_STACKPROTECTOR_STRONG=y
>   CONFIG_CC_VERSION=504
>   # CONFIG_CC_IS_CLANG is not set
>   CONFIG_CC_IS_GCC=y
> 
> Suggested-by: Linus Torvalds <[e-mail]>
> Signed-off-by: Masahiro Yamada <[e-mail]>

I know this is just an RFC/incomplete, but in case it's helpful:

> ---
> 
>  scripts/kconfig/expr.h     |  1 +
>  scripts/kconfig/kconf_id.c |  1 +
>  scripts/kconfig/lkc.h      |  1 +
>  scripts/kconfig/menu.c     |  3 ++
>  scripts/kconfig/symbol.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 80 insertions(+)
> 
> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> index c16e82e..83029f92 100644
> --- a/scripts/kconfig/expr.h
> +++ b/scripts/kconfig/expr.h
> @@ -183,6 +183,7 @@ enum prop_type {
>  	P_IMPLY,    /* imply BAR */
>  	P_RANGE,    /* range 7..100 (for a symbol) */
>  	P_ENV,      /* value from environment variable */
> +	P_SHELL,    /* shell command */
>  	P_SYMBOL,   /* where a symbol is defined */
>  };
>  
> diff --git a/scripts/kconfig/kconf_id.c b/scripts/kconfig/kconf_id.c
> index 3ea9c5f..0db9d1c 100644
> --- a/scripts/kconfig/kconf_id.c
> +++ b/scripts/kconfig/kconf_id.c
> @@ -34,6 +34,7 @@ static struct kconf_id kconf_id_array[] = {
>  	{ "defconfig_list",	T_OPT_DEFCONFIG_LIST,	TF_OPTION },
>  	{ "env",		T_OPT_ENV,		TF_OPTION },
>  	{ "allnoconfig_y",	T_OPT_ALLNOCONFIG_Y,	TF_OPTION },
> +	{ "shell",		T_OPT_SHELL,		TF_OPTION },
>  };
>  
>  #define KCONF_ID_ARRAY_SIZE (sizeof(kconf_id_array)/sizeof(struct kconf_id))
> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> index 4e23feb..8d05042 100644
> --- a/scripts/kconfig/lkc.h
> +++ b/scripts/kconfig/lkc.h
> @@ -60,6 +60,7 @@ enum conf_def_mode {
>  #define T_OPT_DEFCONFIG_LIST	2
>  #define T_OPT_ENV		3
>  #define T_OPT_ALLNOCONFIG_Y	4
> +#define T_OPT_SHELL		5
>  
>  struct kconf_id {
>  	const char *name;
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 9922285..6254dfb 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -216,6 +216,9 @@ void menu_add_option(int token, char *arg)
>  	case T_OPT_ENV:
>  		prop_add_env(arg);
>  		break;
> +	case T_OPT_SHELL:
> +		prop_add_shell(arg);
> +		break;
>  	case T_OPT_ALLNOCONFIG_Y:
>  		current_entry->sym->flags |= SYMBOL_ALLNOCONFIG_Y;
>  		break;
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 893eae6..02ac4f4 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <ctype.h>
> +#include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <regex.h>
> @@ -1370,6 +1371,8 @@ const char *prop_get_type_name(enum prop_type type)
>  		return "prompt";
>  	case P_ENV:
>  		return "env";
> +	case P_SHELL:
> +		return "shell";
>  	case P_COMMENT:
>  		return "comment";
>  	case P_MENU:
> @@ -1420,3 +1423,74 @@ static void prop_add_env(const char *env)
>  	else
>  		menu_warn(current_entry, "environment variable %s undefined", env);
>  }
> +
> +static void prop_add_shell(const char *cmd)
> +{
> +	struct symbol *sym, *sym2;
> +	struct property *prop;
> +	char *expanded_cmd;
> +	FILE *p;
> +	char stdout[256];

Maybe this could be called stdout_buf to avoid confusing it with the
stdio streams. Those are macros too, though glibc just does

	#define stdout stdout

> +	int ret, len;
> +
> +	sym = current_entry->sym;
> +	for_all_properties(sym, prop, P_SHELL) {
> +		sym2 = prop_get_symbol(prop);
> +		if (strcmp(sym2->name, cmd))
> +			menu_warn(current_entry, "redefining shell command symbol from %s",
> +				  sym2->name);
> +		return;
> +	}
> +
> +	prop = prop_alloc(P_SHELL, sym);
> +	prop->expr = expr_alloc_symbol(sym_lookup(cmd, SYMBOL_CONST));
> +
> +	expanded_cmd = sym_expand_string_value(cmd);
> +
> +	/* surround the command with ( ) in case it is piped commands */
> +	len = strlen(expanded_cmd);
> +	expanded_cmd = xrealloc(expanded_cmd, len + 3);
> +	memmove(expanded_cmd + 1, expanded_cmd, len);
> +	expanded_cmd[0] = '(';
> +	expanded_cmd[len + 1] = ')';
> +	expanded_cmd[len + 2] = 0;
> +
> +	switch (sym->type) {
> +	case S_BOOLEAN:
> +		/* suppress both stdout and stderr. */
> +		len = strlen(expanded_cmd) + strlen(" >/dev/null 2>&1") + 1;
> +		expanded_cmd = realloc(expanded_cmd, len);

Could use the new xrealloc().

> +		strcat(expanded_cmd, " >/dev/null 2>&1");
> +
> +		ret = system(expanded_cmd);
> +		sym_add_default(sym, ret == 0 ? "y" : "n");
> +		break;
> +	case S_INT:
> +	case S_HEX:
> +	case S_STRING:
> +		/* suppress only stderr. we want to get stdout. */
> +		len = strlen(expanded_cmd) + strlen(" 2>/dev/null") + 1;
> +		expanded_cmd = realloc(expanded_cmd, len);

Could use the new xrealloc().

> +		strcat(expanded_cmd, " 2>/dev/null");
> +
> +		p = popen(expanded_cmd, "r");

Should be pclose()d.

> +		if (!p)

Could warn.

Maybe it'd be helpful to warn if pclose() != 0 as well (non-zero exit
status or obscure error).

> +			goto free;
> +		if (fgets(stdout, sizeof(stdout), p)) {
> +			stdout[sizeof(stdout) - 1] = 0;

fgets() already guarantees null termination if any characters are read.

strncpy() is that evil one...

> +			len = strlen(stdout);
> +			if (stdout[len - 1] == '\n')
> +				stdout[len - 1] = 0;
> +		} else {
> +			stdout[0] = 0;
> +		}
> +		sym_add_default(sym, stdout);
> +		break;
> +	default:
> +		menu_warn(current_entry, "unsupported type for shell command\n");
> +		break;
> +	}
> +
> +free:
> +	free(expanded_cmd);
> +}
> -- 
> 2.7.4
> 

I think the general behavior makes sense for user-assignable
'option shell' symbols too (though I don't know if they'd ever get used in
practice):

	- The output of the shell command turns into a regular default on
	  user-assignable symbols. User values can override that default.

	- For savedefconfig, user-assignable symbols get written out only if
	  they have been changed from the default given by the shell
	  command. They will be recalculated when the defconfig is used if
	  they weren't changed.

	  Maybe there's some too-obscure-to-worry about cases there, but it
	  seems to work out pretty well.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 9, 2018, 9:19 a.m. UTC | #2

2018-02-09 14:30 GMT+09:00 Ulf Magnusson <[e-mail]>:
> On Fri, Feb 09, 2018 at 01:19:09AM +0900, Masahiro Yamada wrote:
>> This works with bool, int, hex, string types.
>>
>> For bool, the symbol is set to 'y' or 'n' depending on the exit value
>> of the command.
>>
>> For int, hex, string, the symbol is set to the value to the stdout
>> of the command. (only the first line of the stdout)
>>
>> The following shows how to write this and how it works.
>>
>> --------------------(example Kconfig)------------------
>> config srctree
>>         string
>>         option env="srctree"
>>
>> config CC
>>         string
>>         option env="CC"
>>
>> config CC_HAS_STACKPROTECTOR
>>         bool
>>         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
>>
>> config CC_HAS_STACKPROTECTOR_STRONG
>>         bool
>>         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
>>
>> config CC_VERSION
>>         int
>>         option shell="$srctree/scripts/gcc-version.sh $CC | sed 's/^0*//'"
>>         help
>>           gcc-version.sh returns 4 digits number. Unfortunately, the preceding
>>           zero would cause 'number is invalid'.  Cut it off.
>>
>> config CC_IS_CLANG
>>         bool
>>         option shell="$CC --version | grep -q clang"
>>
>> config CC_IS_GCC
>>         bool
>>         option shell="$CC --version | grep -q gcc"
>> -----------------------------------------------------
>>
>>   $ make alldefconfig
>>   scripts/kconfig/conf  --alldefconfig Kconfig
>>   #
>>   # configuration written to .config
>>   #
>>   $ cat .config
>>   #
>>   # Automatically generated file; DO NOT EDIT.
>>   # Linux Kernel Configuration
>>   #
>>   CONFIG_CC_HAS_STACKPROTECTOR=y
>>   CONFIG_CC_HAS_STACKPROTECTOR_STRONG=y
>>   CONFIG_CC_VERSION=504
>>   # CONFIG_CC_IS_CLANG is not set
>>   CONFIG_CC_IS_GCC=y
>>
>> Suggested-by: Linus Torvalds <[e-mail]>
>> Signed-off-by: Masahiro Yamada <[e-mail]>
>
> I know this is just an RFC/incomplete, but in case it's helpful:


Your comments are really helpful, as always!



>> ---
>>
>>  scripts/kconfig/expr.h     |  1 +
>>  scripts/kconfig/kconf_id.c |  1 +
>>  scripts/kconfig/lkc.h      |  1 +
>>  scripts/kconfig/menu.c     |  3 ++
>>  scripts/kconfig/symbol.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 80 insertions(+)
>>
>> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
>> index c16e82e..83029f92 100644
>> --- a/scripts/kconfig/expr.h
>> +++ b/scripts/kconfig/expr.h
>> @@ -183,6 +183,7 @@ enum prop_type {
>>       P_IMPLY,    /* imply BAR */
>>       P_RANGE,    /* range 7..100 (for a symbol) */
>>       P_ENV,      /* value from environment variable */
>> +     P_SHELL,    /* shell command */
>>       P_SYMBOL,   /* where a symbol is defined */
>>  };
>>
>> diff --git a/scripts/kconfig/kconf_id.c b/scripts/kconfig/kconf_id.c
>> index 3ea9c5f..0db9d1c 100644
>> --- a/scripts/kconfig/kconf_id.c
>> +++ b/scripts/kconfig/kconf_id.c
>> @@ -34,6 +34,7 @@ static struct kconf_id kconf_id_array[] = {
>>       { "defconfig_list",     T_OPT_DEFCONFIG_LIST,   TF_OPTION },
>>       { "env",                T_OPT_ENV,              TF_OPTION },
>>       { "allnoconfig_y",      T_OPT_ALLNOCONFIG_Y,    TF_OPTION },
>> +     { "shell",              T_OPT_SHELL,            TF_OPTION },
>>  };
>>
>>  #define KCONF_ID_ARRAY_SIZE (sizeof(kconf_id_array)/sizeof(struct kconf_id))
>> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
>> index 4e23feb..8d05042 100644
>> --- a/scripts/kconfig/lkc.h
>> +++ b/scripts/kconfig/lkc.h
>> @@ -60,6 +60,7 @@ enum conf_def_mode {
>>  #define T_OPT_DEFCONFIG_LIST 2
>>  #define T_OPT_ENV            3
>>  #define T_OPT_ALLNOCONFIG_Y  4
>> +#define T_OPT_SHELL          5
>>
>>  struct kconf_id {
>>       const char *name;
>> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
>> index 9922285..6254dfb 100644
>> --- a/scripts/kconfig/menu.c
>> +++ b/scripts/kconfig/menu.c
>> @@ -216,6 +216,9 @@ void menu_add_option(int token, char *arg)
>>       case T_OPT_ENV:
>>               prop_add_env(arg);
>>               break;
>> +     case T_OPT_SHELL:
>> +             prop_add_shell(arg);
>> +             break;
>>       case T_OPT_ALLNOCONFIG_Y:
>>               current_entry->sym->flags |= SYMBOL_ALLNOCONFIG_Y;
>>               break;
>> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
>> index 893eae6..02ac4f4 100644
>> --- a/scripts/kconfig/symbol.c
>> +++ b/scripts/kconfig/symbol.c
>> @@ -4,6 +4,7 @@
>>   */
>>
>>  #include <ctype.h>
>> +#include <stdio.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <regex.h>
>> @@ -1370,6 +1371,8 @@ const char *prop_get_type_name(enum prop_type type)
>>               return "prompt";
>>       case P_ENV:
>>               return "env";
>> +     case P_SHELL:
>> +             return "shell";
>>       case P_COMMENT:
>>               return "comment";
>>       case P_MENU:
>> @@ -1420,3 +1423,74 @@ static void prop_add_env(const char *env)
>>       else
>>               menu_warn(current_entry, "environment variable %s undefined", env);
>>  }
>> +
>> +static void prop_add_shell(const char *cmd)
>> +{
>> +     struct symbol *sym, *sym2;
>> +     struct property *prop;
>> +     char *expanded_cmd;
>> +     FILE *p;
>> +     char stdout[256];
>
> Maybe this could be called stdout_buf to avoid confusing it with the
> stdio streams. Those are macros too, though glibc just does
>
>         #define stdout stdout

Right.  This is confusing.  Will rename.


>> +     int ret, len;
>> +
>> +     sym = current_entry->sym;
>> +     for_all_properties(sym, prop, P_SHELL) {
>> +             sym2 = prop_get_symbol(prop);
>> +             if (strcmp(sym2->name, cmd))
>> +                     menu_warn(current_entry, "redefining shell command symbol from %s",
>> +                               sym2->name);
>> +             return;
>> +     }
>> +
>> +     prop = prop_alloc(P_SHELL, sym);
>> +     prop->expr = expr_alloc_symbol(sym_lookup(cmd, SYMBOL_CONST));
>> +
>> +     expanded_cmd = sym_expand_string_value(cmd);
>> +
>> +     /* surround the command with ( ) in case it is piped commands */
>> +     len = strlen(expanded_cmd);
>> +     expanded_cmd = xrealloc(expanded_cmd, len + 3);
>> +     memmove(expanded_cmd + 1, expanded_cmd, len);
>> +     expanded_cmd[0] = '(';
>> +     expanded_cmd[len + 1] = ')';
>> +     expanded_cmd[len + 2] = 0;
>> +
>> +     switch (sym->type) {
>> +     case S_BOOLEAN:
>> +             /* suppress both stdout and stderr. */
>> +             len = strlen(expanded_cmd) + strlen(" >/dev/null 2>&1") + 1;
>> +             expanded_cmd = realloc(expanded_cmd, len);
>
> Could use the new xrealloc().

Oops.  I added xrealloc() for this, but missed to use it here, somehow.

>> +             strcat(expanded_cmd, " >/dev/null 2>&1");
>> +
>> +             ret = system(expanded_cmd);
>> +             sym_add_default(sym, ret == 0 ? "y" : "n");
>> +             break;
>> +     case S_INT:
>> +     case S_HEX:
>> +     case S_STRING:
>> +             /* suppress only stderr. we want to get stdout. */
>> +             len = strlen(expanded_cmd) + strlen(" 2>/dev/null") + 1;
>> +             expanded_cmd = realloc(expanded_cmd, len);
>
> Could use the new xrealloc().
>
>> +             strcat(expanded_cmd, " 2>/dev/null");
>> +
>> +             p = popen(expanded_cmd, "r");
>
> Should be pclose()d.

Good catch!

>> +             if (!p)
>
> Could warn.
>
> Maybe it'd be helpful to warn if pclose() != 0 as well (non-zero exit
> status or obscure error).

Yes.


>> +                     goto free;
>> +             if (fgets(stdout, sizeof(stdout), p)) {
>> +                     stdout[sizeof(stdout) - 1] = 0;
>
> fgets() already guarantees null termination if any characters are read.
>
> strncpy() is that evil one...

Oh, I was misunderstanding the fgets() behavior.

You are right.  Will remove this unneeded termination.


>> +                     len = strlen(stdout);
>> +                     if (stdout[len - 1] == '\n')
>> +                             stdout[len - 1] = 0;
>> +             } else {
>> +                     stdout[0] = 0;
>> +             }
>> +             sym_add_default(sym, stdout);
>> +             break;
>> +     default:
>> +             menu_warn(current_entry, "unsupported type for shell command\n");
>> +             break;
>> +     }
>> +
>> +free:
>> +     free(expanded_cmd);
>> +}
>> --
>> 2.7.4
>>
>
> I think the general behavior makes sense for user-assignable
> 'option shell' symbols too (though I don't know if they'd ever get used in
> practice):
>
>         - The output of the shell command turns into a regular default on
>           user-assignable symbols. User values can override that default.
>
>         - For savedefconfig, user-assignable symbols get written out only if
>           they have been changed from the default given by the shell
>           command. They will be recalculated when the defconfig is used if
>           they weren't changed.
>
>           Maybe there's some too-obscure-to-worry about cases there, but it
>           seems to work out pretty well.
>

Observant comments.

Both "option env=" and "option shell="
are turned into 'default' property after all.

config FOO
        string
        option env="foo"

could be written


config FOO
        string
        default env="foo"

(syntax is not so important)


Having multiple defaults with visibility control could be useful.

config FOO
        string "foo"
        default env="foo1" if CASE1
        default env="foo2" if CASE2


shell= seems the same logic.

Ulf Magnusson Feb. 9, 2018, 12:46 p.m. UTC | #3

On Fri, Feb 09, 2018 at 06:19:19PM +0900, Masahiro Yamada wrote:
> 2018-02-09 14:30 GMT+09:00 Ulf Magnusson <[e-mail]>:
> > On Fri, Feb 09, 2018 at 01:19:09AM +0900, Masahiro Yamada wrote:
> >> This works with bool, int, hex, string types.
> >>
> >> For bool, the symbol is set to 'y' or 'n' depending on the exit value
> >> of the command.
> >>
> >> For int, hex, string, the symbol is set to the value to the stdout
> >> of the command. (only the first line of the stdout)
> >>
> >> The following shows how to write this and how it works.
> >>
> >> --------------------(example Kconfig)------------------
> >> config srctree
> >>         string
> >>         option env="srctree"
> >>
> >> config CC
> >>         string
> >>         option env="CC"
> >>
> >> config CC_HAS_STACKPROTECTOR
> >>         bool
> >>         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> >>
> >> config CC_HAS_STACKPROTECTOR_STRONG
> >>         bool
> >>         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> >>
> >> config CC_VERSION
> >>         int
> >>         option shell="$srctree/scripts/gcc-version.sh $CC | sed 's/^0*//'"
> >>         help
> >>           gcc-version.sh returns 4 digits number. Unfortunately, the preceding
> >>           zero would cause 'number is invalid'.  Cut it off.
> >>
> >> config CC_IS_CLANG
> >>         bool
> >>         option shell="$CC --version | grep -q clang"
> >>
> >> config CC_IS_GCC
> >>         bool
> >>         option shell="$CC --version | grep -q gcc"
> >> -----------------------------------------------------
> >>
> >>   $ make alldefconfig
> >>   scripts/kconfig/conf  --alldefconfig Kconfig
> >>   #
> >>   # configuration written to .config
> >>   #
> >>   $ cat .config
> >>   #
> >>   # Automatically generated file; DO NOT EDIT.
> >>   # Linux Kernel Configuration
> >>   #
> >>   CONFIG_CC_HAS_STACKPROTECTOR=y
> >>   CONFIG_CC_HAS_STACKPROTECTOR_STRONG=y
> >>   CONFIG_CC_VERSION=504
> >>   # CONFIG_CC_IS_CLANG is not set
> >>   CONFIG_CC_IS_GCC=y
> >>
> >> Suggested-by: Linus Torvalds <[e-mail]>
> >> Signed-off-by: Masahiro Yamada <[e-mail]>
> >
> > I know this is just an RFC/incomplete, but in case it's helpful:
> 
> 
> Your comments are really helpful, as always!
> 
> 
> 
> >> ---
> >>
> >>  scripts/kconfig/expr.h     |  1 +
> >>  scripts/kconfig/kconf_id.c |  1 +
> >>  scripts/kconfig/lkc.h      |  1 +
> >>  scripts/kconfig/menu.c     |  3 ++
> >>  scripts/kconfig/symbol.c   | 74 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  5 files changed, 80 insertions(+)
> >>
> >> diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
> >> index c16e82e..83029f92 100644
> >> --- a/scripts/kconfig/expr.h
> >> +++ b/scripts/kconfig/expr.h
> >> @@ -183,6 +183,7 @@ enum prop_type {
> >>       P_IMPLY,    /* imply BAR */
> >>       P_RANGE,    /* range 7..100 (for a symbol) */
> >>       P_ENV,      /* value from environment variable */
> >> +     P_SHELL,    /* shell command */
> >>       P_SYMBOL,   /* where a symbol is defined */
> >>  };
> >>
> >> diff --git a/scripts/kconfig/kconf_id.c b/scripts/kconfig/kconf_id.c
> >> index 3ea9c5f..0db9d1c 100644
> >> --- a/scripts/kconfig/kconf_id.c
> >> +++ b/scripts/kconfig/kconf_id.c
> >> @@ -34,6 +34,7 @@ static struct kconf_id kconf_id_array[] = {
> >>       { "defconfig_list",     T_OPT_DEFCONFIG_LIST,   TF_OPTION },
> >>       { "env",                T_OPT_ENV,              TF_OPTION },
> >>       { "allnoconfig_y",      T_OPT_ALLNOCONFIG_Y,    TF_OPTION },
> >> +     { "shell",              T_OPT_SHELL,            TF_OPTION },
> >>  };
> >>
> >>  #define KCONF_ID_ARRAY_SIZE (sizeof(kconf_id_array)/sizeof(struct kconf_id))
> >> diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
> >> index 4e23feb..8d05042 100644
> >> --- a/scripts/kconfig/lkc.h
> >> +++ b/scripts/kconfig/lkc.h
> >> @@ -60,6 +60,7 @@ enum conf_def_mode {
> >>  #define T_OPT_DEFCONFIG_LIST 2
> >>  #define T_OPT_ENV            3
> >>  #define T_OPT_ALLNOCONFIG_Y  4
> >> +#define T_OPT_SHELL          5
> >>
> >>  struct kconf_id {
> >>       const char *name;
> >> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> >> index 9922285..6254dfb 100644
> >> --- a/scripts/kconfig/menu.c
> >> +++ b/scripts/kconfig/menu.c
> >> @@ -216,6 +216,9 @@ void menu_add_option(int token, char *arg)
> >>       case T_OPT_ENV:
> >>               prop_add_env(arg);
> >>               break;
> >> +     case T_OPT_SHELL:
> >> +             prop_add_shell(arg);
> >> +             break;
> >>       case T_OPT_ALLNOCONFIG_Y:
> >>               current_entry->sym->flags |= SYMBOL_ALLNOCONFIG_Y;
> >>               break;
> >> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> >> index 893eae6..02ac4f4 100644
> >> --- a/scripts/kconfig/symbol.c
> >> +++ b/scripts/kconfig/symbol.c
> >> @@ -4,6 +4,7 @@
> >>   */
> >>
> >>  #include <ctype.h>
> >> +#include <stdio.h>
> >>  #include <stdlib.h>
> >>  #include <string.h>
> >>  #include <regex.h>
> >> @@ -1370,6 +1371,8 @@ const char *prop_get_type_name(enum prop_type type)
> >>               return "prompt";
> >>       case P_ENV:
> >>               return "env";
> >> +     case P_SHELL:
> >> +             return "shell";
> >>       case P_COMMENT:
> >>               return "comment";
> >>       case P_MENU:
> >> @@ -1420,3 +1423,74 @@ static void prop_add_env(const char *env)
> >>       else
> >>               menu_warn(current_entry, "environment variable %s undefined", env);
> >>  }
> >> +
> >> +static void prop_add_shell(const char *cmd)
> >> +{
> >> +     struct symbol *sym, *sym2;
> >> +     struct property *prop;
> >> +     char *expanded_cmd;
> >> +     FILE *p;
> >> +     char stdout[256];
> >
> > Maybe this could be called stdout_buf to avoid confusing it with the
> > stdio streams. Those are macros too, though glibc just does
> >
> >         #define stdout stdout
> 
> Right.  This is confusing.  Will rename.
> 
> 
> >> +     int ret, len;
> >> +
> >> +     sym = current_entry->sym;
> >> +     for_all_properties(sym, prop, P_SHELL) {
> >> +             sym2 = prop_get_symbol(prop);
> >> +             if (strcmp(sym2->name, cmd))
> >> +                     menu_warn(current_entry, "redefining shell command symbol from %s",
> >> +                               sym2->name);
> >> +             return;
> >> +     }
> >> +
> >> +     prop = prop_alloc(P_SHELL, sym);
> >> +     prop->expr = expr_alloc_symbol(sym_lookup(cmd, SYMBOL_CONST));
> >> +
> >> +     expanded_cmd = sym_expand_string_value(cmd);
> >> +
> >> +     /* surround the command with ( ) in case it is piped commands */
> >> +     len = strlen(expanded_cmd);
> >> +     expanded_cmd = xrealloc(expanded_cmd, len + 3);
> >> +     memmove(expanded_cmd + 1, expanded_cmd, len);
> >> +     expanded_cmd[0] = '(';
> >> +     expanded_cmd[len + 1] = ')';
> >> +     expanded_cmd[len + 2] = 0;
> >> +
> >> +     switch (sym->type) {
> >> +     case S_BOOLEAN:
> >> +             /* suppress both stdout and stderr. */
> >> +             len = strlen(expanded_cmd) + strlen(" >/dev/null 2>&1") + 1;
> >> +             expanded_cmd = realloc(expanded_cmd, len);
> >
> > Could use the new xrealloc().
> 
> Oops.  I added xrealloc() for this, but missed to use it here, somehow.
> 
> >> +             strcat(expanded_cmd, " >/dev/null 2>&1");
> >> +
> >> +             ret = system(expanded_cmd);
> >> +             sym_add_default(sym, ret == 0 ? "y" : "n");
> >> +             break;
> >> +     case S_INT:
> >> +     case S_HEX:
> >> +     case S_STRING:
> >> +             /* suppress only stderr. we want to get stdout. */
> >> +             len = strlen(expanded_cmd) + strlen(" 2>/dev/null") + 1;
> >> +             expanded_cmd = realloc(expanded_cmd, len);
> >
> > Could use the new xrealloc().
> >
> >> +             strcat(expanded_cmd, " 2>/dev/null");
> >> +
> >> +             p = popen(expanded_cmd, "r");
> >
> > Should be pclose()d.
> 
> Good catch!
> 
> >> +             if (!p)
> >
> > Could warn.
> >
> > Maybe it'd be helpful to warn if pclose() != 0 as well (non-zero exit
> > status or obscure error).
> 
> Yes.
> 
> 
> >> +                     goto free;
> >> +             if (fgets(stdout, sizeof(stdout), p)) {
> >> +                     stdout[sizeof(stdout) - 1] = 0;
> >
> > fgets() already guarantees null termination if any characters are read.
> >
> > strncpy() is that evil one...
> 
> Oh, I was misunderstanding the fgets() behavior.
> 
> You are right.  Will remove this unneeded termination.
> 
> 
> >> +                     len = strlen(stdout);
> >> +                     if (stdout[len - 1] == '\n')
> >> +                             stdout[len - 1] = 0;
> >> +             } else {
> >> +                     stdout[0] = 0;
> >> +             }
> >> +             sym_add_default(sym, stdout);
> >> +             break;
> >> +     default:
> >> +             menu_warn(current_entry, "unsupported type for shell command\n");
> >> +             break;
> >> +     }
> >> +
> >> +free:
> >> +     free(expanded_cmd);
> >> +}
> >> --
> >> 2.7.4
> >>
> >
> > I think the general behavior makes sense for user-assignable
> > 'option shell' symbols too (though I don't know if they'd ever get used in
> > practice):
> >
> >         - The output of the shell command turns into a regular default on
> >           user-assignable symbols. User values can override that default.
> >
> >         - For savedefconfig, user-assignable symbols get written out only if
> >           they have been changed from the default given by the shell
> >           command. They will be recalculated when the defconfig is used if
> >           they weren't changed.
> >
> >           Maybe there's some too-obscure-to-worry about cases there, but it
> >           seems to work out pretty well.
> >
> 
> Observant comments.
> 
> Both "option env=" and "option shell="
> are turned into 'default' property after all.

Yep, took me a while before I discovered that for 'option env'. The
internal UNAME_RELEASE variable is implemented the same way.

> 
> config FOO
>         string
>         option env="foo"
> 
> could be written
> 
> 
> config FOO
>         string
>         default env="foo"
> 
> (syntax is not so important)
> 
> 
> Having multiple defaults with visibility control could be useful.
> 
> config FOO
>         string "foo"
>         default env="foo1" if CASE1
>         default env="foo2" if CASE2
> 
> 
> shell= seems the same logic.

A simpler (and naturally unambiguous) syntax would be possible with a
few more keywords too:

	config FOO
		string "foo"
		default_env "FOO" if CASE1
		default_env "BAR" if CASE2
		default_shell "foo --bar" if CASE3
		default "otherwise"

Speaking of syntax nits, the whole 'option' construct feels pointless.
'option allnoconfig_y' could just be 'allnoconfig_y', etc., the way the
'optional' keyword works for choices.


There's a subtlety re. 'option env' btw: Those symbols never get written
out to .config files or headers, because SYMBOL_AUTO is set on them.

I don't think it would actually hurt to write them out:

	- If the symbol is not user-assignable, then the user value in
	  the .config file will be ignored when the .config file is read
	  back in, and the symbol will always get its value from the
	  environment.
	
	- If the symbol is user-assignable, then it makes sense to
	  remember and respect the user value, if any, since that was
	  the point of making the symbol user-assignable.

For the kernel, writing out 'option env' symbols would mean that ARCH,
SRCARCH, and KERNELVERSION would show up in .config files and autoconf.h
files. I'm not familiar enough with the make parts of Kbuild to know
what the effects of that might be. If it breaks things, then that might
explain why SYMBOL_AUTO is set on them.

They would always be fetched from the environment in any case, since
they are not user-assignable (lack prompts).


Some philosophical rambling re. .config files below:

One thing that makes Kconfig confusing (though it works well enough in
practice) is that .config files both record user selections (the saved
configuration) and serve as a configuration output format for make.

It becomes easier to think about .config files once you realize that
assignments to promptless symbols never have an effect on Kconfig
itself: They're just configuration output, intermixed with the saved
user selections.

Assume 'option env' symbols got written out for example:

	- For a non-user-assignable symbol, the entry in the .config
	  file is just configuration output and ignored by Kconfig,
	  which will fetch the value from the environment instead.

	- For an assignable 'option env' symbol, the entry in the
	  .config file is a saved user selection (as well as
	  configuration output), and will be respected by Kconfig.

defconfig files are closer to a "pure" output-independent saved
configuration format in a way, since they only include user selections
and don't mix it with configuration output.

Hopefully some of that made sense. Don't feel forced to reply to the
rambly parts. It's just a way of thinking of it that helps me. ;)

> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 9, 2018, 8:46 p.m. UTC | #4

On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
> One thing that makes Kconfig confusing (though it works well enough in
> practice) is that .config files both record user selections (the saved
> configuration) and serve as a configuration output format for make.
>
> It becomes easier to think about .config files once you realize that
> assignments to promptless symbols never have an effect on Kconfig
> itself: They're just configuration output, intermixed with the saved
> user selections.
>
> Assume 'option env' symbols got written out for example:
>
>         - For a non-user-assignable symbol, the entry in the .config
>           file is just configuration output and ignored by Kconfig,
>           which will fetch the value from the environment instead.
>
>         - For an assignable 'option env' symbol, the entry in the
>           .config file is a saved user selection (as well as
>           configuration output), and will be respected by Kconfig.

In the stack-protector case, this becomes quite important, since the
goal is to record the user's selection regardless of compiler
capability. For example, if someone selects _REGULAR, it shouldn't
"upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
way to pick "best possible for this compiler", though. If a user had
previously selected _STRONG but they're doing builds with an older
compiler (or a misconfigured newer compiler) without support, the goal
is to _fail_ to build, not silently select _REGULAR.

So, in this case, what's gained is the logic for _AUTO, and the logic
to not show, say, _STRONG when it's not available in the compiler. But
we must still fail to build if _STRONG was in the .config. It can't
silently rewrite it to _REGULAR because the compiler support for
_STRONG regressed.

-Kees

Ulf Magnusson Feb. 10, 2018, 5:48 a.m. UTC | #5

On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
> > One thing that makes Kconfig confusing (though it works well enough in
> > practice) is that .config files both record user selections (the saved
> > configuration) and serve as a configuration output format for make.
> >
> > It becomes easier to think about .config files once you realize that
> > assignments to promptless symbols never have an effect on Kconfig
> > itself: They're just configuration output, intermixed with the saved
> > user selections.
> >
> > Assume 'option env' symbols got written out for example:
> >
> >         - For a non-user-assignable symbol, the entry in the .config
> >           file is just configuration output and ignored by Kconfig,
> >           which will fetch the value from the environment instead.
> >
> >         - For an assignable 'option env' symbol, the entry in the
> >           .config file is a saved user selection (as well as
> >           configuration output), and will be respected by Kconfig.
> 
> In the stack-protector case, this becomes quite important, since the
> goal is to record the user's selection regardless of compiler
> capability. For example, if someone selects _REGULAR, it shouldn't
> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
> way to pick "best possible for this compiler", though. If a user had
> previously selected _STRONG but they're doing builds with an older
> compiler (or a misconfigured newer compiler) without support, the goal
> is to _fail_ to build, not silently select _REGULAR.
> 
> So, in this case, what's gained is the logic for _AUTO, and the logic
> to not show, say, _STRONG when it's not available in the compiler. But
> we must still fail to build if _STRONG was in the .config. It can't
> silently rewrite it to _REGULAR because the compiler support for
> _STRONG regressed.
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Provided that would be the desired behavior:

What about changing the meaning of the choice symbols from e.g. "select
-fstack-protector-strong" to "want -fstack-protector-strong"? Then the
user preference would always be remembered, regardless of what's
available.

Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
fits pretty well here, since it works like a dependency-respecting
select.

	config CC_HAS_STACKPROTECTOR_STRONG
		bool
		option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
	
	config CC_HAS_STACKPROTECTOR
		bool
		option shell="$CC -Werror -fstack-protector -c -x c /dev/null"

	
	choice
		prompt "Stack Protector buffer overflow detection"
		default WANT_CC_STACKPROTECTOR_STRONG
	
	config WANT_CC_STACKPROTECTOR_STRONG
		bool "Strong"
		imply CC_STACKPROTECTOR_STRONG
	
	config WANT_CC_STACKPROTECTOR_REGULAR
		bool "Regular"
		imply CC_STACKPROTECTOR_REGULAR
	
	config WANT_CC_STACKPROTECTOR_NONE
		bool "None"
		imply CC_STACKPROTECTOR_NONE
	
	endchoice
	
	
	config CC_STACKPROTECTOR_STRONG
		bool
		depends on CC_HAS_STACKPROTECTOR_STRONG
	
	config CC_STACKPROTECTOR_REGULAR
		bool
		depends on CC_HAS_STACKPROTECTOR_REGULAR
	
	config CC_STACKPROTECTOR_NONE
		bool

This version has the drawback of always showing all the options, even if
some they wouldn't be available. Kconfig comments could be added to warn
if an option isn't available at least:

	comment "Warning: Your compiler does not support -fstack-protector-strong"
		depends on !CC_HAS_STACKPROTECTOR_STRONG

	config WANT_CC_STACKPROTECTOR_STRONG
		...


	comment "Warning: Your compiler does not support -fstack-protector"
		depends on !CC_HAS_STACKPROTECTOR_REGULAR

	config WANT_CC_STACKPROTECTOR_REGULAR
		...

This final comment might be nice to have too:

	comment "Warning: Selected stack protector not available"
		depends on !(CC_STACKPROTECTOR_STRONG ||
			     CC_STACKPROTECTOR_REGULAR ||
			     CC_STACKPROTECTOR_NONE)

Should probably introduce a clear warning that tells the user what they
need to change in Kconfig if they build with a broken selection too.


CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
way too. Maybe there's something neater.

	config CC_STACKPROTECTOR_AUTO
		bool "Automatic"
		imply CC_STACKPROTECTOR_STRONG
		imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
		imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
						   !CC_HAS_STACKPROTECTOR_REGULAR


Another drawback of this approach is that it breaks existing .config
files (the CC_STACKPROTECTOR_* settings are ignored, since they just
look like "configuration output" to Kconfig now). If that'd be a
problem, the old names could be used instead of
WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
though it'd look a bit cryptic.

Ideas?

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 10, 2018, 7:12 a.m. UTC | #6

2018-02-10 14:48 GMT+09:00 Ulf Magnusson <[e-mail]>:
> On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
>> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
>> > One thing that makes Kconfig confusing (though it works well enough in
>> > practice) is that .config files both record user selections (the saved
>> > configuration) and serve as a configuration output format for make.
>> >
>> > It becomes easier to think about .config files once you realize that
>> > assignments to promptless symbols never have an effect on Kconfig
>> > itself: They're just configuration output, intermixed with the saved
>> > user selections.
>> >
>> > Assume 'option env' symbols got written out for example:
>> >
>> >         - For a non-user-assignable symbol, the entry in the .config
>> >           file is just configuration output and ignored by Kconfig,
>> >           which will fetch the value from the environment instead.
>> >
>> >         - For an assignable 'option env' symbol, the entry in the
>> >           .config file is a saved user selection (as well as
>> >           configuration output), and will be respected by Kconfig.
>>
>> In the stack-protector case, this becomes quite important, since the
>> goal is to record the user's selection regardless of compiler
>> capability. For example, if someone selects _REGULAR, it shouldn't
>> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
>> way to pick "best possible for this compiler", though. If a user had
>> previously selected _STRONG but they're doing builds with an older
>> compiler (or a misconfigured newer compiler) without support, the goal
>> is to _fail_ to build, not silently select _REGULAR.
>>
>> So, in this case, what's gained is the logic for _AUTO, and the logic
>> to not show, say, _STRONG when it's not available in the compiler. But
>> we must still fail to build if _STRONG was in the .config. It can't
>> silently rewrite it to _REGULAR because the compiler support for
>> _STRONG regressed.
>>
>> -Kees
>>
>> --
>> Kees Cook
>> Pixel Security
>
> Provided that would be the desired behavior:
>
> What about changing the meaning of the choice symbols from e.g. "select
> -fstack-protector-strong" to "want -fstack-protector-strong"? Then the
> user preference would always be remembered, regardless of what's
> available.
>
> Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
> fits pretty well here, since it works like a dependency-respecting
> select.
>
>         config CC_HAS_STACKPROTECTOR_STRONG
>                 bool
>                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
>
>         config CC_HAS_STACKPROTECTOR
>                 bool
>                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
>
>
>         choice
>                 prompt "Stack Protector buffer overflow detection"
>                 default WANT_CC_STACKPROTECTOR_STRONG
>
>         config WANT_CC_STACKPROTECTOR_STRONG
>                 bool "Strong"
>                 imply CC_STACKPROTECTOR_STRONG
>
>         config WANT_CC_STACKPROTECTOR_REGULAR
>                 bool "Regular"
>                 imply CC_STACKPROTECTOR_REGULAR
>
>         config WANT_CC_STACKPROTECTOR_NONE
>                 bool "None"
>                 imply CC_STACKPROTECTOR_NONE
>
>         endchoice
>
>
>         config CC_STACKPROTECTOR_STRONG
>                 bool
>                 depends on CC_HAS_STACKPROTECTOR_STRONG


Do you mean

         config CC_STACKPROTECTOR_STRONG
                 bool
                 depends on CC_HAS_STACKPROTECTOR_STRONG && \
                            WANT_CC_STACKPROTECTOR_STRONG

or, maybe


         config CC_STACKPROTECTOR_STRONG
                 bool
                 depends on CC_HAS_STACKPROTECTOR_STRONG
                 default WANT_CC_STACKPROTECTOR_STRONG

?





>         config CC_STACKPROTECTOR_REGULAR
>                 bool
>                 depends on CC_HAS_STACKPROTECTOR_REGULAR
>
>         config CC_STACKPROTECTOR_NONE
>                 bool
>
> This version has the drawback of always showing all the options, even if
> some they wouldn't be available. Kconfig comments could be added to warn
> if an option isn't available at least:
>
>         comment "Warning: Your compiler does not support -fstack-protector-strong"
>                 depends on !CC_HAS_STACKPROTECTOR_STRONG
>
>         config WANT_CC_STACKPROTECTOR_STRONG
>                 ...
>
>
>         comment "Warning: Your compiler does not support -fstack-protector"
>                 depends on !CC_HAS_STACKPROTECTOR_REGULAR
>
>         config WANT_CC_STACKPROTECTOR_REGULAR
>                 ...
>
> This final comment might be nice to have too:
>
>         comment "Warning: Selected stack protector not available"
>                 depends on !(CC_STACKPROTECTOR_STRONG ||
>                              CC_STACKPROTECTOR_REGULAR ||
>                              CC_STACKPROTECTOR_NONE)
>
> Should probably introduce a clear warning that tells the user what they
> need to change in Kconfig if they build with a broken selection too.
>
>
> CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
> way too. Maybe there's something neater.
>
>         config CC_STACKPROTECTOR_AUTO
>                 bool "Automatic"
>                 imply CC_STACKPROTECTOR_STRONG
>                 imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
>                 imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
>                                                    !CC_HAS_STACKPROTECTOR_REGULAR
>
>
> Another drawback of this approach is that it breaks existing .config
> files (the CC_STACKPROTECTOR_* settings are ignored, since they just
> look like "configuration output" to Kconfig now). If that'd be a
> problem, the old names could be used instead of
> WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
> though it'd look a bit cryptic.
>
> Ideas?
>



FWIW, the following is what I was playing with.
(The idea for emitting warnings is Ulf's idea)


------------------>8-------------------
config CC
        string
        option env="CC"

config CC_HAS_STACKPROTECTOR
        bool
        option shell="$CC -Werror -fstack-protector -c -x c /dev/null"

config CC_HAS_STACKPROTECTOR_STRONG
        bool
        option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"

config CC_HAS_STACKPROTECTOR_NONE
        bool
        option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"

config CC_STACKPROTECTOR
        bool

choice
        prompt "Stack Protector buffer overflow detection"

config CC_STACKPROTECTOR_AUTO
        bool "Auto"
        select CC_STACKPROTECTOR if (CC_HAS_STACKPROTECTOR || \
                                     CC_HAS_STACKPROTECTOR_STRONG)

config CC_STACKPROTECTOR_REGULAR
        bool "Regular"
        select CC_STACKPROTECTOR

config CC_STACKPROTECTOR_STRONG
        bool "Strong"
        select CC_STACKPROTECTOR

config CC_STACKPROTECTOR_NONE
        bool "None"

endchoice


comment "(WARNING) stackprotecter was chosen, but your compile does
not support it.  Build will fail"
        depends on CC_STACKPROTECTOR_REGULAR && \
                   !CC_HAS_STACKPROTECTOR

comment "(WARNING) stackprotecter-strong was chosen, but your compile
does not support it.  Build will fail"
        depends on CC_STACKPROTECTOR_STRONG && \
                   !CC_HAS_STACKPROTECTOR_STRONG
------------------------->8---------------------------------





BTW, setting option flags in Makefile is dirty, like follows:


ccflags-$(CONFIG_CC_STACKPROTECTOR_STRONG)  += -fstack-protector-strong
ccflags-$(CONFIG_CC_STACKPROTECTOR_REGULAR) += -fstack-protector

if ($(CONFIG_CC_STACKPROTECTOR_AUTO),y)
ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR)        += -fstack-protector
ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_STRONG) += -fstack-protector-strong
ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
endif

if ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
endif




One idea could be to calculate the compiler option in Kconfig.

config CC_OPT_STACKPROTECTOR
        string
        default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG || \
                                             (CC_STACKPROTECTOR_AUTO && \
                                              CC_HAS_STACKPROTECTOR_STRONG)
        default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR || \
                                              (CC_STACKPROTECTOR_AUTO && \
                                               CC_HAS_STACKPROTECTOR)
        default "-fno-stack-protector"        if CC_HAS_STACKPROTECTOR_NONE



Makefile will become clean.
Of course, this is at the cost of ugliness in Kconfig.

Ulf Magnusson Feb. 10, 2018, 7:49 a.m. UTC | #7

On Sat, Feb 10, 2018 at 04:12:13PM +0900, Masahiro Yamada wrote:
> 2018-02-10 14:48 GMT+09:00 Ulf Magnusson <[e-mail]>:
> > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
> >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
> >> > One thing that makes Kconfig confusing (though it works well enough in
> >> > practice) is that .config files both record user selections (the saved
> >> > configuration) and serve as a configuration output format for make.
> >> >
> >> > It becomes easier to think about .config files once you realize that
> >> > assignments to promptless symbols never have an effect on Kconfig
> >> > itself: They're just configuration output, intermixed with the saved
> >> > user selections.
> >> >
> >> > Assume 'option env' symbols got written out for example:
> >> >
> >> >         - For a non-user-assignable symbol, the entry in the .config
> >> >           file is just configuration output and ignored by Kconfig,
> >> >           which will fetch the value from the environment instead.
> >> >
> >> >         - For an assignable 'option env' symbol, the entry in the
> >> >           .config file is a saved user selection (as well as
> >> >           configuration output), and will be respected by Kconfig.
> >>
> >> In the stack-protector case, this becomes quite important, since the
> >> goal is to record the user's selection regardless of compiler
> >> capability. For example, if someone selects _REGULAR, it shouldn't
> >> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
> >> way to pick "best possible for this compiler", though. If a user had
> >> previously selected _STRONG but they're doing builds with an older
> >> compiler (or a misconfigured newer compiler) without support, the goal
> >> is to _fail_ to build, not silently select _REGULAR.
> >>
> >> So, in this case, what's gained is the logic for _AUTO, and the logic
> >> to not show, say, _STRONG when it's not available in the compiler. But
> >> we must still fail to build if _STRONG was in the .config. It can't
> >> silently rewrite it to _REGULAR because the compiler support for
> >> _STRONG regressed.
> >>
> >> -Kees
> >>
> >> --
> >> Kees Cook
> >> Pixel Security
> >
> > Provided that would be the desired behavior:
> >
> > What about changing the meaning of the choice symbols from e.g. "select
> > -fstack-protector-strong" to "want -fstack-protector-strong"? Then the
> > user preference would always be remembered, regardless of what's
> > available.
> >
> > Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
> > fits pretty well here, since it works like a dependency-respecting
> > select.
> >
> >         config CC_HAS_STACKPROTECTOR_STRONG
> >                 bool
> >                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> >
> >         config CC_HAS_STACKPROTECTOR
> >                 bool
> >                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> >
> >
> >         choice
> >                 prompt "Stack Protector buffer overflow detection"
> >                 default WANT_CC_STACKPROTECTOR_STRONG
> >
> >         config WANT_CC_STACKPROTECTOR_STRONG
> >                 bool "Strong"
> >                 imply CC_STACKPROTECTOR_STRONG
> >
> >         config WANT_CC_STACKPROTECTOR_REGULAR
> >                 bool "Regular"
> >                 imply CC_STACKPROTECTOR_REGULAR
> >
> >         config WANT_CC_STACKPROTECTOR_NONE
> >                 bool "None"
> >                 imply CC_STACKPROTECTOR_NONE
> >
> >         endchoice
> >
> >
> >         config CC_STACKPROTECTOR_STRONG
> >                 bool
> >                 depends on CC_HAS_STACKPROTECTOR_STRONG
> 
> 
> Do you mean
> 
>          config CC_STACKPROTECTOR_STRONG
>                  bool
>                  depends on CC_HAS_STACKPROTECTOR_STRONG && \
>                             WANT_CC_STACKPROTECTOR_STRONG
> 
> or, maybe
> 
> 
>          config CC_STACKPROTECTOR_STRONG
>                  bool
>                  depends on CC_HAS_STACKPROTECTOR_STRONG
>                  default WANT_CC_STACKPROTECTOR_STRONG
> 
> ?

With the 'imply', it should work with just the 'depends on'. I had your
last version earlier though, and it works too.

'imply' kinda makes sense, as in "turn on the strong stack protector if
its dependencies are satisfied".

> 
> 
> 
> 
> 
> >         config CC_STACKPROTECTOR_REGULAR
> >                 bool
> >                 depends on CC_HAS_STACKPROTECTOR_REGULAR
> >
> >         config CC_STACKPROTECTOR_NONE
> >                 bool
> >
> > This version has the drawback of always showing all the options, even if
> > some they wouldn't be available. Kconfig comments could be added to warn
> > if an option isn't available at least:
> >
> >         comment "Warning: Your compiler does not support -fstack-protector-strong"
> >                 depends on !CC_HAS_STACKPROTECTOR_STRONG
> >
> >         config WANT_CC_STACKPROTECTOR_STRONG
> >                 ...
> >
> >
> >         comment "Warning: Your compiler does not support -fstack-protector"
> >                 depends on !CC_HAS_STACKPROTECTOR_REGULAR
> >
> >         config WANT_CC_STACKPROTECTOR_REGULAR
> >                 ...
> >
> > This final comment might be nice to have too:
> >
> >         comment "Warning: Selected stack protector not available"
> >                 depends on !(CC_STACKPROTECTOR_STRONG ||
> >                              CC_STACKPROTECTOR_REGULAR ||
> >                              CC_STACKPROTECTOR_NONE)
> >
> > Should probably introduce a clear warning that tells the user what they
> > need to change in Kconfig if they build with a broken selection too.
> >
> >
> > CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
> > way too. Maybe there's something neater.
> >
> >         config CC_STACKPROTECTOR_AUTO
> >                 bool "Automatic"
> >                 imply CC_STACKPROTECTOR_STRONG
> >                 imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
> >                 imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
> >                                                    !CC_HAS_STACKPROTECTOR_REGULAR
> >
> >
> > Another drawback of this approach is that it breaks existing .config
> > files (the CC_STACKPROTECTOR_* settings are ignored, since they just
> > look like "configuration output" to Kconfig now). If that'd be a
> > problem, the old names could be used instead of
> > WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
> > though it'd look a bit cryptic.
> >
> > Ideas?
> >
> 
> 
> 
> FWIW, the following is what I was playing with.
> (The idea for emitting warnings is Ulf's idea)
> 
> 
> ------------------>8-------------------
> config CC
>         string
>         option env="CC"
> 
> config CC_HAS_STACKPROTECTOR
>         bool
>         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> 
> config CC_HAS_STACKPROTECTOR_STRONG
>         bool
>         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> 
> config CC_HAS_STACKPROTECTOR_NONE
>         bool
>         option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> 
> config CC_STACKPROTECTOR
>         bool
> 
> choice
>         prompt "Stack Protector buffer overflow detection"
> 
> config CC_STACKPROTECTOR_AUTO
>         bool "Auto"
>         select CC_STACKPROTECTOR if (CC_HAS_STACKPROTECTOR || \
>                                      CC_HAS_STACKPROTECTOR_STRONG)

With this approach, I guess you would still need to handle the
CC_STACKPROTECTOR_AUTO logic outside of Kconfig, since e.g.
CC_STACKPROTECTOR_STRONG won't get enabled automatically if supported.

The idea above was to make it "internal" to the Kconfig files (though it
still gets written out), with the
CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} variables automatically getting
set as appropriate.

The build could then the detect if none of
CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} are set and do what's
appropriate (error out in some semi-helpful way or whatever... not
deeply familiar with kernel policy here :).

> 
> config CC_STACKPROTECTOR_REGULAR
>         bool "Regular"
>         select CC_STACKPROTECTOR
> 
> config CC_STACKPROTECTOR_STRONG
>         bool "Strong"
>         select CC_STACKPROTECTOR
> 
> config CC_STACKPROTECTOR_NONE
>         bool "None"
> 
> endchoice
> 
> 
> comment "(WARNING) stackprotecter was chosen, but your compile does
> not support it.  Build will fail"
>         depends on CC_STACKPROTECTOR_REGULAR && \
>                    !CC_HAS_STACKPROTECTOR
> 
> comment "(WARNING) stackprotecter-strong was chosen, but your compile
> does not support it.  Build will fail"
>         depends on CC_STACKPROTECTOR_STRONG && \
>                    !CC_HAS_STACKPROTECTOR_STRONG
> ------------------------->8---------------------------------
> 
> 
> 
> 
> 
> BTW, setting option flags in Makefile is dirty, like follows:
> 
> 
> ccflags-$(CONFIG_CC_STACKPROTECTOR_STRONG)  += -fstack-protector-strong
> ccflags-$(CONFIG_CC_STACKPROTECTOR_REGULAR) += -fstack-protector
> 
> if ($(CONFIG_CC_STACKPROTECTOR_AUTO),y)
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR)        += -fstack-protector
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_STRONG) += -fstack-protector-strong
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> endif
> 
> if ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
> ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> endif
> 
> 
> 
> 
> One idea could be to calculate the compiler option in Kconfig.
> 
> config CC_OPT_STACKPROTECTOR
>         string
>         default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG || \
>                                              (CC_STACKPROTECTOR_AUTO && \
>                                               CC_HAS_STACKPROTECTOR_STRONG)
>         default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR || \
>                                               (CC_STACKPROTECTOR_AUTO && \
>                                                CC_HAS_STACKPROTECTOR)
>         default "-fno-stack-protector"        if CC_HAS_STACKPROTECTOR_NONE

If CC_STACKPROTECTOR_AUTO is made "internal", this could be simplified
to something like

	config CC_OPT_STACKPROTECTOR
		string
		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
		# If the compiler doesn't even support
		# -fno-stack-protector
		default ""

(Last default is just to make the empty string explicit. That's the
value it would get anyway.)

> 
> 
> 
> Makefile will become clean.
> Of course, this is at the cost of ugliness in Kconfig.
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

Please tell me if I've misunderstood some aspect of the old behavior.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 10, 2018, 8:05 a.m. UTC | #8

On Sat, Feb 10, 2018 at 08:49:24AM +0100, Ulf Magnusson wrote:
> On Sat, Feb 10, 2018 at 04:12:13PM +0900, Masahiro Yamada wrote:
> > 2018-02-10 14:48 GMT+09:00 Ulf Magnusson <[e-mail]>:
> > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
> > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
> > >> > One thing that makes Kconfig confusing (though it works well enough in
> > >> > practice) is that .config files both record user selections (the saved
> > >> > configuration) and serve as a configuration output format for make.
> > >> >
> > >> > It becomes easier to think about .config files once you realize that
> > >> > assignments to promptless symbols never have an effect on Kconfig
> > >> > itself: They're just configuration output, intermixed with the saved
> > >> > user selections.
> > >> >
> > >> > Assume 'option env' symbols got written out for example:
> > >> >
> > >> >         - For a non-user-assignable symbol, the entry in the .config
> > >> >           file is just configuration output and ignored by Kconfig,
> > >> >           which will fetch the value from the environment instead.
> > >> >
> > >> >         - For an assignable 'option env' symbol, the entry in the
> > >> >           .config file is a saved user selection (as well as
> > >> >           configuration output), and will be respected by Kconfig.
> > >>
> > >> In the stack-protector case, this becomes quite important, since the
> > >> goal is to record the user's selection regardless of compiler
> > >> capability. For example, if someone selects _REGULAR, it shouldn't
> > >> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
> > >> way to pick "best possible for this compiler", though. If a user had
> > >> previously selected _STRONG but they're doing builds with an older
> > >> compiler (or a misconfigured newer compiler) without support, the goal
> > >> is to _fail_ to build, not silently select _REGULAR.
> > >>
> > >> So, in this case, what's gained is the logic for _AUTO, and the logic
> > >> to not show, say, _STRONG when it's not available in the compiler. But
> > >> we must still fail to build if _STRONG was in the .config. It can't
> > >> silently rewrite it to _REGULAR because the compiler support for
> > >> _STRONG regressed.
> > >>
> > >> -Kees
> > >>
> > >> --
> > >> Kees Cook
> > >> Pixel Security
> > >
> > > Provided that would be the desired behavior:
> > >
> > > What about changing the meaning of the choice symbols from e.g. "select
> > > -fstack-protector-strong" to "want -fstack-protector-strong"? Then the
> > > user preference would always be remembered, regardless of what's
> > > available.
> > >
> > > Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
> > > fits pretty well here, since it works like a dependency-respecting
> > > select.
> > >
> > >         config CC_HAS_STACKPROTECTOR_STRONG
> > >                 bool
> > >                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > >
> > >         config CC_HAS_STACKPROTECTOR
> > >                 bool
> > >                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > >
> > >
> > >         choice
> > >                 prompt "Stack Protector buffer overflow detection"
> > >                 default WANT_CC_STACKPROTECTOR_STRONG
> > >
> > >         config WANT_CC_STACKPROTECTOR_STRONG
> > >                 bool "Strong"
> > >                 imply CC_STACKPROTECTOR_STRONG
> > >
> > >         config WANT_CC_STACKPROTECTOR_REGULAR
> > >                 bool "Regular"
> > >                 imply CC_STACKPROTECTOR_REGULAR
> > >
> > >         config WANT_CC_STACKPROTECTOR_NONE
> > >                 bool "None"
> > >                 imply CC_STACKPROTECTOR_NONE
> > >
> > >         endchoice
> > >
> > >
> > >         config CC_STACKPROTECTOR_STRONG
> > >                 bool
> > >                 depends on CC_HAS_STACKPROTECTOR_STRONG
> > 
> > 
> > Do you mean
> > 
> >          config CC_STACKPROTECTOR_STRONG
> >                  bool
> >                  depends on CC_HAS_STACKPROTECTOR_STRONG && \
> >                             WANT_CC_STACKPROTECTOR_STRONG
> > 
> > or, maybe
> > 
> > 
> >          config CC_STACKPROTECTOR_STRONG
> >                  bool
> >                  depends on CC_HAS_STACKPROTECTOR_STRONG
> >                  default WANT_CC_STACKPROTECTOR_STRONG
> > 
> > ?
> 
> With the 'imply', it should work with just the 'depends on'. I had your
> last version earlier though, and it works too.
> 
> 'imply' kinda makes sense, as in "turn on the strong stack protector if
> its dependencies are satisfied".
> 
> > 
> > 
> > 
> > 
> > 
> > >         config CC_STACKPROTECTOR_REGULAR
> > >                 bool
> > >                 depends on CC_HAS_STACKPROTECTOR_REGULAR
> > >
> > >         config CC_STACKPROTECTOR_NONE
> > >                 bool
> > >
> > > This version has the drawback of always showing all the options, even if
> > > some they wouldn't be available. Kconfig comments could be added to warn
> > > if an option isn't available at least:
> > >
> > >         comment "Warning: Your compiler does not support -fstack-protector-strong"
> > >                 depends on !CC_HAS_STACKPROTECTOR_STRONG
> > >
> > >         config WANT_CC_STACKPROTECTOR_STRONG
> > >                 ...
> > >
> > >
> > >         comment "Warning: Your compiler does not support -fstack-protector"
> > >                 depends on !CC_HAS_STACKPROTECTOR_REGULAR
> > >
> > >         config WANT_CC_STACKPROTECTOR_REGULAR
> > >                 ...
> > >
> > > This final comment might be nice to have too:
> > >
> > >         comment "Warning: Selected stack protector not available"
> > >                 depends on !(CC_STACKPROTECTOR_STRONG ||
> > >                              CC_STACKPROTECTOR_REGULAR ||
> > >                              CC_STACKPROTECTOR_NONE)
> > >
> > > Should probably introduce a clear warning that tells the user what they
> > > need to change in Kconfig if they build with a broken selection too.
> > >
> > >
> > > CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
> > > way too. Maybe there's something neater.
> > >
> > >         config CC_STACKPROTECTOR_AUTO
> > >                 bool "Automatic"
> > >                 imply CC_STACKPROTECTOR_STRONG
> > >                 imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
> > >                 imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
> > >                                                    !CC_HAS_STACKPROTECTOR_REGULAR
> > >
> > >
> > > Another drawback of this approach is that it breaks existing .config
> > > files (the CC_STACKPROTECTOR_* settings are ignored, since they just
> > > look like "configuration output" to Kconfig now). If that'd be a
> > > problem, the old names could be used instead of
> > > WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
> > > though it'd look a bit cryptic.
> > >
> > > Ideas?
> > >
> > 
> > 
> > 
> > FWIW, the following is what I was playing with.
> > (The idea for emitting warnings is Ulf's idea)
> > 
> > 
> > ------------------>8-------------------
> > config CC
> >         string
> >         option env="CC"
> > 
> > config CC_HAS_STACKPROTECTOR
> >         bool
> >         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > 
> > config CC_HAS_STACKPROTECTOR_STRONG
> >         bool
> >         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > 
> > config CC_HAS_STACKPROTECTOR_NONE
> >         bool
> >         option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> > 
> > config CC_STACKPROTECTOR
> >         bool
> > 
> > choice
> >         prompt "Stack Protector buffer overflow detection"
> > 
> > config CC_STACKPROTECTOR_AUTO
> >         bool "Auto"
> >         select CC_STACKPROTECTOR if (CC_HAS_STACKPROTECTOR || \
> >                                      CC_HAS_STACKPROTECTOR_STRONG)
> 
> With this approach, I guess you would still need to handle the
> CC_STACKPROTECTOR_AUTO logic outside of Kconfig, since e.g.
> CC_STACKPROTECTOR_STRONG won't get enabled automatically if supported.
> 
> The idea above was to make it "internal" to the Kconfig files (though it
> still gets written out), with the
> CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} variables automatically getting
> set as appropriate.

That was a confusing way of putting it -- sorry about that.

What I meant was that it would just be a user selection, with all the
logic of selecting one of CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} being
handled internally in the Kconfig files, even in the
CC_STACKPROTECTOR_AUTO case.

Nothing outside of Kconfig would need to check CC_STACKPROTECTOR_AUTO
then.

> 
> The build could then the detect if none of
> CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} are set and do what's
> appropriate (error out in some semi-helpful way or whatever... not
> deeply familiar with kernel policy here :).
> 
> > 
> > config CC_STACKPROTECTOR_REGULAR
> >         bool "Regular"
> >         select CC_STACKPROTECTOR
> > 
> > config CC_STACKPROTECTOR_STRONG
> >         bool "Strong"
> >         select CC_STACKPROTECTOR
> > 
> > config CC_STACKPROTECTOR_NONE
> >         bool "None"
> > 
> > endchoice
> > 
> > 
> > comment "(WARNING) stackprotecter was chosen, but your compile does
> > not support it.  Build will fail"
> >         depends on CC_STACKPROTECTOR_REGULAR && \
> >                    !CC_HAS_STACKPROTECTOR
> > 
> > comment "(WARNING) stackprotecter-strong was chosen, but your compile
> > does not support it.  Build will fail"
> >         depends on CC_STACKPROTECTOR_STRONG && \
> >                    !CC_HAS_STACKPROTECTOR_STRONG
> > ------------------------->8---------------------------------
> > 
> > 
> > 
> > 
> > 
> > BTW, setting option flags in Makefile is dirty, like follows:
> > 
> > 
> > ccflags-$(CONFIG_CC_STACKPROTECTOR_STRONG)  += -fstack-protector-strong
> > ccflags-$(CONFIG_CC_STACKPROTECTOR_REGULAR) += -fstack-protector
> > 
> > if ($(CONFIG_CC_STACKPROTECTOR_AUTO),y)
> > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR)        += -fstack-protector
> > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_STRONG) += -fstack-protector-strong
> > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> > endif
> > 
> > if ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
> > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> > endif
> > 
> > 
> > 
> > 
> > One idea could be to calculate the compiler option in Kconfig.
> > 
> > config CC_OPT_STACKPROTECTOR
> >         string
> >         default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG || \
> >                                              (CC_STACKPROTECTOR_AUTO && \
> >                                               CC_HAS_STACKPROTECTOR_STRONG)
> >         default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR || \
> >                                               (CC_STACKPROTECTOR_AUTO && \
> >                                                CC_HAS_STACKPROTECTOR)
> >         default "-fno-stack-protector"        if CC_HAS_STACKPROTECTOR_NONE
> 
> If CC_STACKPROTECTOR_AUTO is made "internal", this could be simplified
> to something like
> 
> 	config CC_OPT_STACKPROTECTOR
> 		string
> 		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
> 		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
> 		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
> 		# If the compiler doesn't even support
> 		# -fno-stack-protector
> 		default ""
> 
> (Last default is just to make the empty string explicit. That's the
> value it would get anyway.)
> 
> > 
> > 
> > 
> > Makefile will become clean.
> > Of course, this is at the cost of ugliness in Kconfig.
> > 
> > 
> > 
> > 
> > -- 
> > Best Regards
> > Masahiro Yamada
> 
> Please tell me if I've misunderstood some aspect of the old behavior.
> 
> Cheers,
> Ulf

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 10, 2018, 8:55 a.m. UTC | #9

On Sat, Feb 10, 2018 at 09:05:56AM +0100, Ulf Magnusson wrote:
> On Sat, Feb 10, 2018 at 08:49:24AM +0100, Ulf Magnusson wrote:
> > On Sat, Feb 10, 2018 at 04:12:13PM +0900, Masahiro Yamada wrote:
> > > 2018-02-10 14:48 GMT+09:00 Ulf Magnusson <[e-mail]>:
> > > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
> > > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
> > > >> > One thing that makes Kconfig confusing (though it works well enough in
> > > >> > practice) is that .config files both record user selections (the saved
> > > >> > configuration) and serve as a configuration output format for make.
> > > >> >
> > > >> > It becomes easier to think about .config files once you realize that
> > > >> > assignments to promptless symbols never have an effect on Kconfig
> > > >> > itself: They're just configuration output, intermixed with the saved
> > > >> > user selections.
> > > >> >
> > > >> > Assume 'option env' symbols got written out for example:
> > > >> >
> > > >> >         - For a non-user-assignable symbol, the entry in the .config
> > > >> >           file is just configuration output and ignored by Kconfig,
> > > >> >           which will fetch the value from the environment instead.
> > > >> >
> > > >> >         - For an assignable 'option env' symbol, the entry in the
> > > >> >           .config file is a saved user selection (as well as
> > > >> >           configuration output), and will be respected by Kconfig.
> > > >>
> > > >> In the stack-protector case, this becomes quite important, since the
> > > >> goal is to record the user's selection regardless of compiler
> > > >> capability. For example, if someone selects _REGULAR, it shouldn't
> > > >> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
> > > >> way to pick "best possible for this compiler", though. If a user had
> > > >> previously selected _STRONG but they're doing builds with an older
> > > >> compiler (or a misconfigured newer compiler) without support, the goal
> > > >> is to _fail_ to build, not silently select _REGULAR.
> > > >>
> > > >> So, in this case, what's gained is the logic for _AUTO, and the logic
> > > >> to not show, say, _STRONG when it's not available in the compiler. But
> > > >> we must still fail to build if _STRONG was in the .config. It can't
> > > >> silently rewrite it to _REGULAR because the compiler support for
> > > >> _STRONG regressed.
> > > >>
> > > >> -Kees
> > > >>
> > > >> --
> > > >> Kees Cook
> > > >> Pixel Security
> > > >
> > > > Provided that would be the desired behavior:
> > > >
> > > > What about changing the meaning of the choice symbols from e.g. "select
> > > > -fstack-protector-strong" to "want -fstack-protector-strong"? Then the
> > > > user preference would always be remembered, regardless of what's
> > > > available.
> > > >
> > > > Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
> > > > fits pretty well here, since it works like a dependency-respecting
> > > > select.
> > > >
> > > >         config CC_HAS_STACKPROTECTOR_STRONG
> > > >                 bool
> > > >                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > > >
> > > >         config CC_HAS_STACKPROTECTOR
> > > >                 bool
> > > >                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > > >
> > > >
> > > >         choice
> > > >                 prompt "Stack Protector buffer overflow detection"
> > > >                 default WANT_CC_STACKPROTECTOR_STRONG
> > > >
> > > >         config WANT_CC_STACKPROTECTOR_STRONG
> > > >                 bool "Strong"
> > > >                 imply CC_STACKPROTECTOR_STRONG
> > > >
> > > >         config WANT_CC_STACKPROTECTOR_REGULAR
> > > >                 bool "Regular"
> > > >                 imply CC_STACKPROTECTOR_REGULAR
> > > >
> > > >         config WANT_CC_STACKPROTECTOR_NONE
> > > >                 bool "None"
> > > >                 imply CC_STACKPROTECTOR_NONE
> > > >
> > > >         endchoice
> > > >
> > > >
> > > >         config CC_STACKPROTECTOR_STRONG
> > > >                 bool
> > > >                 depends on CC_HAS_STACKPROTECTOR_STRONG
> > > 
> > > 
> > > Do you mean
> > > 
> > >          config CC_STACKPROTECTOR_STRONG
> > >                  bool
> > >                  depends on CC_HAS_STACKPROTECTOR_STRONG && \
> > >                             WANT_CC_STACKPROTECTOR_STRONG
> > > 
> > > or, maybe
> > > 
> > > 
> > >          config CC_STACKPROTECTOR_STRONG
> > >                  bool
> > >                  depends on CC_HAS_STACKPROTECTOR_STRONG
> > >                  default WANT_CC_STACKPROTECTOR_STRONG
> > > 
> > > ?
> > 
> > With the 'imply', it should work with just the 'depends on'. I had your
> > last version earlier though, and it works too.
> > 
> > 'imply' kinda makes sense, as in "turn on the strong stack protector if
> > its dependencies are satisfied".
> > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > >         config CC_STACKPROTECTOR_REGULAR
> > > >                 bool
> > > >                 depends on CC_HAS_STACKPROTECTOR_REGULAR
> > > >
> > > >         config CC_STACKPROTECTOR_NONE
> > > >                 bool
> > > >
> > > > This version has the drawback of always showing all the options, even if
> > > > some they wouldn't be available. Kconfig comments could be added to warn
> > > > if an option isn't available at least:
> > > >
> > > >         comment "Warning: Your compiler does not support -fstack-protector-strong"
> > > >                 depends on !CC_HAS_STACKPROTECTOR_STRONG
> > > >
> > > >         config WANT_CC_STACKPROTECTOR_STRONG
> > > >                 ...
> > > >
> > > >
> > > >         comment "Warning: Your compiler does not support -fstack-protector"
> > > >                 depends on !CC_HAS_STACKPROTECTOR_REGULAR
> > > >
> > > >         config WANT_CC_STACKPROTECTOR_REGULAR
> > > >                 ...
> > > >
> > > > This final comment might be nice to have too:
> > > >
> > > >         comment "Warning: Selected stack protector not available"
> > > >                 depends on !(CC_STACKPROTECTOR_STRONG ||
> > > >                              CC_STACKPROTECTOR_REGULAR ||
> > > >                              CC_STACKPROTECTOR_NONE)
> > > >
> > > > Should probably introduce a clear warning that tells the user what they
> > > > need to change in Kconfig if they build with a broken selection too.
> > > >
> > > >
> > > > CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
> > > > way too. Maybe there's something neater.
> > > >
> > > >         config CC_STACKPROTECTOR_AUTO
> > > >                 bool "Automatic"
> > > >                 imply CC_STACKPROTECTOR_STRONG
> > > >                 imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
> > > >                 imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
> > > >                                                    !CC_HAS_STACKPROTECTOR_REGULAR
> > > >
> > > >
> > > > Another drawback of this approach is that it breaks existing .config
> > > > files (the CC_STACKPROTECTOR_* settings are ignored, since they just
> > > > look like "configuration output" to Kconfig now). If that'd be a
> > > > problem, the old names could be used instead of
> > > > WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
> > > > though it'd look a bit cryptic.
> > > >
> > > > Ideas?
> > > >
> > > 
> > > 
> > > 
> > > FWIW, the following is what I was playing with.
> > > (The idea for emitting warnings is Ulf's idea)
> > > 
> > > 
> > > ------------------>8-------------------
> > > config CC
> > >         string
> > >         option env="CC"
> > > 
> > > config CC_HAS_STACKPROTECTOR
> > >         bool
> > >         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > > 
> > > config CC_HAS_STACKPROTECTOR_STRONG
> > >         bool
> > >         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > > 
> > > config CC_HAS_STACKPROTECTOR_NONE
> > >         bool
> > >         option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> > > 
> > > config CC_STACKPROTECTOR
> > >         bool
> > > 
> > > choice
> > >         prompt "Stack Protector buffer overflow detection"
> > > 
> > > config CC_STACKPROTECTOR_AUTO
> > >         bool "Auto"
> > >         select CC_STACKPROTECTOR if (CC_HAS_STACKPROTECTOR || \
> > >                                      CC_HAS_STACKPROTECTOR_STRONG)
> > 
> > With this approach, I guess you would still need to handle the
> > CC_STACKPROTECTOR_AUTO logic outside of Kconfig, since e.g.
> > CC_STACKPROTECTOR_STRONG won't get enabled automatically if supported.
> > 
> > The idea above was to make it "internal" to the Kconfig files (though it
> > still gets written out), with the
> > CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} variables automatically getting
> > set as appropriate.
> 
> That was a confusing way of putting it -- sorry about that.
> 
> What I meant was that it would just be a user selection, with all the
> logic of selecting one of CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} being
> handled internally in the Kconfig files, even in the
> CC_STACKPROTECTOR_AUTO case.
> 
> Nothing outside of Kconfig would need to check CC_STACKPROTECTOR_AUTO
> then.
> 
> > 
> > The build could then the detect if none of
> > CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} are set and do what's
> > appropriate (error out in some semi-helpful way or whatever... not
> > deeply familiar with kernel policy here :).
> > 
> > > 
> > > config CC_STACKPROTECTOR_REGULAR
> > >         bool "Regular"
> > >         select CC_STACKPROTECTOR
> > > 
> > > config CC_STACKPROTECTOR_STRONG
> > >         bool "Strong"
> > >         select CC_STACKPROTECTOR
> > > 
> > > config CC_STACKPROTECTOR_NONE
> > >         bool "None"
> > > 
> > > endchoice
> > > 
> > > 
> > > comment "(WARNING) stackprotecter was chosen, but your compile does
> > > not support it.  Build will fail"
> > >         depends on CC_STACKPROTECTOR_REGULAR && \
> > >                    !CC_HAS_STACKPROTECTOR
> > > 
> > > comment "(WARNING) stackprotecter-strong was chosen, but your compile
> > > does not support it.  Build will fail"
> > >         depends on CC_STACKPROTECTOR_STRONG && \
> > >                    !CC_HAS_STACKPROTECTOR_STRONG
> > > ------------------------->8---------------------------------
> > > 
> > > 
> > > 
> > > 
> > > 
> > > BTW, setting option flags in Makefile is dirty, like follows:
> > > 
> > > 
> > > ccflags-$(CONFIG_CC_STACKPROTECTOR_STRONG)  += -fstack-protector-strong
> > > ccflags-$(CONFIG_CC_STACKPROTECTOR_REGULAR) += -fstack-protector
> > > 
> > > if ($(CONFIG_CC_STACKPROTECTOR_AUTO),y)
> > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR)        += -fstack-protector
> > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_STRONG) += -fstack-protector-strong
> > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> > > endif
> > > 
> > > if ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
> > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> > > endif
> > > 
> > > 
> > > 
> > > 
> > > One idea could be to calculate the compiler option in Kconfig.
> > > 
> > > config CC_OPT_STACKPROTECTOR
> > >         string
> > >         default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG || \
> > >                                              (CC_STACKPROTECTOR_AUTO && \
> > >                                               CC_HAS_STACKPROTECTOR_STRONG)
> > >         default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR || \
> > >                                               (CC_STACKPROTECTOR_AUTO && \
> > >                                                CC_HAS_STACKPROTECTOR)
> > >         default "-fno-stack-protector"        if CC_HAS_STACKPROTECTOR_NONE
> > 
> > If CC_STACKPROTECTOR_AUTO is made "internal", this could be simplified
> > to something like
> > 
> > 	config CC_OPT_STACKPROTECTOR
> > 		string
> > 		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
> > 		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
> > 		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
> > 		# If the compiler doesn't even support
> > 		# -fno-stack-protector
> > 		default ""
> > 
> > (Last default is just to make the empty string explicit. That's the
> > value it would get anyway.)
> > 
> > > 
> > > 
> > > 
> > > Makefile will become clean.
> > > Of course, this is at the cost of ugliness in Kconfig.
> > > 
> > > 
> > > 
> > > 
> > > -- 
> > > Best Regards
> > > Masahiro Yamada
> > 
> > Please tell me if I've misunderstood some aspect of the old behavior.
> > 
> > Cheers,
> > Ulf
> 
> Cheers,
> Ulf

Here's a complete updated example, with some stuff from Masahiro added.

Turns out warnings inside choices get cut off easily in menuconfig, so I
went with just a single warning instead (which should be enough anyway).

With this version, the only "outputs" that the Makefiles needs to look
at are CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} (and
CC_OPT_STACKPROTECTOR). WANT_CC_OPT_STACKPROTECTOR_AUTO is handled
automatically.

The caveat related to old .config files mentioned above still applies.

How many compilers don't support -fno-stack-protector by the way?

	config CC_HAS_STACKPROTECTOR_STRONG
		bool
		option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
	
	config CC_HAS_STACKPROTECTOR_REGULAR
		bool
		option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
	
	config CC_HAS_STACKPROTECTOR_NONE
		bool
		default y
		option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
	
	
	choice
		prompt "Stack Protector buffer overflow detection"
		default WANT_CC_STACKPROTECTOR_AUTO
	
	config WANT_CC_STACKPROTECTOR_AUTO
		bool "Automatic"
		imply CC_STACKPROTECTOR_STRONG
		imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
		imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
						   !CC_HAS_STACKPROTECTOR_REGULAR
	
	config WANT_CC_STACKPROTECTOR_STRONG
		bool "Strong"
		imply CC_STACKPROTECTOR_STRONG
	
	config WANT_CC_STACKPROTECTOR_REGULAR
		bool "Regular"
		imply CC_STACKPROTECTOR_REGULAR
	
	config WANT_CC_STACKPROTECTOR_NONE
		bool "None"
		imply CC_STACKPROTECTOR_NONE
	
	endchoice
	
	comment "Warning: Selected stack protector not available"
		depends on !(CC_STACKPROTECTOR_STRONG || \
			     CC_STACKPROTECTOR_REGULAR || \
			     CC_STACKPROTECTOR_NONE)
	
	
	config CC_STACKPROTECTOR_STRONG
		bool
		depends on CC_HAS_STACKPROTECTOR_STRONG
	
	config CC_STACKPROTECTOR_REGULAR
		bool
		depends on CC_HAS_STACKPROTECTOR_REGULAR
	
	config CC_STACKPROTECTOR_NONE
		bool
	
	
	config CC_OPT_STACKPROTECTOR
		string
		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
		# If the compiler doesn't even support
		# -fno-stack-protector
		default ""

Of course, at some point you're just moving complexity from one place to
another. Maybe this all-Kconfig approach isn't worth it if people find
it harder to understand. I don't know how bad the Makefiles are here at
the moment.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 10, 2018, 9:21 a.m. UTC | #10

On Sat, Feb 10, 2018 at 09:55:19AM +0100, Ulf Magnusson wrote:
> On Sat, Feb 10, 2018 at 09:05:56AM +0100, Ulf Magnusson wrote:
> > On Sat, Feb 10, 2018 at 08:49:24AM +0100, Ulf Magnusson wrote:
> > > On Sat, Feb 10, 2018 at 04:12:13PM +0900, Masahiro Yamada wrote:
> > > > 2018-02-10 14:48 GMT+09:00 Ulf Magnusson <[e-mail]>:
> > > > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote:
> > > > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <[e-mail]> wrote:
> > > > >> > One thing that makes Kconfig confusing (though it works well enough in
> > > > >> > practice) is that .config files both record user selections (the saved
> > > > >> > configuration) and serve as a configuration output format for make.
> > > > >> >
> > > > >> > It becomes easier to think about .config files once you realize that
> > > > >> > assignments to promptless symbols never have an effect on Kconfig
> > > > >> > itself: They're just configuration output, intermixed with the saved
> > > > >> > user selections.
> > > > >> >
> > > > >> > Assume 'option env' symbols got written out for example:
> > > > >> >
> > > > >> >         - For a non-user-assignable symbol, the entry in the .config
> > > > >> >           file is just configuration output and ignored by Kconfig,
> > > > >> >           which will fetch the value from the environment instead.
> > > > >> >
> > > > >> >         - For an assignable 'option env' symbol, the entry in the
> > > > >> >           .config file is a saved user selection (as well as
> > > > >> >           configuration output), and will be respected by Kconfig.
> > > > >>
> > > > >> In the stack-protector case, this becomes quite important, since the
> > > > >> goal is to record the user's selection regardless of compiler
> > > > >> capability. For example, if someone selects _REGULAR, it shouldn't
> > > > >> "upgrade" to _STRONG. (Similarly for _NONE.) Having _AUTO provides a
> > > > >> way to pick "best possible for this compiler", though. If a user had
> > > > >> previously selected _STRONG but they're doing builds with an older
> > > > >> compiler (or a misconfigured newer compiler) without support, the goal
> > > > >> is to _fail_ to build, not silently select _REGULAR.
> > > > >>
> > > > >> So, in this case, what's gained is the logic for _AUTO, and the logic
> > > > >> to not show, say, _STRONG when it's not available in the compiler. But
> > > > >> we must still fail to build if _STRONG was in the .config. It can't
> > > > >> silently rewrite it to _REGULAR because the compiler support for
> > > > >> _STRONG regressed.
> > > > >>
> > > > >> -Kees
> > > > >>
> > > > >> --
> > > > >> Kees Cook
> > > > >> Pixel Security
> > > > >
> > > > > Provided that would be the desired behavior:
> > > > >
> > > > > What about changing the meaning of the choice symbols from e.g. "select
> > > > > -fstack-protector-strong" to "want -fstack-protector-strong"? Then the
> > > > > user preference would always be remembered, regardless of what's
> > > > > available.
> > > > >
> > > > > Here's a proof-of-concept. I realized that the fancy new 'imply' keyword
> > > > > fits pretty well here, since it works like a dependency-respecting
> > > > > select.
> > > > >
> > > > >         config CC_HAS_STACKPROTECTOR_STRONG
> > > > >                 bool
> > > > >                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > > > >
> > > > >         config CC_HAS_STACKPROTECTOR
> > > > >                 bool
> > > > >                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > > > >
> > > > >
> > > > >         choice
> > > > >                 prompt "Stack Protector buffer overflow detection"
> > > > >                 default WANT_CC_STACKPROTECTOR_STRONG
> > > > >
> > > > >         config WANT_CC_STACKPROTECTOR_STRONG
> > > > >                 bool "Strong"
> > > > >                 imply CC_STACKPROTECTOR_STRONG
> > > > >
> > > > >         config WANT_CC_STACKPROTECTOR_REGULAR
> > > > >                 bool "Regular"
> > > > >                 imply CC_STACKPROTECTOR_REGULAR
> > > > >
> > > > >         config WANT_CC_STACKPROTECTOR_NONE
> > > > >                 bool "None"
> > > > >                 imply CC_STACKPROTECTOR_NONE
> > > > >
> > > > >         endchoice
> > > > >
> > > > >
> > > > >         config CC_STACKPROTECTOR_STRONG
> > > > >                 bool
> > > > >                 depends on CC_HAS_STACKPROTECTOR_STRONG
> > > > 
> > > > 
> > > > Do you mean
> > > > 
> > > >          config CC_STACKPROTECTOR_STRONG
> > > >                  bool
> > > >                  depends on CC_HAS_STACKPROTECTOR_STRONG && \
> > > >                             WANT_CC_STACKPROTECTOR_STRONG
> > > > 
> > > > or, maybe
> > > > 
> > > > 
> > > >          config CC_STACKPROTECTOR_STRONG
> > > >                  bool
> > > >                  depends on CC_HAS_STACKPROTECTOR_STRONG
> > > >                  default WANT_CC_STACKPROTECTOR_STRONG
> > > > 
> > > > ?
> > > 
> > > With the 'imply', it should work with just the 'depends on'. I had your
> > > last version earlier though, and it works too.
> > > 
> > > 'imply' kinda makes sense, as in "turn on the strong stack protector if
> > > its dependencies are satisfied".
> > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > >         config CC_STACKPROTECTOR_REGULAR
> > > > >                 bool
> > > > >                 depends on CC_HAS_STACKPROTECTOR_REGULAR
> > > > >
> > > > >         config CC_STACKPROTECTOR_NONE
> > > > >                 bool
> > > > >
> > > > > This version has the drawback of always showing all the options, even if
> > > > > some they wouldn't be available. Kconfig comments could be added to warn
> > > > > if an option isn't available at least:
> > > > >
> > > > >         comment "Warning: Your compiler does not support -fstack-protector-strong"
> > > > >                 depends on !CC_HAS_STACKPROTECTOR_STRONG
> > > > >
> > > > >         config WANT_CC_STACKPROTECTOR_STRONG
> > > > >                 ...
> > > > >
> > > > >
> > > > >         comment "Warning: Your compiler does not support -fstack-protector"
> > > > >                 depends on !CC_HAS_STACKPROTECTOR_REGULAR
> > > > >
> > > > >         config WANT_CC_STACKPROTECTOR_REGULAR
> > > > >                 ...
> > > > >
> > > > > This final comment might be nice to have too:
> > > > >
> > > > >         comment "Warning: Selected stack protector not available"
> > > > >                 depends on !(CC_STACKPROTECTOR_STRONG ||
> > > > >                              CC_STACKPROTECTOR_REGULAR ||
> > > > >                              CC_STACKPROTECTOR_NONE)
> > > > >
> > > > > Should probably introduce a clear warning that tells the user what they
> > > > > need to change in Kconfig if they build with a broken selection too.
> > > > >
> > > > >
> > > > > CC_STACKPROTECTOR_AUTO could be added to the choice in a slightly kludgy
> > > > > way too. Maybe there's something neater.
> > > > >
> > > > >         config CC_STACKPROTECTOR_AUTO
> > > > >                 bool "Automatic"
> > > > >                 imply CC_STACKPROTECTOR_STRONG
> > > > >                 imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
> > > > >                 imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
> > > > >                                                    !CC_HAS_STACKPROTECTOR_REGULAR
> > > > >
> > > > >
> > > > > Another drawback of this approach is that it breaks existing .config
> > > > > files (the CC_STACKPROTECTOR_* settings are ignored, since they just
> > > > > look like "configuration output" to Kconfig now). If that'd be a
> > > > > problem, the old names could be used instead of
> > > > > WANT_CC_STACKPROTECTOR_STRONG, etc., and new names introduced instead,
> > > > > though it'd look a bit cryptic.
> > > > >
> > > > > Ideas?
> > > > >
> > > > 
> > > > 
> > > > 
> > > > FWIW, the following is what I was playing with.
> > > > (The idea for emitting warnings is Ulf's idea)
> > > > 
> > > > 
> > > > ------------------>8-------------------
> > > > config CC
> > > >         string
> > > >         option env="CC"
> > > > 
> > > > config CC_HAS_STACKPROTECTOR
> > > >         bool
> > > >         option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > > > 
> > > > config CC_HAS_STACKPROTECTOR_STRONG
> > > >         bool
> > > >         option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > > > 
> > > > config CC_HAS_STACKPROTECTOR_NONE
> > > >         bool
> > > >         option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> > > > 
> > > > config CC_STACKPROTECTOR
> > > >         bool
> > > > 
> > > > choice
> > > >         prompt "Stack Protector buffer overflow detection"
> > > > 
> > > > config CC_STACKPROTECTOR_AUTO
> > > >         bool "Auto"
> > > >         select CC_STACKPROTECTOR if (CC_HAS_STACKPROTECTOR || \
> > > >                                      CC_HAS_STACKPROTECTOR_STRONG)
> > > 
> > > With this approach, I guess you would still need to handle the
> > > CC_STACKPROTECTOR_AUTO logic outside of Kconfig, since e.g.
> > > CC_STACKPROTECTOR_STRONG won't get enabled automatically if supported.
> > > 
> > > The idea above was to make it "internal" to the Kconfig files (though it
> > > still gets written out), with the
> > > CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} variables automatically getting
> > > set as appropriate.
> > 
> > That was a confusing way of putting it -- sorry about that.
> > 
> > What I meant was that it would just be a user selection, with all the
> > logic of selecting one of CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} being
> > handled internally in the Kconfig files, even in the
> > CC_STACKPROTECTOR_AUTO case.
> > 
> > Nothing outside of Kconfig would need to check CC_STACKPROTECTOR_AUTO
> > then.
> > 
> > > 
> > > The build could then the detect if none of
> > > CC_STACKPROTECTOR_{REGULAR,STRONG,NONE} are set and do what's
> > > appropriate (error out in some semi-helpful way or whatever... not
> > > deeply familiar with kernel policy here :).
> > > 
> > > > 
> > > > config CC_STACKPROTECTOR_REGULAR
> > > >         bool "Regular"
> > > >         select CC_STACKPROTECTOR
> > > > 
> > > > config CC_STACKPROTECTOR_STRONG
> > > >         bool "Strong"
> > > >         select CC_STACKPROTECTOR
> > > > 
> > > > config CC_STACKPROTECTOR_NONE
> > > >         bool "None"
> > > > 
> > > > endchoice
> > > > 
> > > > 
> > > > comment "(WARNING) stackprotecter was chosen, but your compile does
> > > > not support it.  Build will fail"
> > > >         depends on CC_STACKPROTECTOR_REGULAR && \
> > > >                    !CC_HAS_STACKPROTECTOR
> > > > 
> > > > comment "(WARNING) stackprotecter-strong was chosen, but your compile
> > > > does not support it.  Build will fail"
> > > >         depends on CC_STACKPROTECTOR_STRONG && \
> > > >                    !CC_HAS_STACKPROTECTOR_STRONG
> > > > ------------------------->8---------------------------------
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > BTW, setting option flags in Makefile is dirty, like follows:
> > > > 
> > > > 
> > > > ccflags-$(CONFIG_CC_STACKPROTECTOR_STRONG)  += -fstack-protector-strong
> > > > ccflags-$(CONFIG_CC_STACKPROTECTOR_REGULAR) += -fstack-protector
> > > > 
> > > > if ($(CONFIG_CC_STACKPROTECTOR_AUTO),y)
> > > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR)        += -fstack-protector
> > > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_STRONG) += -fstack-protector-strong
> > > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> > > > endif
> > > > 
> > > > if ($(CONFIG_CC_STACKPROTECTOR_NONE),y)
> > > > ccflags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE)   += -fno-stack-protector
> > > > endif
> > > > 
> > > > 
> > > > 
> > > > 
> > > > One idea could be to calculate the compiler option in Kconfig.
> > > > 
> > > > config CC_OPT_STACKPROTECTOR
> > > >         string
> > > >         default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG || \
> > > >                                              (CC_STACKPROTECTOR_AUTO && \
> > > >                                               CC_HAS_STACKPROTECTOR_STRONG)
> > > >         default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR || \
> > > >                                               (CC_STACKPROTECTOR_AUTO && \
> > > >                                                CC_HAS_STACKPROTECTOR)
> > > >         default "-fno-stack-protector"        if CC_HAS_STACKPROTECTOR_NONE
> > > 
> > > If CC_STACKPROTECTOR_AUTO is made "internal", this could be simplified
> > > to something like
> > > 
> > > 	config CC_OPT_STACKPROTECTOR
> > > 		string
> > > 		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
> > > 		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
> > > 		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
> > > 		# If the compiler doesn't even support
> > > 		# -fno-stack-protector
> > > 		default ""
> > > 
> > > (Last default is just to make the empty string explicit. That's the
> > > value it would get anyway.)
> > > 
> > > > 
> > > > 
> > > > 
> > > > Makefile will become clean.
> > > > Of course, this is at the cost of ugliness in Kconfig.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > Best Regards
> > > > Masahiro Yamada
> > > 
> > > Please tell me if I've misunderstood some aspect of the old behavior.
> > > 
> > > Cheers,
> > > Ulf
> > 
> > Cheers,
> > Ulf
> 
> Here's a complete updated example, with some stuff from Masahiro added.
> 
> Turns out warnings inside choices get cut off easily in menuconfig, so I
> went with just a single warning instead (which should be enough anyway).
> 
> With this version, the only "outputs" that the Makefiles needs to look
> at are CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} (and
> CC_OPT_STACKPROTECTOR). WANT_CC_OPT_STACKPROTECTOR_AUTO is handled
> automatically.
> 
> The caveat related to old .config files mentioned above still applies.
> 
> How many compilers don't support -fno-stack-protector by the way?
> 
> 	config CC_HAS_STACKPROTECTOR_STRONG
> 		bool
> 		option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> 	
> 	config CC_HAS_STACKPROTECTOR_REGULAR
> 		bool
> 		option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> 	
> 	config CC_HAS_STACKPROTECTOR_NONE
> 		bool
> 		default y

This default is left-over testing stuff. Sorry about that.

> 		option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> 	
> 	
> 	choice
> 		prompt "Stack Protector buffer overflow detection"
> 		default WANT_CC_STACKPROTECTOR_AUTO
> 	
> 	config WANT_CC_STACKPROTECTOR_AUTO
> 		bool "Automatic"
> 		imply CC_STACKPROTECTOR_STRONG
> 		imply CC_STACKPROTECTOR_REGULAR if !CC_HAS_STACKPROTECTOR_STRONG
> 		imply CC_STACKPROTECTOR_NONE    if !CC_HAS_STACKPROTECTOR_STRONG && \
> 						   !CC_HAS_STACKPROTECTOR_REGULAR
> 	
> 	config WANT_CC_STACKPROTECTOR_STRONG
> 		bool "Strong"
> 		imply CC_STACKPROTECTOR_STRONG
> 	
> 	config WANT_CC_STACKPROTECTOR_REGULAR
> 		bool "Regular"
> 		imply CC_STACKPROTECTOR_REGULAR
> 	
> 	config WANT_CC_STACKPROTECTOR_NONE
> 		bool "None"
> 		imply CC_STACKPROTECTOR_NONE
> 	
> 	endchoice
> 	
> 	comment "Warning: Selected stack protector not available"
> 		depends on !(CC_STACKPROTECTOR_STRONG || \
> 			     CC_STACKPROTECTOR_REGULAR || \
> 			     CC_STACKPROTECTOR_NONE)
> 	
> 	
> 	config CC_STACKPROTECTOR_STRONG
> 		bool
> 		depends on CC_HAS_STACKPROTECTOR_STRONG
> 	
> 	config CC_STACKPROTECTOR_REGULAR
> 		bool
> 		depends on CC_HAS_STACKPROTECTOR_REGULAR
> 	
> 	config CC_STACKPROTECTOR_NONE
> 		bool
> 	
> 	
> 	config CC_OPT_STACKPROTECTOR
> 		string
> 		default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
> 		default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
> 		default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
> 		# If the compiler doesn't even support
> 		# -fno-stack-protector
> 		default ""
> 
> Of course, at some point you're just moving complexity from one place to
> another. Maybe this all-Kconfig approach isn't worth it if people find
> it harder to understand. I don't know how bad the Makefiles are here at
> the moment.
> 
> Cheers,
> Ulf

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Randy Dunlap Feb. 10, 2018, 6:05 p.m. UTC | #11

On 02/10/2018 12:55 AM, Ulf Magnusson wrote:
> How many compilers don't support -fno-stack-protector by the way?
> 
> 	config CC_HAS_STACKPROTECTOR_STRONG
> 		bool
> 		option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> 	
> 	config CC_HAS_STACKPROTECTOR_REGULAR
> 		bool
> 		option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> 	
> 	config CC_HAS_STACKPROTECTOR_NONE
> 		bool
> 		default y
> 		option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"

I ran:
gcc -Werror -fno-stack-protector -c -x c /dev/null

It worked (gcc (SUSE Linux) 4.8.5) but it did leave a null.o file for me.
Might need to add that to 'make clean' or just rm it immediately.

Kees Cook Feb. 10, 2018, 7:23 p.m. UTC | #12

On Sat, Feb 10, 2018 at 12:55 AM, Ulf Magnusson <[e-mail]> wrote:
> Here's a complete updated example, with some stuff from Masahiro added.
>
> Turns out warnings inside choices get cut off easily in menuconfig, so I
> went with just a single warning instead (which should be enough anyway).
>
> With this version, the only "outputs" that the Makefiles needs to look
> at are CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} (and
> CC_OPT_STACKPROTECTOR). WANT_CC_OPT_STACKPROTECTOR_AUTO is handled
> automatically.
>
> The caveat related to old .config files mentioned above still applies.
>
> How many compilers don't support -fno-stack-protector by the way?
>
>         config CC_HAS_STACKPROTECTOR_STRONG
>                 bool
>                 option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
>
>         config CC_HAS_STACKPROTECTOR_REGULAR
>                 bool
>                 option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
>
>         config CC_HAS_STACKPROTECTOR_NONE
>                 bool
>                 default y
>                 option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"

Instead of just $CC for these, we need the test script that runs with
all the per-architecture flags configured and runs the actual _build_
test of the output. It is, unfortunately, not sufficient to test that
the compiler supports the flag: it has to be tested to produce the
correct output too, as there are continual regressions here (old
compilers, misbuilt compilers, misconfigured compilers, etc).

So, if this could do something like this:

        config CC_HAS_STACKPROTECTOR_STRONG
                bool
                option
shell="scripts/gcc-${ARCH}_${BITS}-has-stack-protector.sh $CC
$KBUILD_CPPFLAGS"

then this could all work from Kconfig.

>         choice
>                 prompt "Stack Protector buffer overflow detection"
>                 default WANT_CC_STACKPROTECTOR_AUTO

Otherwise, this WANT_ approach looks decent.

>         comment "Warning: Selected stack protector not available"
>                 depends on !(CC_STACKPROTECTOR_STRONG || \
>                              CC_STACKPROTECTOR_REGULAR || \
>                              CC_STACKPROTECTOR_NONE)

For WANT...AUTO, a warning is fine. for WANT...STRONG or
WANT...REGULAR this must fail the build.

>         config CC_OPT_STACKPROTECTOR
>                 string
>                 default "-fstack-protector-strong" if CC_STACKPROTECTOR_STRONG
>                 default "-fstack-protector"        if CC_STACKPROTECTOR_REGULAR
>                 default "-fno-stack-protector"     if CC_HAS_STACKPROTECTOR_NONE
>                 # If the compiler doesn't even support
>                 # -fno-stack-protector
>                 default ""
>
> Of course, at some point you're just moving complexity from one place to
> another. Maybe this all-Kconfig approach isn't worth it if people find
> it harder to understand. I don't know how bad the Makefiles are here at
> the moment.

FWIW, it feels more readable in Kconfig, if we can solve the circular
issue of $KBUILD_CPPFLAGS...

-Kees

Linus Torvalds Feb. 10, 2018, 8:08 p.m. UTC | #13

On Sat, Feb 10, 2018 at 11:23 AM, Kees Cook <[e-mail]> wrote:
>
> So, if this could do something like this:
>
>         config CC_HAS_STACKPROTECTOR_STRONG
>                 bool
>                 option
> shell="scripts/gcc-${ARCH}_${BITS}-has-stack-protector.sh $CC
> $KBUILD_CPPFLAGS"

Guys, this is not that important.

Don't make some stupid script for stackprotector. If the user doesn't
have a gcc that supports -fstackprotector-*, then don't show the
options. It matters NOT ONE WHIT whether that then means that
stackprotector will be off by default later.

Seriously. This is classic "Kees thinks that _his_ code is so
important that everybody should get the value _he_ cares about".

That's bullshit. Kees, get over yourself. It's a very common thing to
see, but it's WRONG. The fact that _you_ care about this doesn't mean
that everybody else should too.

The whole point was to simplify Kconfig, not to make it even worse.

                       Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Easton Feb. 11, 2018, 2 a.m. UTC | #14

On Sat, Feb 10, 2018 at 10:05:12AM -0800, Randy Dunlap wrote:
> On 02/10/2018 12:55 AM, Ulf Magnusson wrote:
> > How many compilers don't support -fno-stack-protector by the way?
> > 
> > 	config CC_HAS_STACKPROTECTOR_STRONG
> > 		bool
> > 		option shell="$CC -Werror -fstack-protector-strong -c -x c /dev/null"
> > 	
> > 	config CC_HAS_STACKPROTECTOR_REGULAR
> > 		bool
> > 		option shell="$CC -Werror -fstack-protector -c -x c /dev/null"
> > 	
> > 	config CC_HAS_STACKPROTECTOR_NONE
> > 		bool
> > 		default y
> > 		option shell="$CC -Werror -fno-stack-protector -c -x c /dev/null"
> 
> I ran:
> gcc -Werror -fno-stack-protector -c -x c /dev/null
> 
> It worked (gcc (SUSE Linux) 4.8.5) but it did leave a null.o file for me.
> Might need to add that to 'make clean' or just rm it immediately.

gcc -Werror -fno-stack-protector -c -x c /dev/null -o /dev/null

seems to DTRT without leaving anything behind.

    - Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 11, 2018, 4:13 a.m. UTC | #15

On Sat, Feb 10, 2018 at 12:08 PM, Linus Torvalds
<[e-mail]> wrote:
> On Sat, Feb 10, 2018 at 11:23 AM, Kees Cook <[e-mail]> wrote:
>>
>> So, if this could do something like this:
>>
>>         config CC_HAS_STACKPROTECTOR_STRONG
>>                 bool
>>                 option
>> shell="scripts/gcc-${ARCH}_${BITS}-has-stack-protector.sh $CC
>> $KBUILD_CPPFLAGS"
>
> Guys, this is not that important.
>
> Don't make some stupid script for stackprotector. If the user doesn't
> have a gcc that supports -fstackprotector-*, then don't show the
> options. It matters NOT ONE WHIT whether that then means that
> stackprotector will be off by default later.

What? Maybe you're misunderstanding the script? This script already exists:

$ ls scripts/gcc-x86_*
scripts/gcc-x86_32-has-stack-protector.sh
scripts/gcc-x86_64-has-stack-protector.sh

It's been there since the very beginning when Arjan added it to
validate that the compiler actually produces a stack protector when
you give it -fstack-protector. Older gccs broke this entirely, more
recent misconfigurations (as seen with some of Arnd's local gcc
builds) did similar, and there have been regressions in some versions
where gcc's x86 support flipped to the global canary instead of the
%gs-offset canary.

> Seriously. This is classic "Kees thinks that _his_ code is so
> important that everybody should get the value _he_ cares about".

I care about the kernel build informing people about what's gone wrong
as early as possible instead of producing an unbootable image that
takes forever to debug.

-Kees

Linus Torvalds Feb. 11, 2018, 4:46 a.m. UTC | #16

On Sat, Feb 10, 2018 at 8:13 PM, Kees Cook <[e-mail]> wrote:
>
> It's been there since the very beginning when Arjan added it to
> validate that the compiler actually produces a stack protector when
> you give it -fstack-protector. Older gccs broke this entirely, more
> recent misconfigurations (as seen with some of Arnd's local gcc
> builds) did similar, and there have been regressions in some versions
> where gcc's x86 support flipped to the global canary instead of the
> %gs-offset canary.

Argh. I wanted to get rid of all that entirely, and simplify this all.
The mentioned script (and bugzilla) was from 2006, I assumed this was
all historical.

But if it has broken again since, I guess we need to have a silly script. Grr.

But yes, I also reacted to your earlier " It can't silently rewrite it
to _REGULAR because the compiler support for _STRONG regressed."
Because it damn well can. If the compiler doesn't support
-fstack-protector-strong, we can just fall back on -fstack-protector.
Silently. No extra crazy complex logic for that either.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds Feb. 11, 2018, 7:28 a.m. UTC | #17

On Sat, Feb 10, 2018 at 8:46 PM, Linus Torvalds
<[e-mail]> wrote:
>
> Argh. I wanted to get rid of all that entirely, and simplify this all.
> The mentioned script (and bugzilla) was from 2006, I assumed this was
> all historical.
>
> But if it has broken again since, I guess we need to have a silly script. Grr.

Ok, so this really ended up bothering me.

I was hoping to really just unify all the stupid compiler flag testing
in just the Kconfig files and hoping we could really just use

    config CC_xyz
        bool
        option cc_option "-fwhatever-xyz"

to set them, and then build Kconfig rules from that:

    config USE_xyz
        bool "Some question that needs xyz"
        depends on CC_xyz

and have a nice simple

    ccflags-$(CONFIG_USE_xyz) += -fwhataver-xyz

in the Makefiles.

And one thought I had was "hey, if we need a script for
-fstack-protector, maybe we can simply standardize on _everything_
using a script".

But doing the stats, we test about two _hundred_  different compiler
options, and it really looks like -fstack-protector is the _only_ one
that uses a dedicated script. Everything else is just using the "see
if the compiler accepts the flag". So no, we wouldn't want to
standardize around a script.

We do have a script for some other build options related to gcc
breakage, but not command line flags per se: both 'asm goto' and for
gcc version generation. And gcc plugin compatibility checking.

Oh well. It looks like we really have to have those nasty exceptions
from the normal rules.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 11, 2018, 10:34 a.m. UTC | #18

Looks to me like there's a few unrelated issues here:


1. The stack protector support test scripts

Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
subtly broken kernel from being built.

A few questions:

    - How do things fail with a broken stack protector implementation?

    - How common are those broken compilers?

    - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
      or would a simpler static test work in practice?

      I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
      Kconfig, but should make sure it's actually needed in any case.

      The scripts are already split up as

          scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh

      by the way, though only gcc-x86_32-has-stack-protector.sh and
      gcc-x86_64-has-stack-protector.sh exist.

    - How old do you need to go with GCC for -fno-stack-protector to give an
      error (i.e., for not even the option to be recognized)? Is it still
      warranted to test for it?

Adding some CCs who worked on the stack protector test scripts.

And yeah, I was assuming that needing support scripts would be rare, and that
you'd usually just check whether gcc accepts the flag.

When you Google "gcc broken stack protector", the top hits about are about the
scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
-fno-PIE")).


2. Whether to hide the Kconfig stack protector alternatives or always show them

Or equivalently, whether to automatically fall back on other stack protector
alternatives (including no stack protector) if the one specified in the .config
isn't available.

I'll let you battle this one out. In any case, as a user, I'd want a
super-clear message telling me what to change if the build breaks because of
missing stack protector support.


3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles

I'd just go with whatever is simplest here. I don't find the Kconfig version
too bad, but I'm already very familiar with Kconfig, so it's harder for me to
tell how it looks to other people.

I'd add some comments to explain the idea in the final version.

@Kees:
I was assuming that the Makefiles would error out with a message if none of the
CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.

You could offload part of that check to Kconfig with something like

	config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
		def_bool CC_STACKPROTECTOR_STRONG || \
			 CC_STACKPROTECTOR_REGULAR || \
			 CC_STACKPROTECTOR_NONE

CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
It has the advantage of making the constraint clear in the Kconfig file
at least.

You could add some kind of assert feature to Kconfig too, but IMO it's not
warranted purely for one-offs like this at least.

That's details though. I'd want to explain it with a comment in any case if we
go with something like this, since it's slightly kludgy and subtle
(CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
express it like that directly, since it's derived from other symbols).


Here's an overview of the current Kconfig layout by the way, assuming
the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
implemented in Kconfig:

	# Feature tests
	CC_HAS_STACKPROTECTOR_STRONG
	CC_HAS_STACKPROTECTOR_REGULAR
	CC_HAS_STACKPROTECTOR_NONE

	# User request
	WANT_CC_STACKPROTECTOR_AUTO
	WANT_CC_STACKPROTECTOR_STRONG
	WANT_CC_STACKPROTECTOR_REGULAR
	WANT_CC_STACKPROTECTOR_NONE

	# The actual "output" to the Makefiles
	CC_STACKPROTECTOR_STRONG
	CC_STACKPROTECTOR_REGULAR
	CC_STACKPROTECTOR_NONE

	# Some possible output "nicities"
	CHOSEN_CC_STACKPROTECTOR_AVAILABLE
	CC_OPT_STACKPROTECTOR

Does anyone have objections to the naming or other things? I saw some
references to "Santa's wish list" in messages of commits that dealt with other
variables named WANT_*, though I didn't look into those cases. ;)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 11, 2018, 4:54 p.m. UTC | #19

On Sat, Feb 10, 2018 at 11:28 PM, Linus Torvalds
<[e-mail]> wrote:
> On Sat, Feb 10, 2018 at 8:46 PM, Linus Torvalds
> <[e-mail]> wrote:
>>
>> Argh. I wanted to get rid of all that entirely, and simplify this all.
>> The mentioned script (and bugzilla) was from 2006, I assumed this was
>> all historical.
>>
>> But if it has broken again since, I guess we need to have a silly script. Grr.
> [...]
> Oh well. It looks like we really have to have those nasty exceptions
> from the normal rules.

Yeah, I was really disappointed to discover the broken gcc case Arnd
had while I was testing the new ..._AUTO option. I thought I was going
to be able to throw away a whole bunch of the complexity too. :( And
this was on top of the recent discussion about raising the minimum gcc
level to a place where there wasn't any need for the "old broken gcc"
stack-protectors checks. But, no, that would have been too easy. :(

-Kees

Kees Cook Feb. 11, 2018, 5:56 p.m. UTC | #20

On Sun, Feb 11, 2018 at 2:34 AM, Ulf Magnusson <[e-mail]> wrote:
> Looks to me like there's a few unrelated issues here:
>
>
> 1. The stack protector support test scripts
>
> Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
> subtly broken kernel from being built.
>
> A few questions:
>
>     - How do things fail with a broken stack protector implementation?

There have been several ways I've seen:
- resulting kernel silently doesn't provide the stack protection at all
- resulting build fails at the end trying to link against a missing
global stack canary
- resulting kernel doesn't boot at all due to insane function preamble
on first use of canary

>     - How common are those broken compilers?

I *thought* it was rare (i.e. gcc 4.2) but while working on ..._AUTO I
found breakage in akpm's 4.4 gcc, and all of Arnd's gccs due to some
very strange misconfiguration between the gcc build environment and
other options. So, it turns out this is unfortunately common. The good
news is that it does NOT appear to happen with most distro compilers,
though I've seen Android's compiler regress the global vs %gs at least
once about a year ago.

>     - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
>       or would a simpler static test work in practice?
>
>       I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
>       Kconfig, but should make sure it's actually needed in any case.
>
>       The scripts are already split up as
>
>           scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh
>
>       by the way, though only gcc-x86_32-has-stack-protector.sh and
>       gcc-x86_64-has-stack-protector.sh exist.

I think it would work to skip KBUILD_CPPFLAGS right up until it
didn't. Since we have the arch split, we can already add -m32 to the
32-bit case, etc. However, I worry about interaction with other
selected build options. For example, while retpoline doesn't interact
stack-protector, it's easy to imagine things that might.

(And in thinking about this, does Kconfig know the true $CC in use?
i.e. the configured cross compiler, etc?)

>     - How old do you need to go with GCC for -fno-stack-protector to give an
>       error (i.e., for not even the option to be recognized)? Is it still
>       warranted to test for it?

Old? That's not the case. The check for -fno-stack-protector will
likely be needed forever, as some distro compilers enable
stack-protector by default. So when someone wants to explicitly build
without stack-protector (or if the compiler's stack-protector is
detected as broken), we must force it off for the kernel build.

> Adding some CCs who worked on the stack protector test scripts.
>
> And yeah, I was assuming that needing support scripts would be rare, and that
> you'd usually just check whether gcc accepts the flag.

That would have been nice, yes. :(

> When you Google "gcc broken stack protector", the top hits about are about the
> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
> -fno-PIE")).

That's just the most recent case (from the case where some distro
compilers enabled PIE by default). And was why I was hoping to drop
the scripts entirely.

> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>
> Or equivalently, whether to automatically fall back on other stack protector
> alternatives (including no stack protector) if the one specified in the .config
> isn't available.
>
> I'll let you battle this one out. In any case, as a user, I'd want a
> super-clear message telling me what to change if the build breaks because of
> missing stack protector support.

Yes, exactly.

The reason I built the _AUTO support was to make this easier for
people to not have to think about. I wanted to get rid of explicit
support (i.e. _REGULAR or _STRONG) but some people didn't want _STRONG
(since it has greater code-size impact than _REGULAR), so we've had to
keep that too.

> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>
> I'd just go with whatever is simplest here. I don't find the Kconfig version
> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
> tell how it looks to other people.
>
> I'd add some comments to explain the idea in the final version.
>
> @Kees:
> I was assuming that the Makefiles would error out with a message if none of the
> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.

That doesn't happy either before nor after _AUTO. The default value
before was _NONE, and the default value after is _AUTO, which will use
whatever is available.

> You could offload part of that check to Kconfig with something like
>
>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>                 def_bool CC_STACKPROTECTOR_STRONG || \
>                          CC_STACKPROTECTOR_REGULAR || \
>                          CC_STACKPROTECTOR_NONE
>
> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
> It has the advantage of making the constraint clear in the Kconfig file
> at least.
>
> You could add some kind of assert feature to Kconfig too, but IMO it's not
> warranted purely for one-offs like this at least.

Agreed; I want to do whatever we can to simplify things. :)

> That's details though. I'd want to explain it with a comment in any case if we
> go with something like this, since it's slightly kludgy and subtle
> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
> express it like that directly, since it's derived from other symbols).
>
>
> Here's an overview of the current Kconfig layout by the way, assuming
> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
> implemented in Kconfig:
>
>         # Feature tests
>         CC_HAS_STACKPROTECTOR_STRONG
>         CC_HAS_STACKPROTECTOR_REGULAR
>         CC_HAS_STACKPROTECTOR_NONE

As long as the feature tests include actual compiler output tests,
yeah, this seems fine.

>         # User request
>         WANT_CC_STACKPROTECTOR_AUTO
>         WANT_CC_STACKPROTECTOR_STRONG
>         WANT_CC_STACKPROTECTOR_REGULAR
>         WANT_CC_STACKPROTECTOR_NONE
>
>         # The actual "output" to the Makefiles
>         CC_STACKPROTECTOR_STRONG
>         CC_STACKPROTECTOR_REGULAR
>         CC_STACKPROTECTOR_NONE

This should be fine too (though by this point, I think Kconfig would
already know the specific option, so it could just pass it with a
single output (CC_OPT_STACKPROTECTOR below?)

>         # Some possible output "nicities"
>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>         CC_OPT_STACKPROTECTOR
>
> Does anyone have objections to the naming or other things? I saw some
> references to "Santa's wish list" in messages of commits that dealt with other
> variables named WANT_*, though I didn't look into those cases. ;)

Another case I mentioned before that I just want to make sure we don't
reintroduce the problem of getting "stuck" with a bad .config file.
While adding _STRONG support, I discovered the two-phase Kconfig
resolution that happens during the build. If you selected _STRONG with
a strong-capable compiler, everything was fine. If you then tried to
build with an older compiler, you'd get stuck since _STRONG wasn't
support (as detected during the first Kconfig phase) so the
generated/autoconf.h would never get updated with the newly selected
_REGULAR). I moved the Makefile analysis of available stack-protector
options into the second phase (i.e. after all the Kconfig runs), and
that worked to both unstick such configs and provide a clear message
early in the build about what wasn't available.

If all this detection is getting moved up into Kconfig, I'm worried
we'll end up in this state again. If the answer is "you have to delete
autoconf.h if you change compilers", then that's fine, but it sure
seems unfriendly. :)

-Kees

Linus Torvalds Feb. 11, 2018, 6:13 p.m. UTC | #21

On Sun, Feb 11, 2018 at 9:56 AM, Kees Cook <[e-mail]> wrote:
>
>>     - How common are those broken compilers?
>
> I *thought* it was rare (i.e. gcc 4.2) but while working on ..._AUTO I
> found breakage in akpm's 4.4 gcc, and all of Arnd's gccs due to some
> very strange misconfiguration between the gcc build environment and
> other options. So, it turns out this is unfortunately common. The good
> news is that it does NOT appear to happen with most distro compilers,
> though I've seen Android's compiler regress the global vs %gs at least
> once about a year ago.

Hmm. Ok, so it's not *that* common, and won't affect normal people.

That actually sounds like we could just

 (a) make gcc 4.5 be the minimum required version

 (b) actually error out if we find a bad compiler

Upgrading the minimum required gcc version to 4.5 is pretty much going
to happen _anyway_, because we're starting to rely on "asm goto" for
avoiding speculation.

End result: maybe we can make the configuration phase just use the
standard "does gcc support this flag" logic, and then just have a
separate script that is run to validate that gcc doesn't generate
garbage, and error out loudly if it does.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 11, 2018, 6:34 p.m. UTC | #22

On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
> Old? That's not the case. The check for -fno-stack-protector will
> likely be needed forever, as some distro compilers enable
> stack-protector by default. So when someone wants to explicitly build
> without stack-protector (or if the compiler's stack-protector is
> detected as broken), we must force it off for the kernel build.

What I meant is whether it makes sense to test if the
-fno-stack-protector option is supported. Can we reasonably assume
that passing -fno-stack-protector to the compiler won't cause an
error?

Is it possible to build GCC with no "no stack protector" support? Do
we need to support any compilers that would choke on the
-fno-stack-protector flag itself?

If we can reasonably assume that passing -fno-stack-protector is safe,
then CC_HAS_STACKPROTECTOR_NONE isn't needed.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 11, 2018, 7:39 p.m. UTC | #23

On Sun, Feb 11, 2018 at 10:13 AM, Linus Torvalds
<[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 9:56 AM, Kees Cook <[e-mail]> wrote:
>>
>>>     - How common are those broken compilers?
>>
>> I *thought* it was rare (i.e. gcc 4.2) but while working on ..._AUTO I
>> found breakage in akpm's 4.4 gcc, and all of Arnd's gccs due to some
>> very strange misconfiguration between the gcc build environment and
>> other options. So, it turns out this is unfortunately common. The good
>> news is that it does NOT appear to happen with most distro compilers,
>> though I've seen Android's compiler regress the global vs %gs at least
>> once about a year ago.
>
> Hmm. Ok, so it's not *that* common, and won't affect normal people.
>
> That actually sounds like we could just
>
>  (a) make gcc 4.5 be the minimum required version

I love bumping minimum for so many reason more than just stack protector. :)

>  (b) actually error out if we find a bad compiler

This made akpm and Arnd very very grumpy as it regressed their builds.
That's why I had to deal with the condition very carefully for _AUTO.

> Upgrading the minimum required gcc version to 4.5 is pretty much going
> to happen _anyway_, because we're starting to rely on "asm goto" for
> avoiding speculation.
>
> End result: maybe we can make the configuration phase just use the
> standard "does gcc support this flag" logic, and then just have a
> separate script that is run to validate that gcc doesn't generate
> garbage, and error out loudly if it does.

While it was entirely done in Makefile before, this is what we have
now (except no build failure in _AUTO mode). I think it'd be great to
push as much as possible into Kconfig, though.

One difference between what we have now and this proposal is that
right now, "best available option" detection includes the output test,
which means if you have a broken compiler you get a warning but the
build proceeds with "none" selected. If we only do flag detection,
then the build will fail during the make since the output is bad
(instead of fixing the flag to "none" and just warning).

-Kees

Linus Torvalds Feb. 11, 2018, 7:42 p.m. UTC | #24

On Sun, Feb 11, 2018 at 10:13 AM, Linus Torvalds
<[e-mail]> wrote:
>
> That actually sounds like we could just
>
>  (a) make gcc 4.5 be the minimum required version
>
>  (b) actually error out if we find a bad compiler

Just to explain why that's different from what we do not (apart from
the "error out" thing to actually actively discourage broken
compilers): it simplifies things if we can just add a trivial check as
part of the build process rather than as part of the configuration.

If we don't have to make it part of the configuration, we can use all
the normal Kconfig rules and build rules, and then we can add the
error check to any number of places where we already do various object
file munging.

For example, it would be pretty trivial to do the "check that there's
the right segment access" as part of link-vmlinux.sh or something.

And that would allow us to just use all the regular config
infrastructure, including the (hopefully by 4.17) "cc-option" thing at
Kconfig time.

             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds Feb. 11, 2018, 7:53 p.m. UTC | #25

On Sun, Feb 11, 2018 at 11:39 AM, Kees Cook <[e-mail]> wrote:
>>
>>  (a) make gcc 4.5 be the minimum required version
>
> I love bumping minimum for so many reason more than just stack protector. :)

Well, it's still not a very *big* bump. With modern distros being at
7.3, and people testing pre-releases of gcc-8, something like gcc-4.5
is still pretty darn ancient.

But it would be good to be able to rely on asm goto rather than have
completely different logic for "have to do it by hand". And I do
wonder how many of our "let's test if gcc supports this option" are
completely out-dated.

And in <linux/compiler-gcc.h> we still have tests for truly ancient garbage.

> This made akpm and Arnd very very grumpy as it regressed their builds.
> That's why I had to deal with the condition very carefully for _AUTO.

Well, Arnd build new cross-tools last week, probably because you
really need new tools for other reasons anyway (ie all the spectre
mitigation). So I think Arnd is set.

And akpm being on some ancient stone age system has been an issue
before. The only way to fix it is to break it.

What I would worry about primarily is not one of the odd developers
who can upgrade, but random people in the wild. I don't want to lose
the occasional odd tester that does things nobody else does.

But with gcc-4.5 being 7+ years old, I can't imagine it's a huge issue.

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds Feb. 11, 2018, 8:06 p.m. UTC | #26

On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds
<[e-mail]> wrote:
>
> Well, it's still not a very *big* bump. With modern distros being at
> 7.3, and people testing pre-releases of gcc-8, something like gcc-4.5
> is still pretty darn ancient.

... it's worth noting that our _documentation_ may claim that gcc-3.2
is the minimum supported version, but Arnd pointed out that a few
months ago that apparently nothing older than 4.1 has actually worked
for a longish while, and gcc-4.3 was needed on several architectures.

So the _real_ jump in required gcc version would be from 4.1 (4.3 in
many cases) to 4.5, not from our documented "3.2 minimum".

Arnd claimed that some architectures needed even newer-than-4.3, but I
assume that's limited to things like RISC-V that simply don't have old
gcc support at all.

That was from a discussion about bug report that only happened with
gcc-4.4, and was because gcc-4.4 did insane things, so we were talking
about how it wasn't necessarily worth supporting.

So we really have had a lot of unrelated reasons why just saying
"gcc-4.5 or newer"  would be a good thing.

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 11, 2018, 8:29 p.m. UTC | #27

On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
> Another case I mentioned before that I just want to make sure we don't
> reintroduce the problem of getting "stuck" with a bad .config file.
> While adding _STRONG support, I discovered the two-phase Kconfig
> resolution that happens during the build. If you selected _STRONG with
> a strong-capable compiler, everything was fine. If you then tried to
> build with an older compiler, you'd get stuck since _STRONG wasn't
> support (as detected during the first Kconfig phase) so the
> generated/autoconf.h would never get updated with the newly selected
> _REGULAR). I moved the Makefile analysis of available stack-protector
> options into the second phase (i.e. after all the Kconfig runs), and
> that worked to both unstick such configs and provide a clear message
> early in the build about what wasn't available.
>
> If all this detection is getting moved up into Kconfig, I'm worried
> we'll end up in this state again. If the answer is "you have to delete
> autoconf.h if you change compilers", then that's fine, but it sure
> seems unfriendly. :)

Did you mean include/config/auto.conf? That's the one that gets
included by the Makefiles.

If the feature detection is moved into Kconfig, you should only need
to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
you change the compiler. That will update .config while taking the new
features into account, and then the second phase during 'make' will
update include/config/auto.conf from .config.

That second Kconfig phase generates include/generated/autoconf.h and
include/config/. The include/config/ directory implements dependencies
between source files and Kconfig symbols by turning the symbols into
(empty) files. When building (during the "second phase"), Kconfig
compares .config with include/config/auto.conf to see what changed,
and signals the changes to 'make' by touch'ing the files corresponding
to the changed symbols. The idea is to avoid having to do a full
rebuild whenever the configuration is changed.

Check out scripts/basic/fixdep.c as well if you want to understand how it works.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 11, 2018, 8:42 p.m. UTC | #28

On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>> Another case I mentioned before that I just want to make sure we don't
>> reintroduce the problem of getting "stuck" with a bad .config file.
>> While adding _STRONG support, I discovered the two-phase Kconfig
>> resolution that happens during the build. If you selected _STRONG with
>> a strong-capable compiler, everything was fine. If you then tried to
>> build with an older compiler, you'd get stuck since _STRONG wasn't
>> support (as detected during the first Kconfig phase) so the
>> generated/autoconf.h would never get updated with the newly selected
>> _REGULAR). I moved the Makefile analysis of available stack-protector
>> options into the second phase (i.e. after all the Kconfig runs), and
>> that worked to both unstick such configs and provide a clear message
>> early in the build about what wasn't available.
>>
>> If all this detection is getting moved up into Kconfig, I'm worried
>> we'll end up in this state again. If the answer is "you have to delete
>> autoconf.h if you change compilers", then that's fine, but it sure
>> seems unfriendly. :)
>
> Did you mean include/config/auto.conf? That's the one that gets
> included by the Makefiles.
>
> If the feature detection is moved into Kconfig, you should only need
> to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
> you change the compiler. That will update .config while taking the new
> features into account, and then the second phase during 'make' will
> update include/config/auto.conf from .config.
>
> That second Kconfig phase generates include/generated/autoconf.h and
> include/config/. The include/config/ directory implements dependencies
> between source files and Kconfig symbols by turning the symbols into
> (empty) files. When building (during the "second phase"), Kconfig
> compares .config with include/config/auto.conf to see what changed,
> and signals the changes to 'make' by touch'ing the files corresponding
> to the changed symbols. The idea is to avoid having to do a full
> rebuild whenever the configuration is changed.
>
> Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>
> Cheers,
> Ulf

By the way:

That second phase is also a "normal" Kconfig run in the sense that it
does all the usual dependency checking stuff. Even if .config doesn't
respect dependencies, include/config/auto.conf will. So I think you
might not even need to rerun the configuration (though .config will be
out-of-date until you do).

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 11, 2018, 9:05 p.m. UTC | #29

On Sun, Feb 11, 2018 at 10:34 AM, Ulf Magnusson <[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>> Old? That's not the case. The check for -fno-stack-protector will
>> likely be needed forever, as some distro compilers enable
>> stack-protector by default. So when someone wants to explicitly build
>> without stack-protector (or if the compiler's stack-protector is
>> detected as broken), we must force it off for the kernel build.
>
> What I meant is whether it makes sense to test if the
> -fno-stack-protector option is supported. Can we reasonably assume
> that passing -fno-stack-protector to the compiler won't cause an
> error?

That isn't something I've tested; but I can check if it's useful.

> Is it possible to build GCC with no "no stack protector" support? Do
> we need to support any compilers that would choke on the
> -fno-stack-protector flag itself?
>
> If we can reasonably assume that passing -fno-stack-protector is safe,
> then CC_HAS_STACKPROTECTOR_NONE isn't needed.

Well, there are two situations:

- does the user want to build _without_ stack protector? (which is
something some people want to do, no matter what I think of it)

- did _AUTO discover that stack protector output is broken?

In both cases, we need to pass -fno-stack-protector in case the distro
compiler was built with stack protector enabled by default.

-Kees

Arnd Bergmann Feb. 11, 2018, 9:10 p.m. UTC | #30

On Sun, Feb 11, 2018 at 9:06 PM, Linus Torvalds
<[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds
> <[e-mail]> wrote:
>>
>> Well, it's still not a very *big* bump. With modern distros being at
>> 7.3, and people testing pre-releases of gcc-8, something like gcc-4.5
>> is still pretty darn ancient.
>
> ... it's worth noting that our _documentation_ may claim that gcc-3.2
> is the minimum supported version, but Arnd pointed out that a few
> months ago that apparently nothing older than 4.1 has actually worked
> for a longish while, and gcc-4.3 was needed on several architectures.
>
> So the _real_ jump in required gcc version would be from 4.1 (4.3 in
> many cases) to 4.5, not from our documented "3.2 minimum".
>
> Arnd claimed that some architectures needed even newer-than-4.3, but I
> assume that's limited to things like RISC-V that simply don't have old
> gcc support at all.

Right. Also architecture specific features may need something more recent,
and in some cases like the 'initializer for anonymous union needs extra
curly braces', a trivial change would make it work, but a lot of architectures
have obviously never been built with toolchains old enough to actually
run into those cases.

Geert is the only person I know that actively uses gcc-4.1, and he actually
sent some patches that seem to get additional architectures to build on
that version, when they were previously on gcc-4.3+.

gcc-4.3 in turn is used by default on SLES11, which is still in support,
and I've even worked with someone who used that compiler to build
new kernels, since that was what happened to be installed on his
shared build server. In this case, having gcc-4.3 actively refused to
force him to use a new compiler would have saved us some
debugging trouble.

In my tests last year, I identified gcc-4.6 as a nice minimum level, IIRC
gcc-4.5 was unable to build some of the newer ARM targets.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 11, 2018, 9:11 p.m. UTC | #31

On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds
<[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 11:39 AM, Kees Cook <[e-mail]> wrote:
>> This made akpm and Arnd very very grumpy as it regressed their builds.
>> That's why I had to deal with the condition very carefully for _AUTO.
>
> Well, Arnd build new cross-tools last week, probably because you
> really need new tools for other reasons anyway (ie all the spectre
> mitigation). So I think Arnd is set.
>
> And akpm being on some ancient stone age system has been an issue
> before. The only way to fix it is to break it.

I don't seem to be the one that can declare such things, which is why
I pulled _AUTO out of -mm before the 4.15 merge window. It was
breaking akpm and Arnd, which seemed like a serious problem,
especially since you've yelled about how it's very bad to break
builds, etc.

> What I would worry about primarily is not one of the odd developers
> who can upgrade, but random people in the wild. I don't want to lose
> the occasional odd tester that does things nobody else does.
>
> But with gcc-4.5 being 7+ years old, I can't imagine it's a huge issue.

You have no objection from me at all on this. I have been yelled at
repeatedly (unjustly in this thread even) for "forcing options" on
people. I worked very hard to NOT break people with this, so it didn't
seem like a good plan to knowingly break builds. (And the other
options, _REGULAR and _STRONG, _do_ actually break the build if you
have a broken compiler. But for _AUTO, that seemed like a very bad
idea. I could only imagine the flames.)

-Kees

Kees Cook Feb. 11, 2018, 9:19 p.m. UTC | #32

On Sun, Feb 11, 2018 at 1:10 PM, Arnd Bergmann <[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 9:06 PM, Linus Torvalds
> <[e-mail]> wrote:
>> On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds
>> <[e-mail]> wrote:
>>>
>>> Well, it's still not a very *big* bump. With modern distros being at
>>> 7.3, and people testing pre-releases of gcc-8, something like gcc-4.5
>>> is still pretty darn ancient.
>>
>> ... it's worth noting that our _documentation_ may claim that gcc-3.2
>> is the minimum supported version, but Arnd pointed out that a few
>> months ago that apparently nothing older than 4.1 has actually worked
>> for a longish while, and gcc-4.3 was needed on several architectures.
>>
>> So the _real_ jump in required gcc version would be from 4.1 (4.3 in
>> many cases) to 4.5, not from our documented "3.2 minimum".
>>
>> Arnd claimed that some architectures needed even newer-than-4.3, but I
>> assume that's limited to things like RISC-V that simply don't have old
>> gcc support at all.
>
> Right. Also architecture specific features may need something more recent,
> and in some cases like the 'initializer for anonymous union needs extra
> curly braces', a trivial change would make it work, but a lot of architectures
> have obviously never been built with toolchains old enough to actually
> run into those cases.
>
> Geert is the only person I know that actively uses gcc-4.1, and he actually
> sent some patches that seem to get additional architectures to build on
> that version, when they were previously on gcc-4.3+.
>
> gcc-4.3 in turn is used by default on SLES11, which is still in support,
> and I've even worked with someone who used that compiler to build
> new kernels, since that was what happened to be installed on his
> shared build server. In this case, having gcc-4.3 actively refused to
> force him to use a new compiler would have saved us some
> debugging trouble.
>
> In my tests last year, I identified gcc-4.6 as a nice minimum level, IIRC
> gcc-4.5 was unable to build some of the newer ARM targets.

For reference, the original discussion started here:
https://lkml.org/lkml/2016/12/16/174

I thread-necromancied it here:
https://lkml.org/lkml/2017/4/16/276

Modern analysis of compilers vs versions here:
https://lkml.org/lkml/2017/4/24/481

and seeming conclusion was here:
https://lkml.org/lkml/2017/4/25/66

Quoted:
>> - To keep it simple, we update the README.rst to say that a minimum
>>   gcc-4.3 is required, while recommending gcc-4.9 for all architectures
>> - Support for gcc-4.0 and earlier gets removed from linux/compiler.h,
>>   and instead we add a summary of what I found, explaining that
>>   gcc-4.1 has active users on a few architectures.
>> - We make the Makefile show a warning once during compilation for
>>   gcc earlier than 4.3.

But yes, if Linus wants 4.5 over 4.3, I would agree with Arnd: let's
take it to 4.6 instead.

-Kees

Ulf Magnusson Feb. 11, 2018, 9:22 p.m. UTC | #33

On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>>
>> I'd just go with whatever is simplest here. I don't find the Kconfig version
>> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
>> tell how it looks to other people.
>>
>> I'd add some comments to explain the idea in the final version.
>>
>> @Kees:
>> I was assuming that the Makefiles would error out with a message if none of the
>> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>
> That doesn't happy either before nor after _AUTO. The default value
> before was _NONE, and the default value after is _AUTO, which will use
> whatever is available.

I was thinking in the case where you explicitly select one of
CC_STACKPROTECTOR_{STRONG,REGULAR} and it isn't available. With the
new WANT_* version, that will cause none of
CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} to be set, and in that case
you'd error out to match the old behavior (if I understand it
correctly).

CHOSEN_CC_STACKPROTECTOR_AVAILABLE would just be a helper symbol to
detect that case. It's not like it's a huge deal to detect it on the
Makefile end either, but turns it pretty nice and readable, IMO, with
more of the logic in a single location.

>>         # User request
>>         WANT_CC_STACKPROTECTOR_AUTO
>>         WANT_CC_STACKPROTECTOR_STRONG
>>         WANT_CC_STACKPROTECTOR_REGULAR
>>         WANT_CC_STACKPROTECTOR_NONE
>>
>>         # The actual "output" to the Makefiles
>>         CC_STACKPROTECTOR_STRONG
>>         CC_STACKPROTECTOR_REGULAR
>>         CC_STACKPROTECTOR_NONE
>
> This should be fine too (though by this point, I think Kconfig would
> already know the specific option, so it could just pass it with a
> single output (CC_OPT_STACKPROTECTOR below?)

Ah, yeah, that'd be simpler if all that's needed is the compiler flag.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 11, 2018, 9:35 p.m. UTC | #34

On Sun, Feb 11, 2018 at 10:05 PM, Kees Cook <[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 10:34 AM, Ulf Magnusson <[e-mail]> wrote:
>> On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>>> Old? That's not the case. The check for -fno-stack-protector will
>>> likely be needed forever, as some distro compilers enable
>>> stack-protector by default. So when someone wants to explicitly build
>>> without stack-protector (or if the compiler's stack-protector is
>>> detected as broken), we must force it off for the kernel build.
>>
>> What I meant is whether it makes sense to test if the
>> -fno-stack-protector option is supported. Can we reasonably assume
>> that passing -fno-stack-protector to the compiler won't cause an
>> error?
>
> That isn't something I've tested; but I can check if it's useful.

If it gets rid of a pointless test and symbol, I think it's useful, so
that would be nice. :)

>> Is it possible to build GCC with no "no stack protector" support? Do
>> we need to support any compilers that would choke on the
>> -fno-stack-protector flag itself?
>>
>> If we can reasonably assume that passing -fno-stack-protector is safe,
>> then CC_HAS_STACKPROTECTOR_NONE isn't needed.
>
> Well, there are two situations:
>
> - does the user want to build _without_ stack protector? (which is
> something some people want to do, no matter what I think of it)
>
> - did _AUTO discover that stack protector output is broken?
>
> In both cases, we need to pass -fno-stack-protector in case the distro
> compiler was built with stack protector enabled by default.

Yup, that's already the way it would work. Currently, there's also a
test for whether the compiler supports -fno-stack-protector. It's that
one that I suspect we might be able to get rid of.

Cheers,
Ulf "should merge replies"
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds Feb. 11, 2018, 9:50 p.m. UTC | #35

On Sun, Feb 11, 2018 at 1:19 PM, Kees Cook <[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 1:10 PM, Arnd Bergmann <[e-mail]> wrote:
>>
>> In my tests last year, I identified gcc-4.6 as a nice minimum level, IIRC
>> gcc-4.5 was unable to build some of the newer ARM targets.
>
> But yes, if Linus wants 4.5 over 4.3, I would agree with Arnd: let's
> take it to 4.6 instead.

So it sounds like Arnd knows what the distros have.

Because I think that would actually be the best way to try to
determine where we want to go, because it's what is going to determine
what is most problematic for _users_.

If no distro is on 4.5, then there's no reason to pick that. The
reason I mentioned 4.5 is because that's the "asm goto" point, afaik,
and that is likely to be a requirement in the near future.

If SLES11 is 4.3, that's obviously a concern. Although Arnd seemed to
imply that that had already caused problems, so...

            Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Feb. 11, 2018, 10:29 p.m. UTC | #36

On Sun, Feb 11, 2018 at 10:10 PM, Arnd Bergmann <[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 9:06 PM, Linus Torvalds
> <[e-mail]> wrote:
>> On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds
>> <[e-mail]> wrote:
>>>
>>> Well, it's still not a very *big* bump. With modern distros being at
>>> 7.3, and people testing pre-releases of gcc-8, something like gcc-4.5
>>> is still pretty darn ancient.
>>
>> ... it's worth noting that our _documentation_ may claim that gcc-3.2
>> is the minimum supported version, but Arnd pointed out that a few
>> months ago that apparently nothing older than 4.1 has actually worked
>> for a longish while, and gcc-4.3 was needed on several architectures.
>>
>> So the _real_ jump in required gcc version would be from 4.1 (4.3 in
>> many cases) to 4.5, not from our documented "3.2 minimum".
>>
>> Arnd claimed that some architectures needed even newer-than-4.3, but I
>> assume that's limited to things like RISC-V that simply don't have old
>> gcc support at all.
>
> Right. Also architecture specific features may need something more recent,
> and in some cases like the 'initializer for anonymous union needs extra
> curly braces', a trivial change would make it work, but a lot of architectures
> have obviously never been built with toolchains old enough to actually
> run into those cases.
>
> Geert is the only person I know that actively uses gcc-4.1, and he actually
> sent some patches that seem to get additional architectures to build on
> that version, when they were previously on gcc-4.3+.

And as long as gcc-4.1 helps me finding real bugs (which it did for the
current merge window), I plan to keep on using it.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arnd Bergmann Feb. 12, 2018, 10:44 a.m. UTC | #37

On Sun, Feb 11, 2018 at 10:50 PM, Linus Torvalds
<[e-mail]> wrote:
> On Sun, Feb 11, 2018 at 1:19 PM, Kees Cook <[e-mail]> wrote:
>> On Sun, Feb 11, 2018 at 1:10 PM, Arnd Bergmann <[e-mail]> wrote:
>>>
>>> In my tests last year, I identified gcc-4.6 as a nice minimum level, IIRC
>>> gcc-4.5 was unable to build some of the newer ARM targets.
>>
>> But yes, if Linus wants 4.5 over 4.3, I would agree with Arnd: let's
>> take it to 4.6 instead.
>
> So it sounds like Arnd knows what the distros have.
...
> If no distro is on 4.5, then there's no reason to pick that. The
> reason I mentioned 4.5 is because that's the "asm goto" point, afaik,
> and that is likely to be a requirement in the near future.

Looking at old distros with long support cycles:

Red Hat have either gcc-4.1 (EL5, extended support ends 2020) or
gcc-4.4 (EL6, regular support ends 2020, extended support ends 2024):
https://access.redhat.com/solutions/19458
EL7 uses gcc-4.8 and will be supported until 2024 (no extended support
planned)

SUSE have gcc-4.3 (SLES11, extended support ends 2022) or gcc-4.8
(SLES12, support ends 2024, extended support ends 2027):
https://www.suse.com/lifecycle/

Debian Jessie (oldstable) comes with gcc-4.8 and is supported until June 2018,
extended support until 2020
Debian  Wheezy (oldoldstable) uses gcc-4.6 or 4.7 depending on the
architecture, extended support ends May 2018.

Ubuntu 14.04 is supported until 2019 and uses gcc-4.8

The latest Android SDK provides (known broken) versions of gcc-4.8 and
gcc-4.9 as well as clang.

OpenWRT 14.07 Barrier Breaker uses gcc-4.8, 12.07 Attitude
Adjustment 12.09 used gcc-4.6, but it's very unlikely that anyone
cares about building new kernels with either.

Most embedded distros just build everything from source and are
used to adapting to new requirements.

From that list above, it sounds like going all the way to gcc-4.8 would
be a better candidate than 4.5 or 4.6, if we decide that 4.3 and 4.4 are
both no longer desirable to support.

> If SLES11 is 4.3, that's obviously a concern. Although Arnd seemed to
> imply that that had already caused problems, so...

The problems are mainly an excessive amount of false-positive
warnings, which makes it tricky to new warnings when you make
a mistake.

          Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 12, 2018, 10:44 a.m. UTC | #38

2018-02-11 19:34 GMT+09:00 Ulf Magnusson <[e-mail]>:
> Looks to me like there's a few unrelated issues here:
>
>
> 1. The stack protector support test scripts
>
> Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
> subtly broken kernel from being built.
>
> A few questions:
>
>     - How do things fail with a broken stack protector implementation?
>
>     - How common are those broken compilers?
>
>     - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
>       or would a simpler static test work in practice?
>
>       I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
>       Kconfig, but should make sure it's actually needed in any case.
>
>       The scripts are already split up as
>
>           scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh
>
>       by the way, though only gcc-x86_32-has-stack-protector.sh and
>       gcc-x86_64-has-stack-protector.sh exist.
>
>     - How old do you need to go with GCC for -fno-stack-protector to give an
>       error (i.e., for not even the option to be recognized)? Is it still
>       warranted to test for it?
>
> Adding some CCs who worked on the stack protector test scripts.
>
> And yeah, I was assuming that needing support scripts would be rare, and that
> you'd usually just check whether gcc accepts the flag.
>
> When you Google "gcc broken stack protector", the top hits about are about the
> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
> -fno-PIE")).
>
>
> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>
> Or equivalently, whether to automatically fall back on other stack protector
> alternatives (including no stack protector) if the one specified in the .config
> isn't available.
>
> I'll let you battle this one out. In any case, as a user, I'd want a
> super-clear message telling me what to change if the build breaks because of
> missing stack protector support.
>
>
> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>
> I'd just go with whatever is simplest here. I don't find the Kconfig version
> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
> tell how it looks to other people.
>
> I'd add some comments to explain the idea in the final version.
>
> @Kees:
> I was assuming that the Makefiles would error out with a message if none of the
> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>
> You could offload part of that check to Kconfig with something like
>
>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>                 def_bool CC_STACKPROTECTOR_STRONG || \
>                          CC_STACKPROTECTOR_REGULAR || \
>                          CC_STACKPROTECTOR_NONE
>
> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
> It has the advantage of making the constraint clear in the Kconfig file
> at least.
>
> You could add some kind of assert feature to Kconfig too, but IMO it's not
> warranted purely for one-offs like this at least.
>
> That's details though. I'd want to explain it with a comment in any case if we
> go with something like this, since it's slightly kludgy and subtle
> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
> express it like that directly, since it's derived from other symbols).
>
>
> Here's an overview of the current Kconfig layout by the way, assuming
> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
> implemented in Kconfig:
>
>         # Feature tests
>         CC_HAS_STACKPROTECTOR_STRONG
>         CC_HAS_STACKPROTECTOR_REGULAR
>         CC_HAS_STACKPROTECTOR_NONE
>
>         # User request
>         WANT_CC_STACKPROTECTOR_AUTO
>         WANT_CC_STACKPROTECTOR_STRONG
>         WANT_CC_STACKPROTECTOR_REGULAR
>         WANT_CC_STACKPROTECTOR_NONE
>
>         # The actual "output" to the Makefiles
>         CC_STACKPROTECTOR_STRONG
>         CC_STACKPROTECTOR_REGULAR
>         CC_STACKPROTECTOR_NONE
>
>         # Some possible output "nicities"
>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>         CC_OPT_STACKPROTECTOR
>
> Does anyone have objections to the naming or other things? I saw some
> references to "Santa's wish list" in messages of commits that dealt with other
> variables named WANT_*, though I didn't look into those cases. ;)
>



I think Linus's comment was dismissed here.


Linus said:

> But yes, I also reacted to your earlier " It can't silently rewrite it
> to _REGULAR because the compiler support for _STRONG regressed."
> Because it damn well can. If the compiler doesn't support
> -fstack-protector-strong, we can just fall back on -fstack-protector.
> Silently. No extra crazy complex logic for that either.


If I understood his comment correctly,
we do not need either WANT_* or _AUTO.




Kees' comment:

> In the stack-protector case, this becomes quite important, since the
> goal is to record the user's selection regardless of compiler
> capability. For example, if someone selects _REGULAR, it shouldn't
> "upgrade" to _STRONG. (Similarly for _NONE.)

No.  Kconfig will not do this silently.


"make oldconfig" (or "make silentoldconfig" as well)
always ask users about new symbols.


For example, let's say your compiler supports -fstack-protector
but not -fstack-protector-strong.
CC_STACKPROTECTOR_REGULAR is the best you can choose.

Then, let's say you upgrade your compiler and the new compiler supports
-fstack-protector-strong as well.

In this case, CC_STACKPROTECTOR_STRONG is a newly visible symbol,
so Kconfig will ask you
"Hey, you were previously using _REGULAR, but your new compiler
also supports _STRONG.  Do you want to use it?"


The "make oldconfig" will look like follows:


masahiro@grover:~/workspace/linux-kbuild$ make oldconfig
  HOSTCC  scripts/kconfig/conf.o
  HOSTLD  scripts/kconfig/conf
scripts/kconfig/conf  --oldconfig Kconfig
*
* Restart config...
*
*
* Linux Kernel Configuration
*
Stack Protector buffer overflow detection
  1. Strong (CC_STACKPROTECTOR_STRONG) (NEW)
> 2. Regular (CC_STACKPROTECTOR_REGULAR)
  3. None (CC_STACKPROTECTOR_NONE)
choice[1-3?]:



Please notice the Strong is marked as "(NEW)".

Kconfig handles this really nicely with Linus' suggestion.

[1] Users can select only features supported by your compiler
    - this makes sense.

[2] If you upgrade your compiler and it provides more capability,
    "make (silent)oldconfig" will ask you about new choices.
     - this also makes sense.



So, the following simple implementation works well enough,
doesn't it?

---------------->8---------------
choice
        prompt "Stack Protector buffer overflow detection"

config CC_STACKPROTECTOR_STRONG
        bool "Strong"
        depends on CC_HAS_STACKPROTECTOR_STRONG
        select CC_STACKPROTECTOR

config CC_STACKPROTECTOR_REGULAR
        bool "Regular"
        depends on CC_HAS_STACKPROTECTOR
        select CC_STACKPROTECTOR

config CC_STACKPROTECTOR_NONE
        bool "None"

endchoice
---------------->8-------------------------



BTW, do we need to use 'choice' ?


The problem of using 'choice' is,
it does not work well with allnoconfig.


For all{yes,mod}config, we want to enable as many features as possible.
For allnoconfig, we want to disable as many as possible.

However, the default of 'choice' is always the first visible value.


So, I can suggest to remove _REGULAR and _NONE.

We have just two bool options, like follows.

------------------->8-----------------
config CC_STACKPROTECTOR
        bool "Use stack protector"
        depends on CC_HAS_STACKPROTECTOR

config CC_STACKPROTECTOR_STRONG
        bool "Use strong strong stack protector"
        depends on CC_STACKPROTECTOR
        depends on CC_HAS_STACKPROTECTOR_STRONG
-------------------->8------------------

This will work well for all{yes,mod,no}config.

We will not have a case where
-fstack-protector-strong is supported, but -fstack-protector is not.

Ulf Magnusson Feb. 12, 2018, 11:44 a.m. UTC | #39

On Mon, Feb 12, 2018 at 11:44 AM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-11 19:34 GMT+09:00 Ulf Magnusson <[e-mail]>:
>> Looks to me like there's a few unrelated issues here:
>>
>>
>> 1. The stack protector support test scripts
>>
>> Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
>> subtly broken kernel from being built.
>>
>> A few questions:
>>
>>     - How do things fail with a broken stack protector implementation?
>>
>>     - How common are those broken compilers?
>>
>>     - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
>>       or would a simpler static test work in practice?
>>
>>       I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
>>       Kconfig, but should make sure it's actually needed in any case.
>>
>>       The scripts are already split up as
>>
>>           scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh
>>
>>       by the way, though only gcc-x86_32-has-stack-protector.sh and
>>       gcc-x86_64-has-stack-protector.sh exist.
>>
>>     - How old do you need to go with GCC for -fno-stack-protector to give an
>>       error (i.e., for not even the option to be recognized)? Is it still
>>       warranted to test for it?
>>
>> Adding some CCs who worked on the stack protector test scripts.
>>
>> And yeah, I was assuming that needing support scripts would be rare, and that
>> you'd usually just check whether gcc accepts the flag.
>>
>> When you Google "gcc broken stack protector", the top hits about are about the
>> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
>> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
>> -fno-PIE")).
>>
>>
>> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>>
>> Or equivalently, whether to automatically fall back on other stack protector
>> alternatives (including no stack protector) if the one specified in the .config
>> isn't available.
>>
>> I'll let you battle this one out. In any case, as a user, I'd want a
>> super-clear message telling me what to change if the build breaks because of
>> missing stack protector support.
>>
>>
>> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>>
>> I'd just go with whatever is simplest here. I don't find the Kconfig version
>> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
>> tell how it looks to other people.
>>
>> I'd add some comments to explain the idea in the final version.
>>
>> @Kees:
>> I was assuming that the Makefiles would error out with a message if none of the
>> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>>
>> You could offload part of that check to Kconfig with something like
>>
>>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>                 def_bool CC_STACKPROTECTOR_STRONG || \
>>                          CC_STACKPROTECTOR_REGULAR || \
>>                          CC_STACKPROTECTOR_NONE
>>
>> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
>> It has the advantage of making the constraint clear in the Kconfig file
>> at least.
>>
>> You could add some kind of assert feature to Kconfig too, but IMO it's not
>> warranted purely for one-offs like this at least.
>>
>> That's details though. I'd want to explain it with a comment in any case if we
>> go with something like this, since it's slightly kludgy and subtle
>> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
>> express it like that directly, since it's derived from other symbols).
>>
>>
>> Here's an overview of the current Kconfig layout by the way, assuming
>> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
>> implemented in Kconfig:
>>
>>         # Feature tests
>>         CC_HAS_STACKPROTECTOR_STRONG
>>         CC_HAS_STACKPROTECTOR_REGULAR
>>         CC_HAS_STACKPROTECTOR_NONE
>>
>>         # User request
>>         WANT_CC_STACKPROTECTOR_AUTO
>>         WANT_CC_STACKPROTECTOR_STRONG
>>         WANT_CC_STACKPROTECTOR_REGULAR
>>         WANT_CC_STACKPROTECTOR_NONE
>>
>>         # The actual "output" to the Makefiles
>>         CC_STACKPROTECTOR_STRONG
>>         CC_STACKPROTECTOR_REGULAR
>>         CC_STACKPROTECTOR_NONE
>>
>>         # Some possible output "nicities"
>>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>         CC_OPT_STACKPROTECTOR
>>
>> Does anyone have objections to the naming or other things? I saw some
>> references to "Santa's wish list" in messages of commits that dealt with other
>> variables named WANT_*, though I didn't look into those cases. ;)
>>
>
>
>
> I think Linus's comment was dismissed here.
>
>
> Linus said:
>
>> But yes, I also reacted to your earlier " It can't silently rewrite it
>> to _REGULAR because the compiler support for _STRONG regressed."
>> Because it damn well can. If the compiler doesn't support
>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>> Silently. No extra crazy complex logic for that either.
>
>
> If I understood his comment correctly,
> we do not need either WANT_* or _AUTO.
>
>
>
>
> Kees' comment:
>
>> In the stack-protector case, this becomes quite important, since the
>> goal is to record the user's selection regardless of compiler
>> capability. For example, if someone selects _REGULAR, it shouldn't
>> "upgrade" to _STRONG. (Similarly for _NONE.)
>
> No.  Kconfig will not do this silently.

(Playing devil's advocate...)

If the user selected _STRONG and it becomes unavailable later, it
seems to silently fall back on other options, even for oldnoconfig
(which just checks if there are any new symbols in the choice).

It would be possible to also check if the old user selection still
applies btw. I do that in Kconfiglib. It's arguable whether that
matches the intent of oldnoconfig.

>
>
> "make oldconfig" (or "make silentoldconfig" as well)
> always ask users about new symbols.
>
>
> For example, let's say your compiler supports -fstack-protector
> but not -fstack-protector-strong.
> CC_STACKPROTECTOR_REGULAR is the best you can choose.
>
> Then, let's say you upgrade your compiler and the new compiler supports
> -fstack-protector-strong as well.
>
> In this case, CC_STACKPROTECTOR_STRONG is a newly visible symbol,
> so Kconfig will ask you
> "Hey, you were previously using _REGULAR, but your new compiler
> also supports _STRONG.  Do you want to use it?"
>
>
> The "make oldconfig" will look like follows:
>
>
> masahiro@grover:~/workspace/linux-kbuild$ make oldconfig
>   HOSTCC  scripts/kconfig/conf.o
>   HOSTLD  scripts/kconfig/conf
> scripts/kconfig/conf  --oldconfig Kconfig
> *
> * Restart config...
> *
> *
> * Linux Kernel Configuration
> *
> Stack Protector buffer overflow detection
>   1. Strong (CC_STACKPROTECTOR_STRONG) (NEW)
>> 2. Regular (CC_STACKPROTECTOR_REGULAR)
>   3. None (CC_STACKPROTECTOR_NONE)
> choice[1-3?]:
>
>
>
> Please notice the Strong is marked as "(NEW)".
>
> Kconfig handles this really nicely with Linus' suggestion.
>
> [1] Users can select only features supported by your compiler
>     - this makes sense.
>
> [2] If you upgrade your compiler and it provides more capability,
>     "make (silent)oldconfig" will ask you about new choices.
>      - this also makes sense.

If you switch to a compiler that provides less capability, it'll
silently forget the old user preference though.

>
>
>
> So, the following simple implementation works well enough,
> doesn't it?
>
> ---------------->8---------------
> choice
>         prompt "Stack Protector buffer overflow detection"
>
> config CC_STACKPROTECTOR_STRONG
>         bool "Strong"
>         depends on CC_HAS_STACKPROTECTOR_STRONG
>         select CC_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_REGULAR
>         bool "Regular"
>         depends on CC_HAS_STACKPROTECTOR
>         select CC_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_NONE
>         bool "None"
>
> endchoice
> ---------------->8-------------------------

Obviously much neater at least. :)

>
>
>
> BTW, do we need to use 'choice' ?
>
>
> The problem of using 'choice' is,
> it does not work well with allnoconfig.
>
>
> For all{yes,mod}config, we want to enable as many features as possible.
> For allnoconfig, we want to disable as many as possible.

Oh... right. For allnoconfig, it would always pick the default, so if
the default is "on", it's bad there.

>
> However, the default of 'choice' is always the first visible value.
>
>
> So, I can suggest to remove _REGULAR and _NONE.
>
> We have just two bool options, like follows.
>
> ------------------->8-----------------
> config CC_STACKPROTECTOR
>         bool "Use stack protector"
>         depends on CC_HAS_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_STRONG
>         bool "Use strong strong stack protector"
>         depends on CC_STACKPROTECTOR
>         depends on CC_HAS_STACKPROTECTOR_STRONG
> -------------------->8------------------

Even neater. Much easier to understand.

>
> This will work well for all{yes,mod,no}config.
>
> We will not have a case where
> -fstack-protector-strong is supported, but -fstack-protector is not.
>
>
> --
> Best Regards
> Masahiro Yamada

So there's just the caveat about possibly "forgetting" the user
preferences and automatically falling back on
CC_STACKPROTECTOR_REGULAR or CC_STACKPROTECTOR_NONE when the
environment changes, instead of erroring out.

When you have to think hard about cases where something might break,
it might be a sign that it's not a huge problem in practice, but I
can't really speak for the priorities here.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 12, 2018, 11:49 a.m. UTC | #40

On Mon, Feb 12, 2018 at 12:44 PM, Ulf Magnusson <[e-mail]> wrote:
> On Mon, Feb 12, 2018 at 11:44 AM, Masahiro Yamada
> <[e-mail]> wrote:
>> 2018-02-11 19:34 GMT+09:00 Ulf Magnusson <[e-mail]>:
>>> Looks to me like there's a few unrelated issues here:
>>>
>>>
>>> 1. The stack protector support test scripts
>>>
>>> Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
>>> subtly broken kernel from being built.
>>>
>>> A few questions:
>>>
>>>     - How do things fail with a broken stack protector implementation?
>>>
>>>     - How common are those broken compilers?
>>>
>>>     - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
>>>       or would a simpler static test work in practice?
>>>
>>>       I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
>>>       Kconfig, but should make sure it's actually needed in any case.
>>>
>>>       The scripts are already split up as
>>>
>>>           scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh
>>>
>>>       by the way, though only gcc-x86_32-has-stack-protector.sh and
>>>       gcc-x86_64-has-stack-protector.sh exist.
>>>
>>>     - How old do you need to go with GCC for -fno-stack-protector to give an
>>>       error (i.e., for not even the option to be recognized)? Is it still
>>>       warranted to test for it?
>>>
>>> Adding some CCs who worked on the stack protector test scripts.
>>>
>>> And yeah, I was assuming that needing support scripts would be rare, and that
>>> you'd usually just check whether gcc accepts the flag.
>>>
>>> When you Google "gcc broken stack protector", the top hits about are about the
>>> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
>>> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
>>> -fno-PIE")).
>>>
>>>
>>> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>>>
>>> Or equivalently, whether to automatically fall back on other stack protector
>>> alternatives (including no stack protector) if the one specified in the .config
>>> isn't available.
>>>
>>> I'll let you battle this one out. In any case, as a user, I'd want a
>>> super-clear message telling me what to change if the build breaks because of
>>> missing stack protector support.
>>>
>>>
>>> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>>>
>>> I'd just go with whatever is simplest here. I don't find the Kconfig version
>>> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
>>> tell how it looks to other people.
>>>
>>> I'd add some comments to explain the idea in the final version.
>>>
>>> @Kees:
>>> I was assuming that the Makefiles would error out with a message if none of the
>>> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>>>
>>> You could offload part of that check to Kconfig with something like
>>>
>>>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>>                 def_bool CC_STACKPROTECTOR_STRONG || \
>>>                          CC_STACKPROTECTOR_REGULAR || \
>>>                          CC_STACKPROTECTOR_NONE
>>>
>>> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
>>> It has the advantage of making the constraint clear in the Kconfig file
>>> at least.
>>>
>>> You could add some kind of assert feature to Kconfig too, but IMO it's not
>>> warranted purely for one-offs like this at least.
>>>
>>> That's details though. I'd want to explain it with a comment in any case if we
>>> go with something like this, since it's slightly kludgy and subtle
>>> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
>>> express it like that directly, since it's derived from other symbols).
>>>
>>>
>>> Here's an overview of the current Kconfig layout by the way, assuming
>>> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
>>> implemented in Kconfig:
>>>
>>>         # Feature tests
>>>         CC_HAS_STACKPROTECTOR_STRONG
>>>         CC_HAS_STACKPROTECTOR_REGULAR
>>>         CC_HAS_STACKPROTECTOR_NONE
>>>
>>>         # User request
>>>         WANT_CC_STACKPROTECTOR_AUTO
>>>         WANT_CC_STACKPROTECTOR_STRONG
>>>         WANT_CC_STACKPROTECTOR_REGULAR
>>>         WANT_CC_STACKPROTECTOR_NONE
>>>
>>>         # The actual "output" to the Makefiles
>>>         CC_STACKPROTECTOR_STRONG
>>>         CC_STACKPROTECTOR_REGULAR
>>>         CC_STACKPROTECTOR_NONE
>>>
>>>         # Some possible output "nicities"
>>>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>>         CC_OPT_STACKPROTECTOR
>>>
>>> Does anyone have objections to the naming or other things? I saw some
>>> references to "Santa's wish list" in messages of commits that dealt with other
>>> variables named WANT_*, though I didn't look into those cases. ;)
>>>
>>
>>
>>
>> I think Linus's comment was dismissed here.
>>
>>
>> Linus said:
>>
>>> But yes, I also reacted to your earlier " It can't silently rewrite it
>>> to _REGULAR because the compiler support for _STRONG regressed."
>>> Because it damn well can. If the compiler doesn't support
>>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>>> Silently. No extra crazy complex logic for that either.
>>
>>
>> If I understood his comment correctly,
>> we do not need either WANT_* or _AUTO.
>>
>>
>>
>>
>> Kees' comment:
>>
>>> In the stack-protector case, this becomes quite important, since the
>>> goal is to record the user's selection regardless of compiler
>>> capability. For example, if someone selects _REGULAR, it shouldn't
>>> "upgrade" to _STRONG. (Similarly for _NONE.)
>>
>> No.  Kconfig will not do this silently.
>
> (Playing devil's advocate...)
>
> If the user selected _STRONG and it becomes unavailable later, it
> seems to silently fall back on other options, even for oldnoconfig
> (which just checks if there are any new symbols in the choice).
>
> It would be possible to also check if the old user selection still
> applies btw. I do that in Kconfiglib. It's arguable whether that
> matches the intent of oldnoconfig.

*oldconfig

These things are so closely named. :P

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 12, 2018, 12:54 p.m. UTC | #41

On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
> >> Another case I mentioned before that I just want to make sure we don't
> >> reintroduce the problem of getting "stuck" with a bad .config file.
> >> While adding _STRONG support, I discovered the two-phase Kconfig
> >> resolution that happens during the build. If you selected _STRONG with
> >> a strong-capable compiler, everything was fine. If you then tried to
> >> build with an older compiler, you'd get stuck since _STRONG wasn't
> >> support (as detected during the first Kconfig phase) so the
> >> generated/autoconf.h would never get updated with the newly selected
> >> _REGULAR). I moved the Makefile analysis of available stack-protector
> >> options into the second phase (i.e. after all the Kconfig runs), and
> >> that worked to both unstick such configs and provide a clear message
> >> early in the build about what wasn't available.
> >>
> >> If all this detection is getting moved up into Kconfig, I'm worried
> >> we'll end up in this state again. If the answer is "you have to delete
> >> autoconf.h if you change compilers", then that's fine, but it sure
> >> seems unfriendly. :)
> >
> > Did you mean include/config/auto.conf? That's the one that gets
> > included by the Makefiles.
> >
> > If the feature detection is moved into Kconfig, you should only need
> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
> > you change the compiler. That will update .config while taking the new
> > features into account, and then the second phase during 'make' will
> > update include/config/auto.conf from .config.
> >
> > That second Kconfig phase generates include/generated/autoconf.h and
> > include/config/. The include/config/ directory implements dependencies
> > between source files and Kconfig symbols by turning the symbols into
> > (empty) files. When building (during the "second phase"), Kconfig
> > compares .config with include/config/auto.conf to see what changed,
> > and signals the changes to 'make' by touch'ing the files corresponding
> > to the changed symbols. The idea is to avoid having to do a full
> > rebuild whenever the configuration is changed.
> >
> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
> >
> > Cheers,
> > Ulf
> 
> By the way:
> 
> That second phase is also a "normal" Kconfig run in the sense that it
> does all the usual dependency checking stuff. Even if .config doesn't
> respect dependencies, include/config/auto.conf will. So I think you
> might not even need to rerun the configuration (though .config will be
> out-of-date until you do).
> 
> Cheers,
> Ulf

Seems you'd have to rerun the configuration, because
include/config/auto.conf is only regenerated if it's older than .config.

Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
.config).

	# If .config is newer than include/config/auto.conf, someone tinkered
	# with it and forgot to run make oldconfig.
	# if auto.conf.cmd is missing then we are probably in a cleaned tree so
	# we execute the config step to be sure to catch updated Kconfig files
	include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
		$(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig

silentoldconfig is a terrible name. What it actually does is run that
"second phase" stuff.

Pretty sure that comment lies by the way. 'make oldconfig' doesn't
update include/config/auto.conf. It's probably outdated.


I wonder if it would be simpler to just always run silentoldconfig when
building. It's not that slow on my system:

	$ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
	$ time scripts/kconfig/conf --silentoldconfig Kconfig

	real	0m0.167s
	user	0m0.162s
	sys	0m0.004s

That'd both simplify the Makefiles, and make sure that the latest
features are always used if you do feature testing in Kconfig.

I don't know how strongly people feel about a few tenths of a second
though.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 12, 2018, 1:53 p.m. UTC | #42

2018-02-12 20:44 GMT+09:00 Ulf Magnusson <[e-mail]>:

>>
>> I think Linus's comment was dismissed here.
>>
>>
>> Linus said:
>>
>>> But yes, I also reacted to your earlier " It can't silently rewrite it
>>> to _REGULAR because the compiler support for _STRONG regressed."
>>> Because it damn well can. If the compiler doesn't support
>>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>>> Silently. No extra crazy complex logic for that either.
>>
>>
>> If I understood his comment correctly,
>> we do not need either WANT_* or _AUTO.
>>
>>
>>
>>
>> Kees' comment:
>>
>>> In the stack-protector case, this becomes quite important, since the
>>> goal is to record the user's selection regardless of compiler
>>> capability. For example, if someone selects _REGULAR, it shouldn't
>>> "upgrade" to _STRONG. (Similarly for _NONE.)
>>
>> No.  Kconfig will not do this silently.
>
> (Playing devil's advocate...)
>
> If the user selected _STRONG and it becomes unavailable later, it
> seems to silently fall back on other options, even for oldnoconfig
> (which just checks if there are any new symbols in the choice).
>
> It would be possible to also check if the old user selection still
> applies btw. I do that in Kconfiglib. It's arguable whether that
> matches the intent of oldnoconfig.
>
>>
>>
>> "make oldconfig" (or "make silentoldconfig" as well)
>> always ask users about new symbols.
>>
>>
>> For example, let's say your compiler supports -fstack-protector
>> but not -fstack-protector-strong.
>> CC_STACKPROTECTOR_REGULAR is the best you can choose.
>>
>> Then, let's say you upgrade your compiler and the new compiler supports
>> -fstack-protector-strong as well.
>>
>> In this case, CC_STACKPROTECTOR_STRONG is a newly visible symbol,
>> so Kconfig will ask you
>> "Hey, you were previously using _REGULAR, but your new compiler
>> also supports _STRONG.  Do you want to use it?"
>>
>>
>> The "make oldconfig" will look like follows:
>>
>>
>> masahiro@grover:~/workspace/linux-kbuild$ make oldconfig
>>   HOSTCC  scripts/kconfig/conf.o
>>   HOSTLD  scripts/kconfig/conf
>> scripts/kconfig/conf  --oldconfig Kconfig
>> *
>> * Restart config...
>> *
>> *
>> * Linux Kernel Configuration
>> *
>> Stack Protector buffer overflow detection
>>   1. Strong (CC_STACKPROTECTOR_STRONG) (NEW)
>>> 2. Regular (CC_STACKPROTECTOR_REGULAR)
>>   3. None (CC_STACKPROTECTOR_NONE)
>> choice[1-3?]:
>>
>>
>>
>> Please notice the Strong is marked as "(NEW)".
>>
>> Kconfig handles this really nicely with Linus' suggestion.
>>
>> [1] Users can select only features supported by your compiler
>>     - this makes sense.
>>
>> [2] If you upgrade your compiler and it provides more capability,
>>     "make (silent)oldconfig" will ask you about new choices.
>>      - this also makes sense.
>
> If you switch to a compiler that provides less capability, it'll
> silently forget the old user preference though.


Yes, forget.

So, "make oldconfig" will ask user preference again
when you switch back to a compiler that provides more capability.
This is not rude, right?




To remember user preference (for both upgrade/downgrade capability),
we need 3 symbols for each feature.

[1] WANT_FOO
    A user can enable this option regardless of compiler capability.
    This will remember your preference forever.

[2] CC_HAS_FOO
    Compiler capability.

[3] FOO
    logical 'and' of WANT_FOO and CC_HAS_FOO.

If we do this way, what's the point of
moving compiler capability tests to Kconfig?


[1] is the same as what we do now.

Then, it will be disabled later
if it turns out unsupported by your compiler.

The only difference is,
whether we calculate [2], [3] in Makefile or Kconfig.

Kconfig is, in the end, inter-symbol dependency/visibility solver.
The WANT_ is not using the benefit of Kconfig.
We can calculate the logical 'and' by other means.





The problem I see in this approach is 'savedefconfig'.

'make defconfig && make savedefconfig'
will produce the subset of user preference and compiler capability.

To make arbitrary set of CONFIG options,
we need a full-featured compiler
in order to enable all CC_HAS_... options.


Maybe, we can add a new environment to ease this.

KCONFIG_SHELL_ALWAYS_TRUE
  If this environment is specified, all 'option shell=...'
  will be evaluated as true.  So, all symbols that depend on
  CC_HAS_  will be visible.   This is useful to make
  arbitrary set of CONFIG options regardless of
  your compiler capability.


$ KCONFIG_SHELL_ALWAYS_TRUE=1  make  menuconfig
   -> build up your favorite config setting

$ KCONFIG_SHELL_ALWAYS_TRUE=1 make savedefconfig
   ->  write out the minimum set of config


Thought?







>>
>>
>>
>> So, the following simple implementation works well enough,
>> doesn't it?
>>
>> ---------------->8---------------
>> choice
>>         prompt "Stack Protector buffer overflow detection"
>>
>> config CC_STACKPROTECTOR_STRONG
>>         bool "Strong"
>>         depends on CC_HAS_STACKPROTECTOR_STRONG
>>         select CC_STACKPROTECTOR
>>
>> config CC_STACKPROTECTOR_REGULAR
>>         bool "Regular"
>>         depends on CC_HAS_STACKPROTECTOR
>>         select CC_STACKPROTECTOR
>>
>> config CC_STACKPROTECTOR_NONE
>>         bool "None"
>>
>> endchoice
>> ---------------->8-------------------------
>
> Obviously much neater at least. :)
>
>>
>>
>>
>> BTW, do we need to use 'choice' ?
>>
>>
>> The problem of using 'choice' is,
>> it does not work well with allnoconfig.
>>
>>
>> For all{yes,mod}config, we want to enable as many features as possible.
>> For allnoconfig, we want to disable as many as possible.
>
> Oh... right. For allnoconfig, it would always pick the default, so if
> the default is "on", it's bad there.
>
>>
>> However, the default of 'choice' is always the first visible value.
>>
>>
>> So, I can suggest to remove _REGULAR and _NONE.
>>
>> We have just two bool options, like follows.
>>
>> ------------------->8-----------------
>> config CC_STACKPROTECTOR
>>         bool "Use stack protector"
>>         depends on CC_HAS_STACKPROTECTOR
>>
>> config CC_STACKPROTECTOR_STRONG
>>         bool "Use strong strong stack protector"
>>         depends on CC_STACKPROTECTOR
>>         depends on CC_HAS_STACKPROTECTOR_STRONG
>> -------------------->8------------------
>
> Even neater. Much easier to understand.
>
>>
>> This will work well for all{yes,mod,no}config.
>>
>> We will not have a case where
>> -fstack-protector-strong is supported, but -fstack-protector is not.
>>
>>
>> --
>> Best Regards
>> Masahiro Yamada
>
> So there's just the caveat about possibly "forgetting" the user
> preferences and automatically falling back on
> CC_STACKPROTECTOR_REGULAR or CC_STACKPROTECTOR_NONE when the
> environment changes, instead of erroring out.
>
> When you have to think hard about cases where something might break,
> it might be a sign that it's not a huge problem in practice, but I
> can't really speak for the priorities here.
>

Ulf Magnusson Feb. 12, 2018, 2:13 p.m. UTC | #43

On Mon, Feb 12, 2018 at 2:53 PM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-12 20:44 GMT+09:00 Ulf Magnusson <[e-mail]>:
>
>>>
>>> I think Linus's comment was dismissed here.
>>>
>>>
>>> Linus said:
>>>
>>>> But yes, I also reacted to your earlier " It can't silently rewrite it
>>>> to _REGULAR because the compiler support for _STRONG regressed."
>>>> Because it damn well can. If the compiler doesn't support
>>>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>>>> Silently. No extra crazy complex logic for that either.
>>>
>>>
>>> If I understood his comment correctly,
>>> we do not need either WANT_* or _AUTO.
>>>
>>>
>>>
>>>
>>> Kees' comment:
>>>
>>>> In the stack-protector case, this becomes quite important, since the
>>>> goal is to record the user's selection regardless of compiler
>>>> capability. For example, if someone selects _REGULAR, it shouldn't
>>>> "upgrade" to _STRONG. (Similarly for _NONE.)
>>>
>>> No.  Kconfig will not do this silently.
>>
>> (Playing devil's advocate...)
>>
>> If the user selected _STRONG and it becomes unavailable later, it
>> seems to silently fall back on other options, even for oldnoconfig
>> (which just checks if there are any new symbols in the choice).
>>
>> It would be possible to also check if the old user selection still
>> applies btw. I do that in Kconfiglib. It's arguable whether that
>> matches the intent of oldnoconfig.
>>
>>>
>>>
>>> "make oldconfig" (or "make silentoldconfig" as well)
>>> always ask users about new symbols.
>>>
>>>
>>> For example, let's say your compiler supports -fstack-protector
>>> but not -fstack-protector-strong.
>>> CC_STACKPROTECTOR_REGULAR is the best you can choose.
>>>
>>> Then, let's say you upgrade your compiler and the new compiler supports
>>> -fstack-protector-strong as well.
>>>
>>> In this case, CC_STACKPROTECTOR_STRONG is a newly visible symbol,
>>> so Kconfig will ask you
>>> "Hey, you were previously using _REGULAR, but your new compiler
>>> also supports _STRONG.  Do you want to use it?"
>>>
>>>
>>> The "make oldconfig" will look like follows:
>>>
>>>
>>> masahiro@grover:~/workspace/linux-kbuild$ make oldconfig
>>>   HOSTCC  scripts/kconfig/conf.o
>>>   HOSTLD  scripts/kconfig/conf
>>> scripts/kconfig/conf  --oldconfig Kconfig
>>> *
>>> * Restart config...
>>> *
>>> *
>>> * Linux Kernel Configuration
>>> *
>>> Stack Protector buffer overflow detection
>>>   1. Strong (CC_STACKPROTECTOR_STRONG) (NEW)
>>>> 2. Regular (CC_STACKPROTECTOR_REGULAR)
>>>   3. None (CC_STACKPROTECTOR_NONE)
>>> choice[1-3?]:
>>>
>>>
>>>
>>> Please notice the Strong is marked as "(NEW)".
>>>
>>> Kconfig handles this really nicely with Linus' suggestion.
>>>
>>> [1] Users can select only features supported by your compiler
>>>     - this makes sense.
>>>
>>> [2] If you upgrade your compiler and it provides more capability,
>>>     "make (silent)oldconfig" will ask you about new choices.
>>>      - this also makes sense.
>>
>> If you switch to a compiler that provides less capability, it'll
>> silently forget the old user preference though.
>
>
> Yes, forget.
>
> So, "make oldconfig" will ask user preference again
> when you switch back to a compiler that provides more capability.
> This is not rude, right?

Nopes, makes perfect sense to me.

> To remember user preference (for both upgrade/downgrade capability),
> we need 3 symbols for each feature.
>
> [1] WANT_FOO
>     A user can enable this option regardless of compiler capability.
>     This will remember your preference forever.
>
> [2] CC_HAS_FOO
>     Compiler capability.
>
> [3] FOO
>     logical 'and' of WANT_FOO and CC_HAS_FOO.
>
> If we do this way, what's the point of
> moving compiler capability tests to Kconfig?

I think this would only be done for the stack protector stuff, if it's
really warranted to have an exception there.

For other symbols, you'd just silently turn them off if a feature they
depend on isn't there, which is the way Kconfig usually works anyway.

>
>
> [1] is the same as what we do now.
>
> Then, it will be disabled later
> if it turns out unsupported by your compiler.
>
> The only difference is,
> whether we calculate [2], [3] in Makefile or Kconfig.
>
> Kconfig is, in the end, inter-symbol dependency/visibility solver.
> The WANT_ is not using the benefit of Kconfig.
> We can calculate the logical 'and' by other means.

Yeah, once you go with the WANT_* stuff, it's probably only about
whether it's easiest to do it Kconfig or the Makefiles. If it's in
Kconfig you might have more of the logic in one place at least.

> The problem I see in this approach is 'savedefconfig'.
>
> 'make defconfig && make savedefconfig'
> will produce the subset of user preference and compiler capability.
>
> To make arbitrary set of CONFIG options,
> we need a full-featured compiler
> in order to enable all CC_HAS_... options.
>
>
> Maybe, we can add a new environment to ease this.
>
> KCONFIG_SHELL_ALWAYS_TRUE
>   If this environment is specified, all 'option shell=...'
>   will be evaluated as true.  So, all symbols that depend on
>   CC_HAS_  will be visible.   This is useful to make
>   arbitrary set of CONFIG options regardless of
>   your compiler capability.
>
>
> $ KCONFIG_SHELL_ALWAYS_TRUE=1  make  menuconfig
>    -> build up your favorite config setting
>
> $ KCONFIG_SHELL_ALWAYS_TRUE=1 make savedefconfig
>    ->  write out the minimum set of config
>
>
> Thought?

I wonder if this could cause problems if people start using 'option
shell=...' for other things besides feature tests. It assumes that 'y'
always corresponds to "available" too.

If the cc_opt Kconfig helper is added, then maybe it could just turn
all those 'y'.

Perhaps it wouldn't be so horrible to require people to generate
defconfigs on systems that have all the features they want in the
defconfig either.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 12, 2018, 2:21 p.m. UTC | #44

2018-02-12 21:54 GMT+09:00 Ulf Magnusson <[e-mail]>:
> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>> >> Another case I mentioned before that I just want to make sure we don't
>> >> reintroduce the problem of getting "stuck" with a bad .config file.
>> >> While adding _STRONG support, I discovered the two-phase Kconfig
>> >> resolution that happens during the build. If you selected _STRONG with
>> >> a strong-capable compiler, everything was fine. If you then tried to
>> >> build with an older compiler, you'd get stuck since _STRONG wasn't
>> >> support (as detected during the first Kconfig phase) so the
>> >> generated/autoconf.h would never get updated with the newly selected
>> >> _REGULAR). I moved the Makefile analysis of available stack-protector
>> >> options into the second phase (i.e. after all the Kconfig runs), and
>> >> that worked to both unstick such configs and provide a clear message
>> >> early in the build about what wasn't available.
>> >>
>> >> If all this detection is getting moved up into Kconfig, I'm worried
>> >> we'll end up in this state again. If the answer is "you have to delete
>> >> autoconf.h if you change compilers", then that's fine, but it sure
>> >> seems unfriendly. :)
>> >
>> > Did you mean include/config/auto.conf? That's the one that gets
>> > included by the Makefiles.
>> >
>> > If the feature detection is moved into Kconfig, you should only need
>> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
>> > you change the compiler. That will update .config while taking the new
>> > features into account, and then the second phase during 'make' will
>> > update include/config/auto.conf from .config.
>> >
>> > That second Kconfig phase generates include/generated/autoconf.h and
>> > include/config/. The include/config/ directory implements dependencies
>> > between source files and Kconfig symbols by turning the symbols into
>> > (empty) files. When building (during the "second phase"), Kconfig
>> > compares .config with include/config/auto.conf to see what changed,
>> > and signals the changes to 'make' by touch'ing the files corresponding
>> > to the changed symbols. The idea is to avoid having to do a full
>> > rebuild whenever the configuration is changed.
>> >
>> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>> >
>> > Cheers,
>> > Ulf
>>
>> By the way:
>>
>> That second phase is also a "normal" Kconfig run in the sense that it
>> does all the usual dependency checking stuff. Even if .config doesn't
>> respect dependencies, include/config/auto.conf will. So I think you
>> might not even need to rerun the configuration (though .config will be
>> out-of-date until you do).
>>
>> Cheers,
>> Ulf
>
> Seems you'd have to rerun the configuration, because
> include/config/auto.conf is only regenerated if it's older than .config.
>
> Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
> .config).
>
>         # If .config is newer than include/config/auto.conf, someone tinkered
>         # with it and forgot to run make oldconfig.
>         # if auto.conf.cmd is missing then we are probably in a cleaned tree so
>         # we execute the config step to be sure to catch updated Kconfig files
>         include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>                 $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>
> silentoldconfig is a terrible name. What it actually does is run that
> "second phase" stuff.

Right.  This is a historical misnomer.

My plan is, as already posted below, to rename 'silentoldconfig' to 'synconfig'
https://lkml.org/lkml/2018/1/17/1359



> Pretty sure that comment lies by the way. 'make oldconfig' doesn't
> update include/config/auto.conf. It's probably outdated.

Good catch.


>
> I wonder if it would be simpler to just always run silentoldconfig when
> building. It's not that slow on my system:
>
>         $ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
>         $ time scripts/kconfig/conf --silentoldconfig Kconfig
>
>         real    0m0.167s
>         user    0m0.162s
>         sys     0m0.004s
>
> That'd both simplify the Makefiles, and make sure that the latest
> features are always used if you do feature testing in Kconfig.
>
> I don't know how strongly people feel about a few tenths of a second
> though.


No.  NACK.

silentoldconfig touches include/generated/autoconf.h
so, files that depend on it will be re-compiled, unnecessarily.


silentoldconfig ( 'syncconfig' in a more proper name)
should be run only when necessary.

Masahiro Yamada Feb. 12, 2018, 2:23 p.m. UTC | #45

2018-02-12 23:21 GMT+09:00 Masahiro Yamada <[e-mail]>:
> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <[e-mail]>:
>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>>> >> Another case I mentioned before that I just want to make sure we don't
>>> >> reintroduce the problem of getting "stuck" with a bad .config file.
>>> >> While adding _STRONG support, I discovered the two-phase Kconfig
>>> >> resolution that happens during the build. If you selected _STRONG with
>>> >> a strong-capable compiler, everything was fine. If you then tried to
>>> >> build with an older compiler, you'd get stuck since _STRONG wasn't
>>> >> support (as detected during the first Kconfig phase) so the
>>> >> generated/autoconf.h would never get updated with the newly selected
>>> >> _REGULAR). I moved the Makefile analysis of available stack-protector
>>> >> options into the second phase (i.e. after all the Kconfig runs), and
>>> >> that worked to both unstick such configs and provide a clear message
>>> >> early in the build about what wasn't available.
>>> >>
>>> >> If all this detection is getting moved up into Kconfig, I'm worried
>>> >> we'll end up in this state again. If the answer is "you have to delete
>>> >> autoconf.h if you change compilers", then that's fine, but it sure
>>> >> seems unfriendly. :)
>>> >
>>> > Did you mean include/config/auto.conf? That's the one that gets
>>> > included by the Makefiles.
>>> >
>>> > If the feature detection is moved into Kconfig, you should only need
>>> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
>>> > you change the compiler. That will update .config while taking the new
>>> > features into account, and then the second phase during 'make' will
>>> > update include/config/auto.conf from .config.
>>> >
>>> > That second Kconfig phase generates include/generated/autoconf.h and
>>> > include/config/. The include/config/ directory implements dependencies
>>> > between source files and Kconfig symbols by turning the symbols into
>>> > (empty) files. When building (during the "second phase"), Kconfig
>>> > compares .config with include/config/auto.conf to see what changed,
>>> > and signals the changes to 'make' by touch'ing the files corresponding
>>> > to the changed symbols. The idea is to avoid having to do a full
>>> > rebuild whenever the configuration is changed.
>>> >
>>> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>>> >
>>> > Cheers,
>>> > Ulf
>>>
>>> By the way:
>>>
>>> That second phase is also a "normal" Kconfig run in the sense that it
>>> does all the usual dependency checking stuff. Even if .config doesn't
>>> respect dependencies, include/config/auto.conf will. So I think you
>>> might not even need to rerun the configuration (though .config will be
>>> out-of-date until you do).
>>>
>>> Cheers,
>>> Ulf
>>
>> Seems you'd have to rerun the configuration, because
>> include/config/auto.conf is only regenerated if it's older than .config.
>>
>> Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
>> .config).
>>
>>         # If .config is newer than include/config/auto.conf, someone tinkered
>>         # with it and forgot to run make oldconfig.
>>         # if auto.conf.cmd is missing then we are probably in a cleaned tree so
>>         # we execute the config step to be sure to catch updated Kconfig files
>>         include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>                 $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>
>> silentoldconfig is a terrible name. What it actually does is run that
>> "second phase" stuff.
>
> Right.  This is a historical misnomer.
>
> My plan is, as already posted below, to rename 'silentoldconfig' to 'synconfig'
> https://lkml.org/lkml/2018/1/17/1359
>
>
>
>> Pretty sure that comment lies by the way. 'make oldconfig' doesn't
>> update include/config/auto.conf. It's probably outdated.
>
> Good catch.
>
>
>>
>> I wonder if it would be simpler to just always run silentoldconfig when
>> building. It's not that slow on my system:
>>
>>         $ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
>>         $ time scripts/kconfig/conf --silentoldconfig Kconfig
>>
>>         real    0m0.167s
>>         user    0m0.162s
>>         sys     0m0.004s
>>
>> That'd both simplify the Makefiles, and make sure that the latest
>> features are always used if you do feature testing in Kconfig.
>>
>> I don't know how strongly people feel about a few tenths of a second
>> though.
>
>
> No.  NACK.
>
> silentoldconfig touches include/generated/autoconf.h
> so, files that depend on it will be re-compiled, unnecessarily.


Sorry, I take back this comment.
fixdep takes care of this.

So, autoconf.h is removed from dependency.





>
> silentoldconfig ( 'syncconfig' in a more proper name)
> should be run only when necessary.
>
>
>
>
> --
> Best Regards
> Masahiro Yamada

Ulf Magnusson Feb. 12, 2018, 2:29 p.m. UTC | #46

On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <[e-mail]>:
>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>>> >> Another case I mentioned before that I just want to make sure we don't
>>> >> reintroduce the problem of getting "stuck" with a bad .config file.
>>> >> While adding _STRONG support, I discovered the two-phase Kconfig
>>> >> resolution that happens during the build. If you selected _STRONG with
>>> >> a strong-capable compiler, everything was fine. If you then tried to
>>> >> build with an older compiler, you'd get stuck since _STRONG wasn't
>>> >> support (as detected during the first Kconfig phase) so the
>>> >> generated/autoconf.h would never get updated with the newly selected
>>> >> _REGULAR). I moved the Makefile analysis of available stack-protector
>>> >> options into the second phase (i.e. after all the Kconfig runs), and
>>> >> that worked to both unstick such configs and provide a clear message
>>> >> early in the build about what wasn't available.
>>> >>
>>> >> If all this detection is getting moved up into Kconfig, I'm worried
>>> >> we'll end up in this state again. If the answer is "you have to delete
>>> >> autoconf.h if you change compilers", then that's fine, but it sure
>>> >> seems unfriendly. :)
>>> >
>>> > Did you mean include/config/auto.conf? That's the one that gets
>>> > included by the Makefiles.
>>> >
>>> > If the feature detection is moved into Kconfig, you should only need
>>> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
>>> > you change the compiler. That will update .config while taking the new
>>> > features into account, and then the second phase during 'make' will
>>> > update include/config/auto.conf from .config.
>>> >
>>> > That second Kconfig phase generates include/generated/autoconf.h and
>>> > include/config/. The include/config/ directory implements dependencies
>>> > between source files and Kconfig symbols by turning the symbols into
>>> > (empty) files. When building (during the "second phase"), Kconfig
>>> > compares .config with include/config/auto.conf to see what changed,
>>> > and signals the changes to 'make' by touch'ing the files corresponding
>>> > to the changed symbols. The idea is to avoid having to do a full
>>> > rebuild whenever the configuration is changed.
>>> >
>>> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>>> >
>>> > Cheers,
>>> > Ulf
>>>
>>> By the way:
>>>
>>> That second phase is also a "normal" Kconfig run in the sense that it
>>> does all the usual dependency checking stuff. Even if .config doesn't
>>> respect dependencies, include/config/auto.conf will. So I think you
>>> might not even need to rerun the configuration (though .config will be
>>> out-of-date until you do).
>>>
>>> Cheers,
>>> Ulf
>>
>> Seems you'd have to rerun the configuration, because
>> include/config/auto.conf is only regenerated if it's older than .config.
>>
>> Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
>> .config).
>>
>>         # If .config is newer than include/config/auto.conf, someone tinkered
>>         # with it and forgot to run make oldconfig.
>>         # if auto.conf.cmd is missing then we are probably in a cleaned tree so
>>         # we execute the config step to be sure to catch updated Kconfig files
>>         include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>                 $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>
>> silentoldconfig is a terrible name. What it actually does is run that
>> "second phase" stuff.
>
> Right.  This is a historical misnomer.
>
> My plan is, as already posted below, to rename 'silentoldconfig' to 'synconfig'
> https://lkml.org/lkml/2018/1/17/1359
>
>
>
>> Pretty sure that comment lies by the way. 'make oldconfig' doesn't
>> update include/config/auto.conf. It's probably outdated.
>
> Good catch.
>
>
>>
>> I wonder if it would be simpler to just always run silentoldconfig when
>> building. It's not that slow on my system:
>>
>>         $ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
>>         $ time scripts/kconfig/conf --silentoldconfig Kconfig
>>
>>         real    0m0.167s
>>         user    0m0.162s
>>         sys     0m0.004s
>>
>> That'd both simplify the Makefiles, and make sure that the latest
>> features are always used if you do feature testing in Kconfig.
>>
>> I don't know how strongly people feel about a few tenths of a second
>> though.
>
>
> No.  NACK.
>
> silentoldconfig touches include/generated/autoconf.h
> so, files that depend on it will be re-compiled, unnecessarily.

Hmm... my understanding was that fixdep would replace those direct
dependencies on include/generated/autoconf.h with dependencies on the
magic empty files corresponding to Kconfig symbols in include/config/.

What stuff depends directly on include/generated/autoconf.h?

I'm way more familiar with Kconfig than the kernel Makefiles.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 12, 2018, 2:32 p.m. UTC | #47

On Mon, Feb 12, 2018 at 3:23 PM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-12 23:21 GMT+09:00 Masahiro Yamada <[e-mail]>:
>> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <[e-mail]>:
>>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
>>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
>>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>>>> >> Another case I mentioned before that I just want to make sure we don't
>>>> >> reintroduce the problem of getting "stuck" with a bad .config file.
>>>> >> While adding _STRONG support, I discovered the two-phase Kconfig
>>>> >> resolution that happens during the build. If you selected _STRONG with
>>>> >> a strong-capable compiler, everything was fine. If you then tried to
>>>> >> build with an older compiler, you'd get stuck since _STRONG wasn't
>>>> >> support (as detected during the first Kconfig phase) so the
>>>> >> generated/autoconf.h would never get updated with the newly selected
>>>> >> _REGULAR). I moved the Makefile analysis of available stack-protector
>>>> >> options into the second phase (i.e. after all the Kconfig runs), and
>>>> >> that worked to both unstick such configs and provide a clear message
>>>> >> early in the build about what wasn't available.
>>>> >>
>>>> >> If all this detection is getting moved up into Kconfig, I'm worried
>>>> >> we'll end up in this state again. If the answer is "you have to delete
>>>> >> autoconf.h if you change compilers", then that's fine, but it sure
>>>> >> seems unfriendly. :)
>>>> >
>>>> > Did you mean include/config/auto.conf? That's the one that gets
>>>> > included by the Makefiles.
>>>> >
>>>> > If the feature detection is moved into Kconfig, you should only need
>>>> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
>>>> > you change the compiler. That will update .config while taking the new
>>>> > features into account, and then the second phase during 'make' will
>>>> > update include/config/auto.conf from .config.
>>>> >
>>>> > That second Kconfig phase generates include/generated/autoconf.h and
>>>> > include/config/. The include/config/ directory implements dependencies
>>>> > between source files and Kconfig symbols by turning the symbols into
>>>> > (empty) files. When building (during the "second phase"), Kconfig
>>>> > compares .config with include/config/auto.conf to see what changed,
>>>> > and signals the changes to 'make' by touch'ing the files corresponding
>>>> > to the changed symbols. The idea is to avoid having to do a full
>>>> > rebuild whenever the configuration is changed.
>>>> >
>>>> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>>>> >
>>>> > Cheers,
>>>> > Ulf
>>>>
>>>> By the way:
>>>>
>>>> That second phase is also a "normal" Kconfig run in the sense that it
>>>> does all the usual dependency checking stuff. Even if .config doesn't
>>>> respect dependencies, include/config/auto.conf will. So I think you
>>>> might not even need to rerun the configuration (though .config will be
>>>> out-of-date until you do).
>>>>
>>>> Cheers,
>>>> Ulf
>>>
>>> Seems you'd have to rerun the configuration, because
>>> include/config/auto.conf is only regenerated if it's older than .config.
>>>
>>> Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
>>> .config).
>>>
>>>         # If .config is newer than include/config/auto.conf, someone tinkered
>>>         # with it and forgot to run make oldconfig.
>>>         # if auto.conf.cmd is missing then we are probably in a cleaned tree so
>>>         # we execute the config step to be sure to catch updated Kconfig files
>>>         include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>>                 $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>>
>>> silentoldconfig is a terrible name. What it actually does is run that
>>> "second phase" stuff.
>>
>> Right.  This is a historical misnomer.
>>
>> My plan is, as already posted below, to rename 'silentoldconfig' to 'synconfig'
>> https://lkml.org/lkml/2018/1/17/1359
>>
>>
>>
>>> Pretty sure that comment lies by the way. 'make oldconfig' doesn't
>>> update include/config/auto.conf. It's probably outdated.
>>
>> Good catch.
>>
>>
>>>
>>> I wonder if it would be simpler to just always run silentoldconfig when
>>> building. It's not that slow on my system:
>>>
>>>         $ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
>>>         $ time scripts/kconfig/conf --silentoldconfig Kconfig
>>>
>>>         real    0m0.167s
>>>         user    0m0.162s
>>>         sys     0m0.004s
>>>
>>> That'd both simplify the Makefiles, and make sure that the latest
>>> features are always used if you do feature testing in Kconfig.
>>>
>>> I don't know how strongly people feel about a few tenths of a second
>>> though.
>>
>>
>> No.  NACK.
>>
>> silentoldconfig touches include/generated/autoconf.h
>> so, files that depend on it will be re-compiled, unnecessarily.
>
>
> Sorry, I take back this comment.
> fixdep takes care of this.
>
> So, autoconf.h is removed from dependency.
>

Still something that would need to be looked into, to make sure
there's nothing depending on stuff generated by silentoldconfig that
would get rebuilt over and over.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 12, 2018, 2:39 p.m. UTC | #48

2018-02-12 2:56 GMT+09:00 Kees Cook <[e-mail]>:

> I think it would work to skip KBUILD_CPPFLAGS right up until it
> didn't. Since we have the arch split, we can already add -m32 to the
> 32-bit case, etc. However, I worry about interaction with other
> selected build options. For example, while retpoline doesn't interact
> stack-protector, it's easy to imagine things that might.


One ugly solution could be
to add one more CC_HAS_ for the combination of the two ?


# If two compiler flags interact, the combination should be checked.
# Hopefully, we do not have many cases like this...
config CC_HAS_STACKPROTECTOR_WITH_RETPOLINE
       option cc_option "-fstackprotector  -mindirect-branch=thunk-extern"



> (And in thinking about this, does Kconfig know the true $CC in use?
> i.e. the configured cross compiler, etc?)


I was thinking of removing CONFIG_CROSS_COMPILE.

A user can dynamically change CROSS_COMPILE from
"make menuconfig".

If we continue to support this, $CC changes
according to CONFIG_CROSS_COMPILE.
Then, compiler flags must be re-evaluated
every time a user changes a compiler in use.
It will introduce much more complexity.

The easiest way would be to import $CC from environment
and fix the compiler capability before loading Kconfig.





>>     - How old do you need to go with GCC for -fno-stack-protector to give an
>>       error (i.e., for not even the option to be recognized)? Is it still
>>       warranted to test for it?
>
> Old? That's not the case. The check for -fno-stack-protector will
> likely be needed forever, as some distro compilers enable
> stack-protector by default. So when someone wants to explicitly build
> without stack-protector (or if the compiler's stack-protector is
> detected as broken), we must force it off for the kernel build.
>
>> Adding some CCs who worked on the stack protector test scripts.
>>
>> And yeah, I was assuming that needing support scripts would be rare, and that
>> you'd usually just check whether gcc accepts the flag.
>
> That would have been nice, yes. :(
>
>> When you Google "gcc broken stack protector", the top hits about are about the
>> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
>> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
>> -fno-PIE")).
>
> That's just the most recent case (from the case where some distro
> compilers enabled PIE by default). And was why I was hoping to drop
> the scripts entirely.
>
>> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>>
>> Or equivalently, whether to automatically fall back on other stack protector
>> alternatives (including no stack protector) if the one specified in the .config
>> isn't available.
>>
>> I'll let you battle this one out. In any case, as a user, I'd want a
>> super-clear message telling me what to change if the build breaks because of
>> missing stack protector support.
>
> Yes, exactly.
>
> The reason I built the _AUTO support was to make this easier for
> people to not have to think about. I wanted to get rid of explicit
> support (i.e. _REGULAR or _STRONG) but some people didn't want _STRONG
> (since it has greater code-size impact than _REGULAR), so we've had to
> keep that too.
>
>> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>>
>> I'd just go with whatever is simplest here. I don't find the Kconfig version
>> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
>> tell how it looks to other people.
>>
>> I'd add some comments to explain the idea in the final version.
>>
>> @Kees:
>> I was assuming that the Makefiles would error out with a message if none of the
>> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>
> That doesn't happy either before nor after _AUTO. The default value
> before was _NONE, and the default value after is _AUTO, which will use
> whatever is available.
>
>> You could offload part of that check to Kconfig with something like
>>
>>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>                 def_bool CC_STACKPROTECTOR_STRONG || \
>>                          CC_STACKPROTECTOR_REGULAR || \
>>                          CC_STACKPROTECTOR_NONE
>>
>> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
>> It has the advantage of making the constraint clear in the Kconfig file
>> at least.
>>
>> You could add some kind of assert feature to Kconfig too, but IMO it's not
>> warranted purely for one-offs like this at least.
>
> Agreed; I want to do whatever we can to simplify things. :)
>
>> That's details though. I'd want to explain it with a comment in any case if we
>> go with something like this, since it's slightly kludgy and subtle
>> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
>> express it like that directly, since it's derived from other symbols).
>>
>>
>> Here's an overview of the current Kconfig layout by the way, assuming
>> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
>> implemented in Kconfig:
>>
>>         # Feature tests
>>         CC_HAS_STACKPROTECTOR_STRONG
>>         CC_HAS_STACKPROTECTOR_REGULAR
>>         CC_HAS_STACKPROTECTOR_NONE
>
> As long as the feature tests include actual compiler output tests,
> yeah, this seems fine.
>
>>         # User request
>>         WANT_CC_STACKPROTECTOR_AUTO
>>         WANT_CC_STACKPROTECTOR_STRONG
>>         WANT_CC_STACKPROTECTOR_REGULAR
>>         WANT_CC_STACKPROTECTOR_NONE
>>
>>         # The actual "output" to the Makefiles
>>         CC_STACKPROTECTOR_STRONG
>>         CC_STACKPROTECTOR_REGULAR
>>         CC_STACKPROTECTOR_NONE
>
> This should be fine too (though by this point, I think Kconfig would
> already know the specific option, so it could just pass it with a
> single output (CC_OPT_STACKPROTECTOR below?)
>
>>         # Some possible output "nicities"
>>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>         CC_OPT_STACKPROTECTOR
>>
>> Does anyone have objections to the naming or other things? I saw some
>> references to "Santa's wish list" in messages of commits that dealt with other
>> variables named WANT_*, though I didn't look into those cases. ;)
>
> Another case I mentioned before that I just want to make sure we don't
> reintroduce the problem of getting "stuck" with a bad .config file.
> While adding _STRONG support, I discovered the two-phase Kconfig
> resolution that happens during the build. If you selected _STRONG with
> a strong-capable compiler, everything was fine. If you then tried to
> build with an older compiler, you'd get stuck since _STRONG wasn't
> support (as detected during the first Kconfig phase) so the
> generated/autoconf.h would never get updated with the newly selected
> _REGULAR). I moved the Makefile analysis of available stack-protector
> options into the second phase (i.e. after all the Kconfig runs), and
> that worked to both unstick such configs and provide a clear message
> early in the build about what wasn't available.
>
> If all this detection is getting moved up into Kconfig, I'm worried
> we'll end up in this state again. If the answer is "you have to delete
> autoconf.h if you change compilers", then that's fine, but it sure
> seems unfriendly. :)
>

As Ulf explained, this does not happen.

include/config/auto.conf and include/generated/autoconf.h
are the mirror of the .config.

If .config is updated, silentoldconfig is invoked to sync them.

Ulf Magnusson Feb. 12, 2018, 2:53 p.m. UTC | #49

On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada
<[e-mail]> wrote:
> 'syncconfig' in a more proper name

Wonder if --update-config-files-for-build or something would be an
even better name.

Kinda tough to compress it into something that adheres to *nix
terseness while making it somewhat clear what kind of stuff it deals
with. :P

Cheers,
Ulf

On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <[e-mail]>:
>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>>> >> Another case I mentioned before that I just want to make sure we don't
>>> >> reintroduce the problem of getting "stuck" with a bad .config file.
>>> >> While adding _STRONG support, I discovered the two-phase Kconfig
>>> >> resolution that happens during the build. If you selected _STRONG with
>>> >> a strong-capable compiler, everything was fine. If you then tried to
>>> >> build with an older compiler, you'd get stuck since _STRONG wasn't
>>> >> support (as detected during the first Kconfig phase) so the
>>> >> generated/autoconf.h would never get updated with the newly selected
>>> >> _REGULAR). I moved the Makefile analysis of available stack-protector
>>> >> options into the second phase (i.e. after all the Kconfig runs), and
>>> >> that worked to both unstick such configs and provide a clear message
>>> >> early in the build about what wasn't available.
>>> >>
>>> >> If all this detection is getting moved up into Kconfig, I'm worried
>>> >> we'll end up in this state again. If the answer is "you have to delete
>>> >> autoconf.h if you change compilers", then that's fine, but it sure
>>> >> seems unfriendly. :)
>>> >
>>> > Did you mean include/config/auto.conf? That's the one that gets
>>> > included by the Makefiles.
>>> >
>>> > If the feature detection is moved into Kconfig, you should only need
>>> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
>>> > you change the compiler. That will update .config while taking the new
>>> > features into account, and then the second phase during 'make' will
>>> > update include/config/auto.conf from .config.
>>> >
>>> > That second Kconfig phase generates include/generated/autoconf.h and
>>> > include/config/. The include/config/ directory implements dependencies
>>> > between source files and Kconfig symbols by turning the symbols into
>>> > (empty) files. When building (during the "second phase"), Kconfig
>>> > compares .config with include/config/auto.conf to see what changed,
>>> > and signals the changes to 'make' by touch'ing the files corresponding
>>> > to the changed symbols. The idea is to avoid having to do a full
>>> > rebuild whenever the configuration is changed.
>>> >
>>> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>>> >
>>> > Cheers,
>>> > Ulf
>>>
>>> By the way:
>>>
>>> That second phase is also a "normal" Kconfig run in the sense that it
>>> does all the usual dependency checking stuff. Even if .config doesn't
>>> respect dependencies, include/config/auto.conf will. So I think you
>>> might not even need to rerun the configuration (though .config will be
>>> out-of-date until you do).
>>>
>>> Cheers,
>>> Ulf
>>
>> Seems you'd have to rerun the configuration, because
>> include/config/auto.conf is only regenerated if it's older than .config.
>>
>> Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
>> .config).
>>
>>         # If .config is newer than include/config/auto.conf, someone tinkered
>>         # with it and forgot to run make oldconfig.
>>         # if auto.conf.cmd is missing then we are probably in a cleaned tree so
>>         # we execute the config step to be sure to catch updated Kconfig files
>>         include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>                 $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>
>> silentoldconfig is a terrible name. What it actually does is run that
>> "second phase" stuff.
>
> Right.  This is a historical misnomer.
>
> My plan is, as already posted below, to rename 'silentoldconfig' to 'synconfig'
> https://lkml.org/lkml/2018/1/17/1359
>
>
>
>> Pretty sure that comment lies by the way. 'make oldconfig' doesn't
>> update include/config/auto.conf. It's probably outdated.
>
> Good catch.
>
>
>>
>> I wonder if it would be simpler to just always run silentoldconfig when
>> building. It's not that slow on my system:
>>
>>         $ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
>>         $ time scripts/kconfig/conf --silentoldconfig Kconfig
>>
>>         real    0m0.167s
>>         user    0m0.162s
>>         sys     0m0.004s
>>
>> That'd both simplify the Makefiles, and make sure that the latest
>> features are always used if you do feature testing in Kconfig.
>>
>> I don't know how strongly people feel about a few tenths of a second
>> though.
>
>
> No.  NACK.
>
> silentoldconfig touches include/generated/autoconf.h
> so, files that depend on it will be re-compiled, unnecessarily.
>
>
> silentoldconfig ( 'syncconfig' in a more proper name)
> should be run only when necessary.
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 12, 2018, 3:22 p.m. UTC | #50

2018-02-12 23:53 GMT+09:00 Ulf Magnusson <[e-mail]>:
> On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada
> <[e-mail]> wrote:
>> 'syncconfig' in a more proper name
>
> Wonder if --update-config-files-for-build or something would be an
> even better name.

I want to use a name that ends with 'config' like any other config targets
because:

- I want use the same name for scripts/kconfig/conf option
  and Makefile target to take advantage of 'simple-targets'  [1]

- I want to use pattern rule to descend into scripts/kconfig/  [2]


[1] https://github.com/torvalds/linux/blob/v4.16-rc1/scripts/kconfig/Makefile#L84
[2] https://github.com/torvalds/linux/blob/v4.16-rc1/Makefile#L506



It would be possible to directly descend into scripts/kconfig/ like follows,
but I do not have a good reason to break the convention.

include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
       $(Q)$(MAKE) $(build)=scripts/kconfig  update-config-files-for-build



>
> Kinda tough to compress it into something that adheres to *nix
> terseness while making it somewhat clear what kind of stuff it deals
> with. :P
>








> Cheers,
> Ulf
>
> On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada
> <[e-mail]> wrote:
>> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <[e-mail]>:
>>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote:
>>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <[e-mail]> wrote:
>>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <[e-mail]> wrote:
>>>> >> Another case I mentioned before that I just want to make sure we don't
>>>> >> reintroduce the problem of getting "stuck" with a bad .config file.
>>>> >> While adding _STRONG support, I discovered the two-phase Kconfig
>>>> >> resolution that happens during the build. If you selected _STRONG with
>>>> >> a strong-capable compiler, everything was fine. If you then tried to
>>>> >> build with an older compiler, you'd get stuck since _STRONG wasn't
>>>> >> support (as detected during the first Kconfig phase) so the
>>>> >> generated/autoconf.h would never get updated with the newly selected
>>>> >> _REGULAR). I moved the Makefile analysis of available stack-protector
>>>> >> options into the second phase (i.e. after all the Kconfig runs), and
>>>> >> that worked to both unstick such configs and provide a clear message
>>>> >> early in the build about what wasn't available.
>>>> >>
>>>> >> If all this detection is getting moved up into Kconfig, I'm worried
>>>> >> we'll end up in this state again. If the answer is "you have to delete
>>>> >> autoconf.h if you change compilers", then that's fine, but it sure
>>>> >> seems unfriendly. :)
>>>> >
>>>> > Did you mean include/config/auto.conf? That's the one that gets
>>>> > included by the Makefiles.
>>>> >
>>>> > If the feature detection is moved into Kconfig, you should only need
>>>> > to rerun the configuration (make menuconfig/oldconfig/olddefconfig) if
>>>> > you change the compiler. That will update .config while taking the new
>>>> > features into account, and then the second phase during 'make' will
>>>> > update include/config/auto.conf from .config.
>>>> >
>>>> > That second Kconfig phase generates include/generated/autoconf.h and
>>>> > include/config/. The include/config/ directory implements dependencies
>>>> > between source files and Kconfig symbols by turning the symbols into
>>>> > (empty) files. When building (during the "second phase"), Kconfig
>>>> > compares .config with include/config/auto.conf to see what changed,
>>>> > and signals the changes to 'make' by touch'ing the files corresponding
>>>> > to the changed symbols. The idea is to avoid having to do a full
>>>> > rebuild whenever the configuration is changed.
>>>> >
>>>> > Check out scripts/basic/fixdep.c as well if you want to understand how it works.
>>>> >
>>>> > Cheers,
>>>> > Ulf
>>>>
>>>> By the way:
>>>>
>>>> That second phase is also a "normal" Kconfig run in the sense that it
>>>> does all the usual dependency checking stuff. Even if .config doesn't
>>>> respect dependencies, include/config/auto.conf will. So I think you
>>>> might not even need to rerun the configuration (though .config will be
>>>> out-of-date until you do).
>>>>
>>>> Cheers,
>>>> Ulf
>>>
>>> Seems you'd have to rerun the configuration, because
>>> include/config/auto.conf is only regenerated if it's older than .config.
>>>
>>> Here's the bit in the root Makefile that does it (KCONFIG_CONFIG is
>>> .config).
>>>
>>>         # If .config is newer than include/config/auto.conf, someone tinkered
>>>         # with it and forgot to run make oldconfig.
>>>         # if auto.conf.cmd is missing then we are probably in a cleaned tree so
>>>         # we execute the config step to be sure to catch updated Kconfig files
>>>         include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>>>                 $(Q)$(MAKE) -f $(srctree)/Makefile silentoldconfig
>>>
>>> silentoldconfig is a terrible name. What it actually does is run that
>>> "second phase" stuff.
>>
>> Right.  This is a historical misnomer.
>>
>> My plan is, as already posted below, to rename 'silentoldconfig' to 'synconfig'
>> https://lkml.org/lkml/2018/1/17/1359
>>
>>
>>
>>> Pretty sure that comment lies by the way. 'make oldconfig' doesn't
>>> update include/config/auto.conf. It's probably outdated.
>>
>> Good catch.
>>
>>
>>>
>>> I wonder if it would be simpler to just always run silentoldconfig when
>>> building. It's not that slow on my system:
>>>
>>>         $ export ARCH=x86 SRCARCH=x86 KERNELVERSION=`make kernelversion`
>>>         $ time scripts/kconfig/conf --silentoldconfig Kconfig
>>>
>>>         real    0m0.167s
>>>         user    0m0.162s
>>>         sys     0m0.004s
>>>
>>> That'd both simplify the Makefiles, and make sure that the latest
>>> features are always used if you do feature testing in Kconfig.
>>>
>>> I don't know how strongly people feel about a few tenths of a second
>>> though.
>>
>>
>> No.  NACK.
>>
>> silentoldconfig touches include/generated/autoconf.h
>> so, files that depend on it will be re-compiled, unnecessarily.
>>
>>
>> silentoldconfig ( 'syncconfig' in a more proper name)
>> should be run only when necessary.
>>
>>
>>
>>
>> --
>> Best Regards
>> Masahiro Yamada
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 12, 2018, 3:24 p.m. UTC | #51

On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-12 2:56 GMT+09:00 Kees Cook <[e-mail]>:
>> I think it would work to skip KBUILD_CPPFLAGS right up until it
>> didn't. Since we have the arch split, we can already add -m32 to the
>> 32-bit case, etc. However, I worry about interaction with other
>> selected build options. For example, while retpoline doesn't interact
>> stack-protector, it's easy to imagine things that might.

I want to be sure I clarify: I'm not aware of any negative interaction
between stack-protector and other options at present. I was just
trying to think of an illustrative example. I do know we're about to
get some per-architecture stack protector options (e.g. arm64 using a
per-process canary instead of global), but I *think* that can be
handled in the new Kconfig too.

> One ugly solution could be
> to add one more CC_HAS_ for the combination of the two ?

Yeah, that seems reasonable, but it's a fix after the fact. I guess
we'll have to see.

>> (And in thinking about this, does Kconfig know the true $CC in use?
>> i.e. the configured cross compiler, etc?)
>
> I was thinking of removing CONFIG_CROSS_COMPILE.
>
> A user can dynamically change CROSS_COMPILE from
> "make menuconfig".

Most builds I've seen implement cross compilers as an environment
variable during all "make" invocations.

> If we continue to support this, $CC changes
> according to CONFIG_CROSS_COMPILE.
> Then, compiler flags must be re-evaluated
> every time a user changes a compiler in use.
> It will introduce much more complexity.

Right now, this is just handled in the Makefile: all the right
variables exist, etc.

-Kees

Ulf Magnusson Feb. 12, 2018, 3:35 p.m. UTC | #52

On Mon, Feb 12, 2018 at 4:22 PM, Masahiro Yamada
<[e-mail]> wrote:
> 2018-02-12 23:53 GMT+09:00 Ulf Magnusson <[e-mail]>:
>> On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada
>> <[e-mail]> wrote:
>>> 'syncconfig' in a more proper name
>>
>> Wonder if --update-config-files-for-build or something would be an
>> even better name.
>
> I want to use a name that ends with 'config' like any other config targets
> because:
>
> - I want use the same name for scripts/kconfig/conf option
>   and Makefile target to take advantage of 'simple-targets'  [1]
>
> - I want to use pattern rule to descend into scripts/kconfig/  [2]
>
>
> [1] https://github.com/torvalds/linux/blob/v4.16-rc1/scripts/kconfig/Makefile#L84
> [2] https://github.com/torvalds/linux/blob/v4.16-rc1/Makefile#L506
>
>
>
> It would be possible to directly descend into scripts/kconfig/ like follows,
> but I do not have a good reason to break the convention.
>
> include/config/%.conf: $(KCONFIG_CONFIG) include/config/auto.conf.cmd
>        $(Q)$(MAKE) $(build)=scripts/kconfig  update-config-files-for-build
>

Ah, right, didn't think of that. Thought it might be nice to hint that
it's related to the build phase somehow at least.

Not that important I guess. --syncconfig is a big improvement already.

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 12, 2018, 3:46 p.m. UTC | #53

On Mon, Feb 12, 2018 at 2:44 AM, Masahiro Yamada
<[e-mail]> wrote:
> Linus said:
>
>> But yes, I also reacted to your earlier " It can't silently rewrite it
>> to _REGULAR because the compiler support for _STRONG regressed."
>> Because it damn well can. If the compiler doesn't support
>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>> Silently. No extra crazy complex logic for that either.
>
>
> If I understood his comment correctly,
> we do not need either WANT_* or _AUTO.
>
>
> Kees' comment:
>
>> In the stack-protector case, this becomes quite important, since the
>> goal is to record the user's selection regardless of compiler
>> capability. For example, if someone selects _REGULAR, it shouldn't
>> "upgrade" to _STRONG. (Similarly for _NONE.)
>
> No.  Kconfig will not do this silently.
>
> "make oldconfig" (or "make silentoldconfig" as well)
> always ask users about new symbols.

The case I want to make sure can never happen is to have a config
setting that ends up not actually being true. For example, if
CONFIG_CC_STACKPROTECTOR appears in /proc/config.gz but the kernel
wasn't actually built with a stack protector, that's bad. We end up in
a place where the user can't trust the config to represent the actual
results of the build.

So, as long as the Kconfig options are strongly tied to the compiler
capabilities, we're good on that front.

> So, I can suggest to remove _REGULAR and _NONE.
>
> We have just two bool options, like follows.
>
> ------------------->8-----------------
> config CC_STACKPROTECTOR
>         bool "Use stack protector"
>         depends on CC_HAS_STACKPROTECTOR
>
> config CC_STACKPROTECTOR_STRONG
>         bool "Use strong strong stack protector"
>         depends on CC_STACKPROTECTOR
>         depends on CC_HAS_STACKPROTECTOR_STRONG
> -------------------->8------------------
>
> This will work well for all{yes,mod,no}config.

This two-option arrangement is fine (though it assumes there won't be
another stack protector option in the future).

The issue I have is this removes _AUTO, which I think can be solved in
the two-option arrangement too. The purpose of _AUTO is to effectively
enable stack-protector by default. As this option has been available
for over 10 years, and all distros enable it, it's an obvious
candidate to be enabled-by-default, especially since it kills a class
of exploits (as mentioned in my commit log: BlueBorne was trivially
defeated with stack-protector). So it should be possible to make these
two options "default y", yes?

> We will not have a case where
> -fstack-protector-strong is supported, but -fstack-protector is not.

Correct.

-Kees

Masahiro Yamada Feb. 12, 2018, 4:10 p.m. UTC | #54

2018-02-13 0:46 GMT+09:00 Kees Cook <[e-mail]>:
> On Mon, Feb 12, 2018 at 2:44 AM, Masahiro Yamada
> <[e-mail]> wrote:
>> Linus said:
>>
>>> But yes, I also reacted to your earlier " It can't silently rewrite it
>>> to _REGULAR because the compiler support for _STRONG regressed."
>>> Because it damn well can. If the compiler doesn't support
>>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>>> Silently. No extra crazy complex logic for that either.
>>
>>
>> If I understood his comment correctly,
>> we do not need either WANT_* or _AUTO.
>>
>>
>> Kees' comment:
>>
>>> In the stack-protector case, this becomes quite important, since the
>>> goal is to record the user's selection regardless of compiler
>>> capability. For example, if someone selects _REGULAR, it shouldn't
>>> "upgrade" to _STRONG. (Similarly for _NONE.)
>>
>> No.  Kconfig will not do this silently.
>>
>> "make oldconfig" (or "make silentoldconfig" as well)
>> always ask users about new symbols.
>
> The case I want to make sure can never happen is to have a config
> setting that ends up not actually being true. For example, if
> CONFIG_CC_STACKPROTECTOR appears in /proc/config.gz but the kernel
> wasn't actually built with a stack protector, that's bad. We end up in
> a place where the user can't trust the config to represent the actual
> results of the build.
>
> So, as long as the Kconfig options are strongly tied to the compiler
> capabilities, we're good on that front.
>
>> So, I can suggest to remove _REGULAR and _NONE.
>>
>> We have just two bool options, like follows.
>>
>> ------------------->8-----------------
>> config CC_STACKPROTECTOR
>>         bool "Use stack protector"
>>         depends on CC_HAS_STACKPROTECTOR
>>
>> config CC_STACKPROTECTOR_STRONG
>>         bool "Use strong strong stack protector"
>>         depends on CC_STACKPROTECTOR
>>         depends on CC_HAS_STACKPROTECTOR_STRONG
>> -------------------->8------------------
>>
>> This will work well for all{yes,mod,no}config.
>
> This two-option arrangement is fine (though it assumes there won't be
> another stack protector option in the future).
>
> The issue I have is this removes _AUTO, which I think can be solved in
> the two-option arrangement too. The purpose of _AUTO is to effectively
> enable stack-protector by default. As this option has been available
> for over 10 years, and all distros enable it, it's an obvious
> candidate to be enabled-by-default, especially since it kills a class
> of exploits (as mentioned in my commit log: BlueBorne was trivially
> defeated with stack-protector). So it should be possible to make these
> two options "default y", yes?


Yes.

Both should be "default y" to keep the equivalent behavior
since the current default is _AUTO.




>> We will not have a case where
>> -fstack-protector-strong is supported, but -fstack-protector is not.
>
> Correct.
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Randy Dunlap Feb. 12, 2018, 11:48 p.m. UTC | #55

On 02/12/2018 07:24 AM, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada
> <[e-mail]> wrote:

>>> (And in thinking about this, does Kconfig know the true $CC in use?
>>> i.e. the configured cross compiler, etc?)
>>
>> I was thinking of removing CONFIG_CROSS_COMPILE.
>>
>> A user can dynamically change CROSS_COMPILE from
>> "make menuconfig".
> 
> Most builds I've seen implement cross compilers as an environment
> variable during all "make" invocations.

I agree. I think you would break a bunch of build bots if you remove that.

>> If we continue to support this, $CC changes
>> according to CONFIG_CROSS_COMPILE.
>> Then, compiler flags must be re-evaluated
>> every time a user changes a compiler in use.
>> It will introduce much more complexity.
> 
> Right now, this is just handled in the Makefile: all the right
> variables exist, etc.
> 
> -Kees
>

Masahiro Yamada Feb. 13, 2018, 1:41 a.m. UTC | #56

2018-02-13 8:48 GMT+09:00 Randy Dunlap <[e-mail]>:
> On 02/12/2018 07:24 AM, Kees Cook wrote:
>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada
>> <[e-mail]> wrote:
>
>>>> (And in thinking about this, does Kconfig know the true $CC in use?
>>>> i.e. the configured cross compiler, etc?)
>>>
>>> I was thinking of removing CONFIG_CROSS_COMPILE.
>>>
>>> A user can dynamically change CROSS_COMPILE from
>>> "make menuconfig".
>>
>> Most builds I've seen implement cross compilers as an environment
>> variable during all "make" invocations.
>
> I agree. I think you would break a bunch of build bots if you remove that.


For clarification, I suggested to remove CONFIG_CROSS_COMPILE.

The following code:
https://github.com/torvalds/linux/blob/v4.16-rc1/Makefile#L315
https://github.com/torvalds/linux/blob/v4.16-rc1/init/Kconfig#L58

I hope build bots are not using this.


Passing CROSS_COMPILE via the command line, environment
is still supported.

Randy Dunlap Feb. 13, 2018, 1:53 a.m. UTC | #57

On 02/12/2018 05:41 PM, Masahiro Yamada wrote:
> 2018-02-13 8:48 GMT+09:00 Randy Dunlap <[e-mail]>:
>> On 02/12/2018 07:24 AM, Kees Cook wrote:
>>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada
>>> <[e-mail]> wrote:
>>
>>>>> (And in thinking about this, does Kconfig know the true $CC in use?
>>>>> i.e. the configured cross compiler, etc?)
>>>>
>>>> I was thinking of removing CONFIG_CROSS_COMPILE.
>>>>
>>>> A user can dynamically change CROSS_COMPILE from
>>>> "make menuconfig".
>>>
>>> Most builds I've seen implement cross compilers as an environment
>>> variable during all "make" invocations.
>>
>> I agree. I think you would break a bunch of build bots if you remove that.
> 
> 
> For clarification, I suggested to remove CONFIG_CROSS_COMPILE.
> 
> The following code:
> https://github.com/torvalds/linux/blob/v4.16-rc1/Makefile#L315
> https://github.com/torvalds/linux/blob/v4.16-rc1/init/Kconfig#L58
> 
> I hope build bots are not using this.
> 
> 
> Passing CROSS_COMPILE via the command line, environment
> is still supported.

OK, I misunderstood.  That one can go away IMO.

thanks,

Arnd Bergmann Feb. 13, 2018, 8:35 a.m. UTC | #58

On Tue, Feb 13, 2018 at 2:53 AM, Randy Dunlap <[e-mail]> wrote:
> On 02/12/2018 05:41 PM, Masahiro Yamada wrote:
>> 2018-02-13 8:48 GMT+09:00 Randy Dunlap <[e-mail]>:
>>> On 02/12/2018 07:24 AM, Kees Cook wrote:
>>>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada
>>>> <[e-mail]> wrote:
>>>
>>>>>> (And in thinking about this, does Kconfig know the true $CC in use?
>>>>>> i.e. the configured cross compiler, etc?)
>>>>>
>>>>> I was thinking of removing CONFIG_CROSS_COMPILE.
>>>>>
>>>>> A user can dynamically change CROSS_COMPILE from
>>>>> "make menuconfig".
>>>>
>>>> Most builds I've seen implement cross compilers as an environment
>>>> variable during all "make" invocations.
>>>
>>> I agree. I think you would break a bunch of build bots if you remove that.
>>
>>
>> For clarification, I suggested to remove CONFIG_CROSS_COMPILE.
>>
>> The following code:
>> https://github.com/torvalds/linux/blob/v4.16-rc1/Makefile#L315
>> https://github.com/torvalds/linux/blob/v4.16-rc1/init/Kconfig#L58
>>
>> I hope build bots are not using this.
>>
>>
>> Passing CROSS_COMPILE via the command line, environment
>> is still supported.
>
> OK, I misunderstood.  That one can go away IMO.

Removing it will break the workflow for some people in a minor way
though. Could we have the top-level Makefile try to detect the
CROSS_COMPILE? Instead of just using CC=gcc as the default,
we could check for ${ARCH}!=`uname -m` and then see if $PATH
contains a ${ARCH}-linux-gcc, ${ARCH}-elf-gcc or ${ARCH}-gcc.

it will need slightly more complexity to deal with architectures
that have different identifiers in linux and gcc, but I think it would
be a nice feature anyway.

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Magnusson Feb. 13, 2018, 8:55 a.m. UTC | #59

On Tue, Feb 13, 2018 at 01:10:34AM +0900, Masahiro Yamada wrote:
> 2018-02-13 0:46 GMT+09:00 Kees Cook <[e-mail]>:
> > On Mon, Feb 12, 2018 at 2:44 AM, Masahiro Yamada
> > <[e-mail]> wrote:
> >> Linus said:
> >>
> >>> But yes, I also reacted to your earlier " It can't silently rewrite it
> >>> to _REGULAR because the compiler support for _STRONG regressed."
> >>> Because it damn well can. If the compiler doesn't support
> >>> -fstack-protector-strong, we can just fall back on -fstack-protector.
> >>> Silently. No extra crazy complex logic for that either.
> >>
> >>
> >> If I understood his comment correctly,
> >> we do not need either WANT_* or _AUTO.
> >>
> >>
> >> Kees' comment:
> >>
> >>> In the stack-protector case, this becomes quite important, since the
> >>> goal is to record the user's selection regardless of compiler
> >>> capability. For example, if someone selects _REGULAR, it shouldn't
> >>> "upgrade" to _STRONG. (Similarly for _NONE.)
> >>
> >> No.  Kconfig will not do this silently.
> >>
> >> "make oldconfig" (or "make silentoldconfig" as well)
> >> always ask users about new symbols.
> >
> > The case I want to make sure can never happen is to have a config
> > setting that ends up not actually being true. For example, if
> > CONFIG_CC_STACKPROTECTOR appears in /proc/config.gz but the kernel
> > wasn't actually built with a stack protector, that's bad. We end up in
> > a place where the user can't trust the config to represent the actual
> > results of the build.
> >
> > So, as long as the Kconfig options are strongly tied to the compiler
> > capabilities, we're good on that front.
> >
> >> So, I can suggest to remove _REGULAR and _NONE.
> >>
> >> We have just two bool options, like follows.
> >>
> >> ------------------->8-----------------
> >> config CC_STACKPROTECTOR
> >>         bool "Use stack protector"
> >>         depends on CC_HAS_STACKPROTECTOR
> >>
> >> config CC_STACKPROTECTOR_STRONG
> >>         bool "Use strong strong stack protector"
> >>         depends on CC_STACKPROTECTOR
> >>         depends on CC_HAS_STACKPROTECTOR_STRONG
> >> -------------------->8------------------
> >>
> >> This will work well for all{yes,mod,no}config.
> >
> > This two-option arrangement is fine (though it assumes there won't be
> > another stack protector option in the future).
> >
> > The issue I have is this removes _AUTO, which I think can be solved in
> > the two-option arrangement too. The purpose of _AUTO is to effectively
> > enable stack-protector by default. As this option has been available
> > for over 10 years, and all distros enable it, it's an obvious
> > candidate to be enabled-by-default, especially since it kills a class
> > of exploits (as mentioned in my commit log: BlueBorne was trivially
> > defeated with stack-protector). So it should be possible to make these
> > two options "default y", yes?
> 
> 
> Yes.
> 
> Both should be "default y" to keep the equivalent behavior
> since the current default is _AUTO.
> 

Since the discussions in this thread are going in a million different
directions and making my head hurt, ping me if I'm needed re. the WANT_*
stuff or anything else. Masahiro's two-symbol version is much simpler
and neater if the caveats re. "fallback" are minor enough to not matter.
:)

Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masahiro Yamada Feb. 13, 2018, 8:59 a.m. UTC | #60

2018-02-13 17:35 GMT+09:00 Arnd Bergmann <[e-mail]>:
> On Tue, Feb 13, 2018 at 2:53 AM, Randy Dunlap <[e-mail]> wrote:
>> On 02/12/2018 05:41 PM, Masahiro Yamada wrote:
>>> 2018-02-13 8:48 GMT+09:00 Randy Dunlap <[e-mail]>:
>>>> On 02/12/2018 07:24 AM, Kees Cook wrote:
>>>>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada
>>>>> <[e-mail]> wrote:
>>>>
>>>>>>> (And in thinking about this, does Kconfig know the true $CC in use?
>>>>>>> i.e. the configured cross compiler, etc?)
>>>>>>
>>>>>> I was thinking of removing CONFIG_CROSS_COMPILE.
>>>>>>
>>>>>> A user can dynamically change CROSS_COMPILE from
>>>>>> "make menuconfig".
>>>>>
>>>>> Most builds I've seen implement cross compilers as an environment
>>>>> variable during all "make" invocations.
>>>>
>>>> I agree. I think you would break a bunch of build bots if you remove that.
>>>
>>>
>>> For clarification, I suggested to remove CONFIG_CROSS_COMPILE.
>>>
>>> The following code:
>>> https://github.com/torvalds/linux/blob/v4.16-rc1/Makefile#L315
>>> https://github.com/torvalds/linux/blob/v4.16-rc1/init/Kconfig#L58
>>>
>>> I hope build bots are not using this.
>>>
>>>
>>> Passing CROSS_COMPILE via the command line, environment
>>> is still supported.
>>
>> OK, I misunderstood.  That one can go away IMO.
>
> Removing it will break the workflow for some people in a minor way
> though. Could we have the top-level Makefile try to detect the
> CROSS_COMPILE? Instead of just using CC=gcc as the default,
> we could check for ${ARCH}!=`uname -m` and then see if $PATH
> contains a ${ARCH}-linux-gcc, ${ARCH}-elf-gcc or ${ARCH}-gcc.
>
> it will need slightly more complexity to deal with architectures
> that have different identifiers in linux and gcc, but I think it would
> be a nice feature anyway.
>
>      Arnd

Some architectures such as m68k, mips, etc.
do this in their arch/*/Makefile
by using $(call cc-cross-prefix, ...)

But, currently we do not do this globally.

It would be possible to do it, but it does not mean
backward-compatible with CONFIG_CROSS_COMPILE.

Palmer Dabbelt Feb. 15, 2018, 11:38 p.m. UTC | #61

On Sun, 11 Feb 2018 12:06:35 PST (-0800), Linus Torvalds wrote:
> On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds
> <[e-mail]> wrote:
>>
>> Well, it's still not a very *big* bump. With modern distros being at
>> 7.3, and people testing pre-releases of gcc-8, something like gcc-4.5
>> is still pretty darn ancient.
>
> ... it's worth noting that our _documentation_ may claim that gcc-3.2
> is the minimum supported version, but Arnd pointed out that a few
> months ago that apparently nothing older than 4.1 has actually worked
> for a longish while, and gcc-4.3 was needed on several architectures.
>
> So the _real_ jump in required gcc version would be from 4.1 (4.3 in
> many cases) to 4.5, not from our documented "3.2 minimum".
>
> Arnd claimed that some architectures needed even newer-than-4.3, but I
> assume that's limited to things like RISC-V that simply don't have old
> gcc support at all.

Just for the record, we'd really like users to use GCC 7.3.0 and binutils 2.30 
(or newer), as even though we had support earlier versions (back to binutils 
2.28 and gcc 7.1.0) there's a handful of bugs floating around in our ports 
there.

Of course, I'm not suggesting that as a kernel-wide policy :).  It looks like 
we're going to end up with distributions shipping 7.3.0 and 2.30 as their first 
RISC-V versions, so hopefully we're safe here.

> That was from a discussion about bug report that only happened with
> gcc-4.4, and was because gcc-4.4 did insane things, so we were talking
> about how it wasn't necessarily worth supporting.
>
> So we really have had a lot of unrelated reasons why just saying
> "gcc-4.5 or newer"  would be a good thing.
>
>             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h
index c16e82e..83029f92 100644
--- a/scripts/kconfig/expr.h
+++ b/scripts/kconfig/expr.h
@@ -183,6 +183,7 @@  enum prop_type {
 	P_IMPLY,    /* imply BAR */
 	P_RANGE,    /* range 7..100 (for a symbol) */
 	P_ENV,      /* value from environment variable */
+	P_SHELL,    /* shell command */
 	P_SYMBOL,   /* where a symbol is defined */
 };
 
diff --git a/scripts/kconfig/kconf_id.c b/scripts/kconfig/kconf_id.c
index 3ea9c5f..0db9d1c 100644
--- a/scripts/kconfig/kconf_id.c
+++ b/scripts/kconfig/kconf_id.c
@@ -34,6 +34,7 @@  static struct kconf_id kconf_id_array[] = {
 	{ "defconfig_list",	T_OPT_DEFCONFIG_LIST,	TF_OPTION },
 	{ "env",		T_OPT_ENV,		TF_OPTION },
 	{ "allnoconfig_y",	T_OPT_ALLNOCONFIG_Y,	TF_OPTION },
+	{ "shell",		T_OPT_SHELL,		TF_OPTION },
 };
 
 #define KCONF_ID_ARRAY_SIZE (sizeof(kconf_id_array)/sizeof(struct kconf_id))
diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h
index 4e23feb..8d05042 100644
--- a/scripts/kconfig/lkc.h
+++ b/scripts/kconfig/lkc.h
@@ -60,6 +60,7 @@  enum conf_def_mode {
 #define T_OPT_DEFCONFIG_LIST	2
 #define T_OPT_ENV		3
 #define T_OPT_ALLNOCONFIG_Y	4
+#define T_OPT_SHELL		5
 
 struct kconf_id {
 	const char *name;
diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 9922285..6254dfb 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -216,6 +216,9 @@  void menu_add_option(int token, char *arg)
 	case T_OPT_ENV:
 		prop_add_env(arg);
 		break;
+	case T_OPT_SHELL:
+		prop_add_shell(arg);
+		break;
 	case T_OPT_ALLNOCONFIG_Y:
 		current_entry->sym->flags |= SYMBOL_ALLNOCONFIG_Y;
 		break;
diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
index 893eae6..02ac4f4 100644
--- a/scripts/kconfig/symbol.c
+++ b/scripts/kconfig/symbol.c
@@ -4,6 +4,7 @@ 
  */
 
 #include <ctype.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <regex.h>
@@ -1370,6 +1371,8 @@  const char *prop_get_type_name(enum prop_type type)
 		return "prompt";
 	case P_ENV:
 		return "env";
+	case P_SHELL:
+		return "shell";
 	case P_COMMENT:
 		return "comment";
 	case P_MENU:
@@ -1420,3 +1423,74 @@  static void prop_add_env(const char *env)
 	else
 		menu_warn(current_entry, "environment variable %s undefined", env);
 }
+
+static void prop_add_shell(const char *cmd)
+{
+	struct symbol *sym, *sym2;
+	struct property *prop;
+	char *expanded_cmd;
+	FILE *p;
+	char stdout[256];
+	int ret, len;
+
+	sym = current_entry->sym;
+	for_all_properties(sym, prop, P_SHELL) {
+		sym2 = prop_get_symbol(prop);
+		if (strcmp(sym2->name, cmd))
+			menu_warn(current_entry, "redefining shell command symbol from %s",
+				  sym2->name);
+		return;
+	}
+
+	prop = prop_alloc(P_SHELL, sym);
+	prop->expr = expr_alloc_symbol(sym_lookup(cmd, SYMBOL_CONST));
+
+	expanded_cmd = sym_expand_string_value(cmd);
+
+	/* surround the command with ( ) in case it is piped commands */
+	len = strlen(expanded_cmd);
+	expanded_cmd = xrealloc(expanded_cmd, len + 3);
+	memmove(expanded_cmd + 1, expanded_cmd, len);
+	expanded_cmd[0] = '(';
+	expanded_cmd[len + 1] = ')';
+	expanded_cmd[len + 2] = 0;
+
+	switch (sym->type) {
+	case S_BOOLEAN:
+		/* suppress both stdout and stderr. */
+		len = strlen(expanded_cmd) + strlen(" >/dev/null 2>&1") + 1;
+		expanded_cmd = realloc(expanded_cmd, len);
+		strcat(expanded_cmd, " >/dev/null 2>&1");
+
+		ret = system(expanded_cmd);
+		sym_add_default(sym, ret == 0 ? "y" : "n");
+		break;
+	case S_INT:
+	case S_HEX:
+	case S_STRING:
+		/* suppress only stderr. we want to get stdout. */
+		len = strlen(expanded_cmd) + strlen(" 2>/dev/null") + 1;
+		expanded_cmd = realloc(expanded_cmd, len);
+		strcat(expanded_cmd, " 2>/dev/null");
+
+		p = popen(expanded_cmd, "r");
+		if (!p)
+			goto free;
+		if (fgets(stdout, sizeof(stdout), p)) {
+			stdout[sizeof(stdout) - 1] = 0;
+			len = strlen(stdout);
+			if (stdout[len - 1] == '\n')
+				stdout[len - 1] = 0;
+		} else {
+			stdout[0] = 0;
+		}
+		sym_add_default(sym, stdout);
+		break;
+	default:
+		menu_warn(current_entry, "unsupported type for shell command\n");
+		break;
+	}
+
+free:
+	free(expanded_cmd);
+}