[lkml]   [2010]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] Kprobe: Fixed a leak when a retprobe entry function returns non-zero
    (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.

    > ---
    > 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:
    > 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 -
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to
    > More majordomo info at
    > Please read the FAQ at

    Masami HIRAMATSU
    2nd Dept. Linux Technology Center
    Hitachi, Ltd., Systems Development Laboratory
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2010-12-24 07:53    [W:0.051 / U:23.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site