linux-kernel - Re: [PATCH 6/7] tracing: reset ring buffer when removing modules with events

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 7 May 2009 18:24:18 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Li Zefan <lizf@...fujitsu.com>, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 6/7] tracing: reset ring buffer when removing modules
	with events

On Wed, May 06, 2009 at 11:13:41PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@...hat.com>
> 
> Li Zefan found that there's a race using the event ids of events and
> modules. When a module is loaded, an event id is incremented. We only
> have 16 bits for event ids (65536) and there is a possible (but highly
> unlikely) race that we could load and unload a module that registers
> events so many times that the event id counter overflows.
> 
> When it overflows, it then restarts and goes looking for available
> ids. An id is available if it was added by a module and released.
> 
> The race is if you have one module add an id, and then is removed.
> Another module loaded can use that same event id. But if the old module
> still had events in the ring buffer, the new module's call back would
> get bogus data.  At best (and most likely) the output would just be
> garbage. But if the module for some reason used pointers (not recommended)
> then this could potentially crash.
> 
> The safest thing to do is just reset the ring buffer if a module that
> registered events is removed.
> 
> [ Impact: prevent unpredictable results of event id overflows ]
> 
> Reported-by: Li Zefan <lizf@...fujitsu.com>
> LKML-Reference: <49FEAFD0.30106@...fujitsu.com>
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> ---
>  kernel/trace/trace.c        |   10 ++++++++++
>  kernel/trace/trace.h        |    2 ++
>  kernel/trace/trace_events.c |    9 +++++++++
>  3 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 4164a34..dd40d23 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -639,6 +639,16 @@ void tracing_reset_online_cpus(struct trace_array *tr)
>  		tracing_reset(tr, cpu);
>  }
>  
> +void tracing_reset_current(int cpu)
> +{
> +	tracing_reset(&global_trace, cpu);
> +}
> +
> +void tracing_reset_current_online_cpus(void)
> +{
> +	tracing_reset_online_cpus(&global_trace);
> +}
> +
>  #define SAVED_CMDLINES 128
>  #define NO_CMDLINE_MAP UINT_MAX
>  static unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 777c6c3..ba25793 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -409,6 +409,8 @@ int tracing_is_enabled(void);
>  void trace_wake_up(void);
>  void tracing_reset(struct trace_array *tr, int cpu);
>  void tracing_reset_online_cpus(struct trace_array *tr);
> +void tracing_reset_current(int cpu);
> +void tracing_reset_current_online_cpus(void);
>  int tracing_open_generic(struct inode *inode, struct file *filp);
>  struct dentry *trace_create_file(const char *name,
>  				 mode_t mode,
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 8d579ff..6d2c842 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -932,9 +932,11 @@ static void trace_module_remove_events(struct module *mod)
>  {
>  	struct ftrace_module_file_ops *file_ops;
>  	struct ftrace_event_call *call, *p;
> +	bool found = false;
>  
>  	list_for_each_entry_safe(call, p, &ftrace_events, list) {
>  		if (call->mod == mod) {
> +			found = true;
>  			if (call->enabled) {
>  				call->enabled = 0;
>  				call->unregfunc();
> @@ -957,6 +959,13 @@ static void trace_module_remove_events(struct module *mod)
>  		list_del(&file_ops->list);
>  		kfree(file_ops);
>  	}
> +
> +	/*
> +	 * It is safest to reset the ring buffer if the module being unloaded
> +	 * registered any events.
> +	 */
> +	if (found)
> +		tracing_reset_current_online_cpus();
>  }



I would suggest to send a single trace_printk message after that
which says:

# current trace buffer content deleted due to module %s unloading

So that no one would end up being stucked while searching why tracing
during module unloading (eg: a module_exit callback of a driver)
never produce any trace :-)

Also there is still a narrow race:

tracing_read_pipe() {
  get_entry_from_buffer();
  tracer_output_callback() {
    ...				              module_unloading()
    dereference_freed_pointer_from_entry()

But at least this patch fixes most of the race window.

Frederic.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/