| Message ID | 1518106752-29228-5-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | Kconfig: add new special property shell= to test compiler options in Kconfig | expand |
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 <torvalds@linux-foundation.org> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> 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
2018-02-09 14:30 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > 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 <torvalds@linux-foundation.org> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > 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. -- Best Regards Masahiro Yamada
On Fri, Feb 09, 2018 at 06:19:19PM +0900, Masahiro Yamada wrote: > 2018-02-09 14:30 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > > 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 <torvalds@linux-foundation.org> > >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > > > 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
On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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
On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote: > On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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
2018-02-10 14:48 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote: >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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. -- Best Regards Masahiro Yamada
On Sat, Feb 10, 2018 at 04:12:13PM +0900, Masahiro Yamada wrote: > 2018-02-10 14:48 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote: > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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
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 <ulfalizer@gmail.com>: > > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote: > > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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
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 <ulfalizer@gmail.com>: > > > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote: > > > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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
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 <ulfalizer@gmail.com>: > > > > > On Fri, Feb 09, 2018 at 12:46:54PM -0800, Kees Cook wrote: > > > > >> On Fri, Feb 9, 2018 at 4:46 AM, Ulf Magnusson <ulfalizer@gmail.com> 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
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. -- ~Randy
On Sat, Feb 10, 2018 at 12:55 AM, Ulf Magnusson <ulfalizer@gmail.com> 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 -- Kees Cook Pixel Security
On Sat, Feb 10, 2018 at 11:23 AM, Kees Cook <keescook@chromium.org> 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
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
On Sat, Feb 10, 2018 at 12:08 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Feb 10, 2018 at 11:23 AM, Kees Cook <keescook@chromium.org> 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 -- Kees Cook Pixel Security
On Sat, Feb 10, 2018 at 8:13 PM, Kees Cook <keescook@chromium.org> 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
On Sat, Feb 10, 2018 at 8:46 PM, Linus Torvalds <torvalds@linux-foundation.org> 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
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
On Sat, Feb 10, 2018 at 11:28 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, Feb 10, 2018 at 8:46 PM, Linus Torvalds > <torvalds@linux-foundation.org> 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 Pixel Security
On Sun, Feb 11, 2018 at 2:34 AM, Ulf Magnusson <ulfalizer@gmail.com> 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 -- Kees Cook Pixel Security
On Sun, Feb 11, 2018 at 9:56 AM, Kees Cook <keescook@chromium.org> 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
On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
On Sun, Feb 11, 2018 at 10:13 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, Feb 11, 2018 at 9:56 AM, Kees Cook <keescook@chromium.org> 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 -- Kees Cook Pixel Security
On Sun, Feb 11, 2018 at 10:13 AM, Linus Torvalds <torvalds@linux-foundation.org> 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
On Sun, Feb 11, 2018 at 11:39 AM, Kees Cook <keescook@chromium.org> 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
On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds <torvalds@linux-foundation.org> 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
On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
On Sun, Feb 11, 2018 at 10:34 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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 -- Kees Cook Pixel Security
On Sun, Feb 11, 2018 at 9:06 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds > <torvalds@linux-foundation.org> 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
On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, Feb 11, 2018 at 11:39 AM, Kees Cook <keescook@chromium.org> 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 Pixel Security
On Sun, Feb 11, 2018 at 1:10 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Sun, Feb 11, 2018 at 9:06 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds >> <torvalds@linux-foundation.org> 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 -- Kees Cook Pixel Security
On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
On Sun, Feb 11, 2018 at 10:05 PM, Kees Cook <keescook@chromium.org> wrote: > On Sun, Feb 11, 2018 at 10:34 AM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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"
On Sun, Feb 11, 2018 at 1:19 PM, Kees Cook <keescook@chromium.org> wrote: > On Sun, Feb 11, 2018 at 1:10 PM, Arnd Bergmann <arnd@arndb.de> 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
On Sun, Feb 11, 2018 at 10:10 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Sun, Feb 11, 2018 at 9:06 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds >> <torvalds@linux-foundation.org> 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 -- geert@linux-m68k.org 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
On Sun, Feb 11, 2018 at 10:13:44AM -0800, Linus Torvalds 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 So the unofficial plan was to enforce asm-goto and -fentry support by hard failure to build, which would get us at gcc-4.6 and then remove all the fallback cruft needed for those features -- for x86. If we want to do this tree wide, that's obviously OK with me too ;-) The below is the two force-asm-goto and force-fentry patches folded. --- Makefile | 17 +++++++++++------ arch/x86/Makefile | 25 +++++-------------------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/Makefile b/Makefile index d192dd826cce..1a46f23d0974 100644 --- a/Makefile +++ b/Makefile @@ -489,6 +489,17 @@ KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC) KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC) endif +# check for 'asm goto' +ifeq ($(shell $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y) + CC_HAVE_ASM_GOTO := 1 + KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO + KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO +endif + +ifeq ($(call cc-option-yn, -mfentry), y) + CC_HAVE_FENTRY := 1 +endif + ifeq ($(config-targets),1) # =========================================================================== # *config targets only - make sure prerequisites are updated, and descend @@ -654,12 +665,6 @@ KBUILD_CFLAGS += $(call cc-ifversion, -lt, 0409, \ # Tell gcc to never replace conditional load with a non-conditional one KBUILD_CFLAGS += $(call cc-option,--param=allow-store-data-races=0) -# check for 'asm goto' -ifeq ($(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y) - KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO - KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO -endif - include scripts/Makefile.kcov include scripts/Makefile.gcc-plugins diff --git a/arch/x86/Makefile b/arch/x86/Makefile index fad55160dcb9..35cea458a7be 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -158,27 +158,12 @@ ifdef CONFIG_X86_X32 endif export CONFIG_X86_X32_ABI -# -# If the function graph tracer is used with mcount instead of fentry, -# '-maccumulate-outgoing-args' is needed to prevent a GCC bug -# (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109) -# -ifdef CONFIG_FUNCTION_GRAPH_TRACER - ifndef CONFIG_HAVE_FENTRY - ACCUMULATE_OUTGOING_ARGS := 1 - else - ifeq ($(call cc-option-yn, -mfentry), n) - ACCUMULATE_OUTGOING_ARGS := 1 - - # GCC ignores '-maccumulate-outgoing-args' when used with '-Os'. - # If '-Os' is enabled, disable it and print a warning. - ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE - undefine CONFIG_CC_OPTIMIZE_FOR_SIZE - $(warning Disabling CONFIG_CC_OPTIMIZE_FOR_SIZE. Your compiler does not have -mfentry so you cannot optimize for size with CONFIG_FUNCTION_GRAPH_TRACER.) - endif +ifndef CC_HAVE_ASM_GOTO + $(error Compiler lacks asm-goto support.) +endif - endif - endif +ifndef CC_HAVE_FENTRY + $(error Compiler lacks -mfentry support.) endif #
On Mon, 12 Feb 2018, Peter Zijlstra wrote: > On Sun, Feb 11, 2018 at 10:13:44AM -0800, Linus Torvalds 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 > > So the unofficial plan was to enforce asm-goto and -fentry support by > hard failure to build, which would get us at gcc-4.6 and then remove all Has gcc-4.6 a (planned) retpoline backport? IIRC the cutoff for that was gcc 4.9 Thanks, tglx
On Sun, Feb 11, 2018 at 10:50 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, Feb 11, 2018 at 1:19 PM, Kees Cook <keescook@chromium.org> wrote: >> On Sun, Feb 11, 2018 at 1:10 PM, Arnd Bergmann <arnd@arndb.de> 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
2018-02-11 19:34 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > 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. -- Best Regards Masahiro Yamada
On Mon, Feb 12, 2018 at 11:44 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-11 19:34 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> 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
On Mon, Feb 12, 2018 at 12:44 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > On Mon, Feb 12, 2018 at 11:44 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> 2018-02-11 19:34 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >>> 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
On Mon, Feb 12, 2018 at 11:27:25AM +0100, Thomas Gleixner wrote: > On Mon, 12 Feb 2018, Peter Zijlstra wrote: > > > On Sun, Feb 11, 2018 at 10:13:44AM -0800, Linus Torvalds 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 > > > > So the unofficial plan was to enforce asm-goto and -fentry support by > > hard failure to build, which would get us at gcc-4.6 and then remove all > > Has gcc-4.6 a (planned) retpoline backport? IIRC the cutoff for that was > gcc 4.9 Official GCC will not do retpoline before 4.9 AFAIK. But if someone were to want to build a RETPOLINE=n kernel I don't see why we should mandate retpoline. Also, distro's have backported retpoline much further back already.
On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: > On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: > > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
2018-02-12 20:44 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> >> 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. > -- Best Regards Masahiro Yamada
On Mon, Feb 12, 2018 at 2:53 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-12 20:44 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > >>> >>> 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
2018-02-12 21:54 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: >> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
2018-02-12 23:21 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: > 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: >>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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 -- Best Regards Masahiro Yamada
On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: >>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
On Mon, Feb 12, 2018 at 3:23 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-12 23:21 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>: >> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: >>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
2018-02-12 2:56 GMT+09:00 Kees Cook <keescook@chromium.org>: > 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. -- Best Regards Masahiro Yamada
On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada <yamada.masahiro@socionext.com> 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 <yamada.masahiro@socionext.com> wrote: > 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: >>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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
2018-02-12 23:53 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: > On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada > <yamada.masahiro@socionext.com> 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 > <yamada.masahiro@socionext.com> wrote: >> 2018-02-12 21:54 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >>> On Sun, Feb 11, 2018 at 09:42:09PM +0100, Ulf Magnusson wrote: >>>> On Sun, Feb 11, 2018 at 9:29 PM, Ulf Magnusson <ulfalizer@gmail.com> wrote: >>>> > On Sun, Feb 11, 2018 at 6:56 PM, Kees Cook <keescook@chromium.org> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards Masahiro Yamada
On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-12 2:56 GMT+09:00 Kees Cook <keescook@chromium.org>: >> 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 -- Kees Cook Pixel Security
On Mon, Feb 12, 2018 at 4:22 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2018-02-12 23:53 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>: >> On Mon, Feb 12, 2018 at 3:21 PM, Masahiro Yamada >> <yamada.masahiro@socionext.com> 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
On Mon, Feb 12, 2018 at 2:44 AM, Masahiro Yamada <yamada.masahiro@socionext.com> 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 -- Kees Cook Pixel Security
2018-02-13 0:46 GMT+09:00 Kees Cook <keescook@chromium.org>: > On Mon, Feb 12, 2018 at 2:44 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Best Regards Masahiro Yamada
On Mon, 2018-02-12 at 09:26 +0100, Peter Zijlstra wrote: > On Sun, Feb 11, 2018 at 10:13:44AM -0800, Linus Torvalds 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 > > So the unofficial plan was to enforce asm-goto and -fentry support by > hard failure to build, which would get us at gcc-4.6 and then remove all > the fallback cruft needed for those features -- for x86. If we want to > do this tree wide, that's obviously OK with me too ;-) This would also kill clang support, right?
On Mon, Feb 12, 2018 at 8:19 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Mon, 2018-02-12 at 09:26 +0100, Peter Zijlstra wrote: >> On Sun, Feb 11, 2018 at 10:13:44AM -0800, Linus Torvalds 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 >> >> So the unofficial plan was to enforce asm-goto and -fentry support by >> hard failure to build, which would get us at gcc-4.6 and then remove all >> the fallback cruft needed for those features -- for x86. If we want to >> do this tree wide, that's obviously OK with me too ;-) > > This would also kill clang support, right? That would be bad: Android exclusively builds with clang. -Kees -- Kees Cook Pixel Security
On Mon, Feb 12, 2018 at 04:19:22PM +0000, David Woodhouse wrote: > On Mon, 2018-02-12 at 09:26 +0100, Peter Zijlstra wrote: > > On Sun, Feb 11, 2018 at 10:13:44AM -0800, Linus Torvalds 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 > > > > So the unofficial plan was to enforce asm-goto and -fentry support by > > hard failure to build, which would get us at gcc-4.6 and then remove all > > the fallback cruft needed for those features -- for x86. If we want to > > do this tree wide, that's obviously OK with me too ;-) > > This would also kill clang support, right? Yes, not sure why I should care about that though.
On Mon, Feb 12, 2018 at 08:56:31AM -0800, Kees Cook wrote:
> That would be bad: Android exclusively builds with clang.
So implement asm-goto already, and do asm-cc-output while you're at it.
The whole asm-goto/jump_label stuff really does make a measureable
difference in performance, and its bloody rediculous LLVM doesn't have
it.
And no, a different interface to do something similar is not OK. We're
not going to have a second implementation of jump-labels and everything
else that uses it.
On Mon, Feb 12, 2018 at 9:05 AM, Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Feb 12, 2018 at 08:56:31AM -0800, Kees Cook wrote: >> That would be bad: Android exclusively builds with clang. > > So implement asm-goto already, and do asm-cc-output while you're at it. Yup, I've already been asking for it. I'm hoping there will be more time/attention now that retpoline is "done". -Kees -- Kees Cook Pixel Security
On Mon, 2018-02-12 at 09:33 -0800, Kees Cook wrote: > On Mon, Feb 12, 2018 at 9:05 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Mon, Feb 12, 2018 at 08:56:31AM -0800, Kees Cook wrote: > >> That would be bad: Android exclusively builds with clang. > > > > So implement asm-goto already, and do asm-cc-output while you're at it. > > Yup, I've already been asking for it. I'm hoping there will be more > time/attention now that retpoline is "done". Retpoline isn't done for 32-bit x86. https://bugs.llvm.org/show_bug.cgi?id=36329
On Mon, Feb 12, 2018 at 9:36 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Mon, 2018-02-12 at 09:33 -0800, Kees Cook wrote: >> On Mon, Feb 12, 2018 at 9:05 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> > On Mon, Feb 12, 2018 at 08:56:31AM -0800, Kees Cook wrote: >> >> That would be bad: Android exclusively builds with clang. >> > >> > So implement asm-goto already, and do asm-cc-output while you're at it. >> >> Yup, I've already been asking for it. I'm hoping there will be more >> time/attention now that retpoline is "done". > > Retpoline isn't done for 32-bit x86. > > https://bugs.llvm.org/show_bug.cgi?id=36329 Understood. I should have said: s/now that/when/ -Kees -- Kees Cook Pixel Security
On 02/12/2018 07:24 AM, Kees Cook wrote: > On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> 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 > -- ~Randy
2018-02-13 8:48 GMT+09:00 Randy Dunlap <rdunlap@infradead.org>: > On 02/12/2018 07:24 AM, Kees Cook wrote: >> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada >> <yamada.masahiro@socionext.com> 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. -- Best Regards Masahiro Yamada
On 02/12/2018 05:41 PM, Masahiro Yamada wrote: > 2018-02-13 8:48 GMT+09:00 Randy Dunlap <rdunlap@infradead.org>: >> On 02/12/2018 07:24 AM, Kees Cook wrote: >>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada >>> <yamada.masahiro@socionext.com> 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, -- ~Randy
On Tue, Feb 13, 2018 at 2:53 AM, Randy Dunlap <rdunlap@infradead.org> wrote: > On 02/12/2018 05:41 PM, Masahiro Yamada wrote: >> 2018-02-13 8:48 GMT+09:00 Randy Dunlap <rdunlap@infradead.org>: >>> On 02/12/2018 07:24 AM, Kees Cook wrote: >>>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada >>>> <yamada.masahiro@socionext.com> 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
On Tue, Feb 13, 2018 at 01:10:34AM +0900, Masahiro Yamada wrote: > 2018-02-13 0:46 GMT+09:00 Kees Cook <keescook@chromium.org>: > > On Mon, Feb 12, 2018 at 2:44 AM, Masahiro Yamada > > <yamada.masahiro@socionext.com> 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
2018-02-13 17:35 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > On Tue, Feb 13, 2018 at 2:53 AM, Randy Dunlap <rdunlap@infradead.org> wrote: >> On 02/12/2018 05:41 PM, Masahiro Yamada wrote: >>> 2018-02-13 8:48 GMT+09:00 Randy Dunlap <rdunlap@infradead.org>: >>>> On 02/12/2018 07:24 AM, Kees Cook wrote: >>>>> On Mon, Feb 12, 2018 at 6:39 AM, Masahiro Yamada >>>>> <yamada.masahiro@socionext.com> 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. -- Best Regards Masahiro Yamada
On Sun, 11 Feb 2018 12:06:35 PST (-0800), Linus Torvalds wrote: > On Sun, Feb 11, 2018 at 11:53 AM, Linus Torvalds > <torvalds@linux-foundation.org> 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
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); +}
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 <torvalds@linux-foundation.org> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- 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(+) -- 2.7.4