Re: [PATCH] Kprobe: Fixed a leak when a retprobe entry function returns non-zero

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(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
> --
> 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/
> 


-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com