Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1

[Miroslav Benes <mbenes@suse.cz>]

On Wed, 15 Apr 2015, Minfei Huang wrote:

> On 04/15/15 at 10:30P, Miroslav Benes wrote:
> > On Wed, 15 Apr 2015, Minfei Huang wrote:
> > 
> > > 
> > > Yes, the function name can be changed, before the extra module is
> > > installed to the production system.
> > > 
> > > We discuss around and around, there are still some confusion with it.
> > > 
> > > 1) How does end user know that livepatch can _not_ support the function
> > >     which length is larger than 127. We can not enforce the end user
> > >     to know the livepatch and kallsyms code in detail.
> > > 2) How does end user use livepatch to patch running extra module, once
> > >     the module is running in the production system, if the function name
> > >     is insane.
> > > 3) The error message is ambiguity, if we try to patch the overlength
> > >     function. We can give the error message clearly, once the function
> > >     name is overlength.
> > > 
> > > I think it is better that we can take more time on the people who will
> > > use livepatch frequently.
> > 
> > Just my two cents, even if we admit that such change is worth it (and I 
> > am still not convinced that it is the case), I think it would make sense 
> > to fix it somewhere in kallsyms as Josh proposed. I suspect that when 
> 
> Ohhh...
> 
> Fixing kallsyms to restrict the function name length maybe is not a good
> idea. I have no idea how we should do this, except for the coding
> problems.

Well we do it now via scripts/kallsyms.c when vmlinux is built. Try it. We 
apparently do not do it when kernel modules are built out of the tree (as 
you demonstrated before). So the question is whether we should do it also 
there. That is one thing we try to tell you.

The other one is that 128 characters long function names are insane. 
Probably that is what KSYM_NAME_LEN is for in the first place. Maybe you 
could even try to add the check to checkpatch.pl.

> > function names longer than KSYM_NAME_LEN were common there would be many 
> > similar problems elsewhere in the kernel.
> > 
> > That is you can prepare a patch to kallsyms and submit it there. Not sure 
> > who is the maintainer but he might have an opinion about this...
> > 
> > Thanks,
> > Miroslav
> 
> Hold on, I get a scenario that livepatch may do fatal error. I am fine
> that livepatch do not support overlength function name, because it can
> not corrupt the kernel.
> 
> Once there is a function name A is larger than 127, and another function
> name B is as longer as 127, it is disaster that we want to patch
> function B, if function name of first 127 is same between A and B.

True, but see above.

> Livepatch may find the function of A to patch it. So this patch(2/2) may
> be needed to fix the issue.

Hm, but this patch is not the solution for the issue, or is it? You would 
check only those first KSYM_NAME_LEN characters, but that would not 
differentiate between A and B. Or maybe I do not follow.

Thanks
Miroslav