| Message ID | 20230103124912.2948963-3-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8be9fbd5345da52f4a74f7f81d55ff9fa0a2958e |
| Headers | show |
| Series | ftrace: Add sample code with dynamic ftrace_ops | expand |
On Tue, 3 Jan 2023 12:49:11 +0000 Mark Rutland <mark.rutland@arm.com> 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: stable@vger.kernel.org And Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thanks! > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > Cc: Florent Revest <revest@chromium.org> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > --- > 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 Wed, Jan 04, 2023 at 11:51:49PM +0900, Masami Hiramatsu wrote: > On Tue, 3 Jan 2023 12:49:11 +0000 > Mark Rutland <mark.rutland@arm.com> 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: stable@vger.kernel.org > > And > > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > 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 <mark.rutland@arm.com> > > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > Cc: Florent Revest <revest@chromium.org> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > --- > > 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) <mhiramat@kernel.org>
On Wed, 4 Jan 2023 15:30:16 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > 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
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);