linux-kernel - Re: [PATCH] s390/perf: fix 'start' address of module's map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160726202926.GB5200@kernel.org>
Date:	Tue, 26 Jul 2016 17:29:26 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Christian Borntraeger <borntraeger@...ibm.com>
Cc:	Song Shan Gong <gongss@...ux.vnet.ibm.com>, jolsa@...nel.org,
	dsahern@...il.com, linux-kernel@...r.kernel.org,
	Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH] s390/perf: fix 'start' address of module's map

Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu:
> On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu:
> >> At preset, when creating module's map, perf gets 'start' address by parsing
> >> '/proc/modules', but it's module base address, isn't the start address of
> >> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and
> >> 'PLT' relocations before '.text' section. So there exists an offset between
> >> module base address and '.text' section, which will incur wrong symbol
> >> resolution for modules.
> > 
> > I'll apply this as it fixes the problem for you and we need to get fixes
> > in ASAP to get this into 4.8, but why can't we just use your method for
> > all arches and get rid of this arch__ hook? I.e. if I look here in my
> > x86_64 notebook I see:
> > 
> > [acme@...et linux]$ cat /sys/module/tun/sections/.text 
> > 0xffffffffc0af2000
> > [acme@...et linux]$ grep tun /proc/modules 
> > tun 28672 4 vhost_net, Live 0xffffffffc0af2000
> > [acme@...et linux]$ 
> > 
> > So I could as well use what is in /sys/module/tun/sections/.text instead
> > of reading it from /proc/modules and, in s390, reading it from
> > /sys/module/tun/sections/.text.
> > 
> > Do you see any problem with using this approach for _all_ arches?
> 
> I think it should work well for _all_ arches but it will probably be
> hard to test this without help. 

Well, we could check for the cases we don't know, i.e. read from both
and warn about cases where it is different, except for s390 where we now
which is the right one to pick.

> I wouldn't be surprised if other architectures than s390 actually have
> the same issue, so doing this for everybody might atually fix this somewhere
> else.

Would be nice to get info from other arch people, Michael, how this goes
on ppc?

- Arnaldo
 
> > 
> > - Arnaldo
> > 
> >> Fix this bug by getting 'start' address of module's map from parsing
> >> '/sys/module/[module name]/sections/.text', not from '/proc/modules'.
> >>
> >> Signed-off-by: Song Shan Gong <gongss@...ux.vnet.ibm.com>
> >> Acked-by: Jiri Olsa <jolsa@...nel.org>
> >> ---
> >>  tools/perf/arch/s390/util/Build     |  2 ++
> >>  tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++
> >>  tools/perf/util/machine.c           |  8 ++++++++
> >>  tools/perf/util/machine.h           |  1 +
> >>  4 files changed, 30 insertions(+)
> >>  create mode 100644 tools/perf/arch/s390/util/machine.c
> >>
> >> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build
> >> index 8a61372..5bd7b92 100644
> >> --- a/tools/perf/arch/s390/util/Build
> >> +++ b/tools/perf/arch/s390/util/Build
> >> @@ -2,3 +2,5 @@ libperf-y += header.o
> >>  libperf-y += kvm-stat.o
> >>  
> >>  libperf-$(CONFIG_DWARF) += dwarf-regs.o
> >> +
> >> +libperf-y += machine.o
> >> diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c
> >> new file mode 100644
> >> index 0000000..b9a95a1
> >> --- /dev/null
> >> +++ b/tools/perf/arch/s390/util/machine.c
> >> @@ -0,0 +1,19 @@
> >> +#include <unistd.h>
> >> +#include <stdio.h>
> >> +#include <string.h>
> >> +#include "util.h"
> >> +#include "machine.h"
> >> +#include "api/fs/fs.h"
> >> +
> >> +int arch__fix_module_text_start(u64 *start, const char *name)
> >> +{
> >> +	char path[PATH_MAX];
> >> +
> >> +	snprintf(path, PATH_MAX, "module/%.*s/sections/.text",
> >> +				(int)strlen(name) - 2, name + 1);
> >> +
> >> +	if (sysfs__read_ull(path, (unsigned long long *)start) < 0)
> >> +		return -1;
> >> +
> >> +	return 0;
> >> +}
> >> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> >> index b177218..97cc9f0 100644
> >> --- a/tools/perf/util/machine.c
> >> +++ b/tools/perf/util/machine.c
> >> @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine)
> >>  
> >>  	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> >>  }
> >> +int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
> >> +				const char *name __maybe_unused)
> >> +{
> >> +	return 0;
> >> +}
> >>  
> >>  static int machine__create_module(void *arg, const char *name, u64 start)
> >>  {
> >>  	struct machine *machine = arg;
> >>  	struct map *map;
> >>  
> >> +	if (arch__fix_module_text_start(&start, name) < 0)
> >> +		return -1;
> >> +
> >>  	map = machine__findnew_module_map(machine, start, name);
> >>  	if (map == NULL)
> >>  		return -1;
> >> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> >> index 41ac9cf..20739f7 100644
> >> --- a/tools/perf/util/machine.h
> >> +++ b/tools/perf/util/machine.h
> >> @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine,
> >>  
> >>  struct map *machine__findnew_module_map(struct machine *machine, u64 start,
> >>  					const char *filename);
> >> +int arch__fix_module_text_start(u64 *start, const char *name);
> >>  
> >>  int __machine__load_kallsyms(struct machine *machine, const char *filename,
> >>  			     enum map_type type, bool no_kcore, symbol_filter_t filter);
> >> -- 
> >> 2.3.0
> >