Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758289Ab1DYODY (ORCPT ); Mon, 25 Apr 2011 10:03:24 -0400 Received: from mail4.hitachi.co.jp ([133.145.228.5]:33649 "EHLO mail4.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756683Ab1DYODW (ORCPT ); Mon, 25 Apr 2011 10:03:22 -0400 X-AuditID: b753bd60-a39c8ba00000453f-45-4db57f289711 X-AuditID: b753bd60-a39c8ba00000453f-45-4db57f289711 Message-ID: <4DB57F23.9020305@hitachi.com> Date: Mon, 25 Apr 2011 23:03:15 +0900 From: Masami Hiramatsu Organization: Systems Development Lab., Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2.15) Gecko/20110303 Thunderbird/3.1.9 MIME-Version: 1.0 To: =?ISO-8859-1?Q?Juhani_M=E4kel=E4?= Cc: Hannu Heikkinen , Ananth N Mavinakayanahalli , Anil S Keshavamurthy , "David S. Miller" , linux-kernel@vger.kernel.org Subject: Re: [PATCH] Kprobe: Fixed a leak when a retprobe entry function returns non-zero References: <4D130452.2010706@nokia.com> <4D14429A.3090606@hitachi.com> <4DA951FC.6050807@hitachi.com> In-Reply-To: <4DA951FC.6050807@hitachi.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: AAAAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6001 Lines: 145 Hi Juhani, I'd like to have your signed-off-by for this patch. Could you update it against the latest kernel and add your signed-off-by? Thank you, (2011/04/16 17:23), Masami Hiramatsu wrote: > (2011/04/14 23:21), Hannu Heikkinen wrote: >> Hi, >> >> any progress on this Juhani's fix? > > I think this basically good for me. Could you revise it as > usual style of commit patch on the latest -tip or linus tree? > > Thank you, > >> >> br, >> Hannu >> >> 2010/12/24 Masami Hiramatsu >> >>> (2010/12/23 17:12), Juhani M?kel? wrote: >>>> Dear Sirs, >>>> >>>> Here is a little fix I found necessary to implement in order to perform >>>> some probing: >>>> >>>> ---- >>>> A kretprobe can install an optional entry probe that is called when >>>> the probed function is entered. If the callback returns non zero, >>>> the return probe will not be called and the instance is forgotten. >>>> However, the allocated instance of struct kretprobe_instance was >>>> not returned in the free_instances list. Fixed this by returning >>>> the unused instance to the free list if it was not needed. >>> >>> Right. That must be a memory-leak path! >>> Thank you very much for pointing it out :-) >>> >>> BTW, it seems that other paths have initialized hlist by >>> INIT_HLIST_NODE(). However, actually there is no need to >>> init a node for adding on a hlist again. Just from the viewpoint >>> of maintaining the code, coding style should have coherence and >>> it's better to init by INIT_HLIST_NODE(). >>> >>> (In this function, a node deleted from free_instances hlist is >>> always added on a hlist again. So maybe it's enough to use >>> hlist_del_init() instead of hlist_del() at least here.) >>> >>> Anyway, this should fix the problem, and should be an urgent fix. >>> Thanks! >>> >>> >>>> --- >>>> kernel/kprobes.c | 10 +++++++++- >>>> 1 files changed, 9 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c >>>> index 5240d75..69d0ca9 100644 >>>> --- a/kernel/kprobes.c >>>> +++ b/kernel/kprobes.c >>>> @@ -971,8 +971,16 @@ static int __kprobes pre_handler_kretprobe(struct >>>> kprobe *p, >>>> ri->rp = rp; >>>> ri->task = current; >>>> >>>> - if (rp->entry_handler && rp->entry_handler(ri, regs)) >>>> + if (rp->entry_handler && rp->entry_handler(ri, regs)) { >>>> + /* >>>> + * Restore the instance to the free list >>>> + * if it is not needed any more. >>>> + */ >>>> + spin_lock_irqsave(&rp->lock, flags); >>>> + hlist_add_head(&ri->hlist, &rp->free_instances); >>>> + spin_unlock_irqrestore(&rp->lock, flags); >>>> return 0; >>>> + } >>>> >>>> arch_prepare_kretprobe(ri, regs); >>>> ---- >>>> >>>> I'm not at all positive that this is the right fix, but at least in our >>>> environment it seems to help. Here is some background info: >>>> >>>> We have implemented a kernel module that implements capability check >>>> tracing by adding a kretprobe in the "capable" function. Every time a >>>> capability check is made, it gathers some data about the process being >>>> checked, the capability number and the result of the check. If the check >>>> comes with current->mm == NULL, it's disregarded by the tracer to avoid >>>> unnecessary overhead, and the entry_handler returns value 1. >>>> >>>> Normally this works fine, but this week we noticed that if the module is >>>> compiled in and activated in an early phase in the boot it doesn't work >>>> at all. It appeared that our entry_handler was called as many times as >>>> was set in the maxactive field of the registered probe, and every time >>>> it returned 1 because current->mm was NULL in all of the calls. Then no >>>> more callbacks were made. When the probe was de-registered, the nmissed >>>> counter had a large value (>6000), and after registering it again the >>>> probing started to work. >>>> >>>> This made me suspect a resource leak, and as far as I can see there >>>> indeed is one in kprobe.c::pre_handler_kretprobe. The fix above solved >>>> the problem and seems not to have any undesired side effects. >>>> >>>> We are using kernel version 2.6.32, but it seems to me that the same >>>> problem exists in more current kernels judging by a quick look. >>>> >>>> Why the problem manifests itself only if the tracing is enabled early in >>>> the boot I cannot say. Could it be that if the entry_handler returns 0 >>>> at least once before the free list has been exhausted, it resets the >>>> situation somehow? Or is it that after some point after userspace >>>> initialization current->mm is never NULL? >>>> >>>> The capability tracing module itself is ment for upstream, but I have >>>> been told its code is not kernel quality (not enough comments) and the >>>> implementation lacks obvious features so we have not dared to offer it >>>> yet. It is used for defining profiles for daemon processes currently >>>> running as root by checking what capabilities they actually need and >>>> then assigning them only those, in the context of the MSSF security >>>> framework project: >>>> >>>> >>> http://conference2010.meego.com/session/mobile-simplified-security-framework-overview >>>> >>>> In case you are interested I'm happy to make the code and documentation >>>> available at the forum of your choice. >>>> >>>> Yours sincerely, >>>> Juhani M?kel? >>>> Nixu OPEN - https://www.nixuopen.org -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com -- 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/