tracing: Final fixes and minor updates before sending pull request
Subsystems (5)
Steven Rostedt
maintainercover tracing: Final fixes and minor updates before sending pull request
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace/for-next
Head SHA1: adace4408252cc1c9913958d71e81a688af90a30
Beau Belgrave (6):
tracing/user_events: Set event filter_type from type
tracing: Fix print_fields() for __dyn_loc/__rel_loc tracing/user_events: Ensure write index cannot be negative tracing/user_events: Ensure bit is cleared on unregister tracing/user_events: Prevent same address and bit per process tracing/user_events: Limit max fault-in attempts
Hao Zeng (1):
recordmcount: Fix memory leaks in the uwrite function
Ken Lin (1):
tracing: Add missing spaces in trace_print_hex_seq()
Sergey Senozhatsky (1):
seq_buf: Add seq_buf_do_printk() helper
Tze-nan Wu (1):
ring-buffer: Ensure proper resetting of atomic variables in ring_buffer_reset_online_cpus
Zheng Yejian (1):
ring-buffer: Clearly check null ptr returned by rb_set_head_page()
----
include/linux/seq_buf.h | 2 + kernel/trace/ring_buffer.c | 21 +++- kernel/trace/trace_events_user.c | 126 +++++++++++++++++++--- kernel/trace/trace_output.c | 15 ++- lib/seq_buf.c | 32 ++++++ scripts/recordmcount.c | 6 +- tools/testing/selftests/user_events/abi_test.c | 9 +- tools/testing/selftests/user_events/ftrace_test.c | 14 ++- 8 files changed, 198 insertions(+), 27 deletions(-)
Steven Rostedt
maintainerPATCH 1/11 ring-buffer: Clearly check null ptr returned by rb_set_head_page()
From: Zheng Yejian <[email protected]>
In error case, 'buffer_page' returned by rb_set_head_page() is NULL, currently check '&buffer_page->list' is equivalent to check 'buffer_page' due to 'list' is the first member of 'buffer_page', but suppose it is not some time, 'head_page' would be wild memory while check would be bypassed.
--- kernel/trace/ring_buffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
kernel/trace/ring_buffer.c +3 -2
@@ -2054,10 +2054,11 @@ rb_insert_pages(struct ring_buffer_per_cpu *cpu_buffer)struct list_head *head_page, *prev_page, *r;struct list_head *last_page, *first_page;struct list_head *head_page_with_bit; +struct buffer_page *hpage =rb_set_head_page (cpu_buffer); - head_page = &rb_set_head_page (cpu_buffer)->list; -if (!head_page) +if (!hpage)break ; + head_page = &hpage->list; prev_page = head_page->prev; first_page = pages->next;
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 2/11 tracing/user_events: Set event filter_type from type
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
Users expect that events can be filtered by the kernel. User events currently sets all event fields as FILTER_OTHER which limits to binary filters only. When strings are being used, functionality is reduced.
Use filter_assign_type() to find the most appropriate filter type for each field in user events to ensure full kernel capabilities.
--- kernel/trace/trace_events_user.c | 3 +++ 1 file changed, 3 insertions(+)
kernel/trace/trace_events_user.c +3 -0
@@ -918,6 +918,9 @@ static int user_event_add_field(struct user_event *user, const char *type, field->is_signed = is_signed; field->filter_type = filter_type; +if (filter_type ==FILTER_OTHER ) + field->filter_type =filter_assign_type (type); +list_add (&field->link, &user->fields);/*
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 3/11 tracing: Fix print_fields() for __dyn_loc/__rel_loc
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
Both print_fields() and print_array() do not handle if dynamic data ends at the last byte of the payload for both __dyn_loc and __rel_loc field types. For __rel_loc, the offset was off by 4 bytes, leading to incorrect strings and data being printed out. In print_array() the buffer pos was missed from being advanced, which results in the first payload byte being used as the offset base instead of the field offset.
Advance __rel_loc offset by 4 to ensure correct offset and advance pos to the field offset to ensure correct data is displayed when printing arrays. Change >= to > when checking if data is in-bounds, since it's valid for dynamic data to include the last byte of the payload.
Example outputs for event format:
field:unsigned short common_type; offset:0; size:2; signed:0;
field:unsigned char common_flags; offset:2; size:1; signed:0;
field:unsigned char common_preempt_count; offset:3; size:1; signed:0;
field:int common_pid; offset:4; size:4; signed:1;
field:__rel_loc char text[]; offset:8; size:4; signed:1;Output before:tp_rel_loc: text=<OVERFLOW>
Output after:tp_rel_loc: text=Test
--- kernel/trace/trace_output.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/trace/trace_output.c +6 -4
@@ -819,13 +819,15 @@ static void print_array(struct trace_iterator *iter, void *pos, len = *(int *)pos >>16 ;if (field) - offset += field->offset; + offset += field->offset +sizeof (int ); -if (offset + len >= iter->ent_size) { +if (offset + len > iter->ent_size) {trace_seq_puts (&iter->seq,"<OVERFLOW>" );return ; } + pos = (void *)iter->ent + offset; +for (i =0 ; i < len; i++, pos++) {if (i)trace_seq_putc (&iter->seq,',' );
@@ -861,9 +863,9 @@ static void print_fields(struct trace_iterator *iter, struct trace_event_call *c len = *(int *)pos >>16 ;if (field->filter_type ==FILTER_RDYN_STRING ) - offset += field->offset; + offset += field->offset +sizeof (int ); -if (offset + len >= iter->ent_size) { +if (offset + len > iter->ent_size) {trace_seq_puts (&iter->seq,"<OVERFLOW>" );break ; }
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 4/11 seq_buf: Add seq_buf_do_printk() helper
In reply to Steven RostedtFrom: Sergey Senozhatsky <[email protected]>
Sometimes we use seq_buf to format a string buffer, which we then pass to printk(). However, in certain situations the seq_buf string buffer can get too big, exceeding the PRINTKRB_RECORD_MAX bytes limit, and causing printk() to truncate the string.
Add a new seq_buf helper. This helper prints the seq_buf string buffer line by line, using \n as a delimiter, rather than passing the whole string buffer to printk() at once.
--- include/linux/seq_buf.h | 2 ++ lib/seq_buf.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)
include/linux/seq_buf.h +2 -0
@@ -159,4 +159,6 @@ extern int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);#endif +void seq_buf_do_printk (struct seq_buf *s,const char *lvl); +#endif /* _LINUX_SEQ_BUF_H */
lib/seq_buf.c +32 -0
@@ -93,6 +93,38 @@ int seq_buf_printf(struct seq_buf *s, const char *fmt, ...) }EXPORT_SYMBOL_GPL (seq_buf_printf); +/** +* seq_buf_do_printk - printk seq_buf line by line +* @s: seq_buf descriptor +* @lvl: printk level +* +* printk()-s a multi-line sequential buffer line by line. The function +* makes sure that the buffer in @s is nul terminated and safe to read +* as a string. +*/ +void seq_buf_do_printk (struct seq_buf *s,const char *lvl) +{ +const char *start, *lf; + +if (s->size ==0 || s->len ==0 ) +return ; + +seq_buf_terminate (s); + + start = s->buffer; +while ((lf =strchr (start,'\n' ))) { +int len = lf - start +1 ; + +printk ("%s%.*s" , lvl, len, start); + start = ++lf; + } + +/* No trailing LF */ +if (start < s->buffer + s->len) +printk ("%s%s\n" , lvl, start); +} +EXPORT_SYMBOL_GPL (seq_buf_do_printk); +#ifdef CONFIG_BINARY_PRINTF /** * seq_buf_bprintf - Write the printf string from binary arguments
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 5/11 tracing/user_events: Ensure write index cannot be negative
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
The write index indicates which event the data is for and accesses a per-file array. The index is passed by user processes during write() calls as the first 4 bytes. Ensure that it cannot be negative by returning -EINVAL to prevent out of bounds accesses.
Update ftrace self-test to ensure this occurs properly.
--- kernel/trace/trace_events_user.c | 3 +++ tools/testing/selftests/user_events/ftrace_test.c | 5 +++++ 2 files changed, 8 insertions(+)
kernel/trace/trace_events_user.c +3 -0
@@ -1824,6 +1824,9 @@ static ssize_t user_events_write_core(struct file *file, struct iov_iter *i)if (unlikely (copy_from_iter (&idx,sizeof (idx), i) !=sizeof (idx)))return -EFAULT ; +if (idx <0 ) +return -EINVAL ; +rcu_read_lock_sched (); refs =rcu_dereference_sched (info->refs);
tools/testing/selftests/user_events/ftrace_test.c +5 -0
@@ -296,6 +296,11 @@ TEST_F(user, write_events) {ASSERT_NE (-1 ,writev (self->data_fd, (const struct iovec *)io,3 )); after =trace_bytes ();ASSERT_GT (after, before); + +/* Negative index should fail with EINVAL */ + reg.write_index =-1 ; +ASSERT_EQ (-1 ,writev (self->data_fd, (const struct iovec *)io,3 )); +ASSERT_EQ (EINVAL , errno); } TEST_F(user, write_fault) {
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 6/11 tracing/user_events: Ensure bit is cleared on unregister
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
If an event is enabled and a user process unregisters user_events, the bit is left set. Fix this by always clearing the bit in the user process if unregister is successful.
Update abi self-test to ensure this occurs properly.
--- kernel/trace/trace_events_user.c | 34 +++++++++++++++++++ .../testing/selftests/user_events/abi_test.c | 9 +++-- 2 files changed, 40 insertions(+), 3 deletions(-)
kernel/trace/trace_events_user.c +34 -0
@@ -2149,6 +2149,35 @@ static long user_unreg_get(struct user_unreg __user *ureg,return ret; } +static int user_event_mm_clear_bit (struct user_event_mm *user_mm, +unsigned long uaddr,unsigned char bit) +{ +struct user_event_enabler enabler; +int result; + +memset (&enabler,0 ,sizeof (enabler)); + enabler.addr = uaddr; + enabler.values = bit; +retry: +/* Prevents state changes from racing with new enablers */ +mutex_lock (&event_mutex); + +/* Force the bit to be cleared, since no event is attached */ +mmap_read_lock (user_mm->mm); + result =user_event_enabler_write (user_mm, &enabler, false); +mmap_read_unlock (user_mm->mm); + +mutex_unlock (&event_mutex); + +if (result) { +/* Attempt to fault-in and retry if it worked */ +if (!user_event_mm_fault_in (user_mm, uaddr)) + goto retry; + } + +return result; +} +/* * Unregisters an enablement address/bit within a task/user mm. */
@@ -2193,6 +2222,11 @@ static long user_events_ioctl_unreg(unsigned long uarg)mutex_unlock (&event_mutex); +/* Ensure bit is now cleared for user, regardless of event status */ +if (!ret) + ret =user_event_mm_clear_bit (mm, reg.disable_addr, + reg.disable_bit); +return ret; }
tools/testing/selftests/user_events/abi_test.c +6 -3
@@ -109,13 +109,16 @@ TEST_F(user, enablement) {ASSERT_EQ (0 ,change_event (false));ASSERT_EQ (0 , self->check); -/* Should not change after disable */ +/* Ensure kernel clears bit after disable */ ASSERT_EQ (0 ,change_event (true));ASSERT_EQ (1 , self->check);ASSERT_EQ (0 ,reg_disable (&self->check,0 )); +ASSERT_EQ (0 , self->check); + +/* Ensure doesn't change after unreg */ +ASSERT_EQ (0 ,change_event (true)); +ASSERT_EQ (0 , self->check);ASSERT_EQ (0 ,change_event (false)); -ASSERT_EQ (1 , self->check); - self->check =0 ; } TEST_F(user, bit_sizes) {
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 7/11 tracing/user_events: Prevent same address and bit per process
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
User processes register an address and bit pair for events. If the same address and bit pair are registered multiple times in the same process, it can cause undefined behavior when events are enabled/disabled. When more than one are used, the bit could be turned off by another event being disabled, while the original event is still enabled.
Prevent undefined behavior by checking the current mm to see if any event has already been registered for the address and bit pair. Return EADDRINUSE back to the user process if it's already being used.
Update ftrace self-test to ensure this occurs properly.
--- kernel/trace/trace_events_user.c | 41 +++++++++++++++++++ .../selftests/user_events/ftrace_test.c | 9 +++- 2 files changed, 49 insertions(+), 1 deletion(-)
kernel/trace/trace_events_user.c +41 -0
@@ -419,6 +419,21 @@ static int user_event_enabler_write(struct user_event_mm *mm,return 0 ; } +static bool user_event_enabler_exists (struct user_event_mm *mm, +unsigned long uaddr,unsigned char bit) +{ +struct user_event_enabler *enabler; +struct user_event_enabler *next; + +list_for_each_entry_safe (enabler, next, &mm->enablers, link) { +if (enabler->addr == uaddr && + (enabler->values &ENABLE_VAL_BIT_MASK ) == bit) +return true; + } + +return false; +} +static void user_event_enabler_update (struct user_event *user) {struct user_event_enabler *enabler;
@@ -657,6 +672,22 @@ void user_event_mm_dup(struct task_struct *t, struct user_event_mm *old_mm)user_event_mm_remove (t); } +static bool current_user_event_enabler_exists (unsigned long uaddr, +unsigned char bit) +{ +struct user_event_mm *user_mm =current_user_event_mm (); +bool exists; + +if (!user_mm) +return false; + + exists =user_event_enabler_exists (user_mm, uaddr, bit); + +user_event_mm_put (user_mm); + +return exists; +} +static struct user_event_enabler *user_event_enabler_create (struct user_reg *reg,struct user_event *user,int *write_result)
@@ -2048,6 +2079,16 @@ static long user_events_ioctl_reg(struct user_event_file_info *info,if (ret)return ret; +/* +* Prevent users from using the same address and bit multiple times +* within the same mm address space. This can cause unexpected behavior +* for user processes that is far easier to debug if this is explictly +* an error upon registering. +*/ +if (current_user_event_enabler_exists ((unsigned long )reg.enable_addr, + reg.enable_bit)) +return -EADDRINUSE ; + name =strndup_user ((const char __user *)(uintptr_t )reg.name_args,MAX_EVENT_DESC );
tools/testing/selftests/user_events/ftrace_test.c +8 -1
@@ -219,7 +219,12 @@ TEST_F(user, register_events) {ASSERT_EQ (0 ,ioctl (self->data_fd,DIAG_IOCSREG , ®));ASSERT_EQ (0 , reg.write_index); -/* Multiple registers should result in same index */ +/* Multiple registers to the same addr + bit should fail */ +ASSERT_EQ (-1 ,ioctl (self->data_fd,DIAG_IOCSREG , ®)); +ASSERT_EQ (EADDRINUSE , errno); + +/* Multiple registers to same name should result in same index */ + reg.enable_bit =30 ;ASSERT_EQ (0 ,ioctl (self->data_fd,DIAG_IOCSREG , ®));ASSERT_EQ (0 , reg.write_index);
@@ -242,6 +247,8 @@ TEST_F(user, register_events) {/* Unregister */ ASSERT_EQ (0 ,ioctl (self->data_fd,DIAG_IOCSUNREG , &unreg)); + unreg.disable_bit =30 ; +ASSERT_EQ (0 ,ioctl (self->data_fd,DIAG_IOCSUNREG , &unreg));/* Delete should work only after close and unregister */ close (self->data_fd);
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 8/11 tracing/user_events: Limit max fault-in attempts
In reply to Steven RostedtFrom: Beau Belgrave <[email protected]>
When event enablement changes, user_events attempts to update a bit in the user process. If a fault is hit, an attempt to fault-in the page and the write is retried if the page made it in. While this normally requires a couple attempts, it is possible a bad user process could attempt to cause infinite loops.
Ensure fault-in attempts either sync or async are limited to a max of 10 attempts for each update. When the max is hit, return -EFAULT so another attempt is not made in all cases.
--- kernel/trace/trace_events_user.c | 49 +++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 14 deletions(-)
kernel/trace/trace_events_user.c +35 -14
@@ -123,6 +123,7 @@ struct user_event_enabler_fault {struct work_struct work;struct user_event_mm *mm;struct user_event_enabler *enabler; +int attempt; };static struct kmem_cache *fault_cache;
@@ -266,11 +267,19 @@ static void user_event_enabler_destroy(struct user_event_enabler *enabler)kfree (enabler); } -static int user_event_mm_fault_in (struct user_event_mm *mm,unsigned long uaddr) +static int user_event_mm_fault_in (struct user_event_mm *mm,unsigned long uaddr, +int attempt) {bool unlocked;int ret; +/* +* Normally this is low, ensure that it cannot be taken advantage of by +* bad user processes to cause excessive looping. +*/ +if (attempt >10 ) +return -EFAULT ; +mmap_read_lock (mm->mm);/* Ensure MM has tasks, cannot use after exit_mm() */
@@ -289,7 +298,7 @@ static int user_event_mm_fault_in(struct user_event_mm *mm, unsigned long uaddr)static int user_event_enabler_write (struct user_event_mm *mm,struct user_event_enabler *enabler, -bool fixup_fault); +bool fixup_fault,int *attempt);static void user_event_enabler_fault_fixup (struct work_struct *work) {
@@ -298,9 +307,10 @@ static void user_event_enabler_fault_fixup(struct work_struct *work)struct user_event_enabler *enabler = fault->enabler;struct user_event_mm *mm = fault->mm;unsigned long uaddr = enabler->addr; +int attempt = fault->attempt;int ret; - ret =user_event_mm_fault_in (mm, uaddr); + ret =user_event_mm_fault_in (mm, uaddr, attempt);if (ret && ret != -ENOENT ) {struct user_event *user = enabler->event;
@@ -329,7 +339,7 @@ static void user_event_enabler_fault_fixup(struct work_struct *work)if (!ret) {mmap_read_lock (mm->mm); -user_event_enabler_write (mm, enabler, true); +user_event_enabler_write (mm, enabler, true, &attempt);mmap_read_unlock (mm->mm); } out:
@@ -341,7 +351,8 @@ static void user_event_enabler_fault_fixup(struct work_struct *work) }static bool user_event_enabler_queue_fault (struct user_event_mm *mm, -struct user_event_enabler *enabler) +struct user_event_enabler *enabler, +int attempt) {struct user_event_enabler_fault *fault;
@@ -353,6 +364,7 @@ static bool user_event_enabler_queue_fault(struct user_event_mm *mm,INIT_WORK (&fault->work, user_event_enabler_fault_fixup); fault->mm =user_event_mm_get (mm); fault->enabler = enabler; + fault->attempt = attempt;/* Don't try to queue in again while we have a pending fault */ set_bit (ENABLE_VAL_FAULTING_BIT ,ENABLE_BITOPS (enabler));
@@ -372,7 +384,7 @@ static bool user_event_enabler_queue_fault(struct user_event_mm *mm,static int user_event_enabler_write (struct user_event_mm *mm,struct user_event_enabler *enabler, -bool fixup_fault) +bool fixup_fault,int *attempt) {unsigned long uaddr = enabler->addr;unsigned long *ptr;
@@ -383,6 +395,8 @@ static int user_event_enabler_write(struct user_event_mm *mm,lockdep_assert_held (&event_mutex);mmap_assert_locked (mm->mm); + *attempt +=1 ; +/* Ensure MM has tasks, cannot use after exit_mm() */ if (refcount_read (&mm->tasks) ==0 )return -ENOENT ;
@@ -398,7 +412,7 @@ static int user_event_enabler_write(struct user_event_mm *mm,if (!fixup_fault)return -EFAULT ; -if (!user_event_enabler_queue_fault (mm, enabler)) +if (!user_event_enabler_queue_fault (mm, enabler, *attempt))pr_warn ("user_events: Unable to queue fault handler\n" );return -EFAULT ;
@@ -439,15 +453,19 @@ static void user_event_enabler_update(struct user_event *user)struct user_event_enabler *enabler;struct user_event_mm *mm =user_event_mm_get_all (user);struct user_event_mm *next; +int attempt;while (mm) { next = mm->next;mmap_read_lock (mm->mm);rcu_read_lock (); -list_for_each_entry_rcu (enabler, &mm->enablers, link) -if (enabler->event == user) -user_event_enabler_write (mm, enabler, true); +list_for_each_entry_rcu (enabler, &mm->enablers, link) { +if (enabler->event == user) { + attempt =0 ; +user_event_enabler_write (mm, enabler, true, &attempt); + } + }rcu_read_unlock ();mmap_read_unlock (mm->mm);
@@ -695,6 +713,7 @@ static struct user_event_enablerstruct user_event_enabler *enabler;struct user_event_mm *user_mm;unsigned long uaddr = (unsigned long )reg->enable_addr; +int attempt =0 ; user_mm =current_user_event_mm ();
@@ -715,7 +734,8 @@ static struct user_event_enabler/* Attempt to reflect the current state within the process */ mmap_read_lock (user_mm->mm); - *write_result =user_event_enabler_write (user_mm, enabler, false); + *write_result =user_event_enabler_write (user_mm, enabler, false, + &attempt);mmap_read_unlock (user_mm->mm);/*
@@ -735,7 +755,7 @@ static struct user_event_enablerif (*write_result) {/* Attempt to fault-in and retry if it worked */ -if (!user_event_mm_fault_in (user_mm, uaddr)) +if (!user_event_mm_fault_in (user_mm, uaddr, attempt)) goto retry;kfree (enabler);
@@ -2195,6 +2215,7 @@ static int user_event_mm_clear_bit(struct user_event_mm *user_mm, {struct user_event_enabler enabler;int result; +int attempt =0 ;memset (&enabler,0 ,sizeof (enabler)); enabler.addr = uaddr;
@@ -2205,14 +2226,14 @@ static int user_event_mm_clear_bit(struct user_event_mm *user_mm,/* Force the bit to be cleared, since no event is attached */ mmap_read_lock (user_mm->mm); - result =user_event_enabler_write (user_mm, &enabler, false); + result =user_event_enabler_write (user_mm, &enabler, false, &attempt);mmap_read_unlock (user_mm->mm);mutex_unlock (&event_mutex);if (result) {/* Attempt to fault-in and retry if it worked */ -if (!user_event_mm_fault_in (user_mm, uaddr)) +if (!user_event_mm_fault_in (user_mm, uaddr, attempt)) goto retry; }
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 9/11 recordmcount: Fix memory leaks in the uwrite function
In reply to Steven RostedtFrom: Hao Zeng <[email protected]>
Common realloc mistake: 'file_append' nulled but not freed upon failure
--- scripts/recordmcount.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
scripts/recordmcount.c +5 -1
@@ -110,6 +110,7 @@ static ssize_t uwrite(void const *const buf, size_t const count) {size_t cnt = count;off_t idx =0 ; +void *p =NULL ; file_updated =1 ;
@@ -117,7 +118,10 @@ static ssize_t uwrite(void const *const buf, size_t const count)off_t aoffset = (file_ptr + count) - file_end;if (aoffset > file_append_size) { - file_append =realloc (file_append, aoffset); + p =realloc (file_append, aoffset); +if (!p) +free (file_append); + file_append = p; file_append_size = aoffset; }if (!file_append) {
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 10/11 ring-buffer: Ensure proper resetting of atomic variables in ring_buffer_reset_online_cpus
In reply to Steven RostedtFrom: Tze-nan Wu <[email protected]>
In ring_buffer_reset_online_cpus, the buffer_size_kb write operation may permanently fail if the cpu_online_mask changes between two for_each_online_buffer_cpu loops. The number of increases and decreases on both cpu_buffer->resize_disabled and cpu_buffer->record_disabled may be inconsistent, causing some CPUs to have non-zero values for these atomic variables after the function returns.
This issue can be reproduced by "echo 0 > trace" while hotplugging cpu. After reproducing success, we can find out buffer_size_kb will not be functional anymore.
To prevent leaving 'resize_disabled' and 'record_disabled' non-zero after ring_buffer_reset_online_cpus returns, we ensure that each atomic variable has been set up before atomic_sub() to it.
--- kernel/trace/ring_buffer.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
kernel/trace/ring_buffer.c +13 -3
@@ -5326,6 +5326,9 @@ void ring_buffer_reset_cpu(struct trace_buffer *buffer, int cpu) }EXPORT_SYMBOL_GPL (ring_buffer_reset_cpu); +/* Flag to ensure proper resetting of atomic variables */ +#define RESET_BIT (1 << 30) +/** * ring_buffer_reset_online_cpus - reset a ring buffer per CPU buffer * @buffer: The ring buffer to reset a per cpu buffer of
@@ -5342,20 +5345,27 @@ void ring_buffer_reset_online_cpus(struct trace_buffer *buffer)for_each_online_buffer_cpu (buffer, cpu) { cpu_buffer = buffer->buffers[cpu]; -atomic_inc (&cpu_buffer->resize_disabled); +atomic_add (RESET_BIT , &cpu_buffer->resize_disabled);atomic_inc (&cpu_buffer->record_disabled); }/* Make sure all commits have finished */ synchronize_rcu (); -for_each_online_buffer_cpu (buffer, cpu) { +for_each_buffer_cpu (buffer, cpu) { cpu_buffer = buffer->buffers[cpu]; +/* +* If a CPU came online during the synchronize_rcu(), then +* ignore it. +*/ +if (!(atomic_read (&cpu_buffer->resize_disabled) &RESET_BIT )) +continue ; +reset_disabled_cpu_buffer (cpu_buffer);atomic_dec (&cpu_buffer->record_disabled); -atomic_dec (&cpu_buffer->resize_disabled); +atomic_sub (RESET_BIT , &cpu_buffer->resize_disabled); }mutex_unlock (&buffer->mutex);
Signature · 1 line
2.39.2
Steven Rostedt
maintainerPATCH 11/11 tracing: Add missing spaces in trace_print_hex_seq()
In reply to Steven RostedtFrom: Ken Lin <[email protected]>
If the buffer length is larger than 16 and concatenate is set to false, there would be missing spaces every 16 bytes.
Example:
Before: c5 11 10 50 05 4d 31 40 00 40 00 40 00 4d 31 4000 40 00
After: c5 11 10 50 05 4d 31 40 00 40 00 40 00 4d 31 40 00 40 00
--- kernel/trace/trace_output.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/trace/trace_output.c +4 -1
@@ -221,8 +221,11 @@ trace_print_hex_seq(struct trace_seq *p, const unsigned char *buf, int buf_len,const char *ret =trace_seq_buffer_ptr (p);const char *fmt = concatenate ?"%*phN" :"%*ph" ; -for (i =0 ; i < buf_len; i +=16 ) +for (i =0 ; i < buf_len; i +=16 ) { +if (!concatenate && i !=0 ) +trace_seq_putc (p,' ' );trace_seq_printf (p, fmt,min (buf_len - i,16 ), &buf[i]); + }trace_seq_putc (p,0 );return ret;
Signature · 1 line
2.39.2