linux-kernel - Re: [PATCH] kallsyms: optimize kallsyms_lookup

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] [day] [month] [year] [list]

Date:   Tue, 27 Jun 2017 14:58:40 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>,
        Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        David Laight <David.Laight@...LAB.COM>,
        linuxppc-dev@...ts.ozlabs.org, Paul Clarke <pc@...ibm.com>
Subject: Re: [PATCH] kallsyms: optimize kallsyms_lookup_name() for a few cases

On 2017/04/27 11:21AM, Masami Hiramatsu wrote:
> On Thu, 27 Apr 2017 01:38:10 +0530
> "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:
> 
> > Michael Ellerman wrote:
> > > "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> writes:
> > >> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > >> index 6a3b249a2ae1..d134b060564f 100644
> > >> --- a/kernel/kallsyms.c
> > >> +++ b/kernel/kallsyms.c
> > >> @@ -205,6 +205,12 @@ unsigned long kallsyms_lookup_name(const char *name)
> > >>  	unsigned long i;
> > >>  	unsigned int off;
> > >>  
> > >> +	if (!name || *name == '\0')
> > >> +		return false;
> > >> +
> > >> +	if (strnchr(name, MODULE_NAME_LEN, ':'))
> > >> +		return module_kallsyms_lookup_name(name);
> > >> +
> > >>  	for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> > >>  		off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> > > 	...				
> > > 	}
> > >        	return module_kallsyms_lookup_name(name);
> > > 
> > > Is the rest of the context.
> > > 
> > > Which looks a bit odd, we already did module lookup previously?
> > > 
> > > But it's correct, because you can lookup a symbol in a module without a
> > > module prefix, it just looks in every module.
> > 
> > Yes.
> > 
> > > 
> > > You could invert the logic, ie. check that there isn't a ":" in the name
> > > and only in that case do the for loop, always falling back to module
> > > lookup.
> > > 
> > > Or just add a comment explaining why we call module lookup in two places.
> > 
> > Good point. Here's a v2 - I'm using a goto so as to not indent the code too much.
> > 
> > Thanks for the review!
> > - Naveen
> > 
> > --
> > [PATCH v2] kallsyms: optimize kallsyms_lookup_name() for a few cases
> > 
> > 1. Fail early for invalid/zero length symbols.
> > 2. Detect names of the form <mod:name> and skip checking for kernel
> > symbols in that case.
> > 
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu <mhiramat@...nel.org>

Thanks, Masami!

I am not quite sure who maintains kallsyms...

Ingo,
Can you please help with merging this patch?

Thanks,
Naveen

> 
> Thanks,
> 
> 
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com>
> > ---
> >  kernel/kallsyms.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 6a3b249a2ae1..f7558dc5c6ac 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -205,12 +205,20 @@ unsigned long kallsyms_lookup_name(const char *name)
> >  	unsigned long i;
> >  	unsigned int off;
> >  
> > +	if (!name || *name == '\0')
> > +		return 0;
> > +
> > +	/* For symbols of the form <mod>:<sym>, only check the modules */
> > +	if (strnchr(name, MODULE_NAME_LEN, ':'))
> > +		goto mod;
> > +
> >  	for (i = 0, off = 0; i < kallsyms_num_syms; i++) {
> >  		off = kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf));
> >  
> >  		if (strcmp(namebuf, name) == 0)
> >  			return kallsyms_sym_address(i);
> >  	}
> > +mod:
> >  	return module_kallsyms_lookup_name(name);
> >  }
> >  EXPORT_SYMBOL_GPL(kallsyms_lookup_name);
> > -- 
> > 2.12.2
> > 
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@...nel.org>
>