Setting filters on an ftrace ops results in some memory being allocated
for the filter hashes, which must be freed before the ops can be freed.
This can be done by removing every individual element of the hash by
calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
set, but this is somewhat error prone as it's easy to forget to remove
an element.
Make it easier to clean this up by exporting ftrace_free_filter(), which
can be used to clean up all of the filter hashes after an ftrace_ops has
been unregistered.
Using this, fix the ftrace-direct* samples to free hashes prior to being
unloaded. All other code either removes individual filters explicitly or
is built-in and already calls ftrace_free_filter().
Signed-off-by: Mark Rutland <[email protected]>
Reviewed-by: Steven Rostedt (Google) <[email protected]>
Cc: Florent Revest <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
---
kernel/trace/ftrace.c | 23 ++++++++++++++++++++-
samples/ftrace/ftrace-direct-multi-modify.c | 1 +
samples/ftrace/ftrace-direct-multi.c | 1 +
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 442438b93fe98..750aa3f08b25a 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
}
+/**
+ * ftrace_free_filter - remove all filters for an ftrace_ops
+ * @ops - the ops to remove the filters from
+ */
void ftrace_free_filter(struct ftrace_ops *ops)
{
ftrace_ops_init(ops);
free_ftrace_hash(ops->func_hash->filter_hash);
free_ftrace_hash(ops->func_hash->notrace_hash);
}
+EXPORT_SYMBOL_GPL(ftrace_free_filter);
static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
{
@@ -5839,6 +5844,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
*
* Filters denote which functions should be enabled when tracing is enabled
* If @ip is NULL, it fails to update filter.
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
*/
int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
int remove, int reset)
@@ -5858,7 +5867,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
*
* Filters denote which functions should be enabled when tracing is enabled
* If @ips array or any ip specified within is NULL , it fails to update filter.
- */
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
+*/
int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
unsigned int cnt, int remove, int reset)
{
@@ -5900,6 +5913,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
*
* Filters denote which functions should be enabled when tracing is enabled.
* If @buf is NULL and reset is set, all functions will be enabled for tracing.
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
*/
int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
int len, int reset)
@@ -5919,6 +5936,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
* Notrace Filters denote which functions should not be enabled when tracing
* is enabled. If @buf is NULL and reset is set, all functions will be enabled
* for tracing.
+ *
+ * This can allocate memory which must be freed before @ops can be freed,
+ * either by removing each filtered addr or by using
+ * ftrace_free_filter(@ops).
*/
int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
int len, int reset)
diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
index d52370cad0b6e..a825dbd2c9cfd 100644
--- a/samples/ftrace/ftrace-direct-multi-modify.c
+++ b/samples/ftrace/ftrace-direct-multi-modify.c
@@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
{
kthread_stop(simple_tsk);
unregister_ftrace_direct_multi(&direct, my_tramp);
+ ftrace_free_filter(&direct);
}
module_init(ftrace_direct_multi_init);
diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
index ec1088922517d..d955a26506053 100644
--- a/samples/ftrace/ftrace-direct-multi.c
+++ b/samples/ftrace/ftrace-direct-multi.c
@@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
static void __exit ftrace_direct_multi_exit(void)
{
unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
+ ftrace_free_filter(&direct);
}
module_init(ftrace_direct_multi_init);
--
2.30.2
On Tue, 3 Jan 2023 12:49:11 +0000
Mark Rutland <[email protected]> wrote:
> Setting filters on an ftrace ops results in some memory being allocated
> for the filter hashes, which must be freed before the ops can be freed.
> This can be done by removing every individual element of the hash by
> calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
> set, but this is somewhat error prone as it's easy to forget to remove
> an element.
>
> Make it easier to clean this up by exporting ftrace_free_filter(), which
> can be used to clean up all of the filter hashes after an ftrace_ops has
> been unregistered.
>
> Using this, fix the ftrace-direct* samples to free hashes prior to being
> unloaded. All other code either removes individual filters explicitly or
> is built-in and already calls ftrace_free_filter().
So, it seems to fix memory leaks. Then, it may need to go to stable.
Fixes: e1067a07cfbc ("ftrace/samples: Add module to test multi direct modify interface")
Fixes: 5fae941b9a6f ("ftrace/samples: Add multi direct interface test module")
Cc: [email protected]
And
Reviewed-by: Masami Hiramatsu (Google) <[email protected]>
Thanks!
>
> Signed-off-by: Mark Rutland <[email protected]>
> Reviewed-by: Steven Rostedt (Google) <[email protected]>
> Cc: Florent Revest <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> ---
> kernel/trace/ftrace.c | 23 ++++++++++++++++++++-
> samples/ftrace/ftrace-direct-multi-modify.c | 1 +
> samples/ftrace/ftrace-direct-multi.c | 1 +
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 442438b93fe98..750aa3f08b25a 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
> call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
> }
>
> +/**
> + * ftrace_free_filter - remove all filters for an ftrace_ops
> + * @ops - the ops to remove the filters from
> + */
> void ftrace_free_filter(struct ftrace_ops *ops)
> {
> ftrace_ops_init(ops);
> free_ftrace_hash(ops->func_hash->filter_hash);
> free_ftrace_hash(ops->func_hash->notrace_hash);
> }
> +EXPORT_SYMBOL_GPL(ftrace_free_filter);
>
> static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
> {
> @@ -5839,6 +5844,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
> *
> * Filters denote which functions should be enabled when tracing is enabled
> * If @ip is NULL, it fails to update filter.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
> */
> int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> int remove, int reset)
> @@ -5858,7 +5867,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
> *
> * Filters denote which functions should be enabled when tracing is enabled
> * If @ips array or any ip specified within is NULL , it fails to update filter.
> - */
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
> +*/
> int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
> unsigned int cnt, int remove, int reset)
> {
> @@ -5900,6 +5913,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
> *
> * Filters denote which functions should be enabled when tracing is enabled.
> * If @buf is NULL and reset is set, all functions will be enabled for tracing.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
> */
> int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> int len, int reset)
> @@ -5919,6 +5936,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
> * Notrace Filters denote which functions should not be enabled when tracing
> * is enabled. If @buf is NULL and reset is set, all functions will be enabled
> * for tracing.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by removing each filtered addr or by using
> + * ftrace_free_filter(@ops).
> */
> int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> int len, int reset)
> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> index d52370cad0b6e..a825dbd2c9cfd 100644
> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> @@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
> {
> kthread_stop(simple_tsk);
> unregister_ftrace_direct_multi(&direct, my_tramp);
> + ftrace_free_filter(&direct);
> }
>
> module_init(ftrace_direct_multi_init);
> diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> index ec1088922517d..d955a26506053 100644
> --- a/samples/ftrace/ftrace-direct-multi.c
> +++ b/samples/ftrace/ftrace-direct-multi.c
> @@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
> static void __exit ftrace_direct_multi_exit(void)
> {
> unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
> + ftrace_free_filter(&direct);
> }
>
> module_init(ftrace_direct_multi_init);
> --
> 2.30.2
>
--
Masami Hiramatsu (Google) <[email protected]>
On Wed, Jan 04, 2023 at 11:51:49PM +0900, Masami Hiramatsu wrote:
> On Tue, 3 Jan 2023 12:49:11 +0000
> Mark Rutland <[email protected]> wrote:
>
> > Setting filters on an ftrace ops results in some memory being allocated
> > for the filter hashes, which must be freed before the ops can be freed.
> > This can be done by removing every individual element of the hash by
> > calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
> > set, but this is somewhat error prone as it's easy to forget to remove
> > an element.
> >
> > Make it easier to clean this up by exporting ftrace_free_filter(), which
> > can be used to clean up all of the filter hashes after an ftrace_ops has
> > been unregistered.
> >
> > Using this, fix the ftrace-direct* samples to free hashes prior to being
> > unloaded. All other code either removes individual filters explicitly or
> > is built-in and already calls ftrace_free_filter().
>
> So, it seems to fix memory leaks. Then, it may need to go to stable.
>
> Fixes: e1067a07cfbc ("ftrace/samples: Add module to test multi direct modify interface")
> Fixes: 5fae941b9a6f ("ftrace/samples: Add multi direct interface test module")
> Cc: [email protected]
>
> And
>
> Reviewed-by: Masami Hiramatsu (Google) <[email protected]>
>
> Thanks!
Thanks!
Assuming Steve is also happy with the series, I assume one of you two will pick
this up and fold those in.
I've folded all those tags in my local branch (and pushed that to my
ftrace/ops-sample branch on kernel.org), so if you'd prefer I post a v3 with
those I'm quite happy to do so.
Thanks,
Mark.
>
> >
> > Signed-off-by: Mark Rutland <[email protected]>
> > Reviewed-by: Steven Rostedt (Google) <[email protected]>
> > Cc: Florent Revest <[email protected]>
> > Cc: Masami Hiramatsu <[email protected]>
> > ---
> > kernel/trace/ftrace.c | 23 ++++++++++++++++++++-
> > samples/ftrace/ftrace-direct-multi-modify.c | 1 +
> > samples/ftrace/ftrace-direct-multi.c | 1 +
> > 3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 442438b93fe98..750aa3f08b25a 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
> > call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
> > }
> >
> > +/**
> > + * ftrace_free_filter - remove all filters for an ftrace_ops
> > + * @ops - the ops to remove the filters from
> > + */
> > void ftrace_free_filter(struct ftrace_ops *ops)
> > {
> > ftrace_ops_init(ops);
> > free_ftrace_hash(ops->func_hash->filter_hash);
> > free_ftrace_hash(ops->func_hash->notrace_hash);
> > }
> > +EXPORT_SYMBOL_GPL(ftrace_free_filter);
> >
> > static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
> > {
> > @@ -5839,6 +5844,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
> > *
> > * Filters denote which functions should be enabled when tracing is enabled
> > * If @ip is NULL, it fails to update filter.
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> > */
> > int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> > int remove, int reset)
> > @@ -5858,7 +5867,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
> > *
> > * Filters denote which functions should be enabled when tracing is enabled
> > * If @ips array or any ip specified within is NULL , it fails to update filter.
> > - */
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> > +*/
> > int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
> > unsigned int cnt, int remove, int reset)
> > {
> > @@ -5900,6 +5913,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
> > *
> > * Filters denote which functions should be enabled when tracing is enabled.
> > * If @buf is NULL and reset is set, all functions will be enabled for tracing.
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> > */
> > int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> > int len, int reset)
> > @@ -5919,6 +5936,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
> > * Notrace Filters denote which functions should not be enabled when tracing
> > * is enabled. If @buf is NULL and reset is set, all functions will be enabled
> > * for tracing.
> > + *
> > + * This can allocate memory which must be freed before @ops can be freed,
> > + * either by removing each filtered addr or by using
> > + * ftrace_free_filter(@ops).
> > */
> > int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> > int len, int reset)
> > diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> > index d52370cad0b6e..a825dbd2c9cfd 100644
> > --- a/samples/ftrace/ftrace-direct-multi-modify.c
> > +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> > @@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
> > {
> > kthread_stop(simple_tsk);
> > unregister_ftrace_direct_multi(&direct, my_tramp);
> > + ftrace_free_filter(&direct);
> > }
> >
> > module_init(ftrace_direct_multi_init);
> > diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> > index ec1088922517d..d955a26506053 100644
> > --- a/samples/ftrace/ftrace-direct-multi.c
> > +++ b/samples/ftrace/ftrace-direct-multi.c
> > @@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
> > static void __exit ftrace_direct_multi_exit(void)
> > {
> > unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
> > + ftrace_free_filter(&direct);
> > }
> >
> > module_init(ftrace_direct_multi_init);
> > --
> > 2.30.2
> >
>
>
> --
> Masami Hiramatsu (Google) <[email protected]>
On Wed, 4 Jan 2023 15:30:16 +0000
Mark Rutland <[email protected]> wrote:
> > Reviewed-by: Masami Hiramatsu (Google) <[email protected]>
> >
> > Thanks!
>
> Thanks!
>
> Assuming Steve is also happy with the series, I assume one of you two will pick
> this up and fold those in.
>
> I've folded all those tags in my local branch (and pushed that to my
> ftrace/ops-sample branch on kernel.org), so if you'd prefer I post a v3 with
> those I'm quite happy to do so.
I was planning on looking at this today.
No need to post new patches for tags, that's easy to add. I only care about
code changes for new patches.
-- Steve