Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932346AbbD0Ilw (ORCPT ); Mon, 27 Apr 2015 04:41:52 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35723 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932284AbbD0Ilr (ORCPT ); Mon, 27 Apr 2015 04:41:47 -0400 Date: Mon, 27 Apr 2015 10:41:44 +0200 (CEST) From: Miroslav Benes To: Minfei Huang cc: Minfei Huang , Petr Mladek , Josh Poimboeuf , sjenning@redhat.com, jkosina@suse.cz, vojtech@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] livepatch: Fix the bug if the function name is larger than KSYM_NAME_LEN-1 In-Reply-To: Message-ID: References: <20150414151110.GL4412@treble.hsd1.ky.comcast.net> <20150414162718.GC21044@dhcp128.suse.cz> <20150414170139.GA30740@dhcp-128-1.nay.redhat.com> <20150414184121.GD21044@dhcp128.suse.cz> <20150415021558.GA21518@dhcp-128-1.nay.redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4702 Lines: 111 On Sun, 26 Apr 2015, Minfei Huang wrote: > On 04/15/15 at 01:58P, Miroslav Benes wrote: > > 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. > > > > Hello, guys. > > Do I need to post a patch to fix the above issue? Applied following > patch, livepatch will fail to patch the patch, since there are more than > two symbols to be matched. > If so, I would like to post an official patch to the maillist. > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 284e269..67b237f 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -152,7 +152,7 @@ static int klp_find_callback(void *data, const char *name, > if ((mod && !args->objname) || (!mod && args->objname)) > return 0; > > - if (strcmp(args->name, name)) > + if (strncmp(args->name, name, KSYM_NAME_LEN-1)) > return 0; > > if (args->objname && strcmp(args->objname, mod->name)) This means that in your scenario described above count would be >0 here and kallsyms symbol would not be resolved... which is the same situation as of now without your patch. And you can find this objection above as well. I still think this needs to be fixed somewhere else and you can find hints and points in the thread. Maybe someone else feels differently and will say so... Cheers, Miroslav -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/