| Message ID | 20220414135941.1732585-4-ascull@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | Fuzzing and ASAN for sandbox | expand |
On Thu, Apr 14, 2022 at 01:59:32PM +0000, Andrew Scull wrote: > Use the common infrastructure to create a linker list of the sandbox > command line flags rather than using a custom method. > > The list is changed from containing pointers to containing structs and > the uses are updated accordingly. > > Signed-off-by: Andrew Scull <ascull@google.com> > Reviewed-by: Simon Glass <sjg@chromium.org> > --- > arch/sandbox/cpu/os.c | 21 ++++++++++----------- > arch/sandbox/cpu/start.c | 10 +++++----- > arch/sandbox/cpu/u-boot-spl.lds | 6 ------ > arch/sandbox/cpu/u-boot.lds | 6 ------ > arch/sandbox/include/asm/getopt.h | 19 ++++++++++++------- > arch/sandbox/include/asm/sections.h | 25 ------------------------- > 6 files changed, 27 insertions(+), 60 deletions(-) This particular test causes all of the pytest infrastructure to fail. I believe "make qcheck" will show this, even, for a possibly easier reproducer.
On Fri, 29 Apr 2022 at 08:11, Tom Rini <trini@konsulko.com> wrote: > > On Thu, Apr 14, 2022 at 01:59:32PM +0000, Andrew Scull wrote: > > > Use the common infrastructure to create a linker list of the sandbox > > command line flags rather than using a custom method. > > > > The list is changed from containing pointers to containing structs and > > the uses are updated accordingly. > > > > Signed-off-by: Andrew Scull <ascull@google.com> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > --- > > arch/sandbox/cpu/os.c | 21 ++++++++++----------- > > arch/sandbox/cpu/start.c | 10 +++++----- > > arch/sandbox/cpu/u-boot-spl.lds | 6 ------ > > arch/sandbox/cpu/u-boot.lds | 6 ------ > > arch/sandbox/include/asm/getopt.h | 19 ++++++++++++------- > > arch/sandbox/include/asm/sections.h | 25 ------------------------- > > 6 files changed, 27 insertions(+), 60 deletions(-) > > This particular test causes all of the pytest infrastructure to fail. I > believe "make qcheck" will show this, even, for a possibly easier > reproducer. I didn't run this series through the CI so I hadn't been running the pytest suite. `make qcheck` does run the pytest suite and shows a bunch more ASAN failures. Some are simple but others are less obvious and are going to need some time to dig into. The issues being found by ASAN here are in the main u-boot code, not just the test code, and a couple of them are in crypto components so there might be some more interesting bugs to discover.
On Mon, May 02, 2022 at 09:24:52AM -0700, Andrew Scull wrote: > On Fri, 29 Apr 2022 at 08:11, Tom Rini <trini@konsulko.com> wrote: > > > > On Thu, Apr 14, 2022 at 01:59:32PM +0000, Andrew Scull wrote: > > > > > Use the common infrastructure to create a linker list of the sandbox > > > command line flags rather than using a custom method. > > > > > > The list is changed from containing pointers to containing structs and > > > the uses are updated accordingly. > > > > > > Signed-off-by: Andrew Scull <ascull@google.com> > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > --- > > > arch/sandbox/cpu/os.c | 21 ++++++++++----------- > > > arch/sandbox/cpu/start.c | 10 +++++----- > > > arch/sandbox/cpu/u-boot-spl.lds | 6 ------ > > > arch/sandbox/cpu/u-boot.lds | 6 ------ > > > arch/sandbox/include/asm/getopt.h | 19 ++++++++++++------- > > > arch/sandbox/include/asm/sections.h | 25 ------------------------- > > > 6 files changed, 27 insertions(+), 60 deletions(-) > > > > This particular test causes all of the pytest infrastructure to fail. I > > believe "make qcheck" will show this, even, for a possibly easier > > reproducer. > > I didn't run this series through the CI so I hadn't been running the > pytest suite. `make qcheck` does run the pytest suite and shows a > bunch more ASAN failures. Some are simple but others are less obvious > and are going to need some time to dig into. > > The issues being found by ASAN here are in the main u-boot code, not > just the test code, and a couple of them are in crypto components so > there might be some more interesting bugs to discover. OK, yeah, ASAN failures within the test suite are less important. I forgot to mention maybe I disabled ASAN in sandbox and still got the basic CI failure I mentioned above, bisect'd down to here. Please make sure CI does pass on this for the next iteration and it's OK to go with documenting how to enable ASAN afterwards and that tests need further clean-up.
Looking at this again with a recent fetch of master, LTO is deleting the linker list entries for the getopt declarations. Both clang and gcc do this. I've not got an idea why at the moment and would love to hear any suggestions. On Mon, 2 May 2022 at 17:27, Tom Rini <trini@konsulko.com> wrote: > > On Mon, May 02, 2022 at 09:24:52AM -0700, Andrew Scull wrote: > > On Fri, 29 Apr 2022 at 08:11, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Thu, Apr 14, 2022 at 01:59:32PM +0000, Andrew Scull wrote: > > > > > > > Use the common infrastructure to create a linker list of the sandbox > > > > command line flags rather than using a custom method. > > > > > > > > The list is changed from containing pointers to containing structs and > > > > the uses are updated accordingly. > > > > > > > > Signed-off-by: Andrew Scull <ascull@google.com> > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > --- > > > > arch/sandbox/cpu/os.c | 21 ++++++++++----------- > > > > arch/sandbox/cpu/start.c | 10 +++++----- > > > > arch/sandbox/cpu/u-boot-spl.lds | 6 ------ > > > > arch/sandbox/cpu/u-boot.lds | 6 ------ > > > > arch/sandbox/include/asm/getopt.h | 19 ++++++++++++------- > > > > arch/sandbox/include/asm/sections.h | 25 ------------------------- > > > > 6 files changed, 27 insertions(+), 60 deletions(-) > > > > > > This particular test causes all of the pytest infrastructure to fail. I > > > believe "make qcheck" will show this, even, for a possibly easier > > > reproducer. > > > > I didn't run this series through the CI so I hadn't been running the > > pytest suite. `make qcheck` does run the pytest suite and shows a > > bunch more ASAN failures. Some are simple but others are less obvious > > and are going to need some time to dig into. > > > > The issues being found by ASAN here are in the main u-boot code, not > > just the test code, and a couple of them are in crypto components so > > there might be some more interesting bugs to discover. > > OK, yeah, ASAN failures within the test suite are less important. I > forgot to mention maybe I disabled ASAN in sandbox and still got the > basic CI failure I mentioned above, bisect'd down to here. Please make > sure CI does pass on this for the next iteration and it's OK to go with > documenting how to enable ASAN afterwards and that tests need further > clean-up. > > -- > Tom
> Looking at this again with a recent fetch of master, LTO is deleting > the linker list entries for the getopt declarations. Both clang and > gcc do this. I've not got an idea why at the moment and would love to > hear any suggestions. Turns out that this isn't a new problem and include/event.h already had a work around by adding the 'used' attribute that I copied. Sorting out the rebase, the tests look better locally, up until they ask to run as root, which I don't feel comfortable with, so I am sending it to the CI for more complete coverage.
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index d83c862182..72a72029f2 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -424,9 +424,8 @@ static struct option *long_opts; int os_parse_args(struct sandbox_state *state, int argc, char *argv[]) { - struct sandbox_cmdline_option **sb_opt = - __u_boot_sandbox_option_start(); - size_t num_options = __u_boot_sandbox_option_count(); + struct sandbox_cmdline_option *sb_opt = SANDBOX_CMDLINE_OPT_START(); + size_t num_options = SANDBOX_CMDLINE_OPT_COUNT(); size_t i; int hidden_short_opt; @@ -455,17 +454,17 @@ int os_parse_args(struct sandbox_state *state, int argc, char *argv[]) hidden_short_opt = 0x100; si = 0; for (i = 0; i < num_options; ++i) { - long_opts[i].name = sb_opt[i]->flag; - long_opts[i].has_arg = sb_opt[i]->has_arg ? + long_opts[i].name = sb_opt[i].flag; + long_opts[i].has_arg = sb_opt[i].has_arg ? required_argument : no_argument; long_opts[i].flag = NULL; - if (sb_opt[i]->flag_short) { - short_opts[si++] = long_opts[i].val = sb_opt[i]->flag_short; + if (sb_opt[i].flag_short) { + short_opts[si++] = long_opts[i].val = sb_opt[i].flag_short; if (long_opts[i].has_arg == required_argument) short_opts[si++] = ':'; } else - long_opts[i].val = sb_opt[i]->flag_short = hidden_short_opt++; + long_opts[i].val = sb_opt[i].flag_short = hidden_short_opt++; } short_opts[si] = '\0'; @@ -480,9 +479,9 @@ int os_parse_args(struct sandbox_state *state, int argc, char *argv[]) */ while ((c = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) { for (i = 0; i < num_options; ++i) { - if (sb_opt[i]->flag_short == c) { - if (sb_opt[i]->callback(state, optarg)) { - state->parse_err = sb_opt[i]->flag; + if (sb_opt[i].flag_short == c) { + if (sb_opt[i].callback(state, optarg)) { + state->parse_err = sb_opt[i].flag; return 0; } break; diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index 0f5a87309d..6bcb8ffa28 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -59,9 +59,8 @@ static int h_compare_opt(const void *p1, const void *p2) int sandbox_early_getopt_check(void) { struct sandbox_state *state = state_get_current(); - struct sandbox_cmdline_option **sb_opt = - __u_boot_sandbox_option_start(); - size_t num_options = __u_boot_sandbox_option_count(); + struct sandbox_cmdline_option *sb_opt = SANDBOX_CMDLINE_OPT_START(); + size_t num_options = SANDBOX_CMDLINE_OPT_COUNT(); size_t i; int max_arg_len, max_noarg_len; struct sandbox_cmdline_option **sorted_opt; @@ -85,7 +84,7 @@ int sandbox_early_getopt_check(void) max_arg_len = 0; for (i = 0; i < num_options; ++i) - max_arg_len = max((int)strlen(sb_opt[i]->flag), max_arg_len); + max_arg_len = max((int)strlen(sb_opt[i].flag), max_arg_len); max_noarg_len = max_arg_len + 7; /* Sort the options */ @@ -95,7 +94,8 @@ int sandbox_early_getopt_check(void) printf("No memory to sort options\n"); os_exit(1); } - memcpy(sorted_opt, sb_opt, size); + for (i = 0; i < num_options; ++i) + sorted_opt[i] = &sb_opt[i]; qsort(sorted_opt, num_options, sizeof(*sorted_opt), h_compare_opt); for (i = 0; i < num_options; ++i) { diff --git a/arch/sandbox/cpu/u-boot-spl.lds b/arch/sandbox/cpu/u-boot-spl.lds index 6754f4ef6c..5c19d090cb 100644 --- a/arch/sandbox/cpu/u-boot-spl.lds +++ b/arch/sandbox/cpu/u-boot-spl.lds @@ -20,12 +20,6 @@ SECTIONS *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.priv_data*))) __priv_data_end = .; } - - _u_boot_sandbox_getopt : { - *(.u_boot_sandbox_getopt_start) - KEEP(*(.u_boot_sandbox_getopt)) - *(.u_boot_sandbox_getopt_end) - } } INSERT AFTER .data; diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds index 7abe232ad9..6fa244fae9 100644 --- a/arch/sandbox/cpu/u-boot.lds +++ b/arch/sandbox/cpu/u-boot.lds @@ -13,12 +13,6 @@ SECTIONS KEEP(*(SORT(.u_boot_list*))); } - _u_boot_sandbox_getopt : { - *(.u_boot_sandbox_getopt_start) - *(.u_boot_sandbox_getopt) - *(.u_boot_sandbox_getopt_end) - } - efi_runtime_start : { *(___efi_runtime_start) } diff --git a/arch/sandbox/include/asm/getopt.h b/arch/sandbox/include/asm/getopt.h index d2145ad6e2..a4b510bd20 100644 --- a/arch/sandbox/include/asm/getopt.h +++ b/arch/sandbox/include/asm/getopt.h @@ -9,6 +9,8 @@ #ifndef __SANDBOX_GETOPT_H #define __SANDBOX_GETOPT_H +#include <linker_lists.h> + struct sandbox_state; /* @@ -36,18 +38,13 @@ struct sandbox_cmdline_option { * magic junk that makes this all work. */ #define _SANDBOX_CMDLINE_OPT(f, s, ha, h) \ - static struct sandbox_cmdline_option sandbox_cmdline_option_##f = { \ + ll_entry_declare(struct sandbox_cmdline_option, f, sandbox_getopt) = { \ .flag = #f, \ .flag_short = s, \ .help = h, \ .has_arg = ha, \ .callback = sandbox_cmdline_cb_##f, \ - }; \ - /* Ppointer to the struct in a special section for the linker script */ \ - static __used __section(".u_boot_sandbox_getopt") \ - struct sandbox_cmdline_option \ - *sandbox_cmdline_option_##f##_ptr = \ - &sandbox_cmdline_option_##f + } /** * Macros for end code to declare new command line flags. @@ -69,4 +66,12 @@ struct sandbox_cmdline_option { */ #define SANDBOX_CMDLINE_OPT_SHORT(f, s, ha, h) _SANDBOX_CMDLINE_OPT(f, s, ha, h) +/** Get the start of the list of command line flags. */ +#define SANDBOX_CMDLINE_OPT_START() \ + ll_entry_start(struct sandbox_cmdline_option, sandbox_getopt) + +/** Get the number of entries in the command line flags list. */ +#define SANDBOX_CMDLINE_OPT_COUNT() \ + ll_entry_count(struct sandbox_cmdline_option, sandbox_getopt) + #endif diff --git a/arch/sandbox/include/asm/sections.h b/arch/sandbox/include/asm/sections.h index f4351ae7db..c335cb2061 100644 --- a/arch/sandbox/include/asm/sections.h +++ b/arch/sandbox/include/asm/sections.h @@ -11,29 +11,4 @@ #include <asm-generic/sections.h> -struct sandbox_cmdline_option; - -static inline struct sandbox_cmdline_option ** -__u_boot_sandbox_option_start(void) -{ - static char start[0] __aligned(4) __attribute__((unused)) - __section(".u_boot_sandbox_getopt_start"); - - return (struct sandbox_cmdline_option **)&start; -} - -static inline struct sandbox_cmdline_option ** -__u_boot_sandbox_option_end(void) -{ - static char end[0] __aligned(4) __attribute__((unused)) - __section(".u_boot_sandbox_getopt_end"); - - return (struct sandbox_cmdline_option **)&end; -} - -static inline size_t __u_boot_sandbox_option_count(void) -{ - return __u_boot_sandbox_option_end() - __u_boot_sandbox_option_start(); -} - #endif