Kprobe: Fixed a leak when a retprobe entry function returns non-zero
PATCH 1/1 Kprobe: Fixed a leak when a retprobe entry function returns non-zero
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.
--- kernel/kprobes.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
kernel/kprobes.c +0 -0
@@ -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
Quoted 5 lines · Juhani Mäkelä
Dear Sirs,
Here is a little fix I found necessary to implement in order to perform some probing:
Quoted 7 lines
----
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 structkretprobe_instancewas not returned in thefree_instanceslist. 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!
Quoted 82 lines
--- kernel/kprobes.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)kernel/kprobes.c +9
-2@@ -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 theentry_handlerreturns 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_handlerwas called as many times as was set in the maxactive field of the registered probe, and every time it returned 1 becausecurrent->mmwas 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_handlerreturns 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 initializationcurrent->mmis 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 4 lines
Masami HIRAMATSU 2nd Dept. Linux Technology Center Hitachi, Ltd., Systems Development Laboratory E-mail: [email protected]
Quoted 23 lines · Masami Hiramatsu
Quoted 5 lines · Juhani Mäkelä
Dear Sirs,
Here is a little fix I found necessary to implement in order to perform some probing:
Quoted 7 lines
----
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 structkretprobe_instancewas not returned in thefree_instanceslist. 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 byINIT_HLIST_NODE().
Quoted 7 lines
(In this function, a node deleted from
free_instanceshlist is
always added on a hlist again. So maybe it's enough to usehlist_del_init()instead ofhlist_del()at least here.)Anyway, this should fix the problem, and should be an urgent fix. Thanks!
Hi,
any progress on this?
Haven't seen any fix in mainline.
br,
Hannu
Quoted 95 lines
Quoted 83 lines
--- kernel/kprobes.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)kernel/kprobes.c +9
-2@@ -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 theentry_handlerreturns 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_handlerwas called as many times as was set in the maxactive field of the registered probe, and every time it returned 1 becausecurrent->mmwas 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_handlerreturns 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 initializationcurrent->mmis 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/Signature · 9 lines
Masami HIRAMATSU 2nd Dept. Linux Technology Center Hitachi, Ltd., Systems Development Laboratory E-mail: [email protected] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Quoted 3 lines · Hannu Heikkinen
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,
Quoted 135 lines
br,
Hannu2010/12/24 Masami Hiramatsu <[email protected]>
Quoted 128 lines
Quoted 5 lines · Juhani Mäkelä
Dear Sirs,
Here is a little fix I found necessary to implement in order to perform some probing:
Quoted 7 lines
----
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 structkretprobe_instancewas not returned in thefree_instanceslist. 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 byINIT_HLIST_NODE().(In this function, a node deleted from
free_instanceshlist is
always added on a hlist again. So maybe it's enough to usehlist_del_init()instead ofhlist_del()at least here.)Anyway, this should fix the problem, and should be an urgent fix. Thanks!
Quoted 60 lines
--- kernel/kprobes.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)kernel/kprobes.c +9
-2@@ -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 theentry_handlerreturns 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_handlerwas called as many times as was set in the maxactive field of the registered probe, and every time it returned 1 becausecurrent->mmwas 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_handlerreturns 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 initializationcurrent->mmis never NULL?Quoted 9 lines
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
Quoted 9 lines
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
Quoted 4 lines
the body of a message to [email protected]
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: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]
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,
Quoted 124 lines · Masami Hiramatsu
Quoted 3 lines · Hannu Heikkinen
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,
Quoted 114 lines
br,
Hannu2010/12/24 Masami Hiramatsu <[email protected]>
Quoted 108 lines
Quoted 5 lines · Juhani Mäkelä
Dear Sirs,
Here is a little fix I found necessary to implement in order to perform some probing:
Quoted 7 lines
----
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 structkretprobe_instancewas not returned in thefree_instanceslist. 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 byINIT_HLIST_NODE().(In this function, a node deleted from
free_instanceshlist is
always added on a hlist again. So maybe it's enough to usehlist_del_init()instead ofhlist_del()at least here.)Anyway, this should fix the problem, and should be an urgent fix. Thanks!
Quoted 60 lines
--- kernel/kprobes.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)kernel/kprobes.c +9
-2@@ -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 theentry_handlerreturns 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_handlerwas called as many times as was set in the maxactive field of the registered probe, and every time it returned 1 becausecurrent->mmwas 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_handlerreturns 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 initializationcurrent->mmis never NULL?Quoted 9 lines
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
Quoted 7 lines
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
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]