In addition to patches introduced by commit f45d1225adb0 "tracing: Kernel
access to Ftrace instances") we also need the following patches to reliably
access ftrace instances from other kernel modules or components.
Please review the patches that follow.
Divya Indi (7):
tracing: Required changes to use ftrace_set_clr_event.
tracing: Declare newly exported APIs in include/linux/trace.h
tracing: Verify if trace array exists before destroying it.
tracing: Adding NULL checks
tracing: Handle the trace array ref counter in new functions
tracing: New functions for kernel access to Ftrace instances
tracing: Un-export ftrace_set_clr_event
include/linux/trace.h | 10 +++++
include/linux/trace_events.h | 2 +
kernel/trace/trace.c | 90 ++++++++++++++++++++++++++++++++++++++++++--
kernel/trace/trace.h | 4 +-
kernel/trace/trace_events.c | 25 +++++++++++-
5 files changed, 123 insertions(+), 8 deletions(-)
--
1.8.3.1
Use "trace_array_set_clr_event" to enable/disable events to a trace
array from other kernel modules/components.
Hence, we no longer need to have ftrace_set_clr_event as an exported API.
Signed-off-by: Divya Indi <[email protected]>
Reviewed-By: Aruna Ramakrishna <[email protected]>
---
include/linux/trace_events.h | 1 -
kernel/trace/trace_events.c | 3 +--
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 6da3600..025ae2b 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -542,7 +542,6 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
int trace_set_clr_event(const char *system, const char *event, int set);
int trace_array_set_clr_event(struct trace_array *tr, const char *system,
const char *event, int set);
-int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
/*
* The double __builtin_constant_p is because gcc will give us an error
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 9ee6b52..96dd997 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -795,7 +795,7 @@ static int __ftrace_set_clr_event(struct trace_array *tr, const char *match,
return ret;
}
-int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
+static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
{
char *event = NULL, *sub = NULL, *match;
int ret;
@@ -834,7 +834,6 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
return ret;
}
-EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
/**
* trace_set_clr_event - enable or disable an event
--
1.8.3.1
A trace array can be destroyed from userspace or kernel. Verify if the
trace array exists before proceeding to destroy/remove it.
Signed-off-by: Divya Indi <[email protected]>
Reviewed-By: Aruna Ramakrishna <[email protected]>
---
kernel/trace/trace.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 1c80521..468ecc5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8421,17 +8421,27 @@ static int __remove_instance(struct trace_array *tr)
return 0;
}
-int trace_array_destroy(struct trace_array *tr)
+int trace_array_destroy(struct trace_array *this_tr)
{
+ struct trace_array *tr;
int ret;
- if (!tr)
+ if (!this_tr) {
return -EINVAL;
+ }
mutex_lock(&event_mutex);
mutex_lock(&trace_types_lock);
- ret = __remove_instance(tr);
+ ret = -ENODEV;
+
+ /* Making sure trace array exists before destroying it. */
+ list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+ if (tr == this_tr) {
+ ret = __remove_instance(tr);
+ break;
+ }
+ }
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
--
1.8.3.1
For functions returning a trace array Eg: trace_array_create(), we need to
increment the reference counter associated with the trace array to ensure it
does not get freed when in use.
Once we are done using the trace array, we need to call
trace_array_put() to make sure we are not holding a reference to it
anymore and the instance/trace array can be removed when required.
Hence, additionally exporting trace_array_put().
Example use:
tr = trace_array_create("foo-bar");
// Use this trace array
// Log to this trace array or enable/disable events to this trace array.
....
....
// tr no longer required
trace_array_put();
Signed-off-by: Divya Indi <[email protected]>
Reviewed-By: Aruna Ramakrishna <[email protected]>
---
include/linux/trace.h | 1 +
kernel/trace/trace.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/linux/trace.h b/include/linux/trace.h
index 24fcf07..2c782d5 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -31,6 +31,7 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
const char *fmt, ...);
struct trace_array *trace_array_create(const char *name);
int trace_array_destroy(struct trace_array *tr);
+void trace_array_put(struct trace_array *tr);
#endif /* CONFIG_TRACING */
#endif /* _LINUX_TRACE_H */
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 22bf166..130579b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -297,12 +297,22 @@ static void __trace_array_put(struct trace_array *this_tr)
this_tr->ref--;
}
+/**
+ * trace_array_put - Decrement reference counter for the given trace array.
+ * @tr: Trace array for which reference counter needs to decrement.
+ *
+ * NOTE: Functions like trace_array_create increment the reference counter for
+ * the trace array to ensure they do not get freed while in use. Make sure to
+ * call trace_array_put() when the use is done. This will ensure that the
+ * instance can be later removed.
+ */
void trace_array_put(struct trace_array *this_tr)
{
mutex_lock(&trace_types_lock);
__trace_array_put(this_tr);
mutex_unlock(&trace_types_lock);
}
+EXPORT_SYMBOL_GPL(trace_array_put);
int call_filter_check_discard(struct trace_event_call *call, void *rec,
struct ring_buffer *buffer,
@@ -8302,6 +8312,16 @@ static void update_tracer_options(struct trace_array *tr)
mutex_unlock(&trace_types_lock);
}
+/**
+ * trace_array_create - Create a new trace array with the given name.
+ * @name: The name of the trace array to be created.
+ *
+ * Create and return a trace array with given name if it does not exist.
+ *
+ * NOTE: The reference counter associated with the returned trace array is
+ * incremented to ensure it is not freed when in use. Make sure to call
+ * "trace_array_put" for this trace array when its use is done.
+ */
struct trace_array *trace_array_create(const char *name)
{
struct trace_array *tr;
@@ -8364,6 +8384,8 @@ struct trace_array *trace_array_create(const char *name)
list_add(&tr->list, &ftrace_trace_arrays);
+ tr->ref++;
+
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
@@ -8385,7 +8407,21 @@ struct trace_array *trace_array_create(const char *name)
static int instance_mkdir(const char *name)
{
- return PTR_ERR_OR_ZERO(trace_array_create(name));
+ struct trace_array *tr;
+
+ tr = trace_array_create(name);
+ if (IS_ERR(tr))
+ return PTR_ERR(tr);
+
+ /*
+ * This function does not return a reference to the created trace array,
+ * so the reference counter is to be 0 when it returns.
+ * trace_array_create increments the ref counter, decrement it here
+ * by calling trace_array_put()
+ */
+ trace_array_put(tr);
+
+ return 0;
}
static int __remove_instance(struct trace_array *tr)
@@ -8424,6 +8460,11 @@ static int __remove_instance(struct trace_array *tr)
return 0;
}
+/*
+ * NOTE: An instance cannot be removed if there is still a reference to it.
+ * Make sure to call "trace_array_put" for a trace array returned by functions
+ * like trace_array_create(), otherwise trace_array_destroy will not succeed.
+ */
int trace_array_destroy(struct trace_array *this_tr)
{
struct trace_array *tr;
--
1.8.3.1
Adding 2 new functions -
1) trace_array_lookup : Look up and return a trace array, given its
name.
2) trace_array_set_clr_event : Enable/disable event recording to the
given trace array.
NOTE: trace_array_lookup returns a trace array and also increments the
reference counter associated with the returned trace array. Make sure to
call trace_array_put() once the use is done so that the instance can be
removed at a later time.
Example use:
tr = trace_array_lookup("foo-bar");
if (!tr)
tr = trace_array_create("foo-bar");
// Log to tr
// Enable/disable events to tr
trace_array_set_clr_event(tr, _THIS_IP,"system","event",1);
// Done using tr
trace_array_put(tr);
..
Signed-off-by: Divya Indi <[email protected]>
Reviewed-By: Aruna Ramakrishna <[email protected]>
---
include/linux/trace.h | 2 ++
include/linux/trace_events.h | 2 ++
kernel/trace/trace.c | 28 ++++++++++++++++++++++++++++
kernel/trace/trace_events.c | 22 ++++++++++++++++++++++
4 files changed, 54 insertions(+)
diff --git a/include/linux/trace.h b/include/linux/trace.h
index 2c782d5..05164bb 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -32,6 +32,8 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
struct trace_array *trace_array_create(const char *name);
int trace_array_destroy(struct trace_array *tr);
void trace_array_put(struct trace_array *tr);
+struct trace_array *trace_array_lookup(const char *name);
+
#endif /* CONFIG_TRACING */
#endif /* _LINUX_TRACE_H */
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 0f874fb..6da3600 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -540,6 +540,8 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
#define is_signed_type(type) (((type)(-1)) < (type)1)
int trace_set_clr_event(const char *system, const char *event, int set);
+int trace_array_set_clr_event(struct trace_array *tr, const char *system,
+ const char *event, int set);
int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
/*
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 130579b..fcf42bb 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8516,6 +8516,34 @@ static int instance_rmdir(const char *name)
return ret;
}
+/**
+ * trace_array_lookup - Lookup the trace array, given its name.
+ * @name: The name of the trace array to be looked up.
+ *
+ * Lookup and return the trace array associated with @name.
+ *
+ * NOTE: The reference counter associated with the returned trace array is
+ * incremented to ensure it is not freed when in use. Make sure to call
+ * "trace_array_put" for this trace array when its use is done.
+ */
+struct trace_array *trace_array_lookup(const char *name)
+{
+ struct trace_array *tr;
+
+ mutex_lock(&trace_types_lock);
+
+ list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+ if (tr->name && strcmp(tr->name, name) == 0) {
+ tr->ref++;
+ mutex_unlock(&trace_types_lock);
+ return tr;
+ }
+ }
+ mutex_unlock(&trace_types_lock);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(trace_array_lookup);
+
static __init void create_trace_instances(struct dentry *d_tracer)
{
trace_instance_dir = tracefs_create_instance_dir("instances", d_tracer,
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 99eb5f8..9ee6b52 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -859,6 +859,28 @@ int trace_set_clr_event(const char *system, const char *event, int set)
}
EXPORT_SYMBOL_GPL(trace_set_clr_event);
+/**
+ * trace_array_set_clr_event - enable or disable an event for a trace array
+ * @system: system name to match (NULL for any system)
+ * @event: event name to match (NULL for all events, within system)
+ * @set: 1 to enable, 0 to disable
+ *
+ * This is a way for other parts of the kernel to enable or disable
+ * event recording to instances.
+ *
+ * Returns 0 on success, -EINVAL if the parameters do not match any
+ * registered events.
+ */
+int trace_array_set_clr_event(struct trace_array *tr, const char *system,
+ const char *event, int set)
+{
+ if (!tr)
+ return -ENOENT;
+
+ return __ftrace_set_clr_event(tr, NULL, system, event, set);
+}
+EXPORT_SYMBOL_GPL(trace_array_set_clr_event);
+
/* 128 should be much more than enough */
#define EVENT_BUF_SIZE 127
--
1.8.3.1
Declare the newly introduced and exported APIs in the header file -
include/linux/trace.h. Moving previous declarations from
kernel/trace/trace.h to include/linux/trace.h.
Signed-off-by: Divya Indi <[email protected]>
Reviewed-By: Aruna Ramakrishna <[email protected]>
---
include/linux/trace.h | 7 +++++++
kernel/trace/trace.h | 4 +---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/include/linux/trace.h b/include/linux/trace.h
index b95ffb2..24fcf07 100644
--- a/include/linux/trace.h
+++ b/include/linux/trace.h
@@ -24,6 +24,13 @@ struct trace_export {
int register_ftrace_export(struct trace_export *export);
int unregister_ftrace_export(struct trace_export *export);
+struct trace_array;
+
+void trace_printk_init_buffers(void);
+int trace_array_printk(struct trace_array *tr, unsigned long ip,
+ const char *fmt, ...);
+struct trace_array *trace_array_create(const char *name);
+int trace_array_destroy(struct trace_array *tr);
#endif /* CONFIG_TRACING */
#endif /* _LINUX_TRACE_H */
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 005f086..66ff63e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -11,6 +11,7 @@
#include <linux/mmiotrace.h>
#include <linux/tracepoint.h>
#include <linux/ftrace.h>
+#include <linux/trace.h>
#include <linux/hw_breakpoint.h>
#include <linux/trace_seq.h>
#include <linux/trace_events.h>
@@ -852,8 +853,6 @@ extern int trace_selftest_startup_branch(struct tracer *trace,
extern int
trace_array_vprintk(struct trace_array *tr,
unsigned long ip, const char *fmt, va_list args);
-int trace_array_printk(struct trace_array *tr,
- unsigned long ip, const char *fmt, ...);
int trace_array_printk_buf(struct ring_buffer *buffer,
unsigned long ip, const char *fmt, ...);
void trace_printk_seq(struct trace_seq *s);
@@ -1869,7 +1868,6 @@ extern int trace_event_enable_disable(struct trace_event_file *file,
extern const char *__stop___tracepoint_str[];
void trace_printk_control(bool enabled);
-void trace_printk_init_buffers(void);
void trace_printk_start_comm(void);
int trace_keep_overwrite(struct tracer *tracer, u32 mask, int set);
int set_tracer_flag(struct trace_array *tr, unsigned int mask, int enabled);
--
1.8.3.1
On Mon, 29 Jul 2019 17:02:34 -0700
Divya Indi <[email protected]> wrote:
> Use "trace_array_set_clr_event" to enable/disable events to a trace
> array from other kernel modules/components.
> Hence, we no longer need to have ftrace_set_clr_event as an exported API.
Hmm, this simply reverts patch 1. Why have patch 1 and this patch in
the first place?
-- Steve
>
> Signed-off-by: Divya Indi <[email protected]>
> Reviewed-By: Aruna Ramakrishna <[email protected]>
As part of commit f45d1225adb0 ("tracing: Kernel access to Ftrace
instances") we exported certain functions. Here, we are adding some additional
NULL checks to ensure safe usage by users of these APIs.
Signed-off-by: Divya Indi <[email protected]>
Reviewed-By: Aruna Ramakrishna <[email protected]>
---
kernel/trace/trace.c | 3 +++
kernel/trace/trace_events.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 468ecc5..22bf166 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3205,6 +3205,9 @@ int trace_array_printk(struct trace_array *tr,
if (!(global_trace.trace_flags & TRACE_ITER_PRINTK))
return 0;
+ if (!tr)
+ return -ENOENT;
+
va_start(ap, fmt);
ret = trace_array_vprintk(tr, ip, fmt, ap);
va_end(ap);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index b6b4618..99eb5f8 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -800,6 +800,8 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
char *event = NULL, *sub = NULL, *match;
int ret;
+ if (!tr)
+ return -ENOENT;
/*
* The buf format can be <subsystem>:<event-name>
* *:<event-name> means any event by that name.
--
1.8.3.1
Hi Steven,
On 7/29/19 5:51 PM, Steven Rostedt wrote:
> On Mon, 29 Jul 2019 17:02:34 -0700
> Divya Indi <[email protected]> wrote:
>
>> Use "trace_array_set_clr_event" to enable/disable events to a trace
>> array from other kernel modules/components.
>> Hence, we no longer need to have ftrace_set_clr_event as an exported API.
> Hmm, this simply reverts patch 1. Why have patch 1 and this patch in
> the first place?
Right! First patch fixes issues in a previous commit and then this patch
reverts the part of previous commit that required the fix.
We can eliminate the first patch if it seems counter intuitive.
Thanks,
Divya
>
> -- Steve
>
>> Signed-off-by: Divya Indi <[email protected]>
>> Reviewed-By: Aruna Ramakrishna <[email protected]>
On Tue, 30 Jul 2019 15:29:41 -0700
Divya Indi <[email protected]> wrote:
> Hi Steven,
>
> On 7/29/19 5:51 PM, Steven Rostedt wrote:
> > On Mon, 29 Jul 2019 17:02:34 -0700
> > Divya Indi <[email protected]> wrote:
> >
> >> Use "trace_array_set_clr_event" to enable/disable events to a trace
> >> array from other kernel modules/components.
> >> Hence, we no longer need to have ftrace_set_clr_event as an exported API.
> > Hmm, this simply reverts patch 1. Why have patch 1 and this patch in
> > the first place?
>
> Right! First patch fixes issues in a previous commit and then this patch
> reverts the part of previous commit that required the fix.
>
> We can eliminate the first patch if it seems counter intuitive.
>
>
As a stand alone patch, the first one may be fine. But as part of a
series, it doesn't make sense to add it.
-- Steve
On Fri, 2 Aug 2019 13:41:20 -0700
Divya Indi <[email protected]> wrote:
> > As a stand alone patch, the first one may be fine. But as part of a
> > series, it doesn't make sense to add it.
>
> I see. Will separate this out from the series.
Is that really needed? Do you need to have that patch in the kernel?
Do you plan on marking it for stable?
-- Steve
Hi Steve,
The first patch would be like a temporary fix in case we need more
changes to the patches that add the new function - trace_array_set_clr()
and unexport ftrace_set_clr_event() and might take some time. In which
case I think it would be good to have this in place (But, not part of
this series).
If they all are to go in together as part of the same release ie if all
is good with the concerned patches (Patch 6 & Patch 7), then I think
having this patch would be meaningless.
Thanks,
Divya
On 8/2/19 1:46 PM, Steven Rostedt wrote:
> On Fri, 2 Aug 2019 13:41:20 -0700
> Divya Indi <[email protected]> wrote:
>
>>> As a stand alone patch, the first one may be fine. But as part of a
>>> series, it doesn't make sense to add it.
>> I see. Will separate this out from the series.
> Is that really needed? Do you need to have that patch in the kernel?
>
> Do you plan on marking it for stable?
>
> -- Steve
On Fri, 2 Aug 2019 14:12:54 -0700
Divya Indi <[email protected]> wrote:
> Hi Steve,
>
> The first patch would be like a temporary fix in case we need more
> changes to the patches that add the new function - trace_array_set_clr()
> and unexport ftrace_set_clr_event() and might take some time. In which
> case I think it would be good to have this in place (But, not part of
> this series).
>
> If they all are to go in together as part of the same release ie if all
> is good with the concerned patches (Patch 6 & Patch 7), then I think
> having this patch would be meaningless.
Can you just do this part of patch 6 instead?
+/**
+ * trace_array_set_clr_event - enable or disable an event for a trace array
+ * @system: system name to match (NULL for any system)
+ * @event: event name to match (NULL for all events, within system)
+ * @set: 1 to enable, 0 to disable
+ *
+ * This is a way for other parts of the kernel to enable or disable
+ * event recording to instances.
+ *
+ * Returns 0 on success, -EINVAL if the parameters do not match any
+ * registered events.
+ */
+int trace_array_set_clr_event(struct trace_array *tr, const char *system,
+ const char *event, int set)
+{
+ if (!tr)
+ return -ENOENT;
+
+ return __ftrace_set_clr_event(tr, NULL, system, event, set);
+}
+EXPORT_SYMBOL_GPL(trace_array_set_clr_event);
+
-- Steve
Hi Steven,
On 8/2/19 2:25 PM, Steven Rostedt wrote:
> On Fri, 2 Aug 2019 14:12:54 -0700
> Divya Indi <[email protected]> wrote:
>
>> Hi Steve,
>>
>> The first patch would be like a temporary fix in case we need more
>> changes to the patches that add the new function - trace_array_set_clr()
>> and unexport ftrace_set_clr_event() and might take some time. In which
>> case I think it would be good to have this in place (But, not part of
>> this series).
>>
>> If they all are to go in together as part of the same release ie if all
>> is good with the concerned patches (Patch 6 & Patch 7), then I think
>> having this patch would be meaningless.
> Can you just do this part of patch 6 instead?
Yes, will merge the two ie -
1) Add 2 new functions - trace_array_set_clr_event(), trace_array_lookup()
2) Unexport ftrace_set_clr_event.
into a single patch.
Thanks,
Divya
>
> +/**
> + * trace_array_set_clr_event - enable or disable an event for a trace array
> + * @system: system name to match (NULL for any system)
> + * @event: event name to match (NULL for all events, within system)
> + * @set: 1 to enable, 0 to disable
> + *
> + * This is a way for other parts of the kernel to enable or disable
> + * event recording to instances.
> + *
> + * Returns 0 on success, -EINVAL if the parameters do not match any
> + * registered events.
> + */
> +int trace_array_set_clr_event(struct trace_array *tr, const char *system,
> + const char *event, int set)
> +{
> + if (!tr)
> + return -ENOENT;
> +
> + return __ftrace_set_clr_event(tr, NULL, system, event, set);
> +}
> +EXPORT_SYMBOL_GPL(trace_array_set_clr_event);
> +
>
> -- Steve