| Message ID | 20231002223219.2966816-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v1] bpftool: Align output skeleton ELF code | expand |
On Mon, Oct 2, 2023 at 3:32 PM Ian Rogers <irogers@google.com> wrote: > > libbpf accesses the ELF data requiring at least 8 byte alignment, > however, the data is generated into a C string that doesn't guarantee > alignment. Fix this by assigning to an aligned char array, use sizeof > on the array, less one for the \0 terminator. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- Perhaps this could have a fixes tag: Fixes: d510296d331a ("bpftool: Use syscall/loader program in "prog load" and "gen skeleton" command.") The unaligned problem was seen in perf's offcpu code as well as bcc's libbpf_tools. I didn't see problems with map data and opts data, but inspection of the code shows they likely have the same issue. I was testing with -fsanitize=alignment and -fsanitize-undefined-trap-on-error. Thanks, Ian > tools/bpf/bpftool/gen.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c > index 2883660d6b67..b8ebcee9bc56 100644 > --- a/tools/bpf/bpftool/gen.c > +++ b/tools/bpf/bpftool/gen.c > @@ -1209,7 +1209,7 @@ static int do_skeleton(int argc, char **argv) > codegen("\ > \n\ > \n\ > - s->data = (void *)%2$s__elf_bytes(&s->data_sz); \n\ > + s->data = (void *)%1$s__elf_bytes(&s->data_sz); \n\ > \n\ > obj->skeleton = s; \n\ > return 0; \n\ > @@ -1218,12 +1218,12 @@ static int do_skeleton(int argc, char **argv) > return err; \n\ > } \n\ > \n\ > - static inline const void *%2$s__elf_bytes(size_t *sz) \n\ > + static inline const void *%1$s__elf_bytes(size_t *sz) \n\ > { \n\ > - *sz = %1$d; \n\ > - return (const void *)\"\\ \n\ > - " > - , file_sz, obj_name); > + static const char data[] __attribute__((__aligned__(8))) = \"\\\n\ > + ", > + obj_name > + ); > > /* embed contents of BPF object file */ > print_hex(obj_data, file_sz); > @@ -1231,6 +1231,9 @@ static int do_skeleton(int argc, char **argv) > codegen("\ > \n\ > \"; \n\ > + \n\ > + *sz = sizeof(data) - 1; \n\ > + return (const void *)data; \n\ > } \n\ > \n\ > #ifdef __cplusplus \n\ > -- > 2.42.0.582.g8ccd20d70d-goog >
On 03/10/2023 05:29, Ian Rogers wrote: > On Mon, Oct 2, 2023 at 3:32 PM Ian Rogers <irogers@google.com> wrote: >> >> libbpf accesses the ELF data requiring at least 8 byte alignment, >> however, the data is generated into a C string that doesn't guarantee >> alignment. Fix this by assigning to an aligned char array, use sizeof >> on the array, less one for the \0 terminator. >> >> Signed-off-by: Ian Rogers <irogers@google.com> this looks like a great catch to me! Reviewed-by: Alan Maguire <alan.maguire@oracle.com> >> --- > > Perhaps this could have a fixes tag: > Fixes: d510296d331a ("bpftool: Use syscall/loader program in "prog > load" and "gen skeleton" command.") > Yep, or perhaps Fixes: a6cc6b34b93e ("bpftool: Provide a helper method for accessing skeleton's embedded ELF data") > The unaligned problem was seen in perf's offcpu code as well as bcc's > libbpf_tools. I didn't see problems with map data and opts data, but > inspection of the code shows they likely have the same issue. I was > testing with -fsanitize=alignment and > -fsanitize-undefined-trap-on-error. > > Thanks, > Ian > >> tools/bpf/bpftool/gen.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c >> index 2883660d6b67..b8ebcee9bc56 100644 >> --- a/tools/bpf/bpftool/gen.c >> +++ b/tools/bpf/bpftool/gen.c >> @@ -1209,7 +1209,7 @@ static int do_skeleton(int argc, char **argv) >> codegen("\ >> \n\ >> \n\ >> - s->data = (void *)%2$s__elf_bytes(&s->data_sz); \n\ >> + s->data = (void *)%1$s__elf_bytes(&s->data_sz); \n\ >> \n\ >> obj->skeleton = s; \n\ >> return 0; \n\ >> @@ -1218,12 +1218,12 @@ static int do_skeleton(int argc, char **argv) >> return err; \n\ >> } \n\ >> \n\ >> - static inline const void *%2$s__elf_bytes(size_t *sz) \n\ >> + static inline const void *%1$s__elf_bytes(size_t *sz) \n\ >> { \n\ >> - *sz = %1$d; \n\ >> - return (const void *)\"\\ \n\ >> - " >> - , file_sz, obj_name); >> + static const char data[] __attribute__((__aligned__(8))) = \"\\\n\ >> + ", >> + obj_name >> + ); >> >> /* embed contents of BPF object file */ >> print_hex(obj_data, file_sz); >> @@ -1231,6 +1231,9 @@ static int do_skeleton(int argc, char **argv) >> codegen("\ >> \n\ >> \"; \n\ >> + \n\ >> + *sz = sizeof(data) - 1; \n\ >> + return (const void *)data; \n\ >> } \n\ >> \n\ >> #ifdef __cplusplus \n\ >> -- >> 2.42.0.582.g8ccd20d70d-goog >> >
diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c index 2883660d6b67..b8ebcee9bc56 100644 --- a/tools/bpf/bpftool/gen.c +++ b/tools/bpf/bpftool/gen.c @@ -1209,7 +1209,7 @@ static int do_skeleton(int argc, char **argv) codegen("\ \n\ \n\ - s->data = (void *)%2$s__elf_bytes(&s->data_sz); \n\ + s->data = (void *)%1$s__elf_bytes(&s->data_sz); \n\ \n\ obj->skeleton = s; \n\ return 0; \n\ @@ -1218,12 +1218,12 @@ static int do_skeleton(int argc, char **argv) return err; \n\ } \n\ \n\ - static inline const void *%2$s__elf_bytes(size_t *sz) \n\ + static inline const void *%1$s__elf_bytes(size_t *sz) \n\ { \n\ - *sz = %1$d; \n\ - return (const void *)\"\\ \n\ - " - , file_sz, obj_name); + static const char data[] __attribute__((__aligned__(8))) = \"\\\n\ + ", + obj_name + ); /* embed contents of BPF object file */ print_hex(obj_data, file_sz); @@ -1231,6 +1231,9 @@ static int do_skeleton(int argc, char **argv) codegen("\ \n\ \"; \n\ + \n\ + *sz = sizeof(data) - 1; \n\ + return (const void *)data; \n\ } \n\ \n\ #ifdef __cplusplus \n\
libbpf accesses the ELF data requiring at least 8 byte alignment, however, the data is generated into a C string that doesn't guarantee alignment. Fix this by assigning to an aligned char array, use sizeof on the array, less one for the \0 terminator. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/bpf/bpftool/gen.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)