| Message ID | 20131028132348.62f7e368@tom-ThinkPad-T410 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ming Lei <tom.leiming@gmail.com> writes: > On Mon, 28 Oct 2013 13:44:30 +1030 > Rusty Russell <rusty@rustcorp.com.au> wrote: > >> Ming Lei <tom.leiming@gmail.com> writes: >> >> 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. > > OK, thanks for your comment, and I figured out one way to do it in > scripts/kallsyms.c, could you comment on below patch? Looks great! Seems like we spent more time arguing than it took you to code that up. Acked-by: Rusty Russell <rusty@rustcorp.com.au> Russell, this seems logical for you to take along with the changes which caused the problem? Thanks, Rusty. > -- > From 4327534dedfa60d208ac3e23db7556c243e1c7dc Mon Sep 17 00:00:00 2001 > From: Ming Lei <tom.leiming@gmail.com> > Date: Mon, 28 Oct 2013 13:04:49 +0800 > Subject: [PATCH] scripts/kallsyms: filter symbols not in kernel address space > > This patch uses CONFIG_PAGE_OFFSET to filter symbols which > are not in kernel address space because these symbols are > generally for generating code purpose and can't be run at > kernel mode, so we needn't keep them in /proc/kallsyms. > > For example, on ARM there are some symbols which may be > linked in relocatable code section, then perf can't parse > symbols any more from /proc/kallsyms, this patch fixes the > problem. > > Cc: Russell King <linux@arm.linux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: Michal Marek <mmarek@suse.cz> > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > scripts/kallsyms.c | 12 +++++++++++- > scripts/link-vmlinux.sh | 2 ++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c > index 487ac6f..9a11f9f 100644 > --- a/scripts/kallsyms.c > +++ b/scripts/kallsyms.c > @@ -55,6 +55,7 @@ static struct sym_entry *table; > static unsigned int table_size, table_cnt; > static int all_symbols = 0; > static char symbol_prefix_char = '\0'; > +static unsigned long long kernel_start_addr = 0; > > int token_profit[0x10000]; > > @@ -65,7 +66,10 @@ unsigned char best_table_len[256]; > > static void usage(void) > { > - fprintf(stderr, "Usage: kallsyms [--all-symbols] [--symbol-prefix=<prefix char>] < in.map > out.S\n"); > + fprintf(stderr, "Usage: kallsyms [--all-symbols] " > + "[--symbol-prefix=<prefix char>] " > + "[--page-offset=<CONFIG_PAGE_OFFSET>] " > + "< in.map > out.S\n"); > exit(1); > } > > @@ -194,6 +198,9 @@ static int symbol_valid(struct sym_entry *s) > int i; > int offset = 1; > > + if (s->addr < kernel_start_addr) > + return 0; > + > /* skip prefix char */ > if (symbol_prefix_char && *(s->sym + 1) == symbol_prefix_char) > offset++; > @@ -646,6 +653,9 @@ int main(int argc, char **argv) > if ((*p == '"' && *(p+2) == '"') || (*p == '\'' && *(p+2) == '\'')) > p++; > symbol_prefix_char = *p; > + } else if (strncmp(argv[i], "--page-offset=", 14) == 0) { > + const char *p = &argv[i][14]; > + kernel_start_addr = strtoull(p, NULL, 16); > } else > usage(); > } > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index 0149949..32b10f5 100644 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -82,6 +82,8 @@ kallsyms() > kallsymopt="${kallsymopt} --all-symbols" > fi > > + kallsymopt="${kallsymopt} --page-offset=$CONFIG_PAGE_OFFSET" > + > local aflags="${KBUILD_AFLAGS} ${KBUILD_AFLAGS_KERNEL} \ > ${NOSTDINC_FLAGS} ${LINUXINCLUDE} ${KBUILD_CPPFLAGS}" > > -- > 1.7.9.5 > > > > > Thanks, > -- > Ming Lei
On Mon, Oct 28, 2013 at 04:20:19PM +1030, Rusty Russell wrote: > Ming Lei <tom.leiming@gmail.com> writes: > > On Mon, 28 Oct 2013 13:44:30 +1030 > > Rusty Russell <rusty@rustcorp.com.au> wrote: > > > >> Ming Lei <tom.leiming@gmail.com> writes: > >> > >> 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. > > > > OK, thanks for your comment, and I figured out one way to do it in > > scripts/kallsyms.c, could you comment on below patch? > > Looks great! Seems like we spent more time arguing than it took you to > code that up. > > Acked-by: Rusty Russell <rusty@rustcorp.com.au> > > Russell, this seems logical for you to take along with the changes which > caused the problem? The changes are already in mainline since a long time (back in July/August time). Am I the right person to take stuff for scripts/ ? Isn't that more kbuild territory?
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > On Mon, Oct 28, 2013 at 04:20:19PM +1030, Rusty Russell wrote: >> Ming Lei <tom.leiming@gmail.com> writes: >> > On Mon, 28 Oct 2013 13:44:30 +1030 >> > Rusty Russell <rusty@rustcorp.com.au> wrote: >> > >> >> Ming Lei <tom.leiming@gmail.com> writes: >> >> >> >> 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. >> > >> > OK, thanks for your comment, and I figured out one way to do it in >> > scripts/kallsyms.c, could you comment on below patch? >> >> Looks great! Seems like we spent more time arguing than it took you to >> code that up. >> >> Acked-by: Rusty Russell <rusty@rustcorp.com.au> >> >> Russell, this seems logical for you to take along with the changes which >> caused the problem? > > The changes are already in mainline since a long time (back in July/August > time). Am I the right person to take stuff for scripts/ ? Isn't that > more kbuild territory? Kallsyms tends to fall between modules and scripts. I assume it's not urgent, so no cc:stable on this one. Applied, Rusty.
On Thu, Oct 31, 2013 at 11:14 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: > Russell King - ARM Linux <linux@arm.linux.org.uk> writes: > > Kallsyms tends to fall between modules and scripts. I assume it's not > urgent, so no cc:stable on this one. Rusty, thanks a lot. BTW, there is already other report on the problem: http://lkml.indiana.edu/hypermail/linux/kernel/1310.3/02772.html Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Thu, Oct 31, 2013 at 11:14 AM, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Russell King - ARM Linux <linux@arm.linux.org.uk> writes: >> >> Kallsyms tends to fall between modules and scripts. I assume it's not >> urgent, so no cc:stable on this one. > > Rusty, thanks a lot. > > BTW, there is already other report on the problem: > > http://lkml.indiana.edu/hypermail/linux/kernel/1310.3/02772.html OK, *that* references the commit which is the problem, when went into v3.11 (b9b32bf70f2fb710b07c94e13afbc729afe221da) Unfortunately, *that commit* was cc:stable, so this needs to be cc:stable as well, not just >= 3.11. Looking back on those patches, there's a mass of cc:stable on them. The descriptions are either misleading, or these patches prevent theoretical attacks which means they shouldn't have been cc:stable. Grr... Rusty.
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c index 487ac6f..9a11f9f 100644 --- a/scripts/kallsyms.c +++ b/scripts/kallsyms.c @@ -55,6 +55,7 @@ static struct sym_entry *table; static unsigned int table_size, table_cnt; static int all_symbols = 0; static char symbol_prefix_char = '\0'; +static unsigned long long kernel_start_addr = 0; int token_profit[0x10000]; @@ -65,7 +66,10 @@ unsigned char best_table_len[256]; static void usage(void) { - fprintf(stderr, "Usage: kallsyms [--all-symbols] [--symbol-prefix=<prefix char>] < in.map > out.S\n"); + fprintf(stderr, "Usage: kallsyms [--all-symbols] " + "[--symbol-prefix=<prefix char>] " + "[--page-offset=<CONFIG_PAGE_OFFSET>] " + "< in.map > out.S\n"); exit(1); } @@ -194,6 +198,9 @@ static int symbol_valid(struct sym_entry *s) int i; int offset = 1; + if (s->addr < kernel_start_addr) + return 0; + /* skip prefix char */ if (symbol_prefix_char && *(s->sym + 1) == symbol_prefix_char) offset++; @@ -646,6 +653,9 @@ int main(int argc, char **argv) if ((*p == '"' && *(p+2) == '"') || (*p == '\'' && *(p+2) == '\'')) p++; symbol_prefix_char = *p; + } else if (strncmp(argv[i], "--page-offset=", 14) == 0) { + const char *p = &argv[i][14]; + kernel_start_addr = strtoull(p, NULL, 16); } else usage(); } diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh index 0149949..32b10f5 100644 --- a/scripts/link-vmlinux.sh +++ b/scripts/link-vmlinux.sh @@ -82,6 +82,8 @@ kallsyms() kallsymopt="${kallsymopt} --all-symbols" fi + kallsymopt="${kallsymopt} --page-offset=$CONFIG_PAGE_OFFSET" + local aflags="${KBUILD_AFLAGS} ${KBUILD_AFLAGS_KERNEL} \ ${NOSTDINC_FLAGS} ${LINUXINCLUDE} ${KBUILD_CPPFLAGS}"