trace-seq/seq-buf/x86/printk: Print all stacks from NMI safely
Subsystems (9)
Steven Rostedt
maintainerv5 cover trace-seq/seq-buf/x86/printk: Print all stacks from NMI safely
Version 5!
Full diff between v4 and v5 will be replied to this email.
Here's the summary of the differences since version 4:
. Various clean ups
. Added trace_seq_used() to be used instead of s->seq.len
. Used seq_buf_used() instead of s->len
. Fixed output of ftrace_raw_output_prep() as well as caller code
. Fixed up tracing_fill_pipe_page() The logic was a bit whacky
and this was needed when introducing trace_seq_used()
. Rebased on latest ftrace/core code
. Fixed uprobe to be like probe code in print_uprobe_event()
. Introduced new seq_buf_can_fit() to remove the likelyhood of
screwing up the s->len + * < s->size code. Now it's just seq_buf_can_fit(s, len) which will tell us if the len can still fit in the buffer.
Enjoy,
-- Steve
Steven Rostedt (Red Hat) (26):
tracing: Fix trace_seq_bitmask() to start at current position
tracing: Add trace_seq_has_overflowed() and trace_handle_return() blktrace/tracing: Use trace_seq_has_overflowed() helper function
ring-buffer: Remove check of trace_seq_{puts,printf}() return values
tracing: Have branch tracer use trace_handle_return() helper function
tracing: Have function_graph use trace_seq_has_overflowed() kprobes/tracing: Use trace_seq_has_overflowed() for overflow checks
tracing: Do not check return values of trace_seq_p*() for mmio tracer tracing/probes: Do not use return value of trace_seq_printf() tracing/uprobes: Do not use return values of trace_seq_printf()
tracing: Do not use return values of trace_seq_printf() in syscall tracing
tracing: Remove return values of most trace_seq_*() functions
tracing: Fix return value of ftrace_raw_output_prep()
tracing: Create seq_buf layer in trace_seq
tracing: Convert seq_buf_path() to be like seq_path()
tracing: Convert seq_buf fields to be like seq_file fields
tracing: Add a seq_buf_clear() helper and clear len and readpos in init
seq_buf: Create seq_buf_used() to find out how much was written
tracing: Use trace_seq_used() and seq_buf_used() instead of len
seq_buf: Add seq_buf_can_fit() helper function
tracing: Have seq_buf use full buffer
tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions
seq-buf: Make seq_buf_bprintf() conditional on CONFIG_BINARY_PRINTF
seq_buf: Move the seq_buf code to lib/
printk: Add per_cpu printk func to allow printk to be diverted x86/nmi: Perform a safe NMI stack trace on all CPUs
----
arch/x86/kernel/apic/hw_nmi.c | 91 +++++++- include/linux/ftrace_event.h | 11 + include/linux/percpu.h | 3 + include/linux/printk.h | 2 + include/linux/seq_buf.h | 136 ++++++++++++ include/linux/trace_seq.h | 77 ++++--- include/trace/ftrace.h | 8 +- kernel/printk/printk.c | 38 +++- kernel/trace/blktrace.c | 148 ++++++------- kernel/trace/ring_buffer.c | 75 +++---- kernel/trace/trace.c | 126 ++++++----- kernel/trace/trace.h | 3 +- kernel/trace/trace_branch.c | 15 +- kernel/trace/trace_events.c | 9 +- kernel/trace/trace_functions_graph.c | 393 +++++++++++---------------------- kernel/trace/trace_kprobe.c | 42 ++-- kernel/trace/trace_mmiotrace.c | 52 ++--- kernel/trace/trace_output.c | 417 ++++++++++++++--------------------- kernel/trace/trace_output.h | 16 +- kernel/trace/trace_probe.c | 10 +- kernel/trace/trace_seq.c | 253 +++++++++------------ kernel/trace/trace_syscalls.c | 43 ++-- kernel/trace/trace_uprobe.c | 23 +- lib/Makefile | 2 +- lib/seq_buf.c | 351 +++++++++++++++++++++++++++++ 25 files changed, 1323 insertions(+), 1021 deletions(-)
Steven Rostedt
maintainerPATCH v5 1/26 tracing: Fix trace_seq_bitmask() to start at current position
From: "Steven Rostedt (Red Hat)" <[email protected]>
In trace_seq_bitmask() it calls bitmap_scnprintf() not from the current position of the trace_seq buffer (s->buffer + s->len), but instead from the beginning of the buffer (s->buffer).
Luckily, the only user of this "ipi_raise tracepoint" uses it as the first parameter, and as such, the start of the temp buffer in include/trace/ftrace.h (see __get_bitmask()).
--- kernel/trace/trace_seq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_seq.c +1 -1
@@ -122,7 +122,7 @@ int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,if (s->full || !len)return 0 ; - ret =bitmap_scnprintf (s->buffer, len, maskp, nmaskbits); + ret =bitmap_scnprintf (s->buffer + s->len, len, maskp, nmaskbits); s->len += ret;return 1 ;
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 2/26 tracing: Add trace_seq_has_overflowed() and trace_handle_return()
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Adding a trace_seq_has_overflowed() which returns true if the trace_seq had too much written into it allows us to simplify the code.
Instead of checking the return value of every call to trace_seq_printf() and friends, they can all be called normally, and at the end we can return !trace_seq_has_overflowed() instead.
Several functions also return TRACE_TYPE_PARTIAL_LINE when the trace_seq overflowed and TRACE_TYPE_HANDLED otherwise. Another helper function was created called trace_handle_return() which takes a trace_seq and returns these enums. Using this helper function also simplifies the code.
This change also makes it possible to remove the return values of trace_seq_printf() and friends. They should instead just be void functions.
--- include/linux/ftrace_event.h | 11 ++ include/linux/trace_seq.h | 12 ++ include/trace/ftrace.h | 6 +- kernel/trace/trace.c | 69 +++---- kernel/trace/trace.h | 1 + kernel/trace/trace_output.c | 416 +++++++++++++++++-------------------------- kernel/trace/trace_output.h | 16 +- 7 files changed, 231 insertions(+), 300 deletions(-)
include/linux/ftrace_event.h +11 -0
@@ -138,6 +138,17 @@ enum print_line_t { TRACE_TYPE_NO_CONSUME = 3 /* Handled but ask to not consume */ }; +/* +* Several functions return TRACE_TYPE_PARTIAL_LINE if the trace_seq +* overflowed, and TRACE_TYPE_HANDLED otherwise. This helper function +* simplifies those functions and keeps them in sync. +*/ +static inline enum print_line_t trace_handle_return (struct trace_seq *s) +{ +return trace_seq_has_overflowed (s) ? +TRACE_TYPE_PARTIAL_LINE :TRACE_TYPE_HANDLED ; +} +void tracing_generic_entry_update (struct trace_entry *entry,unsigned long flags,int pc);
include/linux/trace_seq.h +12 -0
@@ -40,6 +40,18 @@ trace_seq_buffer_ptr(struct trace_seq *s)return s->buffer + s->len; } +/** +* trace_seq_has_overflowed - return true if the trace_seq took too much +* @s: trace sequence descriptor +* +* Returns true if too much data was added to the trace_seq and it is +* now full and will not take anymore. +*/ +static inline bool trace_seq_has_overflowed (struct trace_seq *s) +{ +return s->full || s->len >PAGE_SIZE -1 ; +} +/* * Currently only defined when tracing is enabled. */
include/trace/ftrace.h +2 -4
@@ -280,11 +280,9 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags, \if (ret) \return ret; \ \ - ret =trace_seq_printf (s, print); \ -if (!ret) \ -return TRACE_TYPE_PARTIAL_LINE ; \ +trace_seq_printf (s, print); \ \ -return TRACE_TYPE_HANDLED ; \ +return trace_handle_return (s); \ } \ static struct trace_event_functions ftrace_event_type_funcs_##call = { \ .trace = ftrace_raw_output_##call, \
kernel/trace/trace.c +35 -34
118 lines not on this page · raw message
kernel/trace/trace.h +1 -0
@@ -14,6 +14,7 @@#include <linux/trace_seq.h> #include <linux/ftrace_event.h> #include <linux/compiler.h> +#include <linux/trace_seq.h> #ifdef CONFIG_FTRACE_SYSCALLS #include <asm/unistd.h> /* For NR_SYSCALLS */
kernel/trace/trace_output.c +165 -251
717 lines not on this page · raw message
kernel/trace/trace_output.h +5 -11
@@ -35,17 +35,11 @@ trace_print_lat_fmt(struct trace_seq *s, struct trace_entry *entry);extern int __unregister_ftrace_event (struct trace_event *event);extern struct rw_semaphore trace_event_sem; -#define SEQ_PUT_FIELD_RET (s, x) \ -do { \ - if (!trace_seq_putmem(s, &(x), sizeof(x))) \ - return TRACE_TYPE_PARTIAL_LINE; \ -} while (0) - -#define SEQ_PUT_HEX_FIELD_RET (s, x) \ -do { \ - if (!trace_seq_putmem_hex(s, &(x), sizeof(x))) \ - return TRACE_TYPE_PARTIAL_LINE; \ -} while (0) +#define SEQ_PUT_FIELD (s, x) \ + trace_seq_putmem(s, &(x), sizeof(x)) + +#define SEQ_PUT_HEX_FIELD (s, x) \ + trace_seq_putmem_hex(s, &(x), sizeof(x))#endif
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 3/26 blktrace/tracing: Use trace_seq_has_overflowed() helper function
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Checking the return code of every trace_seq_printf() operation and having to return early if it overflowed makes the code messy.
Using the new trace_seq_has_overflowed() and trace_handle_return() functions allows us to clean up the code.
In the future, trace_seq_printf() and friends will be turning into void functions and not returning a value. The trace_seq_has_overflowed() is to be used instead. This cleanup allows that change to take place.
--- kernel/trace/blktrace.c | 148 +++++++++++++++++++++--------------------------- 1 file changed, 66 insertions(+), 82 deletions(-)
kernel/trace/blktrace.c +66 -82
@@ -1142,9 +1142,9 @@ static void get_pdu_remap(const struct trace_entry *ent, r->sector_from =be64_to_cpu (sector_from); } -typedef int (blk_log_action_t ) (struct trace_iterator *iter,const char *act); +typedef void (blk_log_action_t ) (struct trace_iterator *iter,const char *act); -static int blk_log_action_classic (struct trace_iterator *iter,const char *act) +static void blk_log_action_classic (struct trace_iterator *iter,const char *act) {char rwbs[RWBS_LEN ];unsigned long long ts = iter->ts;
@@ -1154,33 +1154,33 @@ static int blk_log_action_classic(struct trace_iterator *iter, const char *act)fill_rwbs (rwbs, t); -return trace_seq_printf (&iter->seq, -"%3d,%-3d %2d %5d.%09lu %5u %2s %3s " , -MAJOR (t->device),MINOR (t->device), iter->cpu, - secs, nsec_rem, iter->ent->pid, act, rwbs); +trace_seq_printf (&iter->seq, +"%3d,%-3d %2d %5d.%09lu %5u %2s %3s " , +MAJOR (t->device),MINOR (t->device), iter->cpu, + secs, nsec_rem, iter->ent->pid, act, rwbs); } -static int blk_log_action (struct trace_iterator *iter,const char *act) +static void blk_log_action (struct trace_iterator *iter,const char *act) {char rwbs[RWBS_LEN ];const struct blk_io_trace *t =te_blk_io_trace (iter->ent);fill_rwbs (rwbs, t); -return trace_seq_printf (&iter->seq,"%3d,%-3d %2s %3s " , -MAJOR (t->device),MINOR (t->device), act, rwbs); +trace_seq_printf (&iter->seq,"%3d,%-3d %2s %3s " , +MAJOR (t->device),MINOR (t->device), act, rwbs); } -static int blk_log_dump_pdu (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_dump_pdu (struct trace_seq *s,const struct trace_entry *ent) {const unsigned char *pdu_buf;int pdu_len; -int i, end, ret; +int i, end; pdu_buf =pdu_start (ent); pdu_len =te_blk_io_trace (ent)->pdu_len;if (!pdu_len) -return 1 ; +return ;/* find the last zero that needs to be printed */ for (end = pdu_len -1 ; end >=0 ; end--)
@@ -1188,119 +1188,107 @@ static int blk_log_dump_pdu(struct trace_seq *s, const struct trace_entry *ent)break ; end++; -if (!trace_seq_putc (s,'(' )) -return 0 ; +trace_seq_putc (s,'(' );for (i =0 ; i < pdu_len; i++) { - ret =trace_seq_printf (s,"%s%02x" , - i ==0 ?"" :" " , pdu_buf[i]); -if (!ret) -return ret; +trace_seq_printf (s,"%s%02x" , + i ==0 ?"" :" " , pdu_buf[i]);/* * stop when the rest is just zeroes and indicate so * with a ".." appended */ -if (i == end && end != pdu_len -1 ) -return trace_seq_puts (s," ..) " ); +if (i == end && end != pdu_len -1 ) { +trace_seq_puts (s," ..) " ); +return ; + } } -return trace_seq_puts (s,") " ); +trace_seq_puts (s,") " ); } -static int blk_log_generic (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_generic (struct trace_seq *s,const struct trace_entry *ent) {char cmd[TASK_COMM_LEN ];trace_find_cmdline (ent->pid, cmd);if (t_action (ent) &BLK_TC_ACT (BLK_TC_PC )) { -int ret; - - ret =trace_seq_printf (s,"%u " ,t_bytes (ent)); -if (!ret) -return 0 ; - ret =blk_log_dump_pdu (s, ent); -if (!ret) -return 0 ; -return trace_seq_printf (s,"[%s]\n" , cmd); +trace_seq_printf (s,"%u " ,t_bytes (ent)); +blk_log_dump_pdu (s, ent); +trace_seq_printf (s,"[%s]\n" , cmd); }else {if (t_sec (ent)) -return trace_seq_printf (s,"%llu + %u [%s]\n" , +trace_seq_printf (s,"%llu + %u [%s]\n" ,t_sector (ent),t_sec (ent), cmd); -return trace_seq_printf (s,"[%s]\n" , cmd); +else +trace_seq_printf (s,"[%s]\n" , cmd); } } -static int blk_log_with_error (struct trace_seq *s, +static void blk_log_with_error (struct trace_seq *s,const struct trace_entry *ent) {if (t_action (ent) &BLK_TC_ACT (BLK_TC_PC )) { -int ret; - - ret =blk_log_dump_pdu (s, ent); -if (ret) -return trace_seq_printf (s,"[%d]\n" ,t_error (ent)); -return 0 ; +blk_log_dump_pdu (s, ent); +trace_seq_printf (s,"[%d]\n" ,t_error (ent)); }else {if (t_sec (ent)) -return trace_seq_printf (s,"%llu + %u [%d]\n" , -t_sector (ent), -t_sec (ent),t_error (ent)); -return trace_seq_printf (s,"%llu [%d]\n" , -t_sector (ent),t_error (ent)); +trace_seq_printf (s,"%llu + %u [%d]\n" , +t_sector (ent), +t_sec (ent),t_error (ent)); +else +trace_seq_printf (s,"%llu [%d]\n" , +t_sector (ent),t_error (ent)); } } -static int blk_log_remap (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_remap (struct trace_seq *s,const struct trace_entry *ent) {struct blk_io_trace_remap r = { .device_from =0 , };get_pdu_remap (ent, &r); -return trace_seq_printf (s,"%llu + %u <- (%d,%d) %llu\n" , -t_sector (ent),t_sec (ent), -MAJOR (r.device_from),MINOR (r.device_from), - (unsigned long long )r.sector_from); +trace_seq_printf (s,"%llu + %u <- (%d,%d) %llu\n" , +t_sector (ent),t_sec (ent), +MAJOR (r.device_from),MINOR (r.device_from), + (unsigned long long )r.sector_from); } -static int blk_log_plug (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_plug (struct trace_seq *s,const struct trace_entry *ent) {char cmd[TASK_COMM_LEN ];trace_find_cmdline (ent->pid, cmd); -return trace_seq_printf (s,"[%s]\n" , cmd); +trace_seq_printf (s,"[%s]\n" , cmd); } -static int blk_log_unplug (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_unplug (struct trace_seq *s,const struct trace_entry *ent) {char cmd[TASK_COMM_LEN ];trace_find_cmdline (ent->pid, cmd); -return trace_seq_printf (s,"[%s] %llu\n" , cmd,get_pdu_int (ent)); +trace_seq_printf (s,"[%s] %llu\n" , cmd,get_pdu_int (ent)); } -static int blk_log_split (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_split (struct trace_seq *s,const struct trace_entry *ent) {char cmd[TASK_COMM_LEN ];trace_find_cmdline (ent->pid, cmd); -return trace_seq_printf (s,"%llu / %llu [%s]\n" ,t_sector (ent), -get_pdu_int (ent), cmd); +trace_seq_printf (s,"%llu / %llu [%s]\n" ,t_sector (ent), +get_pdu_int (ent), cmd); } -static int blk_log_msg (struct trace_seq *s,const struct trace_entry *ent) +static void blk_log_msg (struct trace_seq *s,const struct trace_entry *ent) { -int ret;const struct blk_io_trace *t =te_blk_io_trace (ent); - ret =trace_seq_putmem (s, t +1 , t->pdu_len); -if (ret) -return trace_seq_putc (s,'\n' ); -return ret; +trace_seq_putmem (s, t +1 , t->pdu_len); +trace_seq_putc (s,'\n' ); }/*
@@ -1339,7 +1327,7 @@ static void blk_tracer_reset(struct trace_array *tr)static const struct {const char *act[2 ]; -int (*print)(struct trace_seq *s,const struct trace_entry *ent); +void (*print)(struct trace_seq *s,const struct trace_entry *ent); } what2act[] = { [__BLK_TA_QUEUE] = {{"Q" ,"queue" }, blk_log_generic }, [__BLK_TA_BACKMERGE] = {{"M" ,"backmerge" }, blk_log_generic },
@@ -1364,7 +1352,6 @@ static enum print_line_t print_one_line(struct trace_iterator *iter,struct trace_seq *s = &iter->seq;const struct blk_io_trace *t;u16 what; -int ret;bool long_act;blk_log_action_t *log_action;
@@ -1374,21 +1361,18 @@ static enum print_line_t print_one_line(struct trace_iterator *iter, log_action = classic ? &blk_log_action_classic : &blk_log_action;if (t->action ==BLK_TN_MESSAGE ) { - ret =log_action (iter, long_act ?"message" :"m" ); -if (ret) - ret =blk_log_msg (s, iter->ent); - goto out; +log_action (iter, long_act ?"message" :"m" ); +blk_log_msg (s, iter->ent); }if (unlikely (what ==0 || what >=ARRAY_SIZE (what2act))) - ret =trace_seq_printf (s,"Unknown action %x\n" , what); +trace_seq_printf (s,"Unknown action %x\n" , what);else { - ret =log_action (iter, what2act[what].act[long_act]); -if (ret) - ret = what2act[what].log_action (iter, what2act[what].act[long_act]); + what2act[what].return ret ?TRACE_TYPE_HANDLED :TRACE_TYPE_PARTIAL_LINE ; + +return trace_handle_return (s); } static enum print_line_t blk_trace_event_print(struct trace_iterator *iter,
@@ -1397,7 +1381,7 @@ static enum print_line_t blk_trace_event_print(struct trace_iterator *iter,return print_one_line (iter, false); } -static int blk_trace_synthesize_old_trace (struct trace_iterator *iter) +static void blk_trace_synthesize_old_trace (struct trace_iterator *iter) {struct trace_seq *s = &iter->seq;struct blk_io_trace *t = (struct blk_io_trace *)iter->ent;
@@ -1407,18 +1391,18 @@ static int blk_trace_synthesize_old_trace(struct trace_iterator *iter) .time = iter->ts, }; -if (!trace_seq_putmem (s, &old, offset)) -return 0 ; -return trace_seq_putmem (s, &t->sector, -sizeof (old) - offset + t->pdu_len); +trace_seq_putmem (s, &old, offset); +trace_seq_putmem (s, &t->sector, +sizeof (old) - offset + t->pdu_len); }static enum print_line_t blk_trace_event_print_binary (struct trace_iterator *iter,int flags,struct trace_event *event) { -return blk_trace_synthesize_old_trace (iter) ? -TRACE_TYPE_HANDLED :TRACE_TYPE_PARTIAL_LINE ; +blk_trace_synthesize_old_trace (iter); + +return trace_handle_return (&iter->seq); }static enum print_line_t blk_tracer_print_line (struct trace_iterator *iter)
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 4/26 ring-buffer: Remove check of trace_seq_{puts,printf}() return values
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Remove checking the return value of all trace_seq_puts(). It was wrong anyway as only the last return value mattered. But as the trace_seq_puts() is going to be a void function in the future, we should not be checking the return value of it anyway.
Just return !trace_seq_has_overflowed() instead.
--- kernel/trace/ring_buffer.c | 75 ++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 39 deletions(-)
kernel/trace/ring_buffer.c +36 -39
@@ -34,21 +34,19 @@ static void update_pages_handler(struct work_struct *work);*/ int ring_buffer_print_entry_header (struct trace_seq *s) { -int ret; - - ret =trace_seq_puts (s,"# compressed entry header\n" ); - ret =trace_seq_puts (s,"\ttype_len : 5 bits\n" ); - ret =trace_seq_puts (s,"\ttime_delta : 27 bits\n" ); - ret =trace_seq_puts (s,"\tarray : 32 bits\n" ); - ret =trace_seq_putc (s,'\n' ); - ret =trace_seq_printf (s,"\tpadding : type == %d\n" , -RINGBUF_TYPE_PADDING ); - ret =trace_seq_printf (s,"\ttime_extend : type == %d\n" , -RINGBUF_TYPE_TIME_EXTEND ); - ret =trace_seq_printf (s,"\tdata max type_len == %d\n" , -RINGBUF_TYPE_DATA_TYPE_LEN_MAX ); +trace_seq_puts (s,"# compressed entry header\n" ); +trace_seq_puts (s,"\ttype_len : 5 bits\n" ); +trace_seq_puts (s,"\ttime_delta : 27 bits\n" ); +trace_seq_puts (s,"\tarray : 32 bits\n" ); +trace_seq_putc (s,'\n' ); +trace_seq_printf (s,"\tpadding : type == %d\n" , +RINGBUF_TYPE_PADDING ); +trace_seq_printf (s,"\ttime_extend : type == %d\n" , +RINGBUF_TYPE_TIME_EXTEND ); +trace_seq_printf (s,"\tdata max type_len == %d\n" , +RINGBUF_TYPE_DATA_TYPE_LEN_MAX ); -return ret; +return !trace_seq_has_overflowed (s); }/*
@@ -419,32 +417,31 @@ static inline int test_time_stamp(u64 delta)int ring_buffer_print_page_header (struct trace_seq *s) {struct buffer_data_page field; -int ret; - - ret =trace_seq_printf (s,"\tfield: u64 timestamp;\t" -"offset:0;\tsize:%u;\tsigned:%u;\n" , - (unsigned int )sizeof (field.time_stamp), - (unsigned int )is_signed_type (u64)); - - ret =trace_seq_printf (s,"\tfield: local_t commit;\t" -"offset:%u;\tsize:%u;\tsigned:%u;\n" , - (unsigned int )offsetof(typeof (field ), commit), - (unsigned int )sizeof (field.commit), - (unsigned int )is_signed_type (long)); - - ret =trace_seq_printf (s,"\tfield: int overwrite;\t" -"offset:%u;\tsize:%u;\tsigned:%u;\n" , - (unsigned int )offsetof(typeof (field ), commit), -1 , - (unsigned int )is_signed_type (long)); - - ret =trace_seq_printf (s,"\tfield: char data;\t" -"offset:%u;\tsize:%u;\tsigned:%u;\n" , - (unsigned int )offsetof(typeof (field ), data), - (unsigned int )BUF_PAGE_SIZE , - (unsigned int )is_signed_type (char)); -return ret; +trace_seq_printf (s,"\tfield: u64 timestamp;\t" +"offset:0;\tsize:%u;\tsigned:%u;\n" , + (unsigned int )sizeof (field.time_stamp), + (unsigned int )is_signed_type (u64)); + +trace_seq_printf (s,"\tfield: local_t commit;\t" +"offset:%u;\tsize:%u;\tsigned:%u;\n" , + (unsigned int )offsetof(typeof (field ), commit), + (unsigned int )sizeof (field.commit), + (unsigned int )is_signed_type (long)); + +trace_seq_printf (s,"\tfield: int overwrite;\t" +"offset:%u;\tsize:%u;\tsigned:%u;\n" , + (unsigned int )offsetof(typeof (field ), commit), +1 , + (unsigned int )is_signed_type (long)); + +trace_seq_printf (s,"\tfield: char data;\t" +"offset:%u;\tsize:%u;\tsigned:%u;\n" , + (unsigned int )offsetof(typeof (field ), data), + (unsigned int )BUF_PAGE_SIZE , + (unsigned int )is_signed_type (char)); + +return !trace_seq_has_overflowed (s); }struct rb_irq_work {
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 5/26 tracing: Have branch tracer use trace_handle_return() helper function
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The branch tracer should not be checking the trace_seq_printf() return value as that will soon be void. There's a new trace_handle_return() helper function that will return TRACE_TYPE_PARTIAL_LINE if the trace_seq overflowed and TRACE_TYPE_HANDLED otherwise.
--- kernel/trace/trace_branch.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
kernel/trace/trace_branch.c +7 -8
@@ -151,14 +151,13 @@ static enum print_line_t trace_branch_print(struct trace_iterator *iter,trace_assign_type (field, iter->ent); -if (trace_seq_printf (&iter->seq,"[%s] %s:%s:%d\n" , - field->correct ?" ok " :" MISS " , - field->func, - field->file, - field->line)) -return TRACE_TYPE_PARTIAL_LINE ; - -return TRACE_TYPE_HANDLED ; +trace_seq_printf (&iter->seq,"[%s] %s:%s:%d\n" , + field->correct ?" ok " :" MISS " , + field->func, + field->file, + field->line); + +return trace_handle_return (&iter->seq); }static void branch_print_header (struct seq_file *s)
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 6/26 tracing: Have function_graph use trace_seq_has_overflowed()
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Instead of doing individual checks all over the place that makes the code very messy. Just check trace_seq_has_overflowed() at the end or in strategic places.
This makes the code much cleaner and also helps with getting closer to removing the return values of trace_seq_printf() and friends.
--- kernel/trace/trace.h | 2 +- kernel/trace/trace_functions_graph.c | 382 +++++++++++------------------------ 2 files changed, 118 insertions(+), 266 deletions(-)
kernel/trace/trace.h +1 -1
@@ -726,7 +726,7 @@ extern unsigned long trace_flags;extern enum print_line_t print_graph_function_flags (struct trace_iterator *iter,u32 flags);extern void print_graph_headers_flags (struct seq_file *s,u32 flags); -extern enum print_line_t +extern void trace_print_graph_duration (unsigned long long duration,struct trace_seq *s);extern void graph_trace_open (struct trace_iterator *iter);extern void graph_trace_close (struct trace_iterator *iter);
kernel/trace/trace_functions_graph.c +117 -265
647 lines not on this page · raw message
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 7/26 kprobes/tracing: Use trace_seq_has_overflowed() for overflow checks
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Instead of checking the return value of trace_seq_printf() and friends for overflowing of the buffer, use the trace_seq_has_overflowed() helper function.
This cleans up the code quite a bit and also takes us a step closer to changing the return values of trace_seq_printf() and friends to void.
--- kernel/trace/trace_kprobe.c | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)
kernel/trace/trace_kprobe.c +16 -26
@@ -1024,27 +1024,22 @@ print_kprobe_event(struct trace_iterator *iter, int flags, field = (struct kprobe_trace_entry_head *)iter->ent; tp = container_of(event, struct trace_probe, call.event); -if (!trace_seq_printf (s,"%s: (" ,ftrace_event_name (&tp->call))) - goto partial; +trace_seq_printf (s,"%s: (" ,ftrace_event_name (&tp->call));if (!seq_print_ip_sym (s, field->ip, flags |TRACE_ITER_SYM_OFFSET )) - goto partial; + goto out; -if (!trace_seq_putc (s,')' )) - goto partial; +trace_seq_putc (s,')' ); data = (u8 *)&field[1 ];for (i =0 ; i < tp->nr_args; i++)if (!tp->args[i].type->if (!trace_seq_putc (s,'\n' )) - goto partial; + goto out; -return TRACE_TYPE_HANDLED ; -partial: -return TRACE_TYPE_PARTIAL_LINE ; +trace_seq_putc (s,'\n' ); + out: +return trace_handle_return (s); } static enum print_line_t
@@ -1060,33 +1055,28 @@ print_kretprobe_event(struct trace_iterator *iter, int flags, field = (struct kretprobe_trace_entry_head *)iter->ent; tp = container_of(event, struct trace_probe, call.event); -if (!trace_seq_printf (s,"%s: (" ,ftrace_event_name (&tp->call))) - goto partial; +trace_seq_printf (s,"%s: (" ,ftrace_event_name (&tp->call));if (!seq_print_ip_sym (s, field->ret_ip, flags |TRACE_ITER_SYM_OFFSET )) - goto partial; + goto out; -if (!trace_seq_puts (s," <- " )) - goto partial; +trace_seq_puts (s," <- " );if (!seq_print_ip_sym (s, field->func, flags & ~TRACE_ITER_SYM_OFFSET )) - goto partial; + goto out; -if (!trace_seq_putc (s,')' )) - goto partial; +trace_seq_putc (s,')' ); data = (u8 *)&field[1 ];for (i =0 ; i < tp->nr_args; i++)if (!tp->args[i].type->if (!trace_seq_putc (s,'\n' )) - goto partial; +trace_seq_putc (s,'\n' ); -return TRACE_TYPE_HANDLED ; -partial: -return TRACE_TYPE_PARTIAL_LINE ; + out: +return trace_handle_return (s); }
Signature · 1 line
2.1.1
Re: [PATCH 07/26 v5] kprobes/tracing: Use trace_seq_has_overflowed() for overflow checks
Quoted 6 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Instead of checking the return value of
trace_seq_printf()and friends for overflowing of the buffer, use thetrace_seq_has_overflowed()helper function.
Quoted 8 lines
This cleans up the code quite a bit and also takes us a step closer to changing the return values of
trace_seq_printf()and friends to void.Reviewed-by Srikar Dronamraju <[email protected]>Acked-by Masami Hiramatsu <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 8/26 tracing: Do not check return values of trace_seq_p*() for mmio tracer
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The return values for trace_seq_printf() and friends are going to be removed and they will become void functions. The mmio tracer checked their return and even did so incorrectly.
Some of the funtions which returned the values were never checked themselves. Removing all the checks simplifies the code.
Use trace_seq_has_overflowed() and trace_handle_return() where necessary instead.
--- kernel/trace/trace_mmiotrace.c | 52 +++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 31 deletions(-)
kernel/trace/trace_mmiotrace.c +21 -31
@@ -59,17 +59,15 @@ static void mmio_trace_start(struct trace_array *tr)mmio_reset_data (tr); } -static int mmio_print_pcidev (struct trace_seq *s,const struct pci_dev *dev) +static void mmio_print_pcidev (struct trace_seq *s,const struct pci_dev *dev) { -int ret =0 ;int i;resource_size_t start, end;const struct pci_driver *drv =pci_dev_driver (dev); -/* XXX: incomplete checks for trace_seq_printf() return value */ - ret +=trace_seq_printf (s,"PCIDEV %02x%02x %04x%04x %x" , - dev->bus->number, dev->devfn, - dev->vendor, dev->device, dev->irq); +trace_seq_printf (s,"PCIDEV %02x%02x %04x%04x %x" , + dev->bus->number, dev->devfn, + dev->vendor, dev->device, dev->irq);/* * XXX: is pci_resource_to_user() appropriate, since we are * supposed to interpret the __ioremap() phys_addr argument based on
@@ -77,21 +75,20 @@ static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)*/ for (i =0 ; i <7 ; i++) {pci_resource_to_user (dev, i, &dev->resource[i], &start, &end); - ret +=trace_seq_printf (s," %llx" , +trace_seq_printf (s," %llx" , (unsigned long long )(start | (dev->resource[i].flags &PCI_REGION_FLAG_MASK ))); }for (i =0 ; i <7 ; i++) {pci_resource_to_user (dev, i, &dev->resource[i], &start, &end); - ret +=trace_seq_printf (s," %llx" , +trace_seq_printf (s," %llx" , dev->resource[i].start < dev->resource[i].end ? (unsigned long long )(end - start) +1 :0 ); }if (drv) - ret +=trace_seq_printf (s," %s\n" , drv->name); +trace_seq_printf (s," %s\n" , drv->name);else - ret +=trace_seq_puts (s," \n" ); -return ret; +trace_seq_puts (s," \n" ); }static void destroy_header_iter (struct header_iter *hiter)
@@ -179,28 +176,27 @@ static enum print_line_t mmio_print_rw(struct trace_iterator *iter)unsigned long long t =ns2usecs (iter->ts);unsigned long usec_rem =do_div (t,USEC_PER_SEC );unsigned secs = (unsigned long )t; -int ret =1 ;trace_assign_type (field, entry); rw = &field->rw;switch (rw->opcode) {case MMIO_READ : - ret =trace_seq_printf (s, +trace_seq_printf (s,"R %d %u.%06lu %d 0x%llx 0x%lx 0x%lx %d\n" , rw->width, secs, usec_rem, rw->map_id, (unsigned long long )rw->phys, rw->value, rw->pc,0 );break ;case MMIO_WRITE : - ret =trace_seq_printf (s, +trace_seq_printf (s,"W %d %u.%06lu %d 0x%llx 0x%lx 0x%lx %d\n" , rw->width, secs, usec_rem, rw->map_id, (unsigned long long )rw->phys, rw->value, rw->pc,0 );break ;case MMIO_UNKNOWN_OP : - ret = trace_seq_printf(s, + trace_seq_printf(s, "UNKNOWN %u.%06lu %d 0x%llx %02lx,%02lx," "%02lx 0x%lx %d\n", secs, usec_rem, rw->map_id,
@@ -209,12 +205,11 @@ static enum print_line_t mmio_print_rw(struct trace_iterator *iter) (rw->value >> 0) & 0xff, rw->pc, 0);break ;default : - ret =trace_seq_puts (s,"rw what?\n" ); +trace_seq_puts (s,"rw what?\n" );break ; } -if (ret) -return TRACE_TYPE_HANDLED ; -return TRACE_TYPE_PARTIAL_LINE ; + +return trace_handle_return (s); }static enum print_line_t mmio_print_map (struct trace_iterator *iter)
@@ -226,31 +221,29 @@ static enum print_line_t mmio_print_map(struct trace_iterator *iter)unsigned long long t =ns2usecs (iter->ts);unsigned long usec_rem =do_div (t,USEC_PER_SEC );unsigned secs = (unsigned long )t; -int ret;trace_assign_type (field, entry); m = &field->map;switch (m->opcode) {case MMIO_PROBE : - ret =trace_seq_printf (s, +trace_seq_printf (s,"MAP %u.%06lu %d 0x%llx 0x%lx 0x%lx 0x%lx %d\n" , secs, usec_rem, m->map_id, (unsigned long long )m->phys, m->virt, m->len,0UL ,0 );break ;case MMIO_UNPROBE : - ret =trace_seq_printf (s, +trace_seq_printf (s,"UNMAP %u.%06lu %d 0x%lx %d\n" , secs, usec_rem, m->map_id,0UL ,0 );break ;default : - ret =trace_seq_puts (s,"map what?\n" ); +trace_seq_puts (s,"map what?\n" );break ; } -if (ret) -return TRACE_TYPE_HANDLED ; -return TRACE_TYPE_PARTIAL_LINE ; + +return trace_handle_return (s); }static enum print_line_t mmio_print_mark (struct trace_iterator *iter)
@@ -262,14 +255,11 @@ static enum print_line_t mmio_print_mark(struct trace_iterator *iter)unsigned long long t =ns2usecs (iter->ts);unsigned long usec_rem =do_div (t,USEC_PER_SEC );unsigned secs = (unsigned long )t; -int ret;/* The trailing newline must be in the message. */ - ret =trace_seq_printf (s,"MARK %u.%06lu %s" , secs, usec_rem, msg); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; +trace_seq_printf (s,"MARK %u.%06lu %s" , secs, usec_rem, msg); -return TRACE_TYPE_HANDLED ; +return trace_handle_return (s); }static enum print_line_t mmio_print_line (struct trace_iterator *iter)
Signature · 1 line
2.1.1
Re: [PATCH 08/26 v5] tracing: Do not check return values of trace_seq_p*() for mmio tracer
Quoted 6 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The return values for
trace_seq_printf()and friends are going to be removed and they will become void functions. The mmio tracer checked their return and even did so incorrectly.
Quoted 7 lines
Some of the funtions which returned the values were never checked themselves. Removing all the checks simplifies the code.
Use
trace_seq_has_overflowed()andtrace_handle_return()where necessary instead.Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 9/26 tracing/probes: Do not use return value of trace_seq_printf()
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will soon not have a return value and will only be a void function. Use trace_seq_has_overflowed() instead to know if the trace_seq operations succeeded or not.
--- kernel/trace/trace_probe.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
kernel/trace/trace_probe.c +6 -4
@@ -40,7 +40,8 @@ const char *reserved_field_names[] = {int PRINT_TYPE_FUNC_NAME (type )(struct trace_seq *s,const char *name, \void *data,void *ent) \ { \ -return trace_seq_printf (s," %s=" fmt, name, *(type *)data); \ +trace_seq_printf (s," %s=" fmt, name, *(type *)data); \ +return !trace_seq_has_overflowed (s); \ } \const char PRINT_TYPE_FMT_NAME (type )[] = fmt; \NOKPROBE_SYMBOL (PRINT_TYPE_FUNC_NAME (type));
@@ -61,10 +62,11 @@ int PRINT_TYPE_FUNC_NAME(string)(struct trace_seq *s, const char *name,int len = *(u32 *)data >>16 ;if (!len) -return trace_seq_printf (s," %s=(fault)" , name); +trace_seq_printf (s," %s=(fault)" , name);else -return trace_seq_printf (s," %s=\"%s\"" , name, - (const char *)get_loc_data (data, ent)); +trace_seq_printf (s," %s=\"%s\"" , name, + (const char *)get_loc_data (data, ent)); +return !trace_seq_has_overflowed (s); }NOKPROBE_SYMBOL (PRINT_TYPE_FUNC_NAME (string));
Signature · 1 line
2.1.1
Re: [PATCH 09/26 v5] tracing/probes: Do not use return value of trace_seq_printf()
Quoted 11 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions
trace_seq_printf()and friends will soon not have a return value and will only be a void function. Usetrace_seq_has_overflowed()instead to know if thetrace_seqoperations succeeded or not.Cc Namhyung Kim <[email protected]>Acked-by Masami Hiramatsu <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 10/26 tracing/uprobes: Do not use return values of trace_seq_printf()
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will soon no longer have return values. Using trace_seq_has_overflowed() and trace_handle_return() should be used instead.
--- kernel/trace/trace_uprobe.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
kernel/trace/trace_uprobe.c +10 -13
@@ -852,16 +852,14 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e tu = container_of(event, struct trace_uprobe, tp.call.event);if (is_ret_probe (tu)) { -if (!trace_seq_printf (s,"%s: (0x%lx <- 0x%lx)" , -ftrace_event_name (&tu->tp.call), - entry->vaddr[1 ], entry->vaddr[0 ])) - goto partial; +trace_seq_printf (s,"%s: (0x%lx <- 0x%lx)" , +ftrace_event_name (&tu->tp.call), + entry->vaddr[1 ], entry->vaddr[0 ]); data =DATAOF_TRACE_ENTRY (entry, true); }else { -if (!trace_seq_printf (s,"%s: (0x%lx)" , -ftrace_event_name (&tu->tp.call), - entry->vaddr[0 ])) - goto partial; +trace_seq_printf (s,"%s: (0x%lx)" , +ftrace_event_name (&tu->tp.call), + entry->vaddr[0 ]); data =DATAOF_TRACE_ENTRY (entry, false); }
@@ -869,14 +867,13 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *estruct probe_arg *parg = &tu->tp.args[i];if (!parg->type->if (trace_seq_putc (s,'\n' )) -return TRACE_TYPE_HANDLED ; +trace_seq_putc (s,'\n' ); -partial: -return TRACE_TYPE_PARTIAL_LINE ; + out: +return trace_handle_return (s); } typedef bool (*filter_func_t)(struct uprobe_consumer *self,
Signature · 1 line
2.1.1
Re: [PATCH 10/26 v5] tracing/uprobes: Do not use return values of trace_seq_printf()
Quoted 8 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions
trace_seq_printf()and friends will soon no longer have return values. Usingtrace_seq_has_overflowed()andtrace_handle_return()should be used instead.
This looks good to me :)
Quoted 54 lines
Cc Masami Hiramatsu <[email protected]>Cc Namhyung Kim <[email protected]>Cc Srikar Dronamraju <[email protected]>Signed-off-by Steven Rostedt <[email protected]>--- kernel/trace/trace_uprobe.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)kernel/trace/trace_uprobe.c +10
-13@@ -852,16 +852,14 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *e tu = container_of(event, struct trace_uprobe, tp.call.event);if (is_ret_probe (tu)) { -if (!trace_seq_printf (s,"%s: (0x%lx <- 0x%lx)" , -ftrace_event_name (&tu->tp.call), - entry->vaddr[1 ], entry->vaddr[0 ])) - goto partial; +trace_seq_printf (s,"%s: (0x%lx <- 0x%lx)" , +ftrace_event_name (&tu->tp.call), + entry->vaddr[1 ], entry->vaddr[0 ]); data =DATAOF_TRACE_ENTRY (entry, true); }else { -if (!trace_seq_printf (s,"%s: (0x%lx)" , -ftrace_event_name (&tu->tp.call), - entry->vaddr[0 ])) - goto partial; +trace_seq_printf (s,"%s: (0x%lx)" , +ftrace_event_name (&tu->tp.call), + entry->vaddr[0 ]); data =DATAOF_TRACE_ENTRY (entry, false); }@@ -869,14 +867,13 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *estruct probe_arg *parg = &tu->tp.args[i];if (!parg->type->if (trace_seq_putc (s,'\n' )) -return TRACE_TYPE_HANDLED ; +trace_seq_putc (s,'\n' ); -partial: -return TRACE_TYPE_PARTIAL_LINE ; + out: +return trace_handle_return (s); } typedef bool (*filter_func_t)(struct uprobe_consumer *self,
Signature · 4 lines
Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected]
Re: [PATCH 10/26 v5] tracing/uprobes: Do not use return values of trace_seq_printf()
In reply to Steven RostedtQuoted 6 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions
trace_seq_printf()and friends will soon no longer have return values. Usingtrace_seq_has_overflowed()andtrace_handle_return()should be used instead.
Quoted 6 lines
Cc Masami Hiramatsu <[email protected]>Cc Namhyung Kim <[email protected]>Cc Srikar Dronamraju <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
Looks good to me.
Quoted 1 line
---
Signature · 2 lines
Thanks and Regards Srikar Dronamraju
Re: [PATCH 10/26 v5] tracing/uprobes: Do not use return values of trace_seq_printf()
In reply to Steven RostedtQuoted 6 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The functions
trace_seq_printf()and friends will soon no longer have return values. Usingtrace_seq_has_overflowed()andtrace_handle_return()should be used instead.
Quoted 6 lines
Cc Masami Hiramatsu <[email protected]>Cc Namhyung Kim <[email protected]>Cc Srikar Dronamraju <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 11/26 tracing: Do not use return values of trace_seq_printf() in syscall tracing
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The functions trace_seq_printf() and friends will not be returning values soon and will be void functions. To know if they succeeded or not, the functions trace_seq_has_overflowed() and trace_handle_return() should be used instead.
--- kernel/trace/trace_syscalls.c | 43 +++++++++++++++---------------------------- 1 file changed, 15 insertions(+), 28 deletions(-)
kernel/trace/trace_syscalls.c +15 -28
@@ -114,7 +114,7 @@ print_syscall_enter(struct trace_iterator *iter, int flags,struct trace_entry *ent = iter->ent;struct syscall_trace_enter *trace;struct syscall_metadata *entry; -int i, ret, syscall; +int i, syscall; trace = (typeof (trace ))ent; syscall = trace->nr;
@@ -128,35 +128,24 @@ print_syscall_enter(struct trace_iterator *iter, int flags, goto end; } - ret =trace_seq_printf (s,"%s(" , entry->name); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; +trace_seq_printf (s,"%s(" , entry->name);for (i =0 ; i < entry->nb_args; i++) {/* parameter types */ -if (trace_flags &TRACE_ITER_VERBOSE ) { - ret =trace_seq_printf (s,"%s " , entry->types[i]); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; - } +if (trace_flags &TRACE_ITER_VERBOSE ) +trace_seq_printf (s,"%s " , entry->types[i]); +/* parameter values */ - ret =trace_seq_printf (s,"%s: %lx%s" , entry->args[i], - trace->args[i], - i == entry->nb_args -1 ?"" :", " ); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; +trace_seq_printf (s,"%s: %lx%s" , entry->args[i], + trace->args[i], + i == entry->nb_args -1 ?"" :", " ); } - ret =trace_seq_putc (s,')' ); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; - +trace_seq_putc (s,')' ); end: - ret =trace_seq_putc (s,'\n' ); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; +trace_seq_putc (s,'\n' ); -return TRACE_TYPE_HANDLED ; +return trace_handle_return (s); } static enum print_line_t
@@ -168,7 +157,6 @@ print_syscall_exit(struct trace_iterator *iter, int flags,struct syscall_trace_exit *trace;int syscall;struct syscall_metadata *entry; -int ret; trace = (typeof (trace ))ent; syscall = trace->nr;
@@ -176,7 +164,7 @@ print_syscall_exit(struct trace_iterator *iter, int flags,if (!entry) {trace_seq_putc (s,'\n' ); -return TRACE_TYPE_HANDLED ; + goto out; }if (entry->exit_event->event.type != ent->type) {
@@ -184,12 +172,11 @@ print_syscall_exit(struct trace_iterator *iter, int flags,return TRACE_TYPE_UNHANDLED ; } - ret =trace_seq_printf (s,"%s -> 0x%lx\n" , entry->name, +trace_seq_printf (s,"%s -> 0x%lx\n" , entry->name, trace->ret); -if (!ret) -return TRACE_TYPE_PARTIAL_LINE ; -return TRACE_TYPE_HANDLED ; + out: +return trace_handle_return (s); }extern char *__bad_type_size (void );
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 12/26 tracing: Remove return values of most trace_seq_*() functions
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The trace_seq_printf() and friends are used to store strings into a buffer that can be passed around from function to function. If the trace_seq buffer fills up, it will not print any more. The return values were somewhat inconsistant and using trace_seq_has_overflowed() was a better way to know if the write to the trace_seq buffer succeeded or not.
Now that all users have removed reading the return value of the printf() type functions, they can safely return void and keep future users of them from reading the inconsistent values as well.
--- include/linux/trace_seq.h | 37 +++++++++------------ kernel/trace/trace_seq.c | 84 +++++++++++++---------------------------------- 2 files changed, 38 insertions(+), 83 deletions(-)
include/linux/trace_seq.h +15 -22
@@ -57,40 +57,37 @@ static inline bool trace_seq_has_overflowed(struct trace_seq *s)*/ #ifdef CONFIG_TRACING extern __printf(2, 3) -int trace_seq_printf(struct trace_seq *s, const char *fmt, ...); +void trace_seq_printf(struct trace_seq *s, const char *fmt, ...); extern __printf(2, 0) -int trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args); -extern int +void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args); +extern void trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary); extern int trace_print_seq(struct seq_file *m, struct trace_seq *s); extern int trace_seq_to_user(struct trace_seq *s, char __user *ubuf, int cnt); -extern int trace_seq_puts(struct trace_seq *s, const char *str); -extern int trace_seq_putc(struct trace_seq *s, unsigned char c); -extern int trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len); -extern int trace_seq_putmem_hex(struct trace_seq *s, const void *mem, +extern void trace_seq_puts(struct trace_seq *s, const char *str); +extern void trace_seq_putc(struct trace_seq *s, unsigned char c); +extern void trace_seq_putmem(struct trace_seq *s, const void *mem, unsigned int len); +extern void trace_seq_putmem_hex(struct trace_seq *s, const void *mem, unsigned int len); extern int trace_seq_path(struct trace_seq *s, const struct path *path); -extern int trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp, +extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp, int nmaskbits); #else /* CONFIG_TRACING */ -static inline int trace_seq_printf(struct trace_seq *s, const char *fmt, ...) +static inline void trace_seq_printf(struct trace_seq *s, const char *fmt, ...) { - return 0; } -static inline int +static inline void trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary) { - return 0; } -static inline int +static inline void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp, int nmaskbits) { - return 0; } static inline int trace_print_seq(struct seq_file *m, struct trace_seq *s)
@@ -102,23 +99,19 @@ static inline int trace_seq_to_user(struct trace_seq *s, char __user *ubuf, {return 0 ; } -static inline int trace_seq_puts (struct trace_seq *s,const char *str) +static inline void trace_seq_puts (struct trace_seq *s,const char *str) { -return 0 ; } -static inline int trace_seq_putc (struct trace_seq *s,unsigned char c) +static inline void trace_seq_putc (struct trace_seq *s,unsigned char c) { -return 0 ; } -static inline int +static inline void trace_seq_putmem (struct trace_seq *s,const void *mem,unsigned int len) { -return 0 ; } -static inline int trace_seq_putmem_hex (struct trace_seq *s,const void *mem, +static inline void trace_seq_putmem_hex (struct trace_seq *s,const void *mem,unsigned int len) { -return 0 ; }static inline int trace_seq_path (struct trace_seq *s,const struct path *path) {
kernel/trace/trace_seq.c +23 -61
235 lines not on this page · raw message
Signature · 1 line
2.1.1
Re: [PATCH 12/26 v5] tracing: Remove return values of most trace_seq_*() functions
Quoted 8 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
The
trace_seq_printf()and friends are used to store strings into a buffer that can be passed around from function to function. If thetrace_seqbuffer fills up, it will not print any more. The return values were somewhat inconsistant and usingtrace_seq_has_overflowed()was a better way to know if the write to thetrace_seqbuffer succeeded or not.
Quoted 7 lines
Now that all users have removed reading the return value of the
printf()type functions, they can safely return void and keep future users of them from reading the inconsistent values as well.Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 13/26 tracing: Fix return value of ftrace_raw_output_prep()
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
If the trace_seq of ftrace_raw_output_prep() is full this function returns TRACE_TYPE_PARTIAL_LINE, otherwise it returns zero.
The problem is that TRACE_TYPE_PARTIAL_LINE happens to be zero!
The thing is, the caller of ftrace_raw_output_prep() expects a success to be zero. Change that to expect it to be TRACE_TYPE_HANDLED.
--- include/trace/ftrace.h | 2 +- kernel/trace/trace_output.c | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-)
include/trace/ftrace.h +1 -1
@@ -277,7 +277,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags, \ field = (typeof (field ))iter->ent; \ \ ret =ftrace_raw_output_prep (iter, trace_event); \ -if (ret) \ +if (ret !=TRACE_TYPE_HANDLED ) \return ret; \ \trace_seq_printf (s, print); \
kernel/trace/trace_output.c +1 -4
@@ -196,10 +196,7 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,trace_seq_init (p);trace_seq_printf (s,"%s: " ,ftrace_event_name (event)); -if (trace_seq_has_overflowed (s)) -return TRACE_TYPE_PARTIAL_LINE ; - -return 0 ; +return trace_handle_return (s); }EXPORT_SYMBOL (ftrace_raw_output_prep);
Signature · 1 line
2.1.1
Re: [PATCH 13/26 v5] tracing: Fix return value of ftrace_raw_output_prep()
Quoted 7 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
If the
trace_seqofftrace_raw_output_prep()is full this function returnsTRACE_TYPE_PARTIAL_LINE, otherwise it returns zero.The problem is that
TRACE_TYPE_PARTIAL_LINEhappens to be zero!
Quoted 8 lines
The thing is, the caller of
ftrace_raw_output_prep()expects a success to be zero. Change that to expect it to beTRACE_TYPE_HANDLED.Reminded-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerQuoted 4 lines · On Fri, 14 Nov 2014 23:58:47 -0500 Steven Rostedt <[email protected]> wrote:
Version 5!
Full diff between v4 and v5 will be replied to this email.
Here it is:
-- Steve
include/linux/seq_buf.h +3 -2
@@ -67,7 +67,7 @@ seq_buf_buffer_left(struct seq_buf *s)/* How much buffer was written? */ static inline unsigned int seq_buf_used (struct seq_buf *s) { -return min ((s)->len, (s)->size); +return min (s->len, s->size); }/**
@@ -105,7 +105,8 @@ static inline void seq_buf_commit(struct seq_buf *s, int num)if (num <0 ) {seq_buf_set_overflow (s); }else { -BUG_ON (s->len + num > s->size +1 ); +/* num must be negative on overflow */ +BUG_ON (s->len + num > s->size); s->len += num; } }
include/linux/trace_seq.h +19 -1
@@ -24,6 +24,24 @@ trace_seq_init(struct trace_seq *s) }/** +* trace_seq_used - amount of actual data written to buffer +* @s: trace sequence descriptor +* +* Returns the amount of data written to the buffer. +* +* IMPORTANT! +* +* Use this instead of @s->seq.len if you need to pass the amount +* of data from the buffer to another buffer (userspace, or what not). +* The @s->seq.len on overflow is bigger than the buffer size and +* using it can cause access to undefined memory. +*/ +static inline int trace_seq_used (struct trace_seq *s) +{ +return seq_buf_used (&s->seq); +} + +/** * trace_seq_buffer_ptr - return pointer to next location in buffer * @s: trace sequence descriptor *
@@ -35,7 +53,7 @@ trace_seq_init(struct trace_seq *s)static inline unsigned char *trace_seq_buffer_ptr (struct trace_seq *s) { -return s->buffer + s->seq.len; +return s->buffer +seq_buf_used (&s->seq); }/**
include/trace/ftrace.h +1 -1
@@ -277,7 +277,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags, \ field = (typeof (field ))iter->ent; \ \ ret =ftrace_raw_output_prep (iter, trace_event); \ -if (ret) \ +if (ret !=TRACE_TYPE_HANDLED ) \return ret; \ \trace_seq_printf (s, print); \
kernel/trace/trace.c +29 -15
104 lines not on this page · raw message
kernel/trace/trace_events.c +6 -3
@@ -1044,7 +1044,8 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,mutex_unlock (&event_mutex);if (file) - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
@@ -1210,7 +1211,8 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,trace_seq_init (s);print_subsystem_event_filter (system, s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
@@ -1265,7 +1267,8 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)trace_seq_init (s);func (s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
kernel/trace/trace_functions_graph.c +6 -7
@@ -677,12 +677,8 @@ print_graph_irq(struct trace_iterator *iter, unsigned long addr, }/* Latency format */ -if (trace_flags &TRACE_ITER_LATENCY_FMT ) { - ret =print_graph_lat_fmt (s, ent); -if (ret ==TRACE_TYPE_PARTIAL_LINE ) -return TRACE_TYPE_PARTIAL_LINE ; - } - +if (trace_flags &TRACE_ITER_LATENCY_FMT ) +print_graph_lat_fmt (s, ent); }/* No overhead */
@@ -1157,6 +1153,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,return ret; } +if (trace_seq_has_overflowed (s)) + goto out; +/* Strip ending newline */ if (s->buffer[s->seq.len -1 ] =='\n' ) { s->buffer[s->seq.len -1 ] ='\0' ;
@@ -1164,7 +1163,7 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent, }trace_seq_puts (s," */\n" ); - + out:return trace_handle_return (s); }
kernel/trace/trace_output.c +1 -4
@@ -196,10 +196,7 @@ int ftrace_raw_output_prep(struct trace_iterator *iter,trace_seq_init (p);trace_seq_printf (s,"%s: " ,ftrace_event_name (event)); -if (trace_seq_has_overflowed (s)) -return TRACE_TYPE_PARTIAL_LINE ; - -return 0 ; +return trace_handle_return (s); }EXPORT_SYMBOL (ftrace_raw_output_prep);
kernel/trace/trace_seq.c +2 -5
@@ -30,7 +30,7 @@#define TRACE_SEQ_BUF_LEFT (s) seq_buf_buffer_left(&(s)->seq)/* How much buffer is written? */ -#define TRACE_SEQ_BUF_USED (s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1)) +#define TRACE_SEQ_BUF_USED (s) seq_buf_used(&(s)->seq)/* * trace_seq should work with being initialized with 0s.
@@ -98,7 +98,6 @@ void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)if (unlikely (seq_buf_has_overflowed (&s->seq))) { s->seq.len = save_len; s->full =1 ; -return ; } }EXPORT_SYMBOL_GPL (trace_seq_printf);
@@ -144,20 +143,18 @@ EXPORT_SYMBOL_GPL(trace_seq_bitmask);void trace_seq_vprintf (struct trace_seq *s,const char *fmt,va_list args) {unsigned int save_len = s->seq.len; -int ret;if (s->full)return ;__trace_seq_init (s); - ret =seq_buf_vprintf (&s->seq, fmt, args); +seq_buf_vprintf (&s->seq, fmt, args);/* If we can't write it all, don't bother writing anything */ if (unlikely (seq_buf_has_overflowed (&s->seq))) { s->seq.len = save_len; s->full =1 ; -return ; } }EXPORT_SYMBOL_GPL (trace_seq_vprintf);
kernel/trace/trace_uprobe.c +3 -1
@@ -866,11 +866,13 @@ print_uprobe_event(struct trace_iterator *iter, int flags, struct trace_event *efor (i =0 ; i < tu->tp.nr_args; i++) {struct probe_arg *parg = &tu->tp.args[i]; - parg->type->if (!parg->type->trace_seq_putc (s,'\n' ); + out:return trace_handle_return (s); }
lib/seq_buf.c +11 -6
@@ -16,6 +16,11 @@#include <linux/seq_file.h> #include <linux/seq_buf.h> +static bool seq_buf_can_fit (struct seq_buf *s,size_t len) +{ +return s->len + len <= s->size; +} +/** * seq_buf_print_seq - move the contents of seq_buf into a seq_file * @m: the seq_file descriptor that is the destination
@@ -48,7 +53,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)if (s->len < s->size) { len =vsnprintf (s->buffer + s->len, s->size - s->len, fmt, args); -if (s->len + len <= s->size) { +if (seq_buf_can_fit (s, len)) { s->len += len;return 0 ; }
@@ -141,7 +146,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)if (s->len < s->size) { ret =bstr_printf (s->buffer + s->len, len, fmt, binary); -if (s->len + ret <= s->size) { +if (seq_buf_can_fit (s, ret)) { s->len += ret;return 0 ; }
@@ -166,7 +171,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)WARN_ON (s->size ==0 ); -if (s->len + len <= s->size) { +if (seq_buf_can_fit (s, len)) {memcpy (s->buffer + s->len, str, len); s->len += len;return 0 ;
@@ -188,7 +193,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c) {WARN_ON (s->size ==0 ); -if (s->len +1 <= s->size) { +if (seq_buf_can_fit (s,1 )) { s->buffer[s->len++] = c;return 0 ; }
@@ -212,7 +217,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len) {WARN_ON (s->size ==0 ); -if (s->len + len <= s->size) { +if (seq_buf_can_fit (s, len)) {memcpy (s->buffer + s->len, mem, len); s->len += len;return 0 ;
@@ -332,7 +337,7 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)if (s->len <= s->readpos)return -EBUSY ; - len = s->len - s->readpos; + len =seq_buf_used (s) - s->readpos;if (cnt > len) cnt = len; ret =copy_to_user (ubuf, s->buffer + s->readpos, cnt);
Steven Rostedt
maintainerPATCH v5 14/26 tracing: Create seq_buf layer in trace_seq
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Create a seq_buf layer that trace_seq sits on. The seq_buf will not be limited to page size. This will allow other usages of seq_buf instead of a hard set PAGE_SIZE one that trace_seq has.
--- include/linux/seq_buf.h | 81 +++++++++ include/linux/trace_seq.h | 12 +- kernel/trace/Makefile | 1 + kernel/trace/seq_buf.c | 341 +++++++++++++++++++++++++++++++++++ kernel/trace/trace.c | 39 ++-- kernel/trace/trace_events.c | 6 +- kernel/trace/trace_functions_graph.c | 6 +- kernel/trace/trace_seq.c | 177 +++++++++--------- 8 files changed, 538 insertions(+), 125 deletions(-) create mode 100644 include/linux/seq_buf.h create mode 100644 kernel/trace/seq_buf.c
include/linux/seq_buf.h new file +81 -0
@@ -0,0 +1,81 @@ +#ifndef _LINUX_SEQ_BUF_H +#define _LINUX_SEQ_BUF_H + +#include <linux/fs.h> + +/* +* Trace sequences are used to allow a function to call several other functions +* to create a string of data to use. +*/ + +/** +* seq_buf - seq buffer structure +* @buffer: pointer to the buffer +* @size: size of the buffer +* @len: the amount of data inside the buffer +* @readpos: The next position to read in the buffer. +*/ +struct seq_buf { +unsigned char *buffer; +unsigned int size; +unsigned int len; +unsigned int readpos; +}; + +static inline void +seq_buf_init (struct seq_buf *s,unsigned char *buf,unsigned int size) +{ + s->buffer = buf; + s->size = size; + s->len =0 ; + s->readpos =0 ; +} + +/* +* seq_buf have a buffer that might overflow. When this happens +* the len and size are set to be equal. +*/ +static inline bool +seq_buf_has_overflowed (struct seq_buf *s) +{ +return s->len == s->size; +} + +static inline void +seq_buf_set_overflow (struct seq_buf *s) +{ + s->len = s->size; +} + +/* +* How much buffer is left on the seq_buf? +*/ +static inline unsigned int +seq_buf_buffer_left (struct seq_buf *s) +{ +if (seq_buf_has_overflowed (s)) +return 0 ; + +return (s->size -1 ) - s->len; +} + +extern __printf(2, 3) +int seq_buf_printf(struct seq_buf *s, const char *fmt, ...); +extern __printf(2, 0) +int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args); +extern int +seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary); +extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s); +extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, + int cnt); +extern int seq_buf_puts(struct seq_buf *s, const char *str); +extern int seq_buf_putc(struct seq_buf *s, unsigned char c); +extern int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len); +extern int seq_buf_putmem_hex(struct seq_buf *s, const void *mem, + unsigned int len); +extern int seq_buf_path(struct seq_buf *s, const struct path *path); + +extern int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp, + int nmaskbits); + +#endif /* _LINUX_SEQ_BUF_H */
include/linux/trace_seq.h +5 -7
@@ -1,7 +1,7 @@#ifndef _LINUX_TRACE_SEQ_H#define _LINUX_TRACE_SEQ_H -#include <linux/fs.h> +#include <linux/seq_buf.h> #include <asm/page.h>
@@ -12,16 +12,14 @@struct trace_seq {unsigned char buffer[PAGE_SIZE ]; -unsigned int len; -unsigned int readpos; +struct seq_buf seq;int full; };static inline void trace_seq_init (struct trace_seq *s) { - s->len =0 ; - s->readpos =0 ; +seq_buf_init (&s->seq, s->buffer,PAGE_SIZE ); s->full =0 ; }
@@ -37,7 +35,7 @@ trace_seq_init(struct trace_seq *s)static inline unsigned char *trace_seq_buffer_ptr (struct trace_seq *s) { -return s->buffer + s->len; +return s->buffer + s->seq.len; }/**
@@ -49,7 +47,7 @@ trace_seq_buffer_ptr(struct trace_seq *s)*/ static inline bool trace_seq_has_overflowed (struct trace_seq *s) { -return s->full || s->len >PAGE_SIZE -1 ; +return s->full ||seq_buf_has_overflowed (&s->seq); }/*
kernel/trace/Makefile +1 -0
@@ -29,6 +29,7 @@ obj-$(CONFIG_RING_BUFFER_BENCHMARK) += ring_buffer_benchmark.o obj-$(CONFIG_TRACING) += trace.o obj-$(CONFIG_TRACING) += trace_output.o obj-$(CONFIG_TRACING) += trace_seq.o +obj-$(CONFIG_TRACING) += seq_buf.o obj-$(CONFIG_TRACING) += trace_stat.o obj-$(CONFIG_TRACING) += trace_printk.o obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o
kernel/trace/seq_buf.c new file +341 -0
341 lines not on this page · raw message
kernel/trace/trace.c +21 -18
@@ -939,19 +939,20 @@ out:return ret; } +/* TODO add a seq_buf_to_buffer() */ static ssize_t trace_seq_to_buffer (struct trace_seq *s,void *buf,size_t cnt) {int len; -if (s->len <= s->readpos) +if (s->seq.len <= s->seq.readpos)return -EBUSY ; - len = s->len - s->readpos; + len = s->seq.len - s->seq.readpos;if (cnt > len) cnt = len; -memcpy (buf, s->buffer + s->readpos, cnt); +memcpy (buf, s->buffer + s->seq.readpos, cnt); - s->readpos += cnt; + s->seq.readpos += cnt;return cnt; }
@@ -4315,6 +4316,8 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) goto out; } +trace_seq_init (&iter->seq); +/* * We make a copy of the current tracer to avoid concurrent * changes on it while we are reading.
@@ -4511,18 +4514,18 @@ waitagain:trace_access_lock (iter->cpu_file);while (trace_find_next_entry_inc (iter) !=NULL ) {enum print_line_t ret; -int len = iter->seq.len; +int len = iter->seq.seq.len; ret =print_trace_line (iter);if (ret ==TRACE_TYPE_PARTIAL_LINE ) {/* don't print partial lines */ - iter->seq.len = len; + iter->seq.seq.len = len;break ; }if (ret !=TRACE_TYPE_NO_CONSUME )trace_consume (iter); -if (iter->seq.len >= cnt) +if (iter->seq.seq.len >= cnt)break ;/*
@@ -4538,7 +4541,7 @@ waitagain:/* Now copy what we have to the user */ sret =trace_seq_to_user (&iter->seq, ubuf, cnt); -if (iter->seq.readpos >= iter->seq.len) +if (iter->seq.seq.readpos >= iter->seq.seq.len)trace_seq_init (&iter->seq);/*
@@ -4576,16 +4579,16 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)/* Seq buffer is page-sized, exactly what we need. */ for (;;) { - count = iter->seq.len; + count = iter->seq.seq.len; ret =print_trace_line (iter); - count = iter->seq.len - count; + count = iter->seq.seq.len - count;if (rem < count) { rem =0 ; - iter->seq.len -= count; + iter->seq.seq.len -= count;break ; }if (ret ==TRACE_TYPE_PARTIAL_LINE ) { - iter->seq.len -= count; + iter->seq.seq.len -= count;break ; }
@@ -4666,13 +4669,13 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,/* Copy the data into the page, so we can start over. */ ret =trace_seq_to_buffer (&iter->seq,page_address (spd.pages[i]), - iter->seq.len); + iter->seq.seq.len);if (ret <0 ) {__free_page (spd.pages[i]);break ; } spd.partial[i].offset =0 ; - spd.partial[i].len = iter->seq.len; + spd.partial[i].len = iter->seq.seq.len;trace_seq_init (&iter->seq); }
@@ -5673,7 +5676,7 @@ tracing_stats_read(struct file *filp, char __user *ubuf, cnt =ring_buffer_read_events_cpu (trace_buf->buffer, cpu);trace_seq_printf (s,"read events: %ld\n" , cnt); - count =simple_read_from_buffer (ubuf, count, ppos, s->buffer, s->len); + count =simple_read_from_buffer (ubuf, count, ppos, s->buffer, s->seq.len);kfree (s);
@@ -6636,11 +6639,11 @@ void trace_printk_seq(struct trace_seq *s) {/* Probably should print a warning here. */ -if (s->len >=TRACE_MAX_PRINT ) - s->len =TRACE_MAX_PRINT ; +if (s->seq.len >=TRACE_MAX_PRINT ) + s->seq.len =TRACE_MAX_PRINT ;/* should be zero ended, but we are paranoid. */ - s->buffer[s->len] =0 ; + s->buffer[s->seq.len] =0 ;printk (KERN_TRACE "%s" , s->buffer);
kernel/trace/trace_events.c +3 -3
@@ -1044,7 +1044,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,mutex_unlock (&event_mutex);if (file) - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->len); + r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len);kfree (s);
@@ -1210,7 +1210,7 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,trace_seq_init (s);print_subsystem_event_filter (system, s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->len); + r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len);kfree (s);
@@ -1265,7 +1265,7 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)trace_seq_init (s);func (s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->len); + r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len);kfree (s);
kernel/trace/trace_functions_graph.c +3 -3
@@ -1154,9 +1154,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent, }/* Strip ending newline */ -if (s->buffer[s->len -1 ] =='\n' ) { - s->buffer[s->len -1 ] ='\0' ; - s->len--; +if (s->buffer[s->seq.len -1 ] =='\n' ) { + s->buffer[s->seq.len -1 ] ='\0' ; + s->seq.len--; }trace_seq_puts (s," */\n" );
kernel/trace/trace_seq.c +83 -94
314 lines not on this page · raw message
Signature · 1 line
2.1.1
Re: [PATCH 14/26 v5] tracing: Create seq_buf layer in trace_seq
Quoted 25 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Create a
seq_buflayer thattrace_seqsits on. Theseq_bufwill not be limited to page size. This will allow other usages ofseq_bufinstead of a hard setPAGE_SIZEone thattrace_seqhas.Tested-by Jiri Kosina <[email protected]>Acked-by Jiri Kosina <[email protected]>Reviewed-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>--- include/linux/seq_buf.h | 81 +++++++++ include/linux/trace_seq.h | 12 +- kernel/trace/Makefile | 1 + kernel/trace/seq_buf.c | 341 +++++++++++++++++++++++++++++++++++ kernel/trace/trace.c | 39 ++-- kernel/trace/trace_events.c | 6 +- kernel/trace/trace_functions_graph.c | 6 +- kernel/trace/trace_seq.c | 177 +++++++++--------- 8 files changed, 538 insertions(+), 125 deletions(-) create mode 100644 include/linux/seq_buf.h create mode 100644 kernel/trace/seq_buf.c
[...]
Quoted 4 lines
kernel/trace/trace_functions_graph.c +0
-0
[...]
kernel/trace/trace_functions_graph.c EXPORT_SYMBOL_GPL(trace_seq_vprintf); · line 180
Quoted 7 lines
kernel/trace/trace_functions_graph.c +1
-1@@ -163,21 +176,22 @@ EXPORT_SYMBOL_GPL(trace_seq_vprintf);*/ void trace_seq_bprintf (struct trace_seq *s,const char *fmt,const u32 *binary) { -unsigned int len =TRACE_SEQ_BUF_LEFT (s); +unsigned int save_len = s->seq.len;int ret;
Last but one small thing. The "ret" is assigned but not longer used.
JFYI, I have skipped this patch yesterday because it was long and already marked as reviewed. I could not help myself and double checked it today.
Quoted 10 lines
kernel/trace/trace_functions_graph.c +4
-2-if (s->full || !len) +if (s->full)return ; - ret =bstr_printf (s->buffer + s->len, len, fmt, binary); +__trace_seq_init (s); + + ret =seq_buf_bprintf (&s->seq, fmt, binary);/* If we can't write it all, don't bother writing anything */
kernel/trace/trace_functions_graph.c EXPORT_SYMBOL_GPL(trace_seq_vprintf); · line 195
Quoted 10 lines
kernel/trace/trace_functions_graph.c +2
-3-if (ret >= len) { +if (unlikely (seq_buf_has_overflowed (&s->seq))) { + s->seq.len = save_len; s->full =1 ;return ; } - - s->len += ret; }EXPORT_SYMBOL_GPL (trace_seq_bprintf);
The last small thing is the same unused "ret" in trace_seq_path(). But this one should get fixed in "[PATCH 15/26 v5] tracing: Convert seq_buf_path() to be like seq_path()".
Both these changes are rather cosmetic. Feel free to leave them for some later clean up.
Eureka, I have finished review of the patchset and am happy with it \o/ I am looking forward to see it in the main tree.
Thanks a lot for the hard work.
Best Regards,
Petr
Steven Rostedt
maintainerRe: [PATCH 14/26 v5] tracing: Create seq_buf layer in trace_seq
Quoted 46 lines · On Wed, 19 Nov 2014 15:51:38 +0100 Petr Mladek <[email protected]> wrote:
Quoted 25 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Create a
seq_buflayer thattrace_seqsits on. Theseq_bufwill not be limited to page size. This will allow other usages ofseq_bufinstead of a hard setPAGE_SIZEone thattrace_seqhas.Tested-by Jiri Kosina <[email protected]>Acked-by Jiri Kosina <[email protected]>Reviewed-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>--- include/linux/seq_buf.h | 81 +++++++++ include/linux/trace_seq.h | 12 +- kernel/trace/Makefile | 1 + kernel/trace/seq_buf.c | 341 +++++++++++++++++++++++++++++++++++ kernel/trace/trace.c | 39 ++-- kernel/trace/trace_events.c | 6 +- kernel/trace/trace_functions_graph.c | 6 +- kernel/trace/trace_seq.c | 177 +++++++++--------- 8 files changed, 538 insertions(+), 125 deletions(-) create mode 100644 include/linux/seq_buf.h create mode 100644 kernel/trace/seq_buf.c[...]
Quoted 4 lines
kernel/trace/trace_functions_graph.c +0
-0[...]
kernel/trace/trace_functions_graph.c EXPORT_SYMBOL_GPL(trace_seq_vprintf); · line 180
Quoted 7 lines
kernel/trace/trace_functions_graph.c +1
-1@@ -163,21 +176,22 @@ EXPORT_SYMBOL_GPL(trace_seq_vprintf);*/ void trace_seq_bprintf (struct trace_seq *s,const char *fmt,const u32 *binary) { -unsigned int len =TRACE_SEQ_BUF_LEFT (s); +unsigned int save_len = s->seq.len;int ret;Last but one small thing. The "ret" is assigned but not longer used.
Quoted 3 lines
JFYI, I have skipped this patch yesterday because it was long and already marked as reviewed. I could not help myself and double checked it today.
OK, I'll fix that up.
Quoted 9 lines
Quoted 9 lines
kernel/trace/trace_functions_graph.c +4
-2-if (s->full || !len) +if (s->full)return ; - ret =bstr_printf (s->buffer + s->len, len, fmt, binary); +__trace_seq_init (s); + + ret =seq_buf_bprintf (&s->seq, fmt, binary);
Quoted 15 lines
kernel/trace/trace_functions_graph.c EXPORT_SYMBOL_GPL(trace_seq_vprintf); · line 195
Quoted 11 lines
kernel/trace/trace_functions_graph.c +2
-3/* If we can't write it all, don't bother writing anything */ -if (ret >= len) { +if (unlikely (seq_buf_has_overflowed (&s->seq))) { + s->seq.len = save_len; s->full =1 ;return ; } - - s->len += ret; }EXPORT_SYMBOL_GPL (trace_seq_bprintf);The last small thing is the same unused "ret" in
trace_seq_path(). But this one should get fixed in "[PATCH 15/26 v5] tracing: Convertseq_buf_path()to be likeseq_path()".
OK, I'll try to remember that :-)
Quoted 7 lines
Both these changes are rather cosmetic. Feel free to leave them for some later clean up.
Eureka, I have finished review of the patchset and am happy with it \o/ I am looking forward to see it in the main tree.
Well now it comes down to Linus or Andrew giving me the OK. I've pulled all the trace_seq updates into my ftrace/core branch (which will be going into my for-next branch shortly). I stopped just before this patch. I didn't want to create the seq_buf until I knew it was going to be used by something other than trace_seq.
Quoted 3 lines
Thanks a lot for the hard work.
Yeah, let's hope that it was worth it.
Thanks for the in depth reviews.
-- Steve
Steven Rostedt
maintainerPATCH v5 15/26 tracing: Convert seq_buf_path() to be like seq_path()
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Rewrite seq_buf_path() like it is done in seq_path() and allow it to accept any escape character instead of just "\n".
Making seq_buf_path() like seq_path() will help prevent problems when converting seq_file to use the seq_buf logic.
--- include/linux/seq_buf.h | 2 +- kernel/trace/seq_buf.c | 28 ++++++++++++++++------------ kernel/trace/trace_seq.c | 4 ++-- 3 files changed, 19 insertions(+), 15 deletions(-)
include/linux/seq_buf.h +1 -1
@@ -73,7 +73,7 @@ extern int seq_buf_putc(struct seq_buf *s, unsigned char c);extern int seq_buf_putmem (struct seq_buf *s,const void *mem,unsigned int len);extern int seq_buf_putmem_hex (struct seq_buf *s,const void *mem,unsigned int len); -extern int seq_buf_path (struct seq_buf *s,const struct path *path); +extern int seq_buf_path (struct seq_buf *s,const struct path *path,const char *esc);extern int seq_buf_bitmask (struct seq_buf *s,const unsigned long *maskp,int nmaskbits);
kernel/trace/seq_buf.c +16 -12
@@ -272,28 +272,32 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,* seq_buf_path - copy a path into the sequence buffer * @s: seq_buf descriptor * @path: path to write into the sequence buffer. +* @esc: set of characters to escape in the output * * Write a path name into the sequence buffer. * -* Returns zero on success, -1 on overflow +* Returns the number of written bytes on success, -1 on overflow */ -int seq_buf_path (struct seq_buf *s,const struct path *path) +int seq_buf_path (struct seq_buf *s,const struct path *path,const char *esc) { -unsigned int len =seq_buf_buffer_left (s); -unsigned char *p; +char *buf = s->buffer + s->len; +size_t size =seq_buf_buffer_left (s); +int res =-1 ;WARN_ON (s->size ==0 ); - p =d_path (path, s->buffer + s->len, len); -if (!IS_ERR (p)) { - p =mangle_path (s->buffer + s->len, p,"\n" ); -if (p) { - s->len = p - s->buffer; -return 0 ; +if (size) { +char *p =d_path (path, buf, size); +if (!IS_ERR (p)) { +char *end =mangle_path (buf, p, esc); +if (end) + res = end - buf; } } -seq_buf_set_overflow (s); -return -1 ; +if (res >0 ) + s->len += res; + +return res; }/**
kernel/trace/trace_seq.c +2 -2
@@ -339,7 +339,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)return 0 ; } - ret =seq_buf_path (&s->seq, path); + ret =seq_buf_path (&s->seq, path,"\n" );if (unlikely (seq_buf_has_overflowed (&s->seq))) { s->seq.len = save_len;
@@ -347,7 +347,7 @@ int trace_seq_path(struct trace_seq *s, const struct path *path)return 0 ; } -return ret; +return 1 ; }EXPORT_SYMBOL_GPL (trace_seq_path);
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 16/26 tracing: Convert seq_buf fields to be like seq_file fields
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
In facilitating the conversion of seq_file to use seq_buf, have the seq_buf fields match the types used by seq_file.
--- include/linux/seq_buf.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
include/linux/seq_buf.h +4 -4
@@ -16,10 +16,10 @@* @readpos: The next position to read in the buffer. */ struct seq_buf { -unsigned char *buffer; -unsigned int size; -unsigned int len; -unsigned int readpos; +char *buffer; +size_t size; +size_t len; +loff_t readpos; }; static inline void
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 17/26 tracing: Add a seq_buf_clear() helper and clear len and readpos in init
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function seq_buf_clear() that resets the len and readpos fields of a seq_buf. Currently it is only used in the seq_buf_init() but will be used later when updating the seq_file code.
--- include/linux/seq_buf.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
include/linux/seq_buf.h +7 -2
@@ -22,13 +22,18 @@ struct seq_buf {loff_t readpos; }; +static inline void seq_buf_clear (struct seq_buf *s) +{ + s->len =0 ; + s->readpos =0 ; +} +static inline void seq_buf_init (struct seq_buf *s,unsigned char *buf,unsigned int size) { s->buffer = buf; s->size = size; - s->len =0 ; - s->readpos =0 ; +seq_buf_clear (s); }/*
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 18/26 seq_buf: Create seq_buf_used() to find out how much was written
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function seq_buf_used() that replaces the SEQ_BUF_USED() private macro to let callers have a method to know how much of the seq_buf was written to.
--- include/linux/seq_buf.h | 6 ++++++ kernel/trace/seq_buf.c | 5 +---- 2 files changed, 7 insertions(+), 4 deletions(-)
include/linux/seq_buf.h +6 -0
@@ -64,6 +64,12 @@ seq_buf_buffer_left(struct seq_buf *s)return (s->size -1 ) - s->len; } +/* How much buffer was written? */ +static inline unsigned int seq_buf_used (struct seq_buf *s) +{ +return min (s->len, s->size); +} + extern __printf(2, 3) int seq_buf_printf(struct seq_buf *s, const char *fmt, ...); extern __printf(2, 0)
kernel/trace/seq_buf.c +1 -4
@@ -16,9 +16,6 @@#include <linux/seq_file.h> #include <linux/seq_buf.h> -/* How much buffer is written? */ -#define SEQ_BUF_USED (s) min((s)->len, (s)->size - 1) -/** * seq_buf_print_seq - move the contents of seq_buf into a seq_file * @m: the seq_file descriptor that is the destination
@@ -28,7 +25,7 @@*/ int seq_buf_print_seq (struct seq_file *m,struct seq_buf *s) { -unsigned int len =SEQ_BUF_USED (s); +unsigned int len =seq_buf_used (s);return seq_write (m, s->buffer, len); }
Signature · 1 line
2.1.1
Re: [PATCH 18/26 v5] seq_buf: Create seq_buf_used() to find out how much was written
Quoted 20 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function
seq_buf_used()that replaces theSEQ_BUF_USED()private macro to let callers have a method to know how much of theseq_bufwas written to.Signed-off-by Steven Rostedt <[email protected]>--- include/linux/seq_buf.h | 6 ++++++ kernel/trace/seq_buf.c | 5 +---- 2 files changed, 7 insertions(+), 4 deletions(-)include/linux/seq_buf.h +0
-0
include/linux/seq_buf.h seq_buf_buffer_left(struct seq_buf *s) · line 70
Quoted 7 lines
include/linux/seq_buf.h +4
-0@@ -64,6 +64,12 @@ seq_buf_buffer_left(struct seq_buf *s)return (s->size -1 ) - s->len; } +/* How much buffer was written? */ +static inline unsigned int seq_buf_used (struct seq_buf *s) +{ +return min (s->len, s->size);
To be precise, it should do
return min(s->len, s->size - 1);
at this stage and switch to the above code in ("[PATCH 21/26 v5] tracing: Have seq_buf use full buffer"). Well, it does not cause any big harm. Feel free to add:
for this patch or for the updated one.
Best Regards,
Petr
Quoted 32 lines
include/linux/seq_buf.h +2
-0+} + extern __printf(2, 3) int seq_buf_printf(struct seq_buf *s, const char *fmt, ...); extern __printf(2, 0)kernel/trace/seq_buf.c +1
-4@@ -16,9 +16,6 @@#include <linux/seq_file.h> #include <linux/seq_buf.h> -/* How much buffer is written? */ -#define SEQ_BUF_USED (s) min((s)->len, (s)->size - 1) -/** * seq_buf_print_seq - move the contents of seq_buf into a seq_file * @m: the seq_file descriptor that is the destination @@ -28,7 +25,7 @@*/ int seq_buf_print_seq (struct seq_file *m,struct seq_buf *s) { -unsigned int len =SEQ_BUF_USED (s); +unsigned int len =seq_buf_used (s);return seq_write (m, s->buffer, len); }Signature · 1 line
2.1.1
Steven Rostedt
maintainerRe: [PATCH 18/26 v5] seq_buf: Create seq_buf_used() to find out how much was written
Quoted 31 lines · On Tue, 18 Nov 2014 16:02:04 +0100 Petr Mladek <[email protected]> wrote:
Quoted 20 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function
seq_buf_used()that replaces theSEQ_BUF_USED()private macro to let callers have a method to know how much of theseq_bufwas written to.Signed-off-by Steven Rostedt <[email protected]>--- include/linux/seq_buf.h | 6 ++++++ kernel/trace/seq_buf.c | 5 +---- 2 files changed, 7 insertions(+), 4 deletions(-)include/linux/seq_buf.h +0
-0include/linux/seq_buf.h seq_buf_buffer_left(struct seq_buf *s) · line 70
Quoted 7 lines
include/linux/seq_buf.h +4
-0@@ -64,6 +64,12 @@ seq_buf_buffer_left(struct seq_buf *s)return (s->size -1 ) - s->len; } +/* How much buffer was written? */ +static inline unsigned int seq_buf_used (struct seq_buf *s) +{ +return min (s->len, s->size);To be precise, it should do
Quoted 5 lines
return min(s->len, s->size - 1);at this stage and switch to the above code in ("[PATCH 21/26 v5] tracing: Have
seq_bufuse full buffer"). Well, it does not cause any big harm. Feel free to add:
I actually thought about it, but realized that this only replaces the original use of s->len, and we are only doing this to avoid buffer overflows. If you get garbage, then so be it, as the original code would give garbage too. Remember, some of the functions did have real content in that last byte, even though it was considered "overflowed".
-- Steve
Quoted 5 lines
Reviewed-by Petr Mladek <[email protected]>for this patch or for the updated one.
Re: [PATCH 18/26 v5] seq_buf: Create seq_buf_used() to find out how much was written
Quoted 40 lines · Steven Rostedt
Quoted 31 lines · On Tue, 18 Nov 2014 16:02:04 +0100 Petr Mladek <[email protected]> wrote:
Quoted 20 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a helper function
seq_buf_used()that replaces theSEQ_BUF_USED()private macro to let callers have a method to know how much of theseq_bufwas written to.Signed-off-by Steven Rostedt <[email protected]>--- include/linux/seq_buf.h | 6 ++++++ kernel/trace/seq_buf.c | 5 +---- 2 files changed, 7 insertions(+), 4 deletions(-)include/linux/seq_buf.h +0
-0include/linux/seq_buf.h seq_buf_buffer_left(struct seq_buf *s) · line 70
Quoted 7 lines
include/linux/seq_buf.h +4
-0@@ -64,6 +64,12 @@ seq_buf_buffer_left(struct seq_buf *s)return (s->size -1 ) - s->len; } +/* How much buffer was written? */ +static inline unsigned int seq_buf_used (struct seq_buf *s) +{ +return min (s->len, s->size);To be precise, it should do
Quoted 5 lines
return min(s->len, s->size - 1);at this stage and switch to the above code in ("[PATCH 21/26 v5] tracing: Have
seq_bufuse full buffer"). Well, it does not cause any big harm. Feel free to add:
Quoted 5 lines
I actually thought about it, but realized that this only replaces the original use of
s->len, and we are only doing this to avoid buffer overflows. If you get garbage, then so be it, as the original code would give garbage too. Remember, some of the functions did have real content in that last byte, even though it was considered "overflowed".
I thought that it was based on ftrace_seq code that had
#define TRACE_SEQ_BUF_USED(s) min((s)->len, (unsigned int)(PAGE_SIZE -
1))
But you are right that the callers used s->len because the macro was local in trace_seq.c.
Well, I think that it does not matter much. As you said, there might be a garbage a the end of the buffer. Callers used "len" directly. Also it will be min(s->len, s->size); after the commit "Have seq_buf use full buffer".
Let's leave it as is.
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 19/26 tracing: Use trace_seq_used() and seq_buf_used() instead of len
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
As the seq_buf->len will soon be +1 size when there's an overflow, we must use trace_seq_used() or seq_buf_used() methods to get the real length. This will prevent buffer overflow issues if just the len of the seq_buf descriptor is used to copy memory.
--- include/linux/trace_seq.h | 20 +++++++++++++++- kernel/trace/seq_buf.c | 2 +- kernel/trace/trace.c | 44 ++++++++++++++++++++++++------------ kernel/trace/trace_events.c | 9 +++++--- kernel/trace/trace_functions_graph.c | 5 +++- kernel/trace/trace_seq.c | 2 +- 6 files changed, 60 insertions(+), 22 deletions(-)
include/linux/trace_seq.h +19 -1
@@ -24,6 +24,24 @@ trace_seq_init(struct trace_seq *s) }/** +* trace_seq_used - amount of actual data written to buffer +* @s: trace sequence descriptor +* +* Returns the amount of data written to the buffer. +* +* IMPORTANT! +* +* Use this instead of @s->seq.len if you need to pass the amount +* of data from the buffer to another buffer (userspace, or what not). +* The @s->seq.len on overflow is bigger than the buffer size and +* using it can cause access to undefined memory. +*/ +static inline int trace_seq_used (struct trace_seq *s) +{ +return seq_buf_used (&s->seq); +} + +/** * trace_seq_buffer_ptr - return pointer to next location in buffer * @s: trace sequence descriptor *
@@ -35,7 +53,7 @@ trace_seq_init(struct trace_seq *s)static inline unsigned char *trace_seq_buffer_ptr (struct trace_seq *s) { -return s->buffer + s->seq.len; +return s->buffer +seq_buf_used (&s->seq); }/**
kernel/trace/seq_buf.c +1 -1
@@ -328,7 +328,7 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)if (s->len <= s->readpos)return -EBUSY ; - len = s->len - s->readpos; + len =seq_buf_used (s) - s->readpos;if (cnt > len) cnt = len; ret =copy_to_user (ubuf, s->buffer + s->readpos, cnt);
kernel/trace/trace.c +29 -15
104 lines not on this page · raw message
kernel/trace/trace_events.c +6 -3
@@ -1044,7 +1044,8 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,mutex_unlock (&event_mutex);if (file) - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
@@ -1210,7 +1211,8 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,trace_seq_init (s);print_subsystem_event_filter (system, s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
@@ -1265,7 +1267,8 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)trace_seq_init (s);func (s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
kernel/trace/trace_functions_graph.c +4 -1
@@ -1153,6 +1153,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,return ret; } +if (trace_seq_has_overflowed (s)) + goto out; +/* Strip ending newline */ if (s->buffer[s->seq.len -1 ] =='\n' ) { s->buffer[s->seq.len -1 ] ='\0' ;
@@ -1160,7 +1163,7 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent, }trace_seq_puts (s," */\n" ); - + out:return trace_handle_return (s); }
kernel/trace/trace_seq.c +1 -1
@@ -30,7 +30,7 @@#define TRACE_SEQ_BUF_LEFT (s) seq_buf_buffer_left(&(s)->seq)/* How much buffer is written? */ -#define TRACE_SEQ_BUF_USED (s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1)) +#define TRACE_SEQ_BUF_USED (s) seq_buf_used(&(s)->seq)/* * trace_seq should work with being initialized with 0s.
Signature · 1 line
2.1.1
Steven Rostedt
maintainerRe: [PATCH 19/26 v5] tracing: Use trace_seq_used() and seq_buf_used() instead of len
On Fri, 14 Nov 2014 23:59:06 -0500
Quoted 31 lines
kernel/trace/trace.c +12
-6/* @@ -4575,20 +4575,33 @@ static size_t tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) { size_t count; +int save_len;int ret;/* Seq buffer is page-sized, exactly what we need. */ for (;;) { - count = iter->seq.seq.len; + save_len = iter->seq.seq.len; ret =print_trace_line (iter); - count = iter->seq.seq.len - count; -if (rem < count) { - rem =0 ; - iter->seq.seq.len -= count; + +if (trace_seq_has_overflowed (&iter->seq)) { + iter->seq.seq.len = save_len;break ; } + +/* +* This should not be hit, because it should only +* be set if the iter->seq overflowed. But check it +* anyway to be safe. +*/ if (ret ==TRACE_TYPE_PARTIAL_LINE ) { - iter->seq.seq.len -= count; + iter->seq.seq.len = save_len;
kernel/trace/trace.c static size_t · line 4607
Quoted 10 lines
kernel/trace/trace.c +7
-0+break ; + } + + count =trace_seq_used (&iter->seq) - save_len; +if (rem < count) { + rem =0 ; + iter->seq.seq.len = save_len;;break ; }
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to seq_buf_used(). I'm going to strip this change out and put it before this patch.
-- Steve
Steven Rostedt
maintainerPATCH 1/2 tracing: Clean up tracing_fill_pipe_page()
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.
The function tracing_fill_pipe_page() logic is a little confusing with the use of count saving the seq.len and reusing it.
Instead of subtracting a number that is calculated from the saved value of the seq.len from seq.len, just save the seq.len at the start and if we need to reset it, just assign it again.
When the seq_buf overflow is len == size + 1, the current logic will break. Changing it to use a saved length for resetting back to the original value is more robust and will work when we change the way seq_buf sets the overflow.
--- kernel/trace/trace.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
kernel/trace/trace.c +20 -6
@@ -4575,23 +4575,37 @@ static size_t tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) { size_t count; +int save_len;int ret;/* Seq buffer is page-sized, exactly what we need. */ for (;;) { - count = iter->seq.seq.len; + save_len = iter->seq.seq.len; ret =print_trace_line (iter); - count = iter->seq.seq.len - count; -if (rem < count) { - rem =0 ; - iter->seq.seq.len -= count; + +if (trace_seq_has_overflowed (&iter->seq)) { + iter->seq.seq.len = save_len;break ; } + +/* +* This should not be hit, because it should only +* be set if the iter->seq overflowed. But check it +* anyway to be safe. +*/ if (ret ==TRACE_TYPE_PARTIAL_LINE ) { - iter->seq.seq.len -= count; + iter->seq.seq.len = save_len;break ; } + count = iter->seq.seq.len - save_len; +if (rem < count) { + rem =0 ; + iter->seq.seq.len = save_len; +break ; + } + +if (ret !=TRACE_TYPE_NO_CONSUME )trace_consume (iter); rem -= count;
Signature · 1 line
1.8.1.4
Re: [PATCH 1/2] tracing: Clean up tracing_fill_pipe_page()
Quoted 50 lines · Steven Rostedt
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.The function
tracing_fill_pipe_page()logic is a little confusing with the use of count saving the seq.len and reusing it.Instead of subtracting a number that is calculated from the saved value of the seq.len from seq.len, just save the seq.len at the start and if we need to reset it, just assign it again.
When the
seq_bufoverflow is len == size + 1, the current logic will break. Changing it to use a saved length for resetting back to the original value is more robust and will work when we change the wayseq_bufsets the overflow.Signed-off-by Steven Rostedt <[email protected]>--- kernel/trace/trace.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)kernel/trace/trace.c +6
-5@@ -4575,23 +4575,37 @@ static size_t tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) { size_t count; +int save_len;int ret;/* Seq buffer is page-sized, exactly what we need. */ for (;;) { - count = iter->seq.seq.len; + save_len = iter->seq.seq.len; ret =print_trace_line (iter); - count = iter->seq.seq.len - count; -if (rem < count) { - rem =0 ; - iter->seq.seq.len -= count; + +if (trace_seq_has_overflowed (&iter->seq)) { + iter->seq.seq.len = save_len;break ; } +
kernel/trace/trace.c static size_t · line 4599
Quoted 10 lines
kernel/trace/trace.c +6
-1+/* +* This should not be hit, because it should only +* be set if the iter->seq overflowed. But check it +* anyway to be safe. +*/ if (ret ==TRACE_TYPE_PARTIAL_LINE ) { - iter->seq.seq.len -= count; + iter->seq.seq.len = save_len;break ; }
The two ifs has the same body. Small optimization would be to do:
/*
* The two checks should be equivalent but rather be
* on the safe side.
*/
if (trace_seq_has_overflowed(&iter->seq) ||
ret == TRACE_TYPE_PARTIAL_LINE) {
iter->seq.seq.len = save_len;
break;
}To be honest, the code seems to be a bit twisted. This function is called from tracing_splice_read_pipe(). It copies the trace_seq buffer into spd.page and call trace_seq_init() in a for cycle.
Therefore if the overflow happens, trace_find_next_entry_inc() is not called in tracing_fill_pipe_page() and we work with the same iterator instance next time. It means that the overflow happens most likely again and we fill all remaining spd.pages with no data and are stacked on the same iterator instance.
BTW: The trace_seq_to_buffer() in tracing_splice_read_pipe() is suspicious as well. It passes trace_seq_used(&iter->seq) as the "cnt" parameter. I guess that it should pass the size of the "spd.page" instead.
Also we should somehow handle the situation when some data are not copied. Well, it seems the spd.page has the page size, so it is the same size as the trace_seq buffer.
Well, this patch does not change the behavior. We could solve the above problem later if it is there. Maybe I got it wrong.
Best Regards,
Petr
Quoted 14 lines
kernel/trace/trace.c +8
-0+ count = iter->seq.seq.len - save_len; +if (rem < count) { + rem =0 ; + iter->seq.seq.len = save_len; +break ; + } + +if (ret !=TRACE_TYPE_NO_CONSUME )trace_consume (iter); rem -= count;Signature · 1 line
1.8.1.4
Steven Rostedt
maintainerRe: [PATCH 1/2] tracing: Clean up tracing_fill_pipe_page()
Quoted 64 lines · On Tue, 18 Nov 2014 17:15:46 +0100 Petr Mladek <[email protected]> wrote:
Quoted 50 lines · Steven Rostedt
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.The function
tracing_fill_pipe_page()logic is a little confusing with the use of count saving the seq.len and reusing it.Instead of subtracting a number that is calculated from the saved value of the seq.len from seq.len, just save the seq.len at the start and if we need to reset it, just assign it again.
When the
seq_bufoverflow is len == size + 1, the current logic will break. Changing it to use a saved length for resetting back to the original value is more robust and will work when we change the wayseq_bufsets the overflow.Signed-off-by Steven Rostedt <[email protected]>--- kernel/trace/trace.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)kernel/trace/trace.c +6
-5@@ -4575,23 +4575,37 @@ static size_t tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) { size_t count; +int save_len;int ret;/* Seq buffer is page-sized, exactly what we need. */ for (;;) { - count = iter->seq.seq.len; + save_len = iter->seq.seq.len; ret =print_trace_line (iter); - count = iter->seq.seq.len - count; -if (rem < count) { - rem =0 ; - iter->seq.seq.len -= count; + +if (trace_seq_has_overflowed (&iter->seq)) { + iter->seq.seq.len = save_len;break ; } +kernel/trace/trace.c static size_t · line 4599
Quoted 10 lines
kernel/trace/trace.c +6
-1+/* +* This should not be hit, because it should only +* be set if the iter->seq overflowed. But check it +* anyway to be safe. +*/ if (ret ==TRACE_TYPE_PARTIAL_LINE ) { - iter->seq.seq.len -= count; + iter->seq.seq.len = save_len;break ; }The two ifs has the same body. Small optimization would be to do:
Quoted 9 lines
/*
* The two checks should be equivalent but rather be
* on the safe side.
*/
if (trace_seq_has_overflowed(&iter->seq) ||
ret == TRACE_TYPE_PARTIAL_LINE) {
iter->seq.seq.len = save_len;
break;
}Yeah, I originally had something like that, but I wanted to remove that second check. I left it separate to make it stand out as something that might be removed in the future. Just a preference I guess.
Quoted 5 lines
To be honest, the code seems to be a bit twisted. This function is called from
tracing_splice_read_pipe(). It copies thetrace_seqbuffer into spd.page and calltrace_seq_init()in a for cycle.
Yeah, that splice code confused me too. I'll start looking at it some more and see if it can be fixed up.
Quoted 6 lines
Therefore if the overflow happens,
trace_find_next_entry_inc()is not called intracing_fill_pipe_page()and we work with the same iterator instance next time. It means that the overflow happens most likely again and we fill all remaining spd.pages with no data and are stacked on the same iterator instance.
Luckily, overflows never happen. But if they do, things might break.
Quoted 5 lines
BTW: The
trace_seq_to_buffer()intracing_splice_read_pipe()is suspicious as well. It passestrace_seq_used(&iter->seq)as the "cnt" parameter. I guess that it should pass the size of the "spd.page" instead.
Wow, I should have looked harder at that code when I accepted it. It just "worked" and I was happy. Oh well, another thing to fix up.
Quoted 8 lines
Also we should somehow handle the situation when some data are not copied. Well, it seems the spd.page has the page size, so it is the same size as the
trace_seqbuffer.Well, this patch does not change the behavior. We could solve the above problem later if it is there. Maybe I got it wrong.
No, that code doesn't look too good. That's some old stuff that got in when we were still learning, and if it worked, we added it ;-)
That needs to be cleaned up. I'll put it on my ever growing todo list. Of course if you want to clean it up, feel free to send some patches on top of this. That is, if we get the OK from Linus or Andrew.
Quoted 3 lines
Reviewed-by Petr Mladek <[email protected]>
Thanks,
-- Steve
Re: [PATCH 1/2] tracing: Clean up tracing_fill_pipe_page()
Quoted 77 lines · Steven Rostedt
Quoted 64 lines · On Tue, 18 Nov 2014 17:15:46 +0100 Petr Mladek <[email protected]> wrote:
Quoted 50 lines · Steven Rostedt
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.The function
tracing_fill_pipe_page()logic is a little confusing with the use of count saving the seq.len and reusing it.Instead of subtracting a number that is calculated from the saved value of the seq.len from seq.len, just save the seq.len at the start and if we need to reset it, just assign it again.
When the
seq_bufoverflow is len == size + 1, the current logic will break. Changing it to use a saved length for resetting back to the original value is more robust and will work when we change the wayseq_bufsets the overflow.Signed-off-by Steven Rostedt <[email protected]>--- kernel/trace/trace.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)kernel/trace/trace.c +6
-5@@ -4575,23 +4575,37 @@ static size_t tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) { size_t count; +int save_len;int ret;/* Seq buffer is page-sized, exactly what we need. */ for (;;) { - count = iter->seq.seq.len; + save_len = iter->seq.seq.len; ret =print_trace_line (iter); - count = iter->seq.seq.len - count; -if (rem < count) { - rem =0 ; - iter->seq.seq.len -= count; + +if (trace_seq_has_overflowed (&iter->seq)) { + iter->seq.seq.len = save_len;break ; } +kernel/trace/trace.c static size_t · line 4599
Quoted 10 lines
kernel/trace/trace.c +6
-1+/* +* This should not be hit, because it should only +* be set if the iter->seq overflowed. But check it +* anyway to be safe. +*/ if (ret ==TRACE_TYPE_PARTIAL_LINE ) { - iter->seq.seq.len -= count; + iter->seq.seq.len = save_len;break ; }The two ifs has the same body. Small optimization would be to do:
Quoted 9 lines
/* * The two checks should be equivalent but rather be * on the safe side. */ if (trace_seq_has_overflowed(&iter->seq) || ret == TRACE_TYPE_PARTIAL_LINE) { iter->seq.seq.len = save_len; break; }
Quoted 3 lines
Yeah, I originally had something like that, but I wanted to remove that second check. I left it separate to make it stand out as something that might be removed in the future. Just a preference I guess.
Fair enough.
Quoted 9 lines
Quoted 4 lines
To be honest, the code seems to be a bit twisted. This function is called from
tracing_splice_read_pipe(). It copies thetrace_seqbuffer into spd.page and calltrace_seq_init()in a for cycle.Yeah, that splice code confused me too. I'll start looking at it some more and see if it can be fixed up.
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Therefore if the overflow happens,
trace_find_next_entry_inc()is not called intracing_fill_pipe_page()and we work with the same iterator instance next time. It means that the overflow happens most likely again and we fill all remaining spd.pages with no data and are stacked on the same iterator instance.Luckily, overflows never happen. But if they do, things might break.
I thought so. :-)
Quoted 15 lines
Quoted 5 lines
BTW: The
trace_seq_to_buffer()intracing_splice_read_pipe()is suspicious as well. It passestrace_seq_used(&iter->seq)as the "cnt" parameter. I guess that it should pass the size of the "spd.page" instead.Wow, I should have looked harder at that code when I accepted it. It just "worked" and I was happy. Oh well, another thing to fix up.
Quoted 6 lines
Also we should somehow handle the situation when some data are not copied. Well, it seems the spd.page has the page size, so it is the same size as the
trace_seqbuffer.
Quoted 8 lines
Quoted 2 lines
Well, this patch does not change the behavior. We could solve the above problem later if it is there. Maybe I got it wrong.
No, that code doesn't look too good. That's some old stuff that got in when we were still learning, and if it worked, we added it ;-)
That needs to be cleaned up. I'll put it on my ever growing todo list.
I believe. I am a bit scared to put it on my todo list because these kind of working things tend to just fall down.
Quoted 2 lines
Of course if you want to clean it up, feel free to send some patches on top of this. That is, if we get the OK from Linus or Andrew.
OK, I'll put it on my todo list. Let's see who is faster ;-) And I keep my fingers crossed about the OK from Linus and Andrew.
Best Regards,
Petr
Steven Rostedt
maintainerRe: [PATCH 1/2] tracing: Clean up tracing_fill_pipe_page()
Quoted 2 lines · On Wed, 19 Nov 2014 17:51:49 +0100 Petr Mladek <[email protected]> wrote:
OK, I'll put it on my todo list. Let's see who is faster ;-) And I keep my fingers crossed about the OK from Linus and Andrew.
I'll assume they are going to be OK'd. I'll place these patches in a separate branch and after I finish testing (and get acks from my ftrace/core branch), I'll push them all up to my for-next branch.
Then when the merge window opens, I'll push the normal ftrace/core code, and then push this one separate, and let Linus decide if he wants to pull it or not.
Here's the commits that are in this branch and not in ftrace/core:
+ 1293ffff tracing: Create seq_buf layer in trace_seq
+ b5e206f2 tracing: Convert seq_buf_path() to be like seq_path()
+ 79033e01 tracing: Convert seq_buf fields to be like seq_file fields
+ f1698325 tracing: Add a seq_buf_clear() helper and clear len and readpos in init
+ 2b644c7f seq_buf: Create seq_buf_used() to find out how much was written
+ a5c4b79c tracing: Clean up tracing_fill_pipe_page()
+ 359ba4a9 tracing: Use trace_seq_used() and seq_buf_used() instead of len
+ 3ae8769e tracing: Add paranoid size check in trace_printk_seq()
+ e1fb15f1 seq_buf: Add seq_buf_can_fit() helper function
+ 02f09612 tracing: Have seq_buf use full buffer
+ d9c7f4b1 tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions
+ 678a330a seq-buf: Make seq_buf_bprintf() conditional on CONFIG_BINARY_PRINTF
+ a3608c5d seq_buf: Move the seq_buf code to lib/
+ af8f0ef4 printk: Add per_cpu printk func to allow printk to be diverted
+ a066719f x86/nmi: Perform a safe NMI stack trace on all CPUs
-- Steve
Steven Rostedt
maintainerPATCH 2/2 tracing: Use trace_seq_used() and seq_buf_used() instead of len
In reply to Steven RostedtQuoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.
As the seq_buf->len will soon be +1 size when there's an overflow, we must use trace_seq_used() or seq_buf_used() methods to get the real length. This will prevent buffer overflow issues if just the len of the seq_buf descriptor is used to copy memory.
--- include/linux/trace_seq.h | 20 +++++++++++++++++++- kernel/trace/seq_buf.c | 2 +- kernel/trace/trace.c | 22 +++++++++++----------- kernel/trace/trace_events.c | 9 ++++++--- kernel/trace/trace_functions_graph.c | 5 ++++- kernel/trace/trace_seq.c | 2 +- 6 files changed, 42 insertions(+), 18 deletions(-)
include/linux/trace_seq.h +19 -1
@@ -24,6 +24,24 @@ trace_seq_init(struct trace_seq *s) }/** +* trace_seq_used - amount of actual data written to buffer +* @s: trace sequence descriptor +* +* Returns the amount of data written to the buffer. +* +* IMPORTANT! +* +* Use this instead of @s->seq.len if you need to pass the amount +* of data from the buffer to another buffer (userspace, or what not). +* The @s->seq.len on overflow is bigger than the buffer size and +* using it can cause access to undefined memory. +*/ +static inline int trace_seq_used (struct trace_seq *s) +{ +return seq_buf_used (&s->seq); +} + +/** * trace_seq_buffer_ptr - return pointer to next location in buffer * @s: trace sequence descriptor *
@@ -35,7 +53,7 @@ trace_seq_init(struct trace_seq *s)static inline unsigned char *trace_seq_buffer_ptr (struct trace_seq *s) { -return s->buffer + s->seq.len; +return s->buffer +seq_buf_used (&s->seq); }/**
kernel/trace/seq_buf.c +1 -1
@@ -328,7 +328,7 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt)if (s->len <= s->readpos)return -EBUSY ; - len = s->len - s->readpos; + len =seq_buf_used (s) - s->readpos;if (cnt > len) cnt = len; ret =copy_to_user (ubuf, s->buffer + s->readpos, cnt);
kernel/trace/trace.c +11 -11
@@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) {int len; -if (s->seq.len <= s->seq.readpos) +if (trace_seq_used (s) <= s->seq.readpos)return -EBUSY ; - len = s->seq.len - s->seq.readpos; + len =trace_seq_used (s) - s->seq.readpos;if (cnt > len) cnt = len;memcpy (buf, s->buffer + s->seq.readpos, cnt);
@@ -4514,18 +4514,18 @@ waitagain:trace_access_lock (iter->cpu_file);while (trace_find_next_entry_inc (iter) !=NULL ) {enum print_line_t ret; -int len = iter->seq.seq.len; +int save_len = iter->seq.seq.len; ret =print_trace_line (iter);if (ret ==TRACE_TYPE_PARTIAL_LINE ) {/* don't print partial lines */ - iter->seq.seq.len = len; + iter->seq.seq.len = save_len;break ; }if (ret !=TRACE_TYPE_NO_CONSUME )trace_consume (iter); -if (iter->seq.seq.len >= cnt) +if (trace_seq_used (&iter->seq) >= cnt)break ;/*
@@ -4541,7 +4541,7 @@ waitagain:/* Now copy what we have to the user */ sret =trace_seq_to_user (&iter->seq, ubuf, cnt); -if (iter->seq.seq.readpos >= iter->seq.seq.len) +if (iter->seq.seq.readpos >=trace_seq_used (&iter->seq))trace_seq_init (&iter->seq);/*
@@ -4598,14 +4598,13 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter)break ; } - count = iter->seq.seq.len - save_len; + count =trace_seq_used (&iter->seq) - save_len;if (rem < count) { rem =0 ; iter->seq.seq.len = save_len;break ; } -if (ret !=TRACE_TYPE_NO_CONSUME )trace_consume (iter); rem -= count;
@@ -4683,13 +4682,13 @@ static ssize_t tracing_splice_read_pipe(struct file *filp,/* Copy the data into the page, so we can start over. */ ret =trace_seq_to_buffer (&iter->seq,page_address (spd.pages[i]), - iter->seq.seq.len); +trace_seq_used (&iter->seq));if (ret <0 ) {__free_page (spd.pages[i]);break ; } spd.partial[i].offset =0 ; - spd.partial[i].len = iter->seq.seq.len; + spd.partial[i].len =trace_seq_used (&iter->seq);trace_seq_init (&iter->seq); }
@@ -5690,7 +5689,8 @@ tracing_stats_read(struct file *filp, char __user *ubuf, cnt =ring_buffer_read_events_cpu (trace_buf->buffer, cpu);trace_seq_printf (s,"read events: %ld\n" , cnt); - count =simple_read_from_buffer (ubuf, count, ppos, s->buffer, s->seq.len); + count =simple_read_from_buffer (ubuf, count, ppos, + s->buffer,trace_seq_used (s));kfree (s);
kernel/trace/trace_events.c +6 -3
@@ -1044,7 +1044,8 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,mutex_unlock (&event_mutex);if (file) - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
@@ -1210,7 +1211,8 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,trace_seq_init (s);print_subsystem_event_filter (system, s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
@@ -1265,7 +1267,8 @@ show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)trace_seq_init (s);func (s); - r =simple_read_from_buffer (ubuf, cnt, ppos, s->buffer, s->seq.len); + r =simple_read_from_buffer (ubuf, cnt, ppos, + s->buffer,trace_seq_used (s));kfree (s);
kernel/trace/trace_functions_graph.c +4 -1
@@ -1153,6 +1153,9 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent,return ret; } +if (trace_seq_has_overflowed (s)) + goto out; +/* Strip ending newline */ if (s->buffer[s->seq.len -1 ] =='\n' ) { s->buffer[s->seq.len -1 ] ='\0' ;
@@ -1160,7 +1163,7 @@ print_graph_comment(struct trace_seq *s, struct trace_entry *ent, }trace_seq_puts (s," */\n" ); - + out:return trace_handle_return (s); }
kernel/trace/trace_seq.c +1 -1
@@ -30,7 +30,7 @@#define TRACE_SEQ_BUF_LEFT (s) seq_buf_buffer_left(&(s)->seq)/* How much buffer is written? */ -#define TRACE_SEQ_BUF_USED (s) min((s)->seq.len, (unsigned int)(PAGE_SIZE - 1)) +#define TRACE_SEQ_BUF_USED (s) seq_buf_used(&(s)->seq)/* * trace_seq should work with being initialized with 0s.
Signature · 1 line
1.8.1.4
Re: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 24 lines · Steven Rostedt
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.As the
seq_buf->lenwill soon be +1 size when there's an overflow, we must usetrace_seq_used()orseq_buf_used()methods to get the real length. This will prevent buffer overflow issues if just the len of theseq_bufdescriptor is used to copy memory.Reported-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>--- include/linux/trace_seq.h | 20 +++++++++++++++++++- kernel/trace/seq_buf.c | 2 +- kernel/trace/trace.c | 22 +++++++++++----------- kernel/trace/trace_events.c | 9 ++++++--- kernel/trace/trace_functions_graph.c | 5 ++++- kernel/trace/trace_seq.c | 2 +- 6 files changed, 42 insertions(+), 18 deletions(-)
[...]
Quoted 5 lines
kernel/trace/trace.c +0
-0@@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) {int len;
kernel/trace/trace.c static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) · line 953
Quoted 10 lines
kernel/trace/trace.c +2
-2-if (s->seq.len <= s->seq.readpos) +if (trace_seq_used (s) <= s->seq.readpos)return -EBUSY ; - len = s->seq.len - s->seq.readpos; + len =trace_seq_used (s) - s->seq.readpos;if (cnt > len) cnt = len;memcpy (buf, s->buffer + s->seq.readpos, cnt);
There is one more dangerous usage in trace_printk_seq(). It is on three lines there.
The rest looks good.
Best Regards,
Petr
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 39 lines · On Tue, 18 Nov 2014 17:33:54 +0100 Petr Mladek <[email protected]> wrote:
Quoted 24 lines · Steven Rostedt
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.As the
seq_buf->lenwill soon be +1 size when there's an overflow, we must usetrace_seq_used()orseq_buf_used()methods to get the real length. This will prevent buffer overflow issues if just the len of theseq_bufdescriptor is used to copy memory.Reported-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>--- include/linux/trace_seq.h | 20 +++++++++++++++++++- kernel/trace/seq_buf.c | 2 +- kernel/trace/trace.c | 22 +++++++++++----------- kernel/trace/trace_events.c | 9 ++++++--- kernel/trace/trace_functions_graph.c | 5 ++++- kernel/trace/trace_seq.c | 2 +- 6 files changed, 42 insertions(+), 18 deletions(-)[...]
Quoted 10 lines
kernel/trace/trace.c +1
-1@@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) {int len; -if (s->seq.len <= s->seq.readpos) +if (trace_seq_used (s) <= s->seq.readpos)return -EBUSY ;
Quoted 9 lines
kernel/trace/trace.c static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) · line 953
Quoted 5 lines
kernel/trace/trace.c +1
-1- len = s->seq.len - s->seq.readpos; + len =trace_seq_used (s) - s->seq.readpos;if (cnt > len) cnt = len;memcpy (buf, s->buffer + s->seq.readpos, cnt);There is one more dangerous usage in
trace_printk_seq(). It is on three lines there.
You totally confused me. What usage in trace_printk_seq(), and what three lines?
In this patch, trace_printk_seq() looks like this:
int trace_print_seq(struct seq_file *m, struct trace_seq *s)
{
int ret;
__trace_seq_init(s);
ret = seq_buf_print_seq(m, &s->seq);
/*
* Only reset this buffer if we successfully wrote to the
* seq_file buffer. This lets the caller try again or
* do something else with the contents.
*/
if (!ret)
trace_seq_init(s);
return ret;
}-- Steve
Quoted 5 lines
The rest looks good.
Best Regards,
Petr
Re: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 67 lines · Steven Rostedt
Quoted 39 lines · On Tue, 18 Nov 2014 17:33:54 +0100 Petr Mladek <[email protected]> wrote:
Quoted 24 lines · Steven Rostedt
Quoted 5 lines
I don't like the fact that I did a code structure change with this patch. This patch should be just a simple conversion of len to
seq_buf_used(). I'm going to strip this change out and put it before this patch.As the
seq_buf->lenwill soon be +1 size when there's an overflow, we must usetrace_seq_used()orseq_buf_used()methods to get the real length. This will prevent buffer overflow issues if just the len of theseq_bufdescriptor is used to copy memory.Reported-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>--- include/linux/trace_seq.h | 20 +++++++++++++++++++- kernel/trace/seq_buf.c | 2 +- kernel/trace/trace.c | 22 +++++++++++----------- kernel/trace/trace_events.c | 9 ++++++--- kernel/trace/trace_functions_graph.c | 5 ++++- kernel/trace/trace_seq.c | 2 +- 6 files changed, 42 insertions(+), 18 deletions(-)[...]
Quoted 10 lines
kernel/trace/trace.c +1
-1@@ -944,10 +944,10 @@ static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) {int len; -if (s->seq.len <= s->seq.readpos) +if (trace_seq_used (s) <= s->seq.readpos)return -EBUSY ;Quoted 9 lines
kernel/trace/trace.c static ssize_t trace_seq_to_buffer(struct trace_seq *s, void *buf, size_t cnt) · line 953
Quoted 5 lines
kernel/trace/trace.c +1
-1- len = s->seq.len - s->seq.readpos; + len =trace_seq_used (s) - s->seq.readpos;if (cnt > len) cnt = len;memcpy (buf, s->buffer + s->seq.readpos, cnt);There is one more dangerous usage in
trace_printk_seq(). It is on three lines there.You totally confused me. What usage in
trace_printk_seq(), and what three lines?In this patch,
trace_printk_seq()looks like this:int
trace_print_seq(struct seq_file *m, struct trace_seq *s){ int ret; __trace_seq_init(s); ret = seq_buf_print_seq(m, &s->seq); /* * Only reset this buffer if we successfully wrote to the
Quoted 8 lines
* seq_file buffer. This lets the caller try again or
* do something else with the contents.
*/
if (!ret)
trace_seq_init(s);
return ret;
}The confusion is caused by the 'k' ("print" vs. "printk") in the function name. I was talking about the following function from kernel/trace/trace.c:
voidtrace_printk_seq(struct trace_seq *s)
{
/* Probably should print a warning here. */
if (s->seq.len >= TRACE_MAX_PRINT)
s->seq.len = TRACE_MAX_PRINT;
/* should be zero ended, but we are paranoid. */
s->buffer[s->seq.len] = 0;
printk(KERN_TRACE "%s", s->buffer);
trace_seq_init(s);
}I found it when checking the applied patches in origin/rfc/seq-buf branch. I hope that it was the correct place.
Best Regards,
Petr
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 25 lines · On Wed, 19 Nov 2014 12:40:17 +0100 Petr Mladek <[email protected]> wrote:
Quoted 25 lines
Quoted 3 lines
There is one more dangerous usage in
trace_printk_seq(). It is on three lines there.You totally confused me. What usage in
trace_printk_seq(), and what three lines?In this patch,
trace_printk_seq()looks like this:int
trace_print_seq(struct seq_file *m, struct trace_seq *s){ int ret; __trace_seq_init(s); ret = seq_buf_print_seq(m, &s->seq); /* * Only reset this buffer if we successfully wrote to the * seq_file buffer. This lets the caller try again or * do something else with the contents. */ if (!ret) trace_seq_init(s);
Quoted 6 lines
Quoted 2 lines
return ret; }The confusion is caused by the 'k' ("print" vs. "printk") in the function name. I was talking about the following function from
kernel/trace/trace.c:
Silly 'k', Trix are for kids!
Quoted 16 lines
void
trace_printk_seq(struct trace_seq *s){ /* Probably should print a warning here. */ if (s->seq.len >= TRACE_MAX_PRINT) s->seq.len = TRACE_MAX_PRINT; /* should be zero ended, but we are paranoid. */ s->buffer[s->seq.len] = 0; printk(KERN_TRACE "%s", s->buffer); trace_seq_init(s); }
Quoted 2 lines
I found it when checking the applied patches in origin/rfc/seq-buf branch. I hope that it was the correct place.
Yes, that's the working branch for this code.
Anyway, I saw this and thought about using trace_seq_used(), but then I realized that this is trace_seq code which has a hard coded buffer length of PAGE_SIZE which on all archs is more than 1000 (TRACE_MAX_PRINT).
Regardless of overflow or not (or even if trace_seq is full), that if statement will prevent this from doing any buffer overflows.
s->seq.len will never be more than s->seq.size after the test against TRACE_MAX_PRINT. So I see no harm here.
trace_printk_seq() is for dumping the ring buffer to console, which is usually something done on panic. It's special.
-- Steve
Petr Mladek
Reviewed-byRe: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Wed, 19 Nov 2014 12:40:17 +0100 Petr Mladek <[email protected]> wrote:
Quoted 4 lines
Quoted 3 lines
There is one more dangerous usage in
trace_printk_seq(). It is on three lines there.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
You totally confused me. What usage in
trace_printk_seq(), and what three lines?The confusion is caused by the 'k' ("print" vs. "printk") in the function name. I was talking about the following function from
kernel/trace/trace.c:Silly 'k', Trix are for kids!
:-)
Quoted 26 lines
Quoted 16 lines
void
trace_printk_seq(struct trace_seq *s){ /* Probably should print a warning here. */ if (s->seq.len >= TRACE_MAX_PRINT) s->seq.len = TRACE_MAX_PRINT; /* should be zero ended, but we are paranoid. */ s->buffer[s->seq.len] = 0; printk(KERN_TRACE "%s", s->buffer); trace_seq_init(s); }Quoted 2 lines
I found it when checking the applied patches in origin/rfc/seq-buf branch. I hope that it was the correct place.
Yes, that's the working branch for this code.
Anyway, I saw this and thought about using
trace_seq_used(), but then I realized that this istrace_seqcode which has a hard coded buffer length ofPAGE_SIZEwhich on all archs is more than 1000 (TRACE_MAX_PRINT).
Quoted 5 lines
Regardless of overflow or not (or even if
trace_seqis full), that if statement will prevent this from doing any buffer overflows.
s->seq.lenwill never be more thans->seq.sizeafter the test againstTRACE_MAX_PRINT. So I see no harm here.
Ah, I see. Well, I would feel more comfortable if it uses trace_seq_used() or if there is some explanation in a comment. But you are right, it is safe as it is. Feel free to leave it.
Best Regards,
Petr
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 6 lines · On Wed, 19 Nov 2014 15:40:05 +0100 Petr Mladek <[email protected]> wrote:
Quoted 2 lines
s->seq.lenwill never be more thans->seq.sizeafter the test againstTRACE_MAX_PRINT. So I see no harm here.Ah, I see. Well, I would feel more comfortable if it uses
trace_seq_used()or if there is some explanation in a comment. But you are right, it is safe as it is. Feel free to leave it.
Yeah, a comment would be nice. I may add it late.
Quoted 2 lines
Reviewed-by Petr Mladek <[email protected]>
Thanks,
-- Steve
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
In reply to Petr MladekQuoted 10 lines · On Wed, 19 Nov 2014 15:40:05 +0100 Petr Mladek <[email protected]> wrote:
Quoted 5 lines
Regardless of overflow or not (or even if
trace_seqis full), that if statement will prevent this from doing any buffer overflows.
s->seq.lenwill never be more thans->seq.sizeafter the test againstTRACE_MAX_PRINT. So I see no harm here.Ah, I see. Well, I would feel more comfortable if it uses
trace_seq_used()or if there is some explanation in a comment. But you are right, it is safe as it is. Feel free to leave it.
OK, I added this just for you:
BTW, using trace_seq_used() would not be good enough because it could return s->seq.size. Which would overflow the buffer on the
s->buffer[s->seq.len] = 0;
-- Steve
Quoted 1 line
From
1922acc9987d23d0b9c1ad28dc3eaafc1cf2d6b7Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 19 Nov 2014 10:56:41 -0500
Subject: [PATCH] tracing: Add paranoid size check in trace_printk_seq()
To be really paranoid about writing out of bound data in trace_printk_seq(), add another check of len compared to size.
--- kernel/trace/trace.c | 8 ++++++++ 1 file changed, 8 insertions(+)
kernel/trace/trace.c +8 -0
@@ -6656,6 +6656,14 @@ trace_printk_seq(struct trace_seq *s)if (s->seq.len >=TRACE_MAX_PRINT ) s->seq.len =TRACE_MAX_PRINT ; +/* +* More paranoid code. Although the buffer size is set to +* PAGE_SIZE, and TRACE_MAX_PRINT is 1000, this is just +* an extra layer of protection. +*/ +if (WARN_ON_ONCE (s->seq.len >= s->seq.size)) + s->seq.len = s->seq.size -1 ; +/* should be zero ended, but we are paranoid. */ s->buffer[s->seq.len] =0 ;
Signature · 1 line
1.8.1.4
Petr Mladek
Reviewed-byRe: [PATCH 2/2] tracing: Use trace_seq_used() and seq_buf_used() instead of len
Quoted 14 lines · Steven Rostedt
Quoted 10 lines · On Wed, 19 Nov 2014 15:40:05 +0100 Petr Mladek <[email protected]> wrote:
Quoted 5 lines
Regardless of overflow or not (or even if
trace_seqis full), that if statement will prevent this from doing any buffer overflows.
s->seq.lenwill never be more thans->seq.sizeafter the test againstTRACE_MAX_PRINT. So I see no harm here.Ah, I see. Well, I would feel more comfortable if it uses
trace_seq_used()or if there is some explanation in a comment. But you are right, it is safe as it is. Feel free to leave it.
Quoted 6 lines
OK, I added this just for you:
BTW, using
trace_seq_used()would not be good enough because it could returns->seq.size. Which would overflow the buffer on thes->buffer[s->seq.len] = 0;
Great catch!
Quoted 7 lines
-- Steve
From
1922acc9987d23d0b9c1ad28dc3eaafc1cf2d6b7Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 19 Nov 2014 10:56:41 -0500
Subject: [PATCH] tracing: Add paranoid size check intrace_printk_seq()
Quoted 7 lines
To be really paranoid about writing out of bound data in
trace_printk_seq(), add another check of len compared to size.Suggested-by Petr Mladek <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
Thanks a lot for the addition.
Best Regards,
Petr
Quoted 26 lines
--- kernel/trace/trace.c | 8 ++++++++ 1 file changed, 8 insertions(+)kernel/trace/trace.c +8
-0@@ -6656,6 +6656,14 @@ trace_printk_seq(struct trace_seq *s)if (s->seq.len >=TRACE_MAX_PRINT ) s->seq.len =TRACE_MAX_PRINT ; +/* +* More paranoid code. Although the buffer size is set to +* PAGE_SIZE, and TRACE_MAX_PRINT is 1000, this is just +* an extra layer of protection. +*/ +if (WARN_ON_ONCE (s->seq.len >= s->seq.size)) + s->seq.len = s->seq.size -1 ; +/* should be zero ended, but we are paranoid. */ s->buffer[s->seq.len] =0 ;Signature · 1 line
1.8.1.4
Steven Rostedt
maintainerPATCH v5 20/26 seq_buf: Add seq_buf_can_fit() helper function
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Add a seq_buf_can_fit() helper function that removes the possible mistakes of comparing the seq_buf length plus added data compared to the size of the buffer.
--- kernel/trace/seq_buf.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
kernel/trace/seq_buf.c +10 -5
@@ -16,6 +16,11 @@#include <linux/seq_file.h> #include <linux/seq_buf.h> +static bool seq_buf_can_fit (struct seq_buf *s,size_t len) +{ +return s->len + len < s->size; +} +/** * seq_buf_print_seq - move the contents of seq_buf into a seq_file * @m: the seq_file descriptor that is the destination
@@ -48,7 +53,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)if (s->len < s->size) { len =vsnprintf (s->buffer + s->len, s->size - s->len, fmt, args); -if (s->len + len < s->size) { +if (seq_buf_can_fit (s, len)) { s->len += len;return 0 ; }
@@ -137,7 +142,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)if (s->len < s->size) { ret =bstr_printf (s->buffer + s->len, len, fmt, binary); -if (s->len + ret < s->size) { +if (seq_buf_can_fit (s, ret)) { s->len += ret;return 0 ; }
@@ -161,7 +166,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)WARN_ON (s->size ==0 ); -if (s->len + len < s->size) { +if (seq_buf_can_fit (s, len)) {memcpy (s->buffer + s->len, str, len); s->len += len;return 0 ;
@@ -183,7 +188,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c) {WARN_ON (s->size ==0 ); -if (s->len +1 < s->size) { +if (seq_buf_can_fit (s,1 )) { s->buffer[s->len++] = c;return 0 ; }
@@ -207,7 +212,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len) {WARN_ON (s->size ==0 ); -if (s->len + len < s->size) { +if (seq_buf_can_fit (s, len)) {memcpy (s->buffer + s->len, mem, len); s->len += len;return 0 ;
Signature · 1 line
2.1.1
Steven Rostedt
maintainerRe: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 16 lines · On Fri, 14 Nov 2014 23:59:07 -0500 Steven Rostedt <[email protected]> wrote:
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a
seq_buf_can_fit()helper function that removes the possible mistakes of comparing theseq_buflength plus added data compared to the size of the buffer.Signed-off-by Steven Rostedt <[email protected]>--- kernel/trace/seq_buf.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)kernel/trace/seq_buf.c +0
-0
kernel/trace/seq_buf.c line 18
Quoted 3 lines
kernel/trace/seq_buf.c +0
-0@@ -16,6 +16,11 @@#include <linux/seq_file.h> #include <linux/seq_buf.h>
I'm adding a comment here:
/**
* seq_buf_can_fit - can the new data fit in the current buffer?
* @s: the seq_buf descriptor
* @len: The length to see if it can fit in the current buffer *
* Returns true if there's enough unused space in the seq_buf buffer
* to fit the amount of new data according to @len.
*/
-- Steve
Quoted 5 lines
kernel/trace/seq_buf.c +5
-0+static bool seq_buf_can_fit (struct seq_buf *s,size_t len) +{ +return s->len + len < s->size; +} +
Re: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 8 lines · Steven Rostedt
Quoted 5 lines · On Fri, 14 Nov 2014 23:59:07 -0500 Steven Rostedt <[email protected]> wrote:
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a
seq_buf_can_fit()helper function that removes the possible mistakes of comparing theseq_buflength plus added data compared to the size of the buffer.
[]
Quoted 5 lines
kernel/trace/seq_buf.c line 23
Quoted 5 lines
kernel/trace/seq_buf.c +5
-0+static bool seq_buf_can_fit (struct seq_buf *s,size_t len) +{ +return s->len + len < s->size; +} +
Why is this useful?
Steven Rostedt
maintainerRe: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 9 lines · On Mon, 17 Nov 2014 16:07:33 -0800 Joe Perches <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
Quoted 5 lines · On Fri, 14 Nov 2014 23:59:07 -0500 Steven Rostedt <[email protected]> wrote:
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a
seq_buf_can_fit()helper function that removes the possible mistakes of comparing theseq_buflength plus added data compared to the size of the buffer.
Quoted 8 lines
[]
Quoted 5 lines
kernel/trace/seq_buf.c line 23
Quoted 5 lines
kernel/trace/seq_buf.c +5
-0+static bool seq_buf_can_fit (struct seq_buf *s,size_t len) +{ +return s->len + len < s->size; +} +Why is this useful?
Places the logic in one place and makes the next patch much shorter.
As the change log states, makes mistakes much less likely to happen (note, I made one doing the next change and this makes me more confident not to do another one)
-- Steve
Re: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 21 lines · Steven Rostedt
Quoted 9 lines · On Mon, 17 Nov 2014 16:07:33 -0800 Joe Perches <[email protected]> wrote:
Quoted 8 lines · Steven Rostedt
Quoted 5 lines · On Fri, 14 Nov 2014 23:59:07 -0500 Steven Rostedt <[email protected]> wrote:
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a
seq_buf_can_fit()helper function that removes the possible mistakes of comparing theseq_buflength plus added data compared to the size of the buffer.Quoted 8 lines
[]
Quoted 5 lines
kernel/trace/seq_buf.c line 23
Quoted 5 lines
kernel/trace/seq_buf.c +5
-0+static bool seq_buf_can_fit (struct seq_buf *s,size_t len) +{ +return s->len + len < s->size; +} +Why is this useful?
Quoted 1 line
Places the logic in one place and makes the next patch much shorter.
What "logic" does it place in one place and how does it matter?
I don't see it making mistakes more or less likely, I just see it being used to avoid setting the overflow state which seems like more of an error than anything else.
Why avoid setting overflow at all?
Steven Rostedt
maintainerRe: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 2 lines · On Mon, 17 Nov 2014 16:35:32 -0800 Joe Perches <[email protected]> wrote:
What "logic" does it place in one place and how does it matter?
Look at the next patch.
Quoted 8 lines
I don't see it making mistakes more or less likely, I just see it being used to avoid setting the overflow state which seems like more of an error than anything else.
Why avoid setting overflow at all?
It has nothing to do with overflow. Where did you get that from?
It has to do with updating the seq->len value or not.
-- Steve
Re: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 8 lines · Steven Rostedt
Quoted 2 lines · On Mon, 17 Nov 2014 16:35:32 -0800 Joe Perches <[email protected]> wrote:
What "logic" does it place in one place and how does it matter?
Look at the next patch.
I don't have it and you are not cc'ing me.
I think you are getting carried away with the helpers.
Quoted 6 lines
Quoted 6 lines
I don't see it making mistakes more or less likely, I just see it being used to avoid setting the overflow state which seems like more of an error than anything else.
Why avoid setting overflow at all?
[]
Quoted 1 line
It has nothing to do with overflow. Where did you get that from?
writing to seq_buf really only cares about overflow.
seq_buf -> write to buffer -> overflowed? expand buffer, redo everything else when finished, dump buffer
Steven Rostedt
maintainerRe: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 3 lines · On Mon, 17 Nov 2014 17:07:58 -0800 Joe Perches <[email protected]> wrote:
Quoted 1 line
Look at the next patch.
I don't have it and you are not cc'ing me.
It's on LKML.
Quoted 1 line
I think you are getting carried away with the helpers.
That's nice.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
I don't see it making mistakes more or less likely, I just see it being used to avoid setting the overflow state which seems like more of an error than anything else.
Why avoid setting overflow at all?
Quoted 8 lines
[]
Quoted 1 line
It has nothing to do with overflow. Where did you get that from?
writing to
seq_bufreally only cares about overflow.
seq_buf-> write to buffer -> overflowed? expand buffer, redo everything else when finished, dump buffer
Um, that may be the case for seq_file, but it is not the case for trace_seq. seq_buf is influenced by seq_file because I have a patch to make seq_file use it, but it's also the guts of trace_seq that has some different requirements. And it's also not the case with the users of seq_buf in the last patch.
-- Steve
Re: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 5 lines · Steven Rostedt
Quoted 2 lines · On Mon, 17 Nov 2014 17:07:58 -0800 Joe Perches <[email protected]> wrote:
Quoted 1 line
Look at the next patch.
I don't have it and you are not cc'ing me.
It's on LKML.
And? There's no convenient way to retrieve it.
Quoted 2 lines
Quoted 1 line
I think you are getting carried away with the helpers.
That's nice.
And possibly true.
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I don't see it making mistakes more or less likely, I just see it being used to avoid setting the overflow state which seems like more of an error than anything else.
Why avoid setting overflow at all?
Quoted 8 lines
[]
Quoted 1 line
It has nothing to do with overflow. Where did you get that from?
writing to
seq_bufreally only cares about overflow.
seq_buf-> write to buffer -> overflowed? expand buffer, redo everything else when finished, dump buffer
Quoted 5 lines
Um, that may be the case for
seq_file, but it is not the case fortrace_seq.seq_bufis influenced byseq_filebecause I have a patch to makeseq_fileuse it, but it's also the guts oftrace_seqthat has some different requirements. And it's also not the case with the users ofseq_bufin the last patch.
I think your patch subject description needs expanding. It says seq_buf, nothing about trace.
Perhaps making trace use seq internals is not the right thing to do.
I also think you should break up this perhaps overlarge patch set into multiple independent sets that can be applied in separate chucks rather than all at once.
Steven Rostedt
maintainerRe: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 8 lines · On Mon, 17 Nov 2014 17:48:37 -0800 Joe Perches <[email protected]> wrote:
Quoted 5 lines · Steven Rostedt
Quoted 2 lines · On Mon, 17 Nov 2014 17:07:58 -0800 Joe Perches <[email protected]> wrote:
Quoted 1 line
Look at the next patch.
I don't have it and you are not cc'ing me.
It's on LKML.
And? There's no convenient way to retrieve it.
You're not subscribed?
Quoted 8 lines
Quoted 5 lines
Um, that may be the case for
seq_file, but it is not the case fortrace_seq.seq_bufis influenced byseq_filebecause I have a patch to makeseq_fileuse it, but it's also the guts oftrace_seqthat has some different requirements. And it's also not the case with the users ofseq_bufin the last patch.I think your patch subject description needs expanding. It says
seq_buf, nothing about trace.
It doesn't need to. This helps out the code. seq_buf has nothing to do with seq_file (yet).
Quoted 3 lines
Perhaps making trace use seq internals is not the right thing to do.
But this code comes from the trace_seq internals. It's a way to combine the code between seq_file and trace_seq. It is influenced by seq_file, but the code is trace_seq. Actually, this patch set has nothing to do with seq_file and finishes up solving a problem with printks dump stacks from NMIs.
Look at the path of the seq_buf.c code.
Quoted 5 lines
I also think you should break up this perhaps overlarge patch set into multiple independent sets that can be applied in separate chucks rather than all at once.
Why? I'm the one that is maintaining it. It's going to go through my tree and it has almost all the acks I need.
I will say the first 13 patches have already been acked and reviewed, and are going to be going into my for-next queue soon. I'll be posting a smaller set with the patches that changed.
Well, all but the last 3 patches. Those are the ones that I'm going to work with Linus and Andrew on before I pull them into my tree.
-- Steve
Re: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 5 lines · Steven Rostedt
Quoted 3 lines · On Mon, 17 Nov 2014 17:48:37 -0800 Joe Perches <[email protected]> wrote:
Quoted 2 lines · Steven Rostedt
On Mon, 17 Nov 2014 17:07:58 -0800
Joe Perches <[email protected]> wrote:
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
Look at the next patch.
I don't have it and you are not cc'ing me.
It's on LKML.
And? There's no convenient way to retrieve it.
You're not subscribed?
I am subscribed and I delete stuff, don't you?
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
Um, that may be the case for
seq_file, but it is not the case fortrace_seq.seq_bufis influenced byseq_filebecause I have a patch to makeseq_fileuse it, but it's also the guts oftrace_seqthat has some different requirements. And it's also not the case with the users ofseq_bufin the last patch.I think your patch subject description needs expanding. It says
seq_buf, nothing about trace.It doesn't need to. This helps out the code.
seq_bufhas nothing to do withseq_file(yet).
No, I don't think it does help seq_buf. It only helps trace.
btw:
I think you should cc Al Viro on the entire patch set.
Steven Rostedt
maintainerRe: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
Quoted 7 lines · On Mon, 17 Nov 2014 18:50:52 -0800 Joe Perches <[email protected]> wrote:
Quoted 6 lines · Steven Rostedt
Quoted 4 lines · On Mon, 17 Nov 2014 17:48:37 -0800 Joe Perches <[email protected]> wrote:
Quoted 3 lines · Steven Rostedt
Quoted 1 line · On Mon, 17 Nov 2014 17:07:58 -0800 Joe Perches <[email protected]> wrote:
Quoted 1 line
Look at the next patch.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
I don't have it and you are not cc'ing me.
It's on LKML.
And? There's no convenient way to retrieve it.
You're not subscribed?
I am subscribed and I delete stuff, don't you?
Nope, I keep all LKML messages. I have archives since 2005.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Um, that may be the case for
seq_file, but it is not the case fortrace_seq.seq_bufis influenced byseq_filebecause I have a patch to makeseq_fileuse it, but it's also the guts oftrace_seqthat has some different requirements. And it's also not the case with the users ofseq_bufin the last patch.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
I think your patch subject description needs expanding. It says
seq_buf, nothing about trace.It doesn't need to. This helps out the code.
seq_bufhas nothing to do withseq_file(yet).No, I don't think it does help
seq_buf. It only helps trace.
What are you talking about? seq_buf did not exist until this patch set.
Quoted 5 lines
btw:
I think you should cc Al Viro on the entire patch set.
Why? Currently this is only tracing code. There's nothing in here that has anything to do with VFS.
When I post patches to convert seq_file to use seq_buf, then I'll Cc Al.
-- Steve
Re: [PATCH 20/26 v5] seq_buf: Add seq_buf_can_fit() helper function
In reply to Steven RostedtQuoted 7 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
Add a
seq_buf_can_fit()helper function that removes the possible mistakes of comparing theseq_buflength plus added data compared to the size of the buffer.Signed-off-by Steven Rostedt <[email protected]>
Best Regards,
Petr
Steven Rostedt
maintainerPATCH v5 21/26 tracing: Have seq_buf use full buffer
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Currently seq_buf is full when all but one byte of the buffer is filled. Change it so that the seq_buf is full when all of the buffer is filled.
Some of the functions would fill the buffer completely and report everything was fine. This was inconsistent with the max of size - 1. Changing this to be max of size makes all functions consistent.
--- include/linux/seq_buf.h | 6 +++--- kernel/trace/seq_buf.c | 9 ++++++--- 2 files changed, 9 insertions(+), 6 deletions(-)
include/linux/seq_buf.h +3 -3
@@ -43,13 +43,13 @@ seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)static inline bool seq_buf_has_overflowed (struct seq_buf *s) { -return s->len == s->size; +return s->len > s->size; }static inline void seq_buf_set_overflow (struct seq_buf *s) { - s->len = s->size; + s->len = s->size +1 ; }/*
@@ -61,7 +61,7 @@ seq_buf_buffer_left(struct seq_buf *s)if (seq_buf_has_overflowed (s))return 0 ; -return (s->size -1 ) - s->len; +return s->size - s->len; }/* How much buffer was written? */
kernel/trace/seq_buf.c +6 -3
@@ -18,7 +18,7 @@static bool seq_buf_can_fit (struct seq_buf *s,size_t len) { -return s->len + len < s->size; +return s->len + len <= s->size; }/**
@@ -102,8 +102,11 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,WARN_ON (s->size ==0 );/* -* The last byte of the buffer is used to determine if we -* overflowed or not. +* Note, because bitmap_scnprintf() only returns the number of bytes +* written and not the number that would be written, we use the last +* byte of the buffer to let us know if we overflowed. There's a small +* chance that the bitmap could have fit exactly inside the buffer, but +* it's not that critical if that does happen. */ if (len >1 ) { ret =bitmap_scnprintf (s->buffer + s->len, len, maskp, nmaskbits);
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 22/26 tracing: Add seq_buf_get_buf() and seq_buf_commit() helper functions
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that are used by seq_buf_path(). This makes the code similar to theseq_file: seq_path() function, and will help to be able to consolidate the functions between seq_file and trace_seq.
--- include/linux/seq_buf.h | 41 +++++++++++++++++++++++++++++++++++++++++ kernel/trace/seq_buf.c | 7 +++---- 2 files changed, 44 insertions(+), 4 deletions(-)
include/linux/seq_buf.h +41 -0
@@ -70,6 +70,47 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)return min (s->len, s->size); } +/** +* seq_buf_get_buf - get buffer to write arbitrary data to +* @s: the seq_buf handle +* @bufp: the beginning of the buffer is stored here +* +* Return the number of bytes available in the buffer, or zero if +* there's no space. +*/ +static inline size_t seq_buf_get_buf (struct seq_buf *s,char **bufp) +{ +WARN_ON (s->len > s->size +1 ); + +if (s->len < s->size) { + *bufp = s->buffer + s->len; +return s->size - s->len; + } + + *bufp =NULL ; +return 0 ; +} + +/** +* seq_buf_commit - commit data to the buffer +* @s: the seq_buf handle +* @num: the number of bytes to commit +* +* Commit @num bytes of data written to a buffer previously acquired +* by seq_buf_get. To signal an error condition, or that the data +* didn't fit in the available space, pass a negative @num value. +*/ +static inline void seq_buf_commit (struct seq_buf *s,int num) +{ +if (num <0 ) { +seq_buf_set_overflow (s); + }else { +/* num must be negative on overflow */ +BUG_ON (s->len + num > s->size); + s->len += num; + } +} + extern __printf(2, 3) int seq_buf_printf(struct seq_buf *s, const char *fmt, ...); extern __printf(2, 0)
kernel/trace/seq_buf.c +3 -4
@@ -285,8 +285,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,*/ int seq_buf_path (struct seq_buf *s,const struct path *path,const char *esc) { -char *buf = s->buffer + s->len; -size_t size =seq_buf_buffer_left (s); +char *buf; +size_t size =seq_buf_get_buf (s, &buf);int res =-1 ;WARN_ON (s->size ==0 );
@@ -299,8 +299,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc) res = end - buf; } } -if (res >0 ) - s->len += res; +seq_buf_commit (s, res);return res; }
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 23/26 seq-buf: Make seq_buf_bprintf() conditional on CONFIG_BINARY_PRINTF
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The function bstr_printf() from lib/vsprnintf.c is only available if CONFIG_BINARY_PRINTF is defined. This is due to the only user currently being the tracing infrastructure, which needs to select this config when tracing is configured. Until there is another user of the binary printf formats, this will continue to be the case.
Since seq_buf.c is now lives in lib/ and is compiled even without tracing, it must encompass its use of bstr_printf() which is used by seq_buf_printf(). This too is only used by the tracing infrastructure and is still encapsulated by the CONFIG_BINARY_PRINTF.
--- include/linux/seq_buf.h | 7 +++++-- kernel/trace/seq_buf.c | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-)
include/linux/seq_buf.h +5 -2
@@ -115,8 +115,6 @@ extern __printf(2, 3)int seq_buf_printf (struct seq_buf *s,const char *fmt, ...); extern __printf(2, 0) int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args); -extern int -seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary); extern int seq_buf_print_seq(struct seq_file *m, struct seq_buf *s); extern int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, int cnt);
@@ -130,4 +128,9 @@ extern int seq_buf_path(struct seq_buf *s, const struct path *path, const char *extern int seq_buf_bitmask (struct seq_buf *s,const unsigned long *maskp,int nmaskbits); +#ifdef CONFIG_BINARY_PRINTF +extern int +seq_buf_bprintf (struct seq_buf *s,const char *fmt,const u32 *binary); +#endif +#endif /* _LINUX_SEQ_BUF_H */
kernel/trace/seq_buf.c +2 -0
@@ -119,6 +119,7 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,return -1 ; } +#ifdef CONFIG_BINARY_PRINTF /** * seq_buf_bprintf - Write the printf string from binary arguments * @s: seq_buf descriptor
@@ -153,6 +154,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)seq_buf_set_overflow (s); return -1; } +#endif /* CONFIG_BINARY_PRINTF */ /** * seq_buf_puts - sequence printing of simple string
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 24/26 seq_buf: Move the seq_buf code to lib/
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
The seq_buf functions are rather useful outside of tracing. Instead of having it be dependent on CONFIG_TRACING, move the code into lib/ and allow other users to have access to it even when tracing is not configured.
The seq_buf utility is similar to the seq_file utility, but instead of writing sending data back up to userland, it writes it into a buffer defined at seq_buf_init(). This allows us to send a descriptor around that writes printf() formatted strings into it that can be retrieved later.
It is currently used by the tracing facility for such things like trace events to convert its binary saved data in the ring buffer into an ASCII human readable context to be displayed in /sys/kernel/debug/trace.
It can also be used for doing NMI prints safely from NMI context into the seq_buf and retrieved later and dumped to printk() safely. Doing printk() from an NMI context is dangerous because an NMI can preempt a current printk() and deadlock on it.
--- kernel/trace/Makefile | 1 - kernel/trace/seq_buf.c | 351 ------------------------------------------------- lib/Makefile | 2 +- lib/seq_buf.c | 351 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 352 insertions(+), 353 deletions(-) delete mode 100644 kernel/trace/seq_buf.c create mode 100644 lib/seq_buf.c
kernel/trace/Makefile +0 -1
@@ -29,7 +29,6 @@ obj-$(CONFIG_RING_BUFFER_BENCHMARK) += ring_buffer_benchmark.o obj-$(CONFIG_TRACING) += trace.o obj-$(CONFIG_TRACING) += trace_output.o obj-$(CONFIG_TRACING) += trace_seq.o -obj-$(CONFIG_TRACING) += seq_buf.o obj-$(CONFIG_TRACING) += trace_stat.o obj-$(CONFIG_TRACING) += trace_printk.o obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o
kernel/trace/seq_buf.c deleted +0 -351
351 lines not on this page · raw message
lib/Makefile +1 -1
@@ -13,7 +13,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \ sha1.o md5.o irq_regs.o reciprocal_div.o argv_split.o \ proportions.o flex_proportions.o ratelimit.o show_mem.o \ is_single_threaded.o plist.o decompress.o kobject_uevent.o \ - earlycpio.o + earlycpio.o seq_buf.o obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o lib-$(CONFIG_MMU) += ioremap.o
lib/seq_buf.c new file +351 -0
351 lines not on this page · raw message
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 25/26 printk: Add per_cpu printk func to allow printk to be diverted
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
Being able to divert printk to call another function besides the normal logging is useful for such things like NMI handling. If some functions are to be called from NMI that does printk() it is possible to lock up the box if the nmi handler triggers when another printk is happening.
One example of this use is to perform a stack trace on all CPUs via NMI. But if the NMI is to do the printk() it can cause the system to lock up. By allowing the printk to be diverted to another function that can safely record the printk output and then print it when it in a safe context then NMIs will be safe to call these functions like show_regs().
--- include/linux/percpu.h | 3 +++ include/linux/printk.h | 2 ++ kernel/printk/printk.c | 38 +++++++++++++++++++++++++++++--------- 3 files changed, 34 insertions(+), 9 deletions(-)
include/linux/percpu.h +3 -0
@@ -134,4 +134,7 @@ extern phys_addr_t per_cpu_ptr_to_phys(void *addr); (typeof(type) __percpu *)__alloc_percpu(sizeof(type), \ __alignof__(type )) +/* To avoid include hell, as printk can not declare this, we declare it here */ +DECLARE_PER_CPU (printk_func_t, printk_func); +#endif /* __LINUX_PERCPU_H */
include/linux/printk.h +2 -0
@@ -162,6 +162,8 @@ extern int kptr_restrict;extern void wake_up_klogd (void ); +typedef int (*printk_func_t )(const char *fmt,va_list args); +void log_buf_kexec_setup (void );void __init setup_log_buf (int early);void dump_stack_set_arch_desc (const char *fmt, ...);
kernel/printk/printk.c +29 -9
@@ -1807,6 +1807,30 @@ asmlinkage int printk_emit(int facility, int level, }EXPORT_SYMBOL (printk_emit); +int vprintk_default (const char *fmt,va_list args) +{ +int r; + +#ifdef CONFIG_KGDB_KDB +if (unlikely (kdb_trap_printk)) { + r =vkdb_printf (fmt, args); +return r; + } +#endif + r =vprintk_emit (0 ,-1 ,NULL ,0 , fmt, args); + +return r; +} +EXPORT_SYMBOL_GPL (vprintk_default); + +/* +* This allows printk to be diverted to another function per cpu. +* This is useful for calling printk functions from within NMI +* without worrying about race conditions that can lock up the +* box. +*/ +DEFINE_PER_CPU (printk_func_t, printk_func) = vprintk_default; +/** * printk - print a kernel message * @fmt: format string
@@ -1830,19 +1854,15 @@ EXPORT_SYMBOL(printk_emit);*/ asmlinkage __visible int printk(const char *fmt, ...) { +printk_func_t vprintk_func;va_list args;int r; -#ifdef CONFIG_KGDB_KDB -if (unlikely (kdb_trap_printk)) { -va_start (args, fmt); - r =vkdb_printf (fmt, args); -va_end (args); -return r; - } -#endif va_start (args, fmt); - r =vprintk_emit (0 ,-1 ,NULL ,0 , fmt, args); +preempt_disable (); + vprintk_func =this_cpu_read (printk_func); + r =vprintk_func (fmt, args); +preempt_enable ();va_end (args);return r;
Signature · 1 line
2.1.1
Steven Rostedt
maintainerPATCH v5 26/26 x86/nmi: Perform a safe NMI stack trace on all CPUs
In reply to Steven RostedtFrom: "Steven Rostedt (Red Hat)" <[email protected]>
When trigger_all_cpu_backtrace() is called on x86, it will trigger an NMI on each CPU and call show_regs(). But this can lead to a hard lock up if the NMI comes in on another printk().
In order to avoid this, when the NMI triggers, it switches the printk routine for that CPU to call a NMI safe printk function that records the printk in a per_cpu seq_buf descriptor. After all NMIs have finished recording its data, the seq_bufs are printed in a safe context.
--- arch/x86/kernel/apic/hw_nmi.c | 91 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 86 insertions(+), 5 deletions(-)
arch/x86/kernel/apic/hw_nmi.c +86 -5
@@ -18,6 +18,7 @@#include <linux/nmi.h> #include <linux/module.h> #include <linux/delay.h> +#include <linux/seq_buf.h> #ifdef CONFIG_HARDLOCKUP_DETECTOR u64 hw_nmi_get_sample_period (int watchdog_thresh)
@@ -29,14 +30,35 @@ u64 hw_nmi_get_sample_period(int watchdog_thresh)#ifdef arch_trigger_all_cpu_backtrace/* For reliability, we're prepared to waste bits here. */ static DECLARE_BITMAP(backtrace_mask, NR_CPUS) __read_mostly; +static cpumask_var_t printtrace_mask; + +#define NMI_BUF_SIZE 4096 + +struct nmi_seq_buf { +unsigned char buffer[NMI_BUF_SIZE ]; +struct seq_buf seq; +}; + +/* Safe printing in NMI context */ +static DEFINE_PER_CPU(struct nmi_seq_buf, nmi_print_seq);/* "in progress" flag of arch_trigger_all_cpu_backtrace */ static unsigned long backtrace_flag; +static void print_seq_line (struct nmi_seq_buf *s,int start,int end) +{ +const char *buf = s->buffer + start; + +printk ("%.*s" , (end - start) +1 , buf); +} +void arch_trigger_all_cpu_backtrace (bool include_self) { +struct nmi_seq_buf *s; +int len; +int cpu;int i; -int cpu =get_cpu (); +int this_cpu =get_cpu ();if (test_and_set_bit (0 , &backtrace_flag)) {/*
@@ -49,7 +71,17 @@ void arch_trigger_all_cpu_backtrace(bool include_self)cpumask_copy (to_cpumask (backtrace_mask), cpu_online_mask);if (!include_self) -cpumask_clear_cpu (cpu,to_cpumask (backtrace_mask)); +cpumask_clear_cpu (this_cpu,to_cpumask (backtrace_mask)); + +cpumask_copy (printtrace_mask,to_cpumask (backtrace_mask)); +/* +* Set up per_cpu seq_buf buffers that the NMIs running on the other +* CPUs will write to. +*/ +for_each_cpu (cpu,to_cpumask (backtrace_mask)) { + s = &per_cpu (nmi_print_seq, cpu); +seq_buf_init (&s->seq, s->buffer,NMI_BUF_SIZE ); + }if (!cpumask_empty (to_cpumask (backtrace_mask))) { pr_info("sending NMI to %s CPUs:\n",
@@ -65,11 +97,58 @@ void arch_trigger_all_cpu_backtrace(bool include_self)touch_softlockup_watchdog (); } +/* +* Now that all the NMIs have triggered, we can dump out their +* back traces safely to the console. +*/ +for_each_cpu (cpu, printtrace_mask) { +int last_i =0 ; + + s = &per_cpu (nmi_print_seq, cpu); + len =seq_buf_used (&s->seq); +if (!len) +continue ; + +/* Print line by line. */ +for (i =0 ; i < len; i++) { +if (s->buffer[i] =='\n' ) { +print_seq_line (s, last_i, i); + last_i = i +1 ; + } + } +/* Check if there was a partial line. */ +if (last_i < len) { +print_seq_line (s, last_i, len -1 ); +pr_cont ("\n" ); + } + } +clear_bit (0 , &backtrace_flag);smp_mb__after_atomic ();put_cpu (); } +/* +* It is not safe to call printk() directly from NMI handlers. +* It may be fine if the NMI detected a lock up and we have no choice +* but to do so, but doing a NMI on all other CPUs to get a back trace +* can be done with a sysrq-l. We don't want that to lock up, which +* can happen if the NMI interrupts a printk in progress. +* +* Instead, we redirect the vprintk() to this nmi_vprintk() that writes +* the content into a per cpu seq_buf buffer. Then when the NMIs are +* all done, we can safely dump the contents of the seq_buf to a printk() +* from a non NMI context. +*/ +static int nmi_vprintk (const char *fmt,va_list args) +{ +struct nmi_seq_buf *s =this_cpu_ptr (&nmi_print_seq); +unsigned int len =seq_buf_used (&s->seq); + +seq_buf_vprintf (&s->seq, fmt, args); +return seq_buf_used (&s->seq) - len; +} +static int arch_trigger_all_cpu_backtrace_handler (unsigned int cmd,struct pt_regs *regs) {
@@ -78,12 +157,14 @@ arch_trigger_all_cpu_backtrace_handler(unsigned int cmd, struct pt_regs *regs) cpu =smp_processor_id ();if (cpumask_test_cpu (cpu,to_cpumask (backtrace_mask))) { -static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED; +printk_func_t printk_func_save =this_cpu_read (printk_func); -arch_spin_lock (&lock); +/* Replace printk to write into the NMI seq */ +this_cpu_write (printk_func, nmi_vprintk);printk (KERN_WARNING "NMI backtrace for cpu %d\n" , cpu);show_regs (regs); -arch_spin_unlock (&lock); +this_cpu_write (printk_func, printk_func_save); +cpumask_clear_cpu (cpu,to_cpumask (backtrace_mask));return NMI_HANDLED ; }
Signature · 1 line
2.1.1
Re: [PATCH 26/26 v5] x86/nmi: Perform a safe NMI stack trace on all CPUs
Quoted 11 lines · Steven Rostedt
From: "Steven Rostedt (Red Hat)" <[email protected]>
When
trigger_all_cpu_backtrace()is called on x86, it will trigger an NMI on each CPU and callshow_regs(). But this can lead to a hard lock up if the NMI comes in on anotherprintk().In order to avoid this, when the NMI triggers, it switches the printk routine for that CPU to call a NMI safe printk function that records the printk in a
per_cpuseq_bufdescriptor. After all NMIs have finished recording its data, theseq_bufsare printed in a safe context.
Quoted 6 lines
Tested-by Jiri Kosina <[email protected]>Acked-by Jiri Kosina <[email protected]>Acked-by Paul E. McKenney <[email protected]>Signed-off-by Steven Rostedt <[email protected]>
I really like that the NMI part is fast and we do not longer block many CPUs until the others are finished with priting.
Best Regards,
Petr
Steven Rostedt
maintainerSo far the only difference between v5 I have is that I split the update of tracing_fill_pipe_page() out of "tracing: Use trace_seq_used() and seq_buf_used() instead of len", and this:
-- Steve
kernel/trace/trace.c +1 -1
@@ -4601,7 +4601,7 @@ tracing_fill_pipe_page(size_t rem, struct trace_iterator *iter) count =trace_seq_used (&iter->seq) - save_len;if (rem < count) { rem =0 ; - iter->seq.seq.len = save_len;; + iter->seq.seq.len = save_len;break ; }
kernel/trace/trace_syscalls.c +4 -0
@@ -131,6 +131,10 @@ print_syscall_enter(struct trace_iterator *iter, int flags,trace_seq_printf (s,"%s(" , entry->name);for (i =0 ; i < entry->nb_args; i++) { + +if (trace_seq_has_overflowed (s)) + goto end; +/* parameter types */ if (trace_flags &TRACE_ITER_VERBOSE )trace_seq_printf (s,"%s " , entry->types[i]);
lib/seq_buf.c +8 -0
@@ -16,6 +16,14 @@#include <linux/seq_file.h> #include <linux/seq_buf.h> +/** +* seq_buf_can_fit - can the new data fit in the current buffer? +* @s: the seq_buf descriptor +* @len: The length to see if it can fit in the current buffer +* +* Returns true if there's enough unused space in the seq_buf buffer +* to fit the amount of new data according to @len. +*/ static bool seq_buf_can_fit (struct seq_buf *s,size_t len) {return s->len + len <= s->size;