| Message ID | 1382498320-26594-1-git-send-email-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ming Lei <tom.leiming@gmail.com> writes: > Address of non-module kernel symbol should always be located > from CONFIG_PAGE_OFFSET on, so only show these legal kernel > symbols in /proc/kallsyms. > > On ARM, some symbols(see below) may drop in relocatable code, so > perf can't parse kernel symbols any more from /proc/kallsyms, this > patch fixes the problem. > > 00000000 t __vectors_start > 00000020 A cpu_v7_suspend_size > 00001000 t __stubs_start > 00001004 t vector_rst > 00001020 t vector_irq > 000010a0 t vector_dabt > 00001120 t vector_pabt > 000011a0 t vector_und > 00001220 t vector_addrexcptn > 00001224 t vector_fiq > 00001224 T vector_fiq_offset > > The issue can be fixed in scripts/kallsyms.c too, but looks this > approach is easier. This fix looks hacky; if these symbols are not available, don't just remove them from /proc/kallsyms, but don't put them in the kernel at all. That way, they won't interfere with other kallsyms uses (eg. backtrace). Or, better, figure out a smart way of excluding them by knowing why these symbol addresses are wrong. Thanks, Rusty. > Cc: Russell King <linux@arm.linux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: Chen Gang <gang.chen@asianux.com> > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > kernel/kallsyms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 3127ad5..184f271 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -543,7 +543,7 @@ static int s_show(struct seq_file *m, void *p) > tolower(iter->type); > seq_printf(m, "%pK %c %s\t[%s]\n", (void *)iter->value, > type, iter->name, iter->module_name); > - } else > + } else if (iter->value >= CONFIG_PAGE_OFFSET) > seq_printf(m, "%pK %c %s\n", (void *)iter->value, > iter->type, iter->name); > return 0; > -- > 1.7.9.5
On Thu, Oct 24, 2013 at 9:21 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: > Ming Lei <tom.leiming@gmail.com> writes: >> Address of non-module kernel symbol should always be located >> from CONFIG_PAGE_OFFSET on, so only show these legal kernel >> symbols in /proc/kallsyms. >> >> On ARM, some symbols(see below) may drop in relocatable code, so >> perf can't parse kernel symbols any more from /proc/kallsyms, this >> patch fixes the problem. >> >> 00000000 t __vectors_start >> 00000020 A cpu_v7_suspend_size >> 00001000 t __stubs_start >> 00001004 t vector_rst >> 00001020 t vector_irq >> 000010a0 t vector_dabt >> 00001120 t vector_pabt >> 000011a0 t vector_und >> 00001220 t vector_addrexcptn >> 00001224 t vector_fiq >> 00001224 T vector_fiq_offset >> >> The issue can be fixed in scripts/kallsyms.c too, but looks this >> approach is easier. > > This fix looks hacky; if these symbols are not available, don't just > remove them from /proc/kallsyms, but don't put them in the kernel at > all. > > That way, they won't interfere with other kallsyms uses (eg. backtrace). Yes, I agree. > Or, better, figure out a smart way of excluding them by knowing why > these symbol addresses are wrong. Actually the problem is caused by commit b9b32bf70f2(ARM: use linker magic for vectors and vector stubs), maybe Russell can figure out a smart way to exclude these symbols. Thanks,
On Thu, Oct 24, 2013 at 11:51:18AM +1030, Rusty Russell wrote: > Ming Lei <tom.leiming@gmail.com> writes: > > Address of non-module kernel symbol should always be located > > from CONFIG_PAGE_OFFSET on, so only show these legal kernel > > symbols in /proc/kallsyms. > > > > On ARM, some symbols(see below) may drop in relocatable code, so > > perf can't parse kernel symbols any more from /proc/kallsyms, this > > patch fixes the problem. > > > > 00000000 t __vectors_start > > 00000020 A cpu_v7_suspend_size > > 00001000 t __stubs_start > > 00001004 t vector_rst > > 00001020 t vector_irq > > 000010a0 t vector_dabt > > 00001120 t vector_pabt > > 000011a0 t vector_und > > 00001220 t vector_addrexcptn > > 00001224 t vector_fiq > > 00001224 T vector_fiq_offset > > > > The issue can be fixed in scripts/kallsyms.c too, but looks this > > approach is easier. > > This fix looks hacky; if these symbols are not available, don't just > remove them from /proc/kallsyms, but don't put them in the kernel at > all. How do you "don't put them in the kernel at all" when they're used by the kernel internally as offsets? If you mean, just get rid of them, shall I just add these as magic numbers instead based on the values in this email? Is that really a sane solution? No, we have to keep these symbols IMHO.
On Thu, Oct 24, 2013 at 4:45 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Oct 24, 2013 at 11:51:18AM +1030, Rusty Russell wrote: >> Ming Lei <tom.leiming@gmail.com> writes: >> > Address of non-module kernel symbol should always be located >> > from CONFIG_PAGE_OFFSET on, so only show these legal kernel >> > symbols in /proc/kallsyms. >> > >> > On ARM, some symbols(see below) may drop in relocatable code, so >> > perf can't parse kernel symbols any more from /proc/kallsyms, this >> > patch fixes the problem. >> > >> > 00000000 t __vectors_start >> > 00000020 A cpu_v7_suspend_size >> > 00001000 t __stubs_start >> > 00001004 t vector_rst >> > 00001020 t vector_irq >> > 000010a0 t vector_dabt >> > 00001120 t vector_pabt >> > 000011a0 t vector_und >> > 00001220 t vector_addrexcptn >> > 00001224 t vector_fiq >> > 00001224 T vector_fiq_offset >> > >> > The issue can be fixed in scripts/kallsyms.c too, but looks this >> > approach is easier. >> >> This fix looks hacky; if these symbols are not available, don't just >> remove them from /proc/kallsyms, but don't put them in the kernel at >> all. > > How do you "don't put them in the kernel at all" when they're used by > the kernel internally as offsets? > > If you mean, just get rid of them, shall I just add these as magic > numbers instead based on the values in this email? Is that really a > sane solution? > > No, we have to keep these symbols IMHO. If so, looks we have to hide them to userspace, so the patch should be OK because the approach is correct and no obvious side-effect. Thanks,
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Thu, Oct 24, 2013 at 11:51:18AM +1030, Rusty Russell wrote: >> Ming Lei <tom.leiming@gmail.com> writes: >> > Address of non-module kernel symbol should always be located >> > from CONFIG_PAGE_OFFSET on, so only show these legal kernel >> > symbols in /proc/kallsyms. >> > >> > On ARM, some symbols(see below) may drop in relocatable code, so >> > perf can't parse kernel symbols any more from /proc/kallsyms, this >> > patch fixes the problem. >> > >> > 00000000 t __vectors_start >> > 00000020 A cpu_v7_suspend_size >> > 00001000 t __stubs_start >> > 00001004 t vector_rst >> > 00001020 t vector_irq >> > 000010a0 t vector_dabt >> > 00001120 t vector_pabt >> > 000011a0 t vector_und >> > 00001220 t vector_addrexcptn >> > 00001224 t vector_fiq >> > 00001224 T vector_fiq_offset >> > >> > The issue can be fixed in scripts/kallsyms.c too, but looks this >> > approach is easier. >> >> This fix looks hacky; if these symbols are not available, don't just >> remove them from /proc/kallsyms, but don't put them in the kernel at >> all. > > How do you "don't put them in the kernel at all" when they're used by > the kernel internally as offsets? Sorry, I was imprecise. I was referring to the kernel's kallsyms tables produced by scripts/kallsyms.c. This patch left them in the the kallsyms tables and filtered them out from /proc/kallsyms. It's weird that cpu_v7_suspend_size appeared above, since kallsyms should filter out 'A' symbols already. > If you mean, just get rid of them, shall I just add these as magic > numbers instead based on the values in this email? Is that really a > sane solution? > > No, we have to keep these symbols IMHO. Can you make them absolute symbols? That should Just Work for kallsyms. Cheers, Rusty.
On Fri, Oct 25, 2013 at 7:08 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: > > Sorry, I was imprecise. I was referring to the kernel's kallsyms > tables produced by scripts/kallsyms.c. This patch left them in the > the kallsyms tables and filtered them out from /proc/kallsyms. Yes, but it isn't easy to do it by script/kallsyms.c , and IMO, it should be correct to hide them for user space but keep them in kallsyms table. > > It's weird that cpu_v7_suspend_size appeared above, since kallsyms > should filter out 'A' symbols already. Sorry, the 'A' symbol is a mistake, but the others do exist in /proc/kallsyms. Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Fri, Oct 25, 2013 at 7:08 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> Sorry, I was imprecise. I was referring to the kernel's kallsyms >> tables produced by scripts/kallsyms.c. This patch left them in the >> the kallsyms tables and filtered them out from /proc/kallsyms. > > Yes, but it isn't easy to do it by script/kallsyms.c , and IMO, it should > be correct to hide them for user space but keep them in kallsyms table. So they'll appear in backtraces? And turn up randomly for other symbol dereferences? I don't think you really want this! >> It's weird that cpu_v7_suspend_size appeared above, since kallsyms >> should filter out 'A' symbols already. > > Sorry, the 'A' symbol is a mistake, but the others do exist in /proc/kallsyms. Ah, OK. Cheers, Rusty.
On Fri, Oct 25, 2013 at 1:50 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: > Ming Lei <tom.leiming@gmail.com> writes: >> On Fri, Oct 25, 2013 at 7:08 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> >>> Sorry, I was imprecise. I was referring to the kernel's kallsyms >>> tables produced by scripts/kallsyms.c. This patch left them in the >>> the kallsyms tables and filtered them out from /proc/kallsyms. >> >> Yes, but it isn't easy to do it by script/kallsyms.c , and IMO, it should >> be correct to hide them for user space but keep them in kallsyms table. > > So they'll appear in backtraces? And turn up randomly for other symbol > dereferences? > > I don't think you really want this! Basically these symbols are only used to generate code, and in kernel mode, CPU won't run into the corresponding addresses because the generate code is copied to other address during booting, so I understand they won't appear in backtraces. Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Fri, Oct 25, 2013 at 1:50 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Ming Lei <tom.leiming@gmail.com> writes: >>> On Fri, Oct 25, 2013 at 7:08 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>> >>>> Sorry, I was imprecise. I was referring to the kernel's kallsyms >>>> tables produced by scripts/kallsyms.c. This patch left them in the >>>> the kallsyms tables and filtered them out from /proc/kallsyms. >>> >>> Yes, but it isn't easy to do it by script/kallsyms.c , and IMO, it should >>> be correct to hide them for user space but keep them in kallsyms table. >> >> So they'll appear in backtraces? And turn up randomly for other symbol >> dereferences? >> >> I don't think you really want this! > > Basically these symbols are only used to generate code, and in > kernel mode, CPU won't run into the corresponding addresses > because the generate code is copied to other address during booting, > so I understand they won't appear in backtraces. An oops occurs when something went *wrong*. We look up all kinds of stuff. Are you so sure that *none* of the callers will ever see these strange symbols and produce a confusing result? Cheers, Rusty.
On Fri, Oct 25, 2013 at 7:58 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> Basically these symbols are only used to generate code, and in >> kernel mode, CPU won't run into the corresponding addresses >> because the generate code is copied to other address during booting, >> so I understand they won't appear in backtraces. > > An oops occurs when something went *wrong*. We look up all kinds of > stuff. Are you so sure that *none* of the callers will ever see these > strange symbols and produce a confusing result? Suppose that might happen, kernel should be smart enough to know that the address is not inside kernel address space and won't produce confusing result, right? Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Fri, Oct 25, 2013 at 7:58 PM, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> >>> Basically these symbols are only used to generate code, and in >>> kernel mode, CPU won't run into the corresponding addresses >>> because the generate code is copied to other address during booting, >>> so I understand they won't appear in backtraces. >> >> An oops occurs when something went *wrong*. We look up all kinds of >> stuff. Are you so sure that *none* of the callers will ever see these >> strange symbols and produce a confusing result? > > Suppose that might happen, kernel should be smart enough to know > that the address is not inside kernel address space and won't produce > confusing result, right? I don't know... It would be your job, as the person making the change, to find all the users of kallsyms and prove that. This is why it is easier not to include incorrect values in the kernel's kallsyms in the first place. Hope that helps, Rusty.
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 3127ad5..184f271 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -543,7 +543,7 @@ static int s_show(struct seq_file *m, void *p) tolower(iter->type); seq_printf(m, "%pK %c %s\t[%s]\n", (void *)iter->value, type, iter->name, iter->module_name); - } else + } else if (iter->value >= CONFIG_PAGE_OFFSET) seq_printf(m, "%pK %c %s\n", (void *)iter->value, iter->type, iter->name); return 0;
Address of non-module kernel symbol should always be located from CONFIG_PAGE_OFFSET on, so only show these legal kernel symbols in /proc/kallsyms. On ARM, some symbols(see below) may drop in relocatable code, so perf can't parse kernel symbols any more from /proc/kallsyms, this patch fixes the problem. 00000000 t __vectors_start 00000020 A cpu_v7_suspend_size 00001000 t __stubs_start 00001004 t vector_rst 00001020 t vector_irq 000010a0 t vector_dabt 00001120 t vector_pabt 000011a0 t vector_und 00001220 t vector_addrexcptn 00001224 t vector_fiq 00001224 T vector_fiq_offset The issue can be fixed in scripts/kallsyms.c too, but looks this approach is easier. Cc: Russell King <linux@arm.linux.org.uk> Cc: linux-arm-kernel@lists.infradead.org Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Chen Gang <gang.chen@asianux.com> Signed-off-by: Ming Lei <tom.leiming@gmail.com> --- kernel/kallsyms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)