tracing: More updates for 6.1
Subsystems (6)
Steven Rostedt
maintainercover tracing: More updates for 6.1
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next
Head SHA1: 933678b6183bbe7afa332e70132065db3305ee44
Beau Belgrave (6):
tracing/user_events: Use NULL for strstr checks tracing/user_events: Use WRITE instead of READ for io vector import tracing/user_events: Ensure user provided strings are safely formatted tracing/user_events: Use refcount instead of atomic for ref tracking tracing/user_events: Use bits vs bytes for enabled status page data tracing/user_events: Update ABI documentation to align to bits vs bytes
Colin Ian King (1):
tracing: Fix spelling mistake "preapre" -> "prepare"
Song Liu (1):
ftrace: Fix recursive locking direct_mutex in ftrace_modify_direct_caller
Steven Rostedt (Google) (7):
ring-buffer: Allow splice to read previous partially read pages
ring-buffer: Have the shortest_full queue be the shortest not longest
ring-buffer: Check pending waiters when doing wake ups as well
ring-buffer: Add ring_buffer_wake_waiters()
tracing: Wake up ring buffer waiters on closing of the file
tracing: Add ioctl() to force ring buffer waiters to wake up
tracing: Wake up waiters when tracing is disabled
----
Documentation/trace/user_events.rst | 86 +++++--- include/linux/ring_buffer.h | 2 +- include/linux/trace_events.h | 1 + include/linux/user_events.h | 15 +- kernel/trace/ftrace.c | 6 +- kernel/trace/ring_buffer.c | 54 ++++- kernel/trace/trace.c | 45 ++++- kernel/trace/trace_events_user.c | 228 +++++++++++++++------- samples/user_events/example.c | 25 ++- tools/testing/selftests/user_events/ftrace_test.c | 47 ++++- tools/testing/selftests/user_events/perf_test.c | 11 +- 11 files changed, 383 insertions(+), 137 deletions(-)
Steven Rostedt
maintainerPATCH 1/15 ftrace: Fix recursive locking direct_mutex in ftrace_modify_direct_caller
From: Song Liu <[email protected]>
Naveen reported recursive locking of direct_mutex with sample ftrace-direct-modify.ko:
[ 74.762406] WARNING: possible recursive locking detected
[ 74.762887] 6.0.0-rc6+ #33 Not tainted
[ 74.763216] --------------------------------------------
[ 74.763672] event-sample-fn/1084 is trying to acquire lock:
[ 74.764152] ffffffff86c9d6b0 (direct_mutex){+.+.}-{3:3}, at: \
register_ftrace_function+0x1f/0x180
[ 74.764922]
[ 74.764922] but task is already holding lock:
[ 74.765421] ffffffff86c9d6b0 (direct_mutex){+.+.}-{3:3}, at: \
modify_ftrace_direct+0x34/0x1f0
[ 74.766142]
[ 74.766142] other info that might help us debug this:
[ 74.766701] Possible unsafe locking scenario:
[ 74.766701]
[ 74.767216] CPU0
[ 74.767437] ----
[ 74.767656] lock(direct_mutex);
[ 74.767952] lock(direct_mutex);
[ 74.768245]
[ 74.768245] *** DEADLOCK ***
[ 74.768245]
[ 74.768750] May be due to missing lock nesting notation
[ 74.768750]
[ 74.769332] 1 lock held by event-sample-fn/1084:
[ 74.769731] #0: ffffffff86c9d6b0 (direct_mutex){+.+.}-{3:3}, at: \
modify_ftrace_direct+0x34/0x1f0
[ 74.770496]
[ 74.770496] stack backtrace:
[ 74.770884] CPU: 4 PID: 1084 Comm: event-sample-fn Not tainted ...
[ 74.771498] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), ...
[ 74.772474] Call Trace:
[ 74.772696] <TASK>
[ 74.772896] dump_stack_lvl+0x44/0x5b
[ 74.773223] __lock_acquire.cold.74+0xac/0x2b7
[ 74.773616] lock_acquire+0xd2/0x310
[ 74.773936] ? register_ftrace_function+0x1f/0x180
[ 74.774357] ? lock_is_held_type+0xd8/0x130
[ 74.774744] ? my_tramp2+0x11/0x11 [ftrace_direct_modify]
[ 74.775213] __mutex_lock+0x99/0x1010
[ 74.775536] ? register_ftrace_function+0x1f/0x180
[ 74.775954] ? slab_free_freelist_hook.isra.43+0x115/0x160
[ 74.776424] ? ftrace_set_hash+0x195/0x220
[ 74.776779] ? register_ftrace_function+0x1f/0x180
[ 74.777194] ? kfree+0x3e1/0x440
[ 74.777482] ? my_tramp2+0x11/0x11 [ftrace_direct_modify]
[ 74.777941] ? __schedule+0xb40/0xb40
[ 74.778258] ? register_ftrace_function+0x1f/0x180
[ 74.778672] ? my_tramp1+0xf/0xf [ftrace_direct_modify]
[ 74.779128] register_ftrace_function+0x1f/0x180
[ 74.779527] ? ftrace_set_filter_ip+0x33/0x70
[ 74.779910] ? __schedule+0xb40/0xb40
[ 74.780231] ? my_tramp1+0xf/0xf [ftrace_direct_modify]
[ 74.780678] ? my_tramp2+0x11/0x11 [ftrace_direct_modify]
[ 74.781147] ftrace_modify_direct_caller+0x5b/0x90
[ 74.781563] ? 0xffffffffa0201000
[ 74.781859] ? my_tramp1+0xf/0xf [ftrace_direct_modify]
[ 74.782309] modify_ftrace_direct+0x1b2/0x1f0
[ 74.782690] ? __schedule+0xb40/0xb40
[ 74.783014] ? simple_thread+0x2a/0xb0 [ftrace_direct_modify]
[ 74.783508] ? __schedule+0xb40/0xb40
[ 74.783832] ? my_tramp2+0x11/0x11 [ftrace_direct_modify]
[ 74.784294] simple_thread+0x76/0xb0 [ftrace_direct_modify]
[ 74.784766] kthread+0xf5/0x120
[ 74.785052] ? kthread_complete_and_exit+0x20/0x20
[ 74.785464] ret_from_fork+0x22/0x30
[ 74.785781] </TASK>Fix this by using register_ftrace_function_nolock in ftrace_modify_direct_caller.
--- kernel/trace/ftrace.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
kernel/trace/ftrace.c +5 -1
@@ -5427,6 +5427,8 @@ static struct ftrace_ops stub_ops = {* it is safe to modify the ftrace record, where it should be * currently calling @old_addr directly, to call @new_addr. * +* This is called with direct_mutex locked. +* * Safety checks should be made to make sure that the code at * @rec->ip is currently calling @old_addr. And this must * also update entry->direct to @new_addr.
@@ -5439,6 +5441,8 @@ int __weak ftrace_modify_direct_caller(struct ftrace_func_entry *entry,unsigned long ip = rec->ip;int ret; +lockdep_assert_held (&direct_mutex); +/* * The ftrace_lock was used to determine if the record * had more than one registered user to it. If it did,
@@ -5461,7 +5465,7 @@ int __weak ftrace_modify_direct_caller(struct ftrace_func_entry *entry,if (ret) goto out_lock; - ret =register_ftrace_function (&stub_ops); + ret =register_ftrace_function_nolock (&stub_ops);if (ret) {ftrace_set_filter_ip (&stub_ops, ip,1 ,0 ); goto out_lock;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 2/15 ring-buffer: Allow splice to read previous partially read pages
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
If a page is partially read, and then the splice system call is run against the ring buffer, it will always fail to read, no matter how much is in the ring buffer. That's because the code path for a partial read of the page does will fail if the "full" flag is set.
The splice system call wants full pages, so if the read of the ring buffer is not yet full, it should return zero, and the splice will block. But if a previous read was done, where the beginning has been consumed, it should still be given to the splice caller if the rest of the page has been written to.
This caused the splice command to never consume data in this scenario, and let the ring buffer just fill up and lose events.
--- kernel/trace/ring_buffer.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
kernel/trace/ring_buffer.c +9 -1
@@ -5616,7 +5616,15 @@ int ring_buffer_read_page(struct trace_buffer *buffer,unsigned int pos =0 ;unsigned int size; -if (full) +/* +* If a full page is expected, this can still be returned +* if there's been a previous partial read and the +* rest of the page can be read and the commit page is off +* the reader page. +*/ +if (full && + (!read || (len < (commit - read)) || + cpu_buffer->reader_page == cpu_buffer->commit_page)) goto out_unlock;if (len > (commit - read))
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 3/15 ring-buffer: Have the shortest_full queue be the shortest not longest
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
The logic to know when the shortest waiters on the ring buffer should be woken up or not has uses a less than instead of a greater than compare, which causes the shortest_full to actually be the longest.
--- kernel/trace/ring_buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/ring_buffer.c +1 -1
@@ -1011,7 +1011,7 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full) nr_pages = cpu_buffer->nr_pages; dirty =ring_buffer_nr_dirty_pages (buffer, cpu);if (!cpu_buffer->shortest_full || - cpu_buffer->shortest_full < full) + cpu_buffer->shortest_full > full) cpu_buffer->shortest_full = full;raw_spin_unlock_irqrestore (&cpu_buffer->reader_lock, flags); if (!pagebusy &&
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 4/15 ring-buffer: Check pending waiters when doing wake ups as well
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
The wake up waiters only checks the "wakeup_full" variable and not the "full_waiters_pending". The full_waiters_pending is set when a waiter is added to the wait queue. The wakeup_full is only set when an event is triggered, and it clears the full_waiters_pending to avoid multiple calls to irq_work_queue().
The irq_work callback really needs to check both wakeup_full as well as full_waiters_pending such that this code can be used to wake up waiters when a file is closed that represents the ring buffer and the waiters need to be woken up.
--- kernel/trace/ring_buffer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/trace/ring_buffer.c +2 -1
@@ -917,8 +917,9 @@ static void rb_wake_up_waiters(struct irq_work *work) struct rb_irq_work *rbwork = container_of(work, struct rb_irq_work, work);wake_up_all (&rbwork->waiters); -if (rbwork->wakeup_full) { +if (rbwork->full_waiters_pending || rbwork->wakeup_full) { rbwork->wakeup_full = false; + rbwork->full_waiters_pending = false;wake_up_all (&rbwork->full_waiters); } }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 5/15 ring-buffer: Add ring_buffer_wake_waiters()
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
On closing of a file that represents a ring buffer or flushing the file, there may be waiters on the ring buffer that needs to be woken up and exit the ring_buffer_wait() function.
Add ring_buffer_wake_waiters() to wake up the waiters on the ring buffer and allow them to exit the wait loop.
--- include/linux/ring_buffer.h | 2 +- kernel/trace/ring_buffer.c | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-)
include/linux/ring_buffer.h +1 -1
@@ -101,7 +101,7 @@ __ring_buffer_alloc(unsigned long size, unsigned flags, struct lock_class_key *kint ring_buffer_wait (struct trace_buffer *buffer,int cpu,int full);__poll_t ring_buffer_poll_wait (struct trace_buffer *buffer,int cpu,struct file *filp,poll_table *poll_table); - +void ring_buffer_wake_waiters (struct trace_buffer *buffer,int cpu);#define RING_BUFFER_ALL_CPUS -1
kernel/trace/ring_buffer.c +39 -0
@@ -413,6 +413,7 @@ struct rb_irq_work {struct irq_work work;wait_queue_head_t waiters;wait_queue_head_t full_waiters; +long wait_index;bool waiters_pending;bool full_waiters_pending;bool wakeup_full;
@@ -924,6 +925,37 @@ static void rb_wake_up_waiters(struct irq_work *work) } } +/** +* ring_buffer_wake_waiters - wake up any waiters on this ring buffer +* @buffer: The ring buffer to wake waiters on +* +* In the case of a file that represents a ring buffer is closing, +* it is prudent to wake up any waiters that are on this. +*/ +void ring_buffer_wake_waiters (struct trace_buffer *buffer,int cpu) +{ +struct ring_buffer_per_cpu *cpu_buffer; +struct rb_irq_work *rbwork; + +if (cpu ==RING_BUFFER_ALL_CPUS ) { + +/* Wake up individual ones too. One level recursion */ +for_each_buffer_cpu (buffer, cpu) +ring_buffer_wake_waiters (buffer, cpu); + + rbwork = &buffer->irq_work; + }else { + cpu_buffer = buffer->buffers[cpu]; + rbwork = &cpu_buffer->irq_work; + } + + rbwork->wait_index++; +/* make sure the waiters see the new index */ +smp_wmb (); + +rb_wake_up_waiters (&rbwork->work); +} +/** * ring_buffer_wait - wait for input to the ring buffer * @buffer: buffer to wait on
@@ -939,6 +971,7 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full)struct ring_buffer_per_cpu *cpu_buffer;DEFINE_WAIT (wait);struct rb_irq_work *work; +long wait_index;int ret =0 ;/*
@@ -957,6 +990,7 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full) work = &cpu_buffer->irq_work; } + wait_index =READ_ONCE (work->wait_index);while (true) {if (full)
@@ -1021,6 +1055,11 @@ int ring_buffer_wait(struct trace_buffer *buffer, int cpu, int full) }schedule (); + +/* Make sure to see the new wait index */ +smp_rmb (); +if (wait_index != work->wait_index) +break ; }if (full)
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 6/15 tracing: Wake up ring buffer waiters on closing of the file
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
When the file that represents the ring buffer is closed, there may be waiters waiting on more input from the ring buffer. Call ring_buffer_wake_waiters() to wake up any waiters when the file is closed.
--- include/linux/trace_events.h | 1 + kernel/trace/trace.c | 15 +++++++++++++++ 2 files changed, 16 insertions(+)
include/linux/trace_events.h +1 -0
@@ -92,6 +92,7 @@ struct trace_iterator {unsigned int temp_size;char *fmt;/* modified format holder */ unsigned int fmt_size; +long wait_index;/* trace_seq for __print_flags() and __print_symbolic() etc. */ struct trace_seq tmp_seq;
kernel/trace/trace.c +15 -0
@@ -8160,6 +8160,12 @@ static int tracing_buffers_release(struct inode *inode, struct file *file)__trace_array_put (iter->tr); + iter->wait_index++; +/* Make sure the waiters see the new wait_index */ +smp_wmb (); + +ring_buffer_wake_waiters (iter->array_buffer->buffer, iter->cpu_file); +if (info->spare)ring_buffer_free_read_page (iter->array_buffer->buffer, info->spare_cpu, info->spare);
@@ -8313,6 +8319,8 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,/* did we read anything? */ if (!spd.nr_pages) { +long wait_index; +if (ret) goto out;
@@ -8320,10 +8328,17 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,if ((file->f_flags &O_NONBLOCK ) || (flags &SPLICE_F_NONBLOCK )) goto out; + wait_index =READ_ONCE (iter->wait_index); + ret =wait_on_pipe (iter, iter->tr->buffer_percent);if (ret) goto out; +/* Make sure we see the new wait_index */ +smp_rmb (); +if (wait_index != iter->wait_index) + goto out; + goto again; }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 7/15 tracing: Add ioctl() to force ring buffer waiters to wake up
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
If a process is waiting on the ring buffer for data, there currently isn't a clean way to force it to wake up. Add an ioctl call that will force any tasks that are waiting on the trace_pipe_raw file to wake up.
--- kernel/trace/trace.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
kernel/trace/trace.c +22 -0
@@ -8349,12 +8349,34 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,return ret; } +/* An ioctl call with cmd 0 to the ring buffer file will wake up all waiters */ +static long tracing_buffers_ioctl (struct file *file,unsigned int cmd,unsigned long arg) +{ +struct ftrace_buffer_info *info = file->private_data; +struct trace_iterator *iter = &info->iter; + +if (cmd) +return -ENOIOCTLCMD ; + +mutex_lock (&trace_types_lock); + + iter->wait_index++; +/* Make sure the waiters see the new wait_index */ +smp_wmb (); + +ring_buffer_wake_waiters (iter->array_buffer->buffer, iter->cpu_file); + +mutex_unlock (&trace_types_lock); +return 0 ; +} +static const struct file_operations tracing_buffers_fops = { .open = tracing_buffers_open, .read = tracing_buffers_read, .poll = tracing_buffers_poll, .release = tracing_buffers_release, .splice_read = tracing_buffers_splice_read, + .unlocked_ioctl = tracing_buffers_ioctl, .llseek = no_llseek, };
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 8/15 tracing: Wake up waiters when tracing is disabled
In reply to Steven RostedtFrom: "Steven Rostedt (Google)" <[email protected]>
When tracing is disabled, there's no reason that waiters should stay waiting, wake them up, otherwise tasks get stuck when they should be flushing the buffers.
--- kernel/trace/trace.c | 6 ++++++ 1 file changed, 6 insertions(+)
kernel/trace/trace.c +6 -0
@@ -8334,6 +8334,10 @@ tracing_buffers_splice_read(struct file *file, loff_t *ppos,if (ret) goto out; +/* No need to wait after waking up when tracing is off */ +if (!tracer_tracing_is_on (iter->tr)) + goto out; +/* Make sure we see the new wait_index */ smp_rmb ();if (wait_index != iter->wait_index)
@@ -9065,6 +9069,8 @@ rb_simple_write(struct file *filp, const char __user *ubuf,tracer_tracing_off (tr);if (tr->current_trace->stop) tr->current_trace->stop (tr); +/* Wake up any waiters */ +ring_buffer_wake_waiters (buffer,RING_BUFFER_ALL_CPUS ); }mutex_unlock (&trace_types_lock); }
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 9/15 tracing: Fix spelling mistake "preapre" -> "prepare"
In reply to Steven RostedtFrom: Colin Ian King <[email protected]>
There is a spelling mistake in the trace text. Fix it.
--- kernel/trace/trace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace.c +1 -1
@@ -10157,7 +10157,7 @@ __init static int tracer_alloc_buffers(void)* buffer. The memory will be removed once the "instance" is removed. */ ret =cpuhp_setup_state_multi (CPUHP_TRACE_RB_PREPARE , -"trace/RB:preapre" , trace_rb_cpu_prepare, +"trace/RB:prepare" , trace_rb_cpu_prepare,NULL );if (ret <0 ) goto out_free_cpumask;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 10/15 tracing/user_events: Use NULL for strstr checks
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
Trivial fix to ensure strstr checks use NULL instead of 0.
--- kernel/trace/trace_events_user.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/trace/trace_events_user.c +3 -3
@@ -277,7 +277,7 @@ static int user_event_add_field(struct user_event *user, const char *type, goto add_field; add_validator: -if (strstr (type,"char" ) !=0 ) +if (strstr (type,"char" ) !=NULL ) validator_flags |=VALIDATOR_ENSURE_NULL ; validator =kmalloc (sizeof (*validator),GFP_KERNEL );
@@ -458,7 +458,7 @@ static const char *user_field_format(const char *type)return "%d" ;if (strcmp (type,"unsigned char" ) ==0 )return "%u" ; -if (strstr (type,"char[" ) !=0 ) +if (strstr (type,"char[" ) !=NULL )return "%s" ;/* Unknown, likely struct, allowed treat as 64-bit */
@@ -479,7 +479,7 @@ static bool user_field_is_dyn_string(const char *type, const char **str_func)return false; check: -return strstr (type,"char" ) !=0 ; +return strstr (type,"char" ) !=NULL ; }#define LEN_OR_ZERO (len ? len - pos : 0)
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 11/15 tracing/user_events: Use WRITE instead of READ for io vector import
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
import_single_range expects the direction/rw to be where it came from, not the protection/limit. Since the import is in a write path use WRITE.
--- kernel/trace/trace_events_user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/trace/trace_events_user.c +2 -1
@@ -1245,7 +1245,8 @@ static ssize_t user_events_write(struct file *file, const char __user *ubuf,if (unlikely (*ppos !=0 ))return -EFAULT ; -if (unlikely (import_single_range (READ , (char *)ubuf, count, &iov, &i))) +if (unlikely (import_single_range (WRITE , (char __user *)ubuf, + count, &iov, &i)))return -EFAULT ;return user_events_write_core (file, &i);
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 12/15 tracing/user_events: Ensure user provided strings are safely formatted
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
User processes can provide bad strings that may cause issues or leak kernel details back out. Don't trust the content of these strings when formatting strings for matching.
This also moves to a consistent dynamic length string creation model.
--- kernel/trace/trace_events_user.c | 91 +++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 32 deletions(-)
kernel/trace/trace_events_user.c +59 -32
@@ -45,7 +45,6 @@#define MAX_EVENT_DESC 512#define EVENT_NAME (user_event) ((user_event)->tracepoint.name)#define MAX_FIELD_ARRAY_SIZE 1024 -#define MAX_FIELD_ARG_NAME 256static char *register_page_data;
@@ -483,6 +482,48 @@ static bool user_field_is_dyn_string(const char *type, const char **str_func) }#define LEN_OR_ZERO (len ? len - pos : 0) +static int user_dyn_field_set_string (int argc,const char **argv,int *iout, +char *buf,int len,bool *colon) +{ +int pos =0 , i = *iout; + + *colon = false; + +for (; i < argc; ++i) { +if (i != *iout) + pos +=snprintf (buf + pos,LEN_OR_ZERO ," " ); + + pos +=snprintf (buf + pos,LEN_OR_ZERO ,"%s" , argv[i]); + +if (strchr (argv[i],';' )) { + ++i; + *colon = true; +break ; + } + } + +/* Actual set, advance i */ +if (len !=0 ) + *iout = i; + +return pos +1 ; +} + +static int user_field_set_string (struct ftrace_event_field *field, +char *buf,int len,bool colon) +{ +int pos =0 ; + + pos +=snprintf (buf + pos,LEN_OR_ZERO ,"%s" , field->type); + pos +=snprintf (buf + pos,LEN_OR_ZERO ," " ); + pos +=snprintf (buf + pos,LEN_OR_ZERO ,"%s" , field->name); + +if (colon) + pos +=snprintf (buf + pos,LEN_OR_ZERO ,";" ); + +return pos +1 ; +} +static int user_event_set_print_fmt (struct user_event *user,char *buf,int len) {struct ftrace_event_field *field, *next;
@@ -926,49 +967,35 @@ static int user_event_free(struct dyn_event *ev)static bool user_field_match (struct ftrace_event_field *field,int argc,const char **argv,int *iout) { -char *field_name, *arg_name; -int len, pos, i = *iout; +char *field_name =NULL , *dyn_field_name =NULL ;bool colon = false, match = false; +int dyn_len, len; -if (i >= argc) +if (*iout >= argc)return false; - len =MAX_FIELD_ARG_NAME ; - field_name =kmalloc (len,GFP_KERNEL ); - arg_name =kmalloc (len,GFP_KERNEL ); + dyn_len =user_dyn_field_set_string (argc, argv, iout, dyn_field_name, +0 , &colon); -if (!arg_name || !field_name) - goto out; - - pos =0 ; - -for (; i < argc; ++i) { -if (i != *iout) - pos +=snprintf (arg_name + pos, len - pos," " ); + len =user_field_set_string (field, field_name,0 , colon); - pos +=snprintf (arg_name + pos, len - pos, argv[i]); - -if (strchr (argv[i],';' )) { - ++i; - colon = true; -break ; - } - } +if (dyn_len != len) +return false; - pos =0 ; + dyn_field_name =kmalloc (dyn_len,GFP_KERNEL ); + field_name =kmalloc (len,GFP_KERNEL ); - pos +=snprintf (field_name + pos, len - pos, field->type); - pos +=snprintf (field_name + pos, len - pos," " ); - pos +=snprintf (field_name + pos, len - pos, field->name); +if (!dyn_field_name || !field_name) + goto out; -if (colon) - pos +=snprintf (field_name + pos, len - pos,";" ); +user_dyn_field_set_string (argc, argv, iout, dyn_field_name, + dyn_len, &colon); - *iout = i; +user_field_set_string (field, field_name, len, colon); - match =strcmp (arg_name, field_name) ==0 ; + match =strcmp (dyn_field_name, field_name) ==0 ; out: -kfree (arg_name); +kfree (dyn_field_name);kfree (field_name);return match;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 13/15 tracing/user_events: Use refcount instead of atomic for ref tracking
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
User processes could open up enough event references to cause rollovers. These could cause use after free scenarios, which we do not want. Switching to refcount APIs prevent this, but will leak memory once saturated.
Once saturated, user processes can still use the events. This prevents a bad user process from stopping existing telemetry from being emitted.
--- kernel/trace/trace_events_user.c | 53 +++++++++++++++----------------- 1 file changed, 24 insertions(+), 29 deletions(-)
kernel/trace/trace_events_user.c +24 -29
@@ -14,6 +14,7 @@#include <linux/uio.h> #include <linux/ioctl.h> #include <linux/jhash.h> +#include <linux/refcount.h> #include <linux/trace_events.h> #include <linux/tracefs.h> #include <linux/types.h>
@@ -57,7 +58,7 @@ static DECLARE_BITMAP(page_bitmap, MAX_EVENTS);* within a file a user_event might be created if it does not * already exist. These are globally used and their lifetime * is tied to the refcnt member. These cannot go away until the -* refcnt reaches zero. +* refcnt reaches one. */ struct user_event {struct tracepoint tracepoint;
@@ -67,7 +68,7 @@ struct user_event {struct hlist_node node;struct list_head fields;struct list_head validators; -atomic_t refcnt; +refcount_t refcnt;int index;int flags;int min_size;
@@ -105,6 +106,12 @@ static u32 user_event_key(char *name)return jhash (name,strlen (name),0 ); } +static __always_inline __must_check +bool user_event_last_ref(struct user_event *user) +{ +return refcount_read (&user->refcnt) ==1 ; +} +static __always_inline __must_check size_t copy_nofault(void *addr, size_t bytes, struct iov_iter *i) {
@@ -662,7 +669,7 @@ static struct user_event *find_user_event(char *name, u32 *outkey)hash_for_each_possible (register_table, user, node, key)if (!strcmp (EVENT_NAME (user), name)) { -atomic_inc (&user->refcnt); +refcount_inc (&user->refcnt);return user; }
@@ -876,12 +883,12 @@ static int user_event_reg(struct trace_event_call *call,return ret; inc: -atomic_inc (&user->refcnt); +refcount_inc (&user->refcnt);update_reg_page_for (user);return 0 ; dec:update_reg_page_for (user); -atomic_dec (&user->refcnt); +refcount_dec (&user->refcnt);return 0 ; }
@@ -907,7 +914,7 @@ static int user_event_create(const char *raw_command) ret =user_event_parse_cmd (name, &user);if (!ret) -atomic_dec (&user->refcnt); +refcount_dec (&user->refcnt);mutex_unlock (®_mutex);
@@ -951,14 +958,14 @@ static bool user_event_is_busy(struct dyn_event *ev) { struct user_event *user = container_of(ev, struct user_event, devent); -return atomic_read (&user->refcnt) !=0 ; +return !user_event_last_ref (user); }static int user_event_free (struct dyn_event *ev) { struct user_event *user = container_of(ev, struct user_event, devent); -if (atomic_read (&user->refcnt) !=0 ) +if (!user_event_last_ref (user))return -EBUSY ;return destroy_user_event (user);
@@ -1137,8 +1144,8 @@ static int user_event_parse(char *name, char *args, char *flags, user->index = index; -/* Ensure we track ref */ -atomic_inc (&user->refcnt); +/* Ensure we track self ref and caller ref (2) */ +refcount_set (&user->refcnt,2 );dyn_event_init (&user->devent, &user_event_dops);dyn_event_add (&user->devent, &user->call);
@@ -1164,29 +1171,17 @@ static int user_event_parse(char *name, char *args, char *flags,static int delete_user_event (char *name) {u32 key; -int ret;struct user_event *user =find_user_event (name, &key);if (!user)return -ENOENT ; -/* Ensure we are the last ref */ -if (atomic_read (&user->refcnt) !=1 ) { - ret = -EBUSY ; - goto put_ref; - } - - ret =destroy_user_event (user); +refcount_dec (&user->refcnt); -if (ret) - goto put_ref; - -return ret; -put_ref: -/* No longer have this ref */ -atomic_dec (&user->refcnt); +if (!user_event_last_ref (user)) +return -EBUSY ; -return ret; +return destroy_user_event (user); }/*
@@ -1314,7 +1309,7 @@ static int user_events_ref_add(struct file *file, struct user_event *user) new_refs->events[i] = user; -atomic_inc (&user->refcnt); +refcount_inc (&user->refcnt);rcu_assign_pointer (file->private_data, new_refs);
@@ -1374,7 +1369,7 @@ static long user_events_ioctl_reg(struct file *file, unsigned long uarg) ret =user_events_ref_add (file, user);/* No longer need parse ref, ref_add either worked or not */ -atomic_dec (&user->refcnt); +refcount_dec (&user->refcnt);/* Positive number is index and valid */ if (ret <0 )
@@ -1464,7 +1459,7 @@ static int user_events_release(struct inode *node, struct file *file) user = refs->events[i];if (user) -atomic_dec (&user->refcnt); +refcount_dec (&user->refcnt); } out: file->private_data =NULL ;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 14/15 tracing/user_events: Use bits vs bytes for enabled status page data
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
User processes may require many events and when they do the cache performance of a byte index status check is less ideal than a bit index. The previous event limit per-page was 4096, the new limit is 32,768.
This change adds a bitwise index to the user_reg struct. Programs check that the bit at status_bit has a bit set within the status page(s).
--- include/linux/user_events.h | 15 +--- kernel/trace/trace_events_user.c | 75 +++++++++++++++++-- samples/user_events/example.c | 25 +++++-- .../selftests/user_events/ftrace_test.c | 47 ++++++++++-- .../testing/selftests/user_events/perf_test.c | 11 ++- 5 files changed, 135 insertions(+), 38 deletions(-)
include/linux/user_events.h +3 -12
@@ -20,15 +20,6 @@#define USER_EVENTS_SYSTEM "user_events"#define USER_EVENTS_PREFIX "u:" -/* Bits 0-6 are for known probe types, Bit 7 is for unknown probes */ -#define EVENT_BIT_FTRACE 0 -#define EVENT_BIT_PERF 1 -#define EVENT_BIT_OTHER 7 - -#define EVENT_STATUS_FTRACE (1 << EVENT_BIT_FTRACE) -#define EVENT_STATUS_PERF (1 << EVENT_BIT_PERF) -#define EVENT_STATUS_OTHER (1 << EVENT_BIT_OTHER) -/* Create dynamic location entry within a 32-bit value */ #define DYN_LOC (offset, size) ((size) << 16 | (offset))
@@ -45,12 +36,12 @@ struct user_reg {/* Input: Pointer to string with event name, description and flags */ __u64 name_args; -/* Output: Byte index of the event within the status page */ -__u32 status_index; +/* Output: Bitwise index of the event within the status page */ +__u32 status_bit;/* Output: Index of the event to use when writing data */ __u32 write_index; -}; +} __attribute__((__packed__));#define DIAG_IOC_MAGIC '*'
kernel/trace/trace_events_user.c +67 -8
138 lines not on this page · raw message
samples/user_events/example.c +18 -7
@@ -12,13 +12,21 @@#include <fcntl.h> #include <stdio.h> #include <unistd.h> +#include <asm/bitsperlong.h> +#include <endian.h> #include <linux/user_events.h> +#if __BITS_PER_LONG ==64 +#define endian_swap (x) htole64(x) +#else +#define endian_swap (x) htole32(x) +#endif +/* Assumes debugfs is mounted */ const char *data_file ="/sys/kernel/debug/tracing/user_events_data" ;const char *status_file ="/sys/kernel/debug/tracing/user_events_status" ; -static int event_status (char **status) +static int event_status (long **status) {int fd =open (status_file,O_RDONLY );
@@ -33,7 +41,8 @@ static int event_status(char **status)return 0 ; } -static int event_reg (int fd,const char *command,int *status,int *write) +static int event_reg (int fd,const char *command,long *index,long *mask, +int *write) {struct user_reg reg = {0 };
@@ -43,7 +52,8 @@ static int event_reg(int fd, const char *command, int *status, int *write)if (ioctl (fd,DIAG_IOCSREG , ®) ==-1 )return -1 ; - *status = reg.status_index; + *index = reg.status_bit / __BITS_PER_LONG; + *mask =endian_swap (1L << (reg.status_bit % __BITS_PER_LONG)); *write = reg.write_index;return 0 ;
@@ -51,8 +61,9 @@ static int event_reg(int fd, const char *command, int *status, int *write)int main (int argc,char **argv) { -int data_fd, status, write; -char *status_page; +int data_fd, write; +long index, mask; +long *status_page;struct iovec io[2 ];__u32 count =0 ;
@@ -61,7 +72,7 @@ int main(int argc, char **argv) data_fd =open (data_file,O_RDWR ); -if (event_reg (data_fd,"test u32 count" , &status, &write) ==-1 ) +if (event_reg (data_fd,"test u32 count" , &index, &mask, &write) ==-1 )return errno;/* Setup iovec */
@@ -75,7 +86,7 @@ int main(int argc, char **argv)getchar ();/* Check if anyone is listening */ -if (status_page[status]) { +if (status_page[index] & mask) {/* Yep, trace out our data */ writev (data_fd, (const struct iovec *)io,2 );
tools/testing/selftests/user_events/ftrace_test.c +39 -8
120 lines not on this page · raw message
tools/testing/selftests/user_events/perf_test.c +8 -3
@@ -35,6 +35,11 @@ static long perf_event_open(struct perf_event_attr *pe, pid_t pid,return syscall (__NR_perf_event_open, pe, pid, cpu, group_fd, flags); } +static inline int status_check (char *status_page,int status_bit) +{ +return status_page[status_bit >>3 ] & (1 << (status_bit &7 )); +} +static int get_id (void ) {FILE *fp =fopen (id_file,"r" );
@@ -120,8 +125,8 @@ TEST_F(user, perf_write) {/* Register should work */ ASSERT_EQ (0 ,ioctl (self->data_fd,DIAG_IOCSREG , ®));ASSERT_EQ (0 , reg.write_index); -ASSERT_NE (0 , reg.status_index); -ASSERT_EQ (0 , status_page[reg.status_index]); +ASSERT_NE (0 , reg.status_bit); +ASSERT_EQ (0 ,status_check (status_page, reg.status_bit));/* Id should be there */ id =get_id ();
@@ -144,7 +149,7 @@ TEST_F(user, perf_write) {ASSERT_NE (MAP_FAILED , perf_page);/* Status should be updated */ -ASSERT_EQ (EVENT_STATUS_PERF , status_page[reg.status_index]); +ASSERT_NE (0 ,status_check (status_page, reg.status_bit)); event.index = reg.write_index; event.field1 =0xc001 ;
Signature · 1 line
2.35.1
Steven Rostedt
maintainerPATCH 15/15 tracing/user_events: Update ABI documentation to align to bits vs bytes
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
Update the documentation to reflect the new ABI requirements and how to use the byte index with the mask properly to check event status.
--- Documentation/trace/user_events.rst | 86 +++++++++++++++++++---------- 1 file changed, 58 insertions(+), 28 deletions(-)
Documentation/trace/user_events.rst +58 -28
@@ -20,14 +20,14 @@ dynamic_events is the same as the ioctl with the u: prefix applied. Typically programs will register a set of events that they wish to expose to tools that can read trace_events (such as ftrace and perf). The registration -process gives back two ints to the program for each event. The first int is the -status index. This index describes which byte in the +process gives back two ints to the program for each event. The first int is +the status bit. This describes which bit in little-endian format in the /sys/kernel/debug/tracing/user_events_status file represents this event. The -second int is the write index. This index describes the data when a write() or +second int is the write index which describes the data when a write() or writev() is called on the /sys/kernel/debug/tracing/user_events_data file. -The structures referenced in this document are contained with the -/include/uap/linux/user_events.h file in the source tree. +The structures referenced in this document are contained within the +/include/uapi/linux/user_events.h file in the source tree. **NOTE:** *Both user_events_status and user_events_data are under the tracefs filesystem and may be mounted at different paths than above.*
@@ -38,18 +38,18 @@ Registering within a user process is done via ioctl() out to the /sys/kernel/debug/tracing/user_events_data file. The command to issue is DIAG_IOCSREG. -This command takes a struct user_reg as an argument:: +This command takes a packed struct user_reg as an argument:: struct user_reg { u32 size; u64 name_args; - u32 status_index; + u32 status_bit; u32 write_index; }; The struct user_reg requires two inputs, the first is the size of the structure to ensure forward and backward compatibility. The second is the command string -to issue for registering. Upon success two outputs are set, the status index +to issue for registering. Upon success two outputs are set, the status bit and the write index. User based events show up under tracefs like any other event under the
@@ -111,15 +111,56 @@ in realtime. This allows user programs to only incur the cost of the write() or writev() calls when something is actively attached to the event. User programs call mmap() on /sys/kernel/debug/tracing/user_events_status to -check the status for each event that is registered. The byte to check in the -file is given back after the register ioctl() via user_reg.status_index. +check the status for each event that is registered. The bit to check in the +file is given back after the register ioctl() via user_reg.status_bit. The bit +is always in little-endian format. Programs can check if the bit is set either +using a byte-wise index with a mask or a long-wise index with a little-endian +mask. + Currently the size of user_events_status is a single page, however, custom kernel configurations can change this size to allow more user based events. In all cases the size of the file is a multiple of a page size. -For example, if the register ioctl() gives back a status_index of 3 you would -check byte 3 of the returned mmap data to see if anything is attached to that -event. +For example, if the register ioctl() gives back a status_bit of 3 you would +check byte 0 (3 / 8) of the returned mmap data and then AND the result with 8 +(1 << (3 % 8)) to see if anything is attached to that event. + +A byte-wise index check is performed as follows:: + + int index, mask; + char *status_page; + + index = status_bit / 8; + mask = 1 << (status_bit % 8); + + ... + + if (status_page[index] & mask) { + /* Enabled */ + } + +A long-wise index check is performed as follows:: + + #include <asm/bitsperlong.h> + #include <endian.h> + + #if __BITS_PER_LONG == 64 + #define endian_swap(x) htole64(x) + #else + #define endian_swap(x) htole32(x) + #endif + + long index, mask, *status_page; + + index = status_bit / __BITS_PER_LONG; + mask = 1L << (status_bit % __BITS_PER_LONG); + mask = endian_swap(mask); + + ... + + if (status_page[index] & mask) { + /* Enabled */ + } Administrators can easily check the status of all registered events by reading the user_events_status file directly via a terminal. The output is as follows::
@@ -137,7 +178,7 @@ For example, on a system that has a single event the output looks like this:: Active: 1 Busy: 0 - Max: 4096 + Max: 32768 If a user enables the user event via ftrace, the output would change to this::
@@ -145,21 +186,10 @@ If a user enables the user event via ftrace, the output would change to this:: Active: 1 Busy: 1 - Max: 4096 - -**NOTE:** *A status index of 0 will never be returned. This allows user -programs to have an index that can be used on error cases.* - -Status Bits -^^^^^^^^^^^ -The byte being checked will be non-zero if anything is attached. Programs can -check specific bits in the byte to see what mechanism has been attached. - -The following values are defined to aid in checking what has been attached: - -**EVENT_STATUS_FTRACE** - Bit set if ftrace has been attached (Bit 0). + Max: 32768 -**EVENT_STATUS_PERF** - Bit set if perf has been attached (Bit 1). +**NOTE:** *A status bit of 0 will never be returned. This allows user programs +to have a bit that can be used on error cases.* Writing Data ------------
Signature · 1 line
2.35.1