| Message ID | 20250326001122.421996-2-s.gupta@arista.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [6.1,6.6,6.12,6.13] scripts/sorttable: fix ELF64 mcount_loc address parsing when compiling on 32-bit | expand |
On Tue, Mar 25, 2025 at 05:06:56PM -0700, Sahil Gupta wrote: > The ftrace __mcount_loc buildtime sort does not work properly when the host is > 32-bit and the target is 64-bit. sorttable parses the start and stop addresses > by calling strtoul on the buffer holding the hexadecimal string. Since the > target is 64-bit but unsigned long on 32-bit machines is 32 bits, strtoul, > and by extension the start and stop addresses, can max out to 2^32 - 1. > > This patch adds a new macro, parse_addr, that corresponds to a strtoul > or strtoull call based on whether you are operating on a 32-bit ELF or > a 64-bit ELF. This way, the correct width is guaranteed whether or not > the host is 32-bit. This should cleanly apply on all of the 6.x stable > kernels. > > Manually verified that the __mcount_loc section is sorted by parsing the > ELF and verified tests corresponding to CONFIG_FTRACE_SORT_STARTUP_TEST > for kernels built on a 32-bit and a 64-bit host. > > Signed-off-by: Sahil Gupta <s.gupta@arista.com> > --- > scripts/sorttable.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) What is the upstream git commit of this? If it's not upstream, then you need to document the heck out of why we can't take whatever is upstream already, which I don't see here :( thanks, greg k-h
On Tue, 25 Mar 2025 20:23:31 -0400 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Mar 25, 2025 at 05:06:56PM -0700, Sahil Gupta wrote: > > The ftrace __mcount_loc buildtime sort does not work properly when the host is > > 32-bit and the target is 64-bit. sorttable parses the start and stop addresses > > by calling strtoul on the buffer holding the hexadecimal string. Since the > > target is 64-bit but unsigned long on 32-bit machines is 32 bits, strtoul, > > and by extension the start and stop addresses, can max out to 2^32 - 1. > > > > This patch adds a new macro, parse_addr, that corresponds to a strtoul > > or strtoull call based on whether you are operating on a 32-bit ELF or > > a 64-bit ELF. This way, the correct width is guaranteed whether or not > > the host is 32-bit. This should cleanly apply on all of the 6.x stable > > kernels. > > > > Manually verified that the __mcount_loc section is sorted by parsing the > > ELF and verified tests corresponding to CONFIG_FTRACE_SORT_STARTUP_TEST > > for kernels built on a 32-bit and a 64-bit host. > > > > Signed-off-by: Sahil Gupta <s.gupta@arista.com> > > --- > > scripts/sorttable.h | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > What is the upstream git commit of this? > > If it's not upstream, then you need to document the heck out of why we > can't take whatever is upstream already, which I don't see here :( I guess it is loosely based on 4acda8edefa1 ("scripts/sorttable: Get start/stop_mcount_loc from ELF file directly"), which may take a bit of work to backport (or we just add everything that this commit depends on). -- Steve
On Tue, 25 Mar 2025 20:32:36 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > I guess it is loosely based on 4acda8edefa1 ("scripts/sorttable: Get > start/stop_mcount_loc from ELF file directly"), which may take a bit of > work to backport (or we just add everything that this commit depends on). And looking at what was done, it was my rewrite of the sorttable.c code. If it's OK to backport a rewrite, then we could just do that. See commits: 4f48a28b37d5 scripts/sorttable: Remove unused write functions 7ffc0d0819f4 scripts/sorttable: Make compare_extable() into two functions 157fb5b3cfd2 scripts/sorttable: Convert Elf_Ehdr to union 545f6cf8f4c9 scripts/sorttable: Replace Elf_Shdr Macro with a union 200d015e73b4 scripts/sorttable: Convert Elf_Sym MACRO over to a union 1dfb59a228dd scripts/sorttable: Add helper functions for Elf_Ehdr 67afb7f50440 scripts/sorttable: Add helper functions for Elf_Shdr 17bed33ac12f scripts/sorttable: Add helper functions for Elf_Sym 58d87678a0f4 scripts/sorttable: Move code from sorttable.h into sorttable.c -- Steev
On Tue, Mar 25, 2025 at 08:37:23PM -0400, Steven Rostedt wrote: > On Tue, 25 Mar 2025 20:32:36 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > I guess it is loosely based on 4acda8edefa1 ("scripts/sorttable: Get > > start/stop_mcount_loc from ELF file directly"), which may take a bit of > > work to backport (or we just add everything that this commit depends on). > > And looking at what was done, it was my rewrite of the sorttable.c code. > > If it's OK to backport a rewrite, then we could just do that. > > See commits: > > 4f48a28b37d5 scripts/sorttable: Remove unused write functions > 7ffc0d0819f4 scripts/sorttable: Make compare_extable() into two functions > 157fb5b3cfd2 scripts/sorttable: Convert Elf_Ehdr to union > 545f6cf8f4c9 scripts/sorttable: Replace Elf_Shdr Macro with a union > 200d015e73b4 scripts/sorttable: Convert Elf_Sym MACRO over to a union > 1dfb59a228dd scripts/sorttable: Add helper functions for Elf_Ehdr > 67afb7f50440 scripts/sorttable: Add helper functions for Elf_Shdr > 17bed33ac12f scripts/sorttable: Add helper functions for Elf_Sym > 58d87678a0f4 scripts/sorttable: Move code from sorttable.h into sorttable.c Backport away!
On Tue, 25 Mar 2025 20:45:00 -0400 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Mar 25, 2025 at 08:37:23PM -0400, Steven Rostedt wrote: > > On Tue, 25 Mar 2025 20:32:36 -0400 > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > I guess it is loosely based on 4acda8edefa1 ("scripts/sorttable: Get > > > start/stop_mcount_loc from ELF file directly"), which may take a bit of > > > work to backport (or we just add everything that this commit depends on). > > > > And looking at what was done, it was my rewrite of the sorttable.c code. > > > > If it's OK to backport a rewrite, then we could just do that. > > > > See commits: > > > > 4f48a28b37d5 scripts/sorttable: Remove unused write functions > > 7ffc0d0819f4 scripts/sorttable: Make compare_extable() into two functions > > 157fb5b3cfd2 scripts/sorttable: Convert Elf_Ehdr to union > > 545f6cf8f4c9 scripts/sorttable: Replace Elf_Shdr Macro with a union > > 200d015e73b4 scripts/sorttable: Convert Elf_Sym MACRO over to a union > > 1dfb59a228dd scripts/sorttable: Add helper functions for Elf_Ehdr > > 67afb7f50440 scripts/sorttable: Add helper functions for Elf_Shdr > > 17bed33ac12f scripts/sorttable: Add helper functions for Elf_Sym > > 58d87678a0f4 scripts/sorttable: Move code from sorttable.h into sorttable.c > > Backport away! Actually, I only did a git log on scripts/sorttable.c. I left out sorttable.h which gives me this list: $ git log --pretty=oneline --abbrev-commit --reverse v6.12..4acda8edefa1ce66d3de845f1c12745721cd14c3 scripts/sorttable.[ch] 0210d251162f scripts/sorttable: fix orc_sort_cmp() to maintain symmetry and transitivity 28b24394c6e9 scripts/sorttable: Remove unused macro defines 4f48a28b37d5 scripts/sorttable: Remove unused write functions 6f2c2f93a190 scripts/sorttable: Remove unneeded Elf_Rel 66990c003306 scripts/sorttable: Have the ORC code use the _r() functions to read 7ffc0d0819f4 scripts/sorttable: Make compare_extable() into two functions 157fb5b3cfd2 scripts/sorttable: Convert Elf_Ehdr to union 545f6cf8f4c9 scripts/sorttable: Replace Elf_Shdr Macro with a union 200d015e73b4 scripts/sorttable: Convert Elf_Sym MACRO over to a union 1dfb59a228dd scripts/sorttable: Add helper functions for Elf_Ehdr 67afb7f50440 scripts/sorttable: Add helper functions for Elf_Shdr 17bed33ac12f scripts/sorttable: Add helper functions for Elf_Sym 1b649e6ab8dc scripts/sorttable: Use uint64_t for mcount sorting 58d87678a0f4 scripts/sorttable: Move code from sorttable.h into sorttable.c 4acda8edefa1 scripts/sorttable: Get start/stop_mcount_loc from ELF file directly -- Steve
Works for me. At least from our previous discussion, it seemed like backporting would be laborious, which is why I offered this alternative patch. But if we can just backport the series, then there's no reason to not do so. Thanks, Sahil On Tue, Mar 25, 2025 at 7:47 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 25 Mar 2025 20:45:00 -0400 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Tue, Mar 25, 2025 at 08:37:23PM -0400, Steven Rostedt wrote: > > > On Tue, 25 Mar 2025 20:32:36 -0400 > > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > I guess it is loosely based on 4acda8edefa1 ("scripts/sorttable: Get > > > > start/stop_mcount_loc from ELF file directly"), which may take a bit of > > > > work to backport (or we just add everything that this commit depends on). > > > > > > And looking at what was done, it was my rewrite of the sorttable.c code. > > > > > > If it's OK to backport a rewrite, then we could just do that. > > > > > > See commits: > > > > > > 4f48a28b37d5 scripts/sorttable: Remove unused write functions > > > 7ffc0d0819f4 scripts/sorttable: Make compare_extable() into two functions > > > 157fb5b3cfd2 scripts/sorttable: Convert Elf_Ehdr to union > > > 545f6cf8f4c9 scripts/sorttable: Replace Elf_Shdr Macro with a union > > > 200d015e73b4 scripts/sorttable: Convert Elf_Sym MACRO over to a union > > > 1dfb59a228dd scripts/sorttable: Add helper functions for Elf_Ehdr > > > 67afb7f50440 scripts/sorttable: Add helper functions for Elf_Shdr > > > 17bed33ac12f scripts/sorttable: Add helper functions for Elf_Sym > > > 58d87678a0f4 scripts/sorttable: Move code from sorttable.h into sorttable.c > > > > Backport away! > > Actually, I only did a git log on scripts/sorttable.c. I left out > sorttable.h which gives me this list: > > $ git log --pretty=oneline --abbrev-commit --reverse v6.12..4acda8edefa1ce66d3de845f1c12745721cd14c3 scripts/sorttable.[ch] > > 0210d251162f scripts/sorttable: fix orc_sort_cmp() to maintain symmetry and transitivity > 28b24394c6e9 scripts/sorttable: Remove unused macro defines > 4f48a28b37d5 scripts/sorttable: Remove unused write functions > 6f2c2f93a190 scripts/sorttable: Remove unneeded Elf_Rel > 66990c003306 scripts/sorttable: Have the ORC code use the _r() functions to read > 7ffc0d0819f4 scripts/sorttable: Make compare_extable() into two functions > 157fb5b3cfd2 scripts/sorttable: Convert Elf_Ehdr to union > 545f6cf8f4c9 scripts/sorttable: Replace Elf_Shdr Macro with a union > 200d015e73b4 scripts/sorttable: Convert Elf_Sym MACRO over to a union > 1dfb59a228dd scripts/sorttable: Add helper functions for Elf_Ehdr > 67afb7f50440 scripts/sorttable: Add helper functions for Elf_Shdr > 17bed33ac12f scripts/sorttable: Add helper functions for Elf_Sym > 1b649e6ab8dc scripts/sorttable: Use uint64_t for mcount sorting > 58d87678a0f4 scripts/sorttable: Move code from sorttable.h into sorttable.c > 4acda8edefa1 scripts/sorttable: Get start/stop_mcount_loc from ELF file directly > > -- Steve
diff --git a/scripts/sorttable.h b/scripts/sorttable.h index 7bd0184380d3..9ed7acca9f30 100644 --- a/scripts/sorttable.h +++ b/scripts/sorttable.h @@ -40,6 +40,7 @@ #undef uint_t #undef _r #undef _w +#undef parse_addr #ifdef SORTTABLE_64 # define extable_ent_size 16 @@ -65,6 +66,7 @@ # define uint_t uint64_t # define _r r8 # define _w w8 +# define parse_addr(buf) strtoull(buf, NULL, 16) #else # define extable_ent_size 8 # define compare_extable compare_extable_32 @@ -89,6 +91,7 @@ # define uint_t uint32_t # define _r r # define _w w +# define parse_addr(buf) strtoul(buf, NULL, 16) #endif #if defined(SORTTABLE_64) && defined(UNWINDER_ORC_ENABLED) @@ -246,13 +249,13 @@ static void get_mcount_loc(uint_t *_start, uint_t *_stop) len = strlen(start_buff); start_buff[len - 1] = '\0'; } - *_start = strtoul(start_buff, NULL, 16); + *_start = parse_addr(start_buff); while (fgets(stop_buff, sizeof(stop_buff), file_stop) != NULL) { len = strlen(stop_buff); stop_buff[len - 1] = '\0'; } - *_stop = strtoul(stop_buff, NULL, 16); + *_stop = parse_addr(stop_buff); pclose(file_start); pclose(file_stop);
The ftrace __mcount_loc buildtime sort does not work properly when the host is 32-bit and the target is 64-bit. sorttable parses the start and stop addresses by calling strtoul on the buffer holding the hexadecimal string. Since the target is 64-bit but unsigned long on 32-bit machines is 32 bits, strtoul, and by extension the start and stop addresses, can max out to 2^32 - 1. This patch adds a new macro, parse_addr, that corresponds to a strtoul or strtoull call based on whether you are operating on a 32-bit ELF or a 64-bit ELF. This way, the correct width is guaranteed whether or not the host is 32-bit. This should cleanly apply on all of the 6.x stable kernels. Manually verified that the __mcount_loc section is sorted by parsing the ELF and verified tests corresponding to CONFIG_FTRACE_SORT_STARTUP_TEST for kernels built on a 32-bit and a 64-bit host. Signed-off-by: Sahil Gupta <s.gupta@arista.com> --- scripts/sorttable.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)