| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZXG7xAXkKvQkpaRn@google.com>
Date: Thu, 7 Dec 2023 13:34:12 +0100
From: Dmytro Maluka <dmaluka@...omium.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Sean Paul <seanpaul@...omium.org>,
Arun Easi <aeasi@...vell.com>, Daniel Wagner <dwagner@...e.de>
Subject: Re: [PATCH v2] tracing: Allow creating instances with specified
system events
On Tue, Dec 05, 2023 at 09:18:36PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
>
> A trace instance may only need to enable specific events. As the eventfs
> directory of an instance currently creates all events which adds overhead,
> allow internal instances to be created with just the events in systems
> that they care about. This currently only deals with systems and not
> individual events, but this should bring down the overhead of creating
> instances for specific use cases quite bit.
>
> The trace_array_get_by_name() now has another parameter "systems". This
> parameter is a const string pointer of a comma/space separated list of
> event systems that should be created by the trace_array. (Note if the
> trace_array already exists, this parameter is ignored).
>
> The list of systems is saved and if a module is loaded, its events will
> not be added unless the system for those events also match the systems
> string.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
Tested-by: Dmytro Maluka <dmaluka@...omium.org>
> Changes since v1: https://lore.kernel.org/all/20231128122117.2276f4a7@gandalf.local.home/
>
> - Made it so that dynamic events need to be specified in order to add
> to the instance. They are no longer automatically allowed.
>
> drivers/scsi/qla2xxx/qla_os.c | 2 +-
> include/linux/trace.h | 4 +--
> kernel/trace/trace.c | 23 +++++++++++---
> kernel/trace/trace.h | 1 +
> kernel/trace/trace_boot.c | 2 +-
> kernel/trace/trace_events.c | 48 +++++++++++++++++++++++++++--
> samples/ftrace/sample-trace-array.c | 2 +-
> 7 files changed, 70 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 03348f605c2e..dd674378f2f3 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -2889,7 +2889,7 @@ static void qla2x00_iocb_work_fn(struct work_struct *work)
> static void
> qla_trace_init(void)
> {
> - qla_trc_array = trace_array_get_by_name("qla2xxx");
> + qla_trc_array = trace_array_get_by_name("qla2xxx", NULL);
> if (!qla_trc_array) {
> ql_log(ql_log_fatal, NULL, 0x0001,
> "Unable to create qla2xxx trace instance, instance logging will be disabled.\n");
> diff --git a/include/linux/trace.h b/include/linux/trace.h
> index 2a70a447184c..fdcd76b7be83 100644
> --- a/include/linux/trace.h
> +++ b/include/linux/trace.h
> @@ -51,7 +51,7 @@ int trace_array_printk(struct trace_array *tr, unsigned long ip,
> const char *fmt, ...);
> int trace_array_init_printk(struct trace_array *tr);
> void trace_array_put(struct trace_array *tr);
> -struct trace_array *trace_array_get_by_name(const char *name);
> +struct trace_array *trace_array_get_by_name(const char *name, const char *systems);
> int trace_array_destroy(struct trace_array *tr);
>
> /* For osnoise tracer */
> @@ -84,7 +84,7 @@ static inline int trace_array_init_printk(struct trace_array *tr)
> static inline void trace_array_put(struct trace_array *tr)
> {
> }
> -static inline struct trace_array *trace_array_get_by_name(const char *name)
> +static inline struct trace_array *trace_array_get_by_name(const char *name, const char *systems)
> {
> return NULL;
> }
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index fbcd3bafb93e..31fb03d105e9 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -9480,7 +9480,8 @@ static int trace_array_create_dir(struct trace_array *tr)
> return ret;
> }
>
> -static struct trace_array *trace_array_create(const char *name)
> +static struct trace_array *
> +trace_array_create_systems(const char *name, const char *systems)
> {
> struct trace_array *tr;
> int ret;
> @@ -9500,6 +9501,12 @@ static struct trace_array *trace_array_create(const char *name)
> if (!zalloc_cpumask_var(&tr->pipe_cpumask, GFP_KERNEL))
> goto out_free_tr;
>
> + if (systems) {
> + tr->system_names = kstrdup_const(systems, GFP_KERNEL);
> + if (!tr->system_names)
> + goto out_free_tr;
> + }
> +
> tr->trace_flags = global_trace.trace_flags & ~ZEROED_TRACE_FLAGS;
>
> cpumask_copy(tr->tracing_cpumask, cpu_all_mask);
> @@ -9546,12 +9553,18 @@ static struct trace_array *trace_array_create(const char *name)
> free_trace_buffers(tr);
> free_cpumask_var(tr->pipe_cpumask);
> free_cpumask_var(tr->tracing_cpumask);
> + kfree_const(tr->system_names);
> kfree(tr->name);
> kfree(tr);
>
> return ERR_PTR(ret);
> }
>
> +static struct trace_array *trace_array_create(const char *name)
> +{
> + return trace_array_create_systems(name, NULL);
> +}
> +
> static int instance_mkdir(const char *name)
> {
> struct trace_array *tr;
> @@ -9577,6 +9590,7 @@ static int instance_mkdir(const char *name)
> /**
> * trace_array_get_by_name - Create/Lookup a trace array, given its name.
> * @name: The name of the trace array to be looked up/created.
> + * @system: A list of systems to create event directories for (NULL for all)
> *
> * Returns pointer to trace array with given name.
> * NULL, if it cannot be created.
> @@ -9590,7 +9604,7 @@ static int instance_mkdir(const char *name)
> * trace_array_put() is called, user space can not delete it.
> *
> */
> -struct trace_array *trace_array_get_by_name(const char *name)
> +struct trace_array *trace_array_get_by_name(const char *name, const char *systems)
> {
> struct trace_array *tr;
>
> @@ -9602,7 +9616,7 @@ struct trace_array *trace_array_get_by_name(const char *name)
> goto out_unlock;
> }
>
> - tr = trace_array_create(name);
> + tr = trace_array_create_systems(name, systems);
>
> if (IS_ERR(tr))
> tr = NULL;
> @@ -9649,6 +9663,7 @@ static int __remove_instance(struct trace_array *tr)
>
> free_cpumask_var(tr->pipe_cpumask);
> free_cpumask_var(tr->tracing_cpumask);
> + kfree_const(tr->system_names);
> kfree(tr->name);
> kfree(tr);
>
> @@ -10367,7 +10382,7 @@ __init static void enable_instances(void)
> if (IS_ENABLED(CONFIG_TRACER_MAX_TRACE))
> do_allocate_snapshot(tok);
>
> - tr = trace_array_get_by_name(tok);
> + tr = trace_array_get_by_name(tok, NULL);
> if (!tr) {
> pr_warn("Failed to create instance buffer %s\n", curr_str);
> continue;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index b7f4ea25a194..97b01dfd7070 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -377,6 +377,7 @@ struct trace_array {
> unsigned char trace_flags_index[TRACE_FLAGS_MAX_SIZE];
> unsigned int flags;
> raw_spinlock_t start_lock;
> + const char *system_names;
> struct list_head err_log;
> struct dentry *dir;
> struct dentry *options;
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index 7ccc7a8e155b..dbe29b4c6a7a 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -633,7 +633,7 @@ trace_boot_init_instances(struct xbc_node *node)
> if (!p || *p == '\0')
> continue;
>
> - tr = trace_array_get_by_name(p);
> + tr = trace_array_get_by_name(p, NULL);
> if (!tr) {
> pr_err("Failed to get trace instance %s\n", p);
> continue;
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index f29e815ca5b2..b70d03818038 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2896,6 +2896,27 @@ void trace_event_eval_update(struct trace_eval_map **map, int len)
> up_write(&trace_event_sem);
> }
>
> +static bool event_in_systems(struct trace_event_call *call,
> + const char *systems)
> +{
> + const char *system;
> + const char *p;
> +
> + if (!systems)
> + return true;
> +
> + system = call->class->system;
> + p = strstr(systems, system);
> + if (!p)
> + return false;
> +
> + if (p != systems && !isspace(*(p - 1)) && *(p - 1) != ',')
> + return false;
> +
> + p += strlen(system);
> + return !*p || isspace(*p) || *p == ',';
> +}
> +
> static struct trace_event_file *
> trace_create_new_event(struct trace_event_call *call,
> struct trace_array *tr)
> @@ -2905,9 +2926,12 @@ trace_create_new_event(struct trace_event_call *call,
> struct trace_event_file *file;
> unsigned int first;
>
> + if (!event_in_systems(call, tr->system_names))
> + return NULL;
> +
> file = kmem_cache_alloc(file_cachep, GFP_TRACE);
> if (!file)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> pid_list = rcu_dereference_protected(tr->filtered_pids,
> lockdep_is_held(&event_mutex));
> @@ -2972,8 +2996,17 @@ __trace_add_new_event(struct trace_event_call *call, struct trace_array *tr)
> struct trace_event_file *file;
>
> file = trace_create_new_event(call, tr);
> + /*
> + * trace_create_new_event() returns ERR_PTR(-ENOMEM) if failed
> + * allocation, or NULL if the event is not part of the tr->system_names.
> + * When the event is not part of the tr->system_names, return zero, not
> + * an error.
> + */
> if (!file)
> - return -ENOMEM;
> + return 0;
> +
> + if (IS_ERR(file))
> + return PTR_ERR(file);
>
> if (eventdir_initialized)
> return event_create_dir(tr->event_dir, file);
> @@ -3012,8 +3045,17 @@ __trace_early_add_new_event(struct trace_event_call *call,
> int ret;
>
> file = trace_create_new_event(call, tr);
> + /*
> + * trace_create_new_event() returns ERR_PTR(-ENOMEM) if failed
> + * allocation, or NULL if the event is not part of the tr->system_names.
> + * When the event is not part of the tr->system_names, return zero, not
> + * an error.
> + */
> if (!file)
> - return -ENOMEM;
> + return 0;
> +
> + if (IS_ERR(file))
> + return PTR_ERR(file);
>
> ret = event_define_fields(call);
> if (ret)
> diff --git a/samples/ftrace/sample-trace-array.c b/samples/ftrace/sample-trace-array.c
> index 6aba02a31c96..d0ee9001c7b3 100644
> --- a/samples/ftrace/sample-trace-array.c
> +++ b/samples/ftrace/sample-trace-array.c
> @@ -105,7 +105,7 @@ static int __init sample_trace_array_init(void)
> * NOTE: This function increments the reference counter
> * associated with the trace array - "tr".
> */
> - tr = trace_array_get_by_name("sample-instance");
> + tr = trace_array_get_by_name("sample-instance", "sched,timer,kprobes");
>
> if (!tr)
> return -1;
> --
> 2.42.0
>
Powered by blists - more mailing lists