| Message ID | 20230517131820.936553-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kallsyms: remove unused arch_get_kallsym() helper | expand |
On Wed, May 17, 2023 at 03:18:07PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The arch_get_kallsym() function was introduced so that x86 could override > it, but that override was removed in bf904d2762ee ("x86/pti/64: Remove > the SYSCALL64 entry trampoline"), so now this does nothing except causing > a warning about a missing prototype: > > kernel/kallsyms.c:662:12: error: no previous prototype for 'arch_get_kallsym' [-Werror=missing-prototypes] > 662 | int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, > > Restore the old behavior before d83212d5dd67 ("kallsyms, x86: Export > addresses of PTI entry trampolines") to simplify the code and avoid > the warning. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Shouldn't this go through x86 as this sort of fixesss commit bf904d2762ee ("x86/pti/64: Remove the SYSCALL64 entry trampoline")? Luis > --- > kernel/kallsyms.c | 28 +--------------------------- > 1 file changed, 1 insertion(+), 27 deletions(-) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index e01c435595f9..dac13659601f 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -646,7 +646,6 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address) > /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ > struct kallsym_iter { > loff_t pos; > - loff_t pos_arch_end; > loff_t pos_mod_end; > loff_t pos_ftrace_mod_end; > loff_t pos_bpf_end; > @@ -659,29 +658,9 @@ struct kallsym_iter { > int show_value; > }; > > -int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, > - char *type, char *name) > -{ > - return -EINVAL; > -} > - > -static int get_ksymbol_arch(struct kallsym_iter *iter) > -{ > - int ret = arch_get_kallsym(iter->pos - kallsyms_num_syms, > - &iter->value, &iter->type, > - iter->name); > - > - if (ret < 0) { > - iter->pos_arch_end = iter->pos; > - return 0; > - } > - > - return 1; > -} > - > static int get_ksymbol_mod(struct kallsym_iter *iter) > { > - int ret = module_get_kallsym(iter->pos - iter->pos_arch_end, > + int ret = module_get_kallsym(iter->pos - kallsyms_num_syms, > &iter->value, &iter->type, > iter->name, iter->module_name, > &iter->exported); > @@ -764,7 +743,6 @@ static void reset_iter(struct kallsym_iter *iter, loff_t new_pos) > iter->nameoff = get_symbol_offset(new_pos); > iter->pos = new_pos; > if (new_pos == 0) { > - iter->pos_arch_end = 0; > iter->pos_mod_end = 0; > iter->pos_ftrace_mod_end = 0; > iter->pos_bpf_end = 0; > @@ -780,10 +758,6 @@ static int update_iter_mod(struct kallsym_iter *iter, loff_t pos) > { > iter->pos = pos; > > - if ((!iter->pos_arch_end || iter->pos_arch_end > pos) && > - get_ksymbol_arch(iter)) > - return 1; > - > if ((!iter->pos_mod_end || iter->pos_mod_end > pos) && > get_ksymbol_mod(iter)) > return 1; > -- > 2.39.2 >
On Wed, May 24, 2023, at 07:07, Luis Chamberlain wrote: > On Wed, May 17, 2023 at 03:18:07PM +0200, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> The arch_get_kallsym() function was introduced so that x86 could override >> it, but that override was removed in bf904d2762ee ("x86/pti/64: Remove >> the SYSCALL64 entry trampoline"), so now this does nothing except causing >> a warning about a missing prototype: >> >> kernel/kallsyms.c:662:12: error: no previous prototype for 'arch_get_kallsym' [-Werror=missing-prototypes] >> 662 | int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, >> >> Restore the old behavior before d83212d5dd67 ("kallsyms, x86: Export >> addresses of PTI entry trampolines") to simplify the code and avoid >> the warning. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Shouldn't this go through x86 as this sort of fixesss commit > bf904d2762ee ("x86/pti/64: Remove the SYSCALL64 entry trampoline")? That works for me as well, as long as someone picks it up. It's not really x86 any more though since that commit is five years old and removed the last reference from the x86 code. I sent it to you since you are the one that merged most of the kallsyms patches through the module tree, but I guess you are not actually maintaining that file (not blaming you, I'd also try to stay away from kallsyms). I can resend it to Andrew for the -mm tree. Arnd
On Wed, May 24, 2023 at 08:25:13AM +0200, Arnd Bergmann wrote: > On Wed, May 24, 2023, at 07:07, Luis Chamberlain wrote: > > On Wed, May 17, 2023 at 03:18:07PM +0200, Arnd Bergmann wrote: > >> From: Arnd Bergmann <arnd@arndb.de> > >> > >> The arch_get_kallsym() function was introduced so that x86 could override > >> it, but that override was removed in bf904d2762ee ("x86/pti/64: Remove > >> the SYSCALL64 entry trampoline"), so now this does nothing except causing > >> a warning about a missing prototype: > >> > >> kernel/kallsyms.c:662:12: error: no previous prototype for 'arch_get_kallsym' [-Werror=missing-prototypes] > >> 662 | int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, > >> > >> Restore the old behavior before d83212d5dd67 ("kallsyms, x86: Export > >> addresses of PTI entry trampolines") to simplify the code and avoid > >> the warning. > >> > >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > Shouldn't this go through x86 as this sort of fixesss commit > > bf904d2762ee ("x86/pti/64: Remove the SYSCALL64 entry trampoline")? > > That works for me as well, as long as someone picks it up. It's > not really x86 any more though since that commit is five years > old and removed the last reference from the x86 code. Fair enough. > I sent it to you since you are the one that merged most of > the kallsyms patches through the module tree, but I guess > you are not actually maintaining that file (not blaming you, > I'd also try to stay away from kallsyms). > > I can resend it to Andrew for the -mm tree. OK, I just took the patch in, it's on the train, better get on before it gets lost. Luis
+ Alan Maguire On Wed, May 24, 2023 at 12:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Wed, May 24, 2023 at 08:25:13AM +0200, Arnd Bergmann wrote: > > On Wed, May 24, 2023, at 07:07, Luis Chamberlain wrote: > > > On Wed, May 17, 2023 at 03:18:07PM +0200, Arnd Bergmann wrote: > > >> From: Arnd Bergmann <arnd@arndb.de> > > >> > > >> The arch_get_kallsym() function was introduced so that x86 could override > > >> it, but that override was removed in bf904d2762ee ("x86/pti/64: Remove > > >> the SYSCALL64 entry trampoline"), so now this does nothing except causing > > >> a warning about a missing prototype: > > >> > > >> kernel/kallsyms.c:662:12: error: no previous prototype for 'arch_get_kallsym' [-Werror=missing-prototypes] > > >> 662 | int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, > > >> > > >> Restore the old behavior before d83212d5dd67 ("kallsyms, x86: Export > > >> addresses of PTI entry trampolines") to simplify the code and avoid > > >> the warning. > > >> > > >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > > > Shouldn't this go through x86 as this sort of fixesss commit > > > bf904d2762ee ("x86/pti/64: Remove the SYSCALL64 entry trampoline")? > > > > That works for me as well, as long as someone picks it up. It's > > not really x86 any more though since that commit is five years > > old and removed the last reference from the x86 code. > > Fair enough. > > > I sent it to you since you are the one that merged most of > > the kallsyms patches through the module tree, but I guess > > you are not actually maintaining that file (not blaming you, > > I'd also try to stay away from kallsyms). > > > > I can resend it to Andrew for the -mm tree. > > OK, I just took the patch in, it's on the train, better get on before > it gets lost. This change broke compilation of BPF selftests in modules-next branch: progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in 'struct kallsym_iter' if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) ~~~~ ^ progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in 'struct kallsym_iter' if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) ~~~~ ^ I haven't looked into the proper fix for it yet. Thanks, Song
On Thu, May 25, 2023 at 06:45:35PM -0700, Song Liu wrote: > + Alan Maguire > > On Wed, May 24, 2023 at 12:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Wed, May 24, 2023 at 08:25:13AM +0200, Arnd Bergmann wrote: > > > On Wed, May 24, 2023, at 07:07, Luis Chamberlain wrote: > > > > On Wed, May 17, 2023 at 03:18:07PM +0200, Arnd Bergmann wrote: > > > >> From: Arnd Bergmann <arnd@arndb.de> > > > >> > > > >> The arch_get_kallsym() function was introduced so that x86 could override > > > >> it, but that override was removed in bf904d2762ee ("x86/pti/64: Remove > > > >> the SYSCALL64 entry trampoline"), so now this does nothing except causing > > > >> a warning about a missing prototype: > > > >> > > > >> kernel/kallsyms.c:662:12: error: no previous prototype for 'arch_get_kallsym' [-Werror=missing-prototypes] > > > >> 662 | int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, > > > >> > > > >> Restore the old behavior before d83212d5dd67 ("kallsyms, x86: Export > > > >> addresses of PTI entry trampolines") to simplify the code and avoid > > > >> the warning. > > > >> > > > >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > > > > > Shouldn't this go through x86 as this sort of fixesss commit > > > > bf904d2762ee ("x86/pti/64: Remove the SYSCALL64 entry trampoline")? > > > > > > That works for me as well, as long as someone picks it up. It's > > > not really x86 any more though since that commit is five years > > > old and removed the last reference from the x86 code. > > > > Fair enough. > > > > > I sent it to you since you are the one that merged most of > > > the kallsyms patches through the module tree, but I guess > > > you are not actually maintaining that file (not blaming you, > > > I'd also try to stay away from kallsyms). > > > > > > I can resend it to Andrew for the -mm tree. > > > > OK, I just took the patch in, it's on the train, better get on before > > it gets lost. > > This change broke compilation of BPF selftests in modules-next > branch: > > progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in > 'struct kallsym_iter' > if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > ~~~~ ^ > progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in > 'struct kallsym_iter' > if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > ~~~~ ^ > > I haven't looked into the proper fix for it yet. A quick attempt: Arnd, can you verify? diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c index 5ddcc46fd886..521267818f4d 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c @@ -59,9 +59,7 @@ int dump_ksym(struct bpf_iter__ksym *ctx) } else { BPF_SEQ_PRINTF(seq, "0x%llx %c %s ", value, type, iter->name); } - if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) - BPF_SEQ_PRINTF(seq, "CORE "); - else if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) + if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) BPF_SEQ_PRINTF(seq, "MOD "); else if (!iter->pos_ftrace_mod_end || iter->pos_ftrace_mod_end > iter->pos) BPF_SEQ_PRINTF(seq, "FTRACE_MOD ");
On Fri, May 26, 2023, at 05:24, Luis Chamberlain wrote: > On Thu, May 25, 2023 at 06:45:35PM -0700, Song Liu wrote: >> On Wed, May 24, 2023 at 12:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: >> >> This change broke compilation of BPF selftests in modules-next >> branch: >> >> progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in >> 'struct kallsym_iter' >> if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) >> ~~~~ ^ >> progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in >> 'struct kallsym_iter' >> if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) >> ~~~~ ^ >> >> I haven't looked into the proper fix for it yet. > > A quick attempt: > > Arnd, can you verify? > > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > index 5ddcc46fd886..521267818f4d 100644 > --- a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > @@ -59,9 +59,7 @@ int dump_ksym(struct bpf_iter__ksym *ctx) > } else { > BPF_SEQ_PRINTF(seq, "0x%llx %c %s ", value, type, iter->name); > } > - if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > - BPF_SEQ_PRINTF(seq, "CORE "); > - else if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) > + if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) > BPF_SEQ_PRINTF(seq, "MOD "); > else if (!iter->pos_ftrace_mod_end || iter->pos_ftrace_mod_end > > iter->pos) > BPF_SEQ_PRINTF(seq, "FTRACE_MOD "); This looks correct to me, but I'm still failing to cross-build the selftests on my randconfig build setup, so I can't confirm that this avoids the build failure, and I don't understand the code well enough to be sure. Arnd
On 26/05/2023 07:41, Arnd Bergmann wrote: > On Fri, May 26, 2023, at 05:24, Luis Chamberlain wrote: >> On Thu, May 25, 2023 at 06:45:35PM -0700, Song Liu wrote: >>> On Wed, May 24, 2023 at 12:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: >>> >>> This change broke compilation of BPF selftests in modules-next >>> branch: >>> >>> progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in >>> 'struct kallsym_iter' >>> if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) >>> ~~~~ ^ >>> progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in >>> 'struct kallsym_iter' >>> if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) >>> ~~~~ ^ >>> >>> I haven't looked into the proper fix for it yet. >> >> A quick attempt: >> >> Arnd, can you verify? >> >> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c >> b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c >> index 5ddcc46fd886..521267818f4d 100644 >> --- a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c >> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c >> @@ -59,9 +59,7 @@ int dump_ksym(struct bpf_iter__ksym *ctx) >> } else { >> BPF_SEQ_PRINTF(seq, "0x%llx %c %s ", value, type, iter->name); >> } >> - if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) >> - BPF_SEQ_PRINTF(seq, "CORE "); >> - else if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) >> + if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) >> BPF_SEQ_PRINTF(seq, "MOD "); >> else if (!iter->pos_ftrace_mod_end || iter->pos_ftrace_mod_end > >> iter->pos) >> BPF_SEQ_PRINTF(seq, "FTRACE_MOD "); > > This looks correct to me, but I'm still failing to cross-build > the selftests on my randconfig build setup, so I can't confirm that > this avoids the build failure, and I don't understand the code well > enough to be sure. > Thanks for the fix! The change above works ; maybe having anything less than iter->pos_mod_end marked as a "CORE/MOD " symbol might be worth tweaking, but that's a minor thing. before: CLNG-BPF [test_maps] bpf_iter_ksym.bpf.o progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in 'struct kallsym_iter' if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) ~~~~ ^ progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in 'struct kallsym_iter' if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) ~~~~ ^ 2 errors generated. after the above is applied, bpf selftests build and iter ksym test passes: $ sudo ./test_progs -t bpf_iter ... #12/37 bpf_iter/ksym:OK ... Summary: 3/39 PASSED, 0 SKIPPED, 0 FAILED Feel free to add a Tested-by: Alan Maguire <alan.maguire@oracle.com> ...if needed. Thanks! Alan
On Fri, May 26, 2023 at 03:39:22PM +0100, Alan Maguire wrote: > On 26/05/2023 07:41, Arnd Bergmann wrote: > > On Fri, May 26, 2023, at 05:24, Luis Chamberlain wrote: > >> On Thu, May 25, 2023 at 06:45:35PM -0700, Song Liu wrote: > >>> On Wed, May 24, 2023 at 12:24 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > >>> > >>> This change broke compilation of BPF selftests in modules-next > >>> branch: > >>> > >>> progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in > >>> 'struct kallsym_iter' > >>> if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > >>> ~~~~ ^ > >>> progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in > >>> 'struct kallsym_iter' > >>> if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > >>> ~~~~ ^ > >>> > >>> I haven't looked into the proper fix for it yet. > >> > >> A quick attempt: > >> > >> Arnd, can you verify? > >> > >> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > >> b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > >> index 5ddcc46fd886..521267818f4d 100644 > >> --- a/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > >> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_ksym.c > >> @@ -59,9 +59,7 @@ int dump_ksym(struct bpf_iter__ksym *ctx) > >> } else { > >> BPF_SEQ_PRINTF(seq, "0x%llx %c %s ", value, type, iter->name); > >> } > >> - if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > >> - BPF_SEQ_PRINTF(seq, "CORE "); > >> - else if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) > >> + if (!iter->pos_mod_end || iter->pos_mod_end > iter->pos) > >> BPF_SEQ_PRINTF(seq, "MOD "); > >> else if (!iter->pos_ftrace_mod_end || iter->pos_ftrace_mod_end > > >> iter->pos) > >> BPF_SEQ_PRINTF(seq, "FTRACE_MOD "); > > > > This looks correct to me, but I'm still failing to cross-build > > the selftests on my randconfig build setup, so I can't confirm that > > this avoids the build failure, and I don't understand the code well > > enough to be sure. > > > > Thanks for the fix! The change above works ; maybe having > anything less than iter->pos_mod_end marked as a "CORE/MOD " symbol > might be worth tweaking, but that's a minor thing. > > before: > > CLNG-BPF [test_maps] bpf_iter_ksym.bpf.o > progs/bpf_iter_ksym.c:62:13: error: no member named 'pos_arch_end' in > 'struct kallsym_iter' > if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > ~~~~ ^ > progs/bpf_iter_ksym.c:62:35: error: no member named 'pos_arch_end' in > 'struct kallsym_iter' > if (!iter->pos_arch_end || iter->pos_arch_end > iter->pos) > ~~~~ ^ > 2 errors generated. > > after the above is applied, bpf selftests build and iter ksym test > passes: > > $ sudo ./test_progs -t bpf_iter > ... > #12/37 bpf_iter/ksym:OK > ... > Summary: 3/39 PASSED, 0 SKIPPED, 0 FAILED > > Feel free to add a > > Tested-by: Alan Maguire <alan.maguire@oracle.com> I just folded this into Arnd's patch and pushed to modules-next. Thanks! Luis
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index e01c435595f9..dac13659601f 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -646,7 +646,6 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address) /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ struct kallsym_iter { loff_t pos; - loff_t pos_arch_end; loff_t pos_mod_end; loff_t pos_ftrace_mod_end; loff_t pos_bpf_end; @@ -659,29 +658,9 @@ struct kallsym_iter { int show_value; }; -int __weak arch_get_kallsym(unsigned int symnum, unsigned long *value, - char *type, char *name) -{ - return -EINVAL; -} - -static int get_ksymbol_arch(struct kallsym_iter *iter) -{ - int ret = arch_get_kallsym(iter->pos - kallsyms_num_syms, - &iter->value, &iter->type, - iter->name); - - if (ret < 0) { - iter->pos_arch_end = iter->pos; - return 0; - } - - return 1; -} - static int get_ksymbol_mod(struct kallsym_iter *iter) { - int ret = module_get_kallsym(iter->pos - iter->pos_arch_end, + int ret = module_get_kallsym(iter->pos - kallsyms_num_syms, &iter->value, &iter->type, iter->name, iter->module_name, &iter->exported); @@ -764,7 +743,6 @@ static void reset_iter(struct kallsym_iter *iter, loff_t new_pos) iter->nameoff = get_symbol_offset(new_pos); iter->pos = new_pos; if (new_pos == 0) { - iter->pos_arch_end = 0; iter->pos_mod_end = 0; iter->pos_ftrace_mod_end = 0; iter->pos_bpf_end = 0; @@ -780,10 +758,6 @@ static int update_iter_mod(struct kallsym_iter *iter, loff_t pos) { iter->pos = pos; - if ((!iter->pos_arch_end || iter->pos_arch_end > pos) && - get_ksymbol_arch(iter)) - return 1; - if ((!iter->pos_mod_end || iter->pos_mod_end > pos) && get_ksymbol_mod(iter)) return 1;