tracing: addition of event probe
Steven Rostedt
maintainercover tracing: addition of event probe
Hi Tzvetomir,
Here's the updates to your patch. the first patch is your WIP that I forward ported to the latest kernel. The second patches is my hacks to hook with the triggers. It's still buggy and needs a bit of work (as we talked about).
I'll comment in the code. Please just merge the two patches together, and fix them up to a proper patch that we can send to LKML in a week or two.
Tom, I Cc'd you because I wanted you to see where we are at, and because I mentioned this work with the fix I sent to Linus (removing 'char *' as a string for histograms). If you apply that patch, and these two, you can play with it a little, but it is buggy. It's not even in a "RFC" level yet. Thus, I'm Cc'ing you as more of an FYI than to ask you to review the patches. There's more work to be done.
I'll reply to this patch set stating some of the know issues.
Steven Rostedt (VMware) (1):
tracing: Update to the event probes
Tzvetomir (VMware) Stoyanov (1):
Add new kprobe on tracepoint
----
include/linux/kprobes.h | 13 ++ include/linux/trace_events.h | 4 + kernel/trace/trace.h | 5 + kernel/trace/trace_kprobe.c | 521 +++++++++++++++++++++++++++++++++++++++++-- kernel/trace/trace_probe.c | 25 ++- kernel/trace/trace_probe.h | 7 +- kernel/trace/trace_uprobe.c | 2 +- 7 files changed, 554 insertions(+), 23 deletions(-)
Steven Rostedt
maintainerPATCH 1/2 Add new kprobe on tracepoint
From: "Tzvetomir (VMware) Stoyanov" <[email protected]>
Hi Steven,
That is my progress so far on kprobe tracepoint implementation, it is still far from a working prototype. Looks like I have to add a new trigger type, as existing ones cannot be used. Triggers are designed to be configured from the user and that's why there is a lot of logic for that - the cmd_ops field in struct event_trigger_data. There are a lot of mandatory callbacks, that should be dummy functions in the new trigger type, as it will not be set through tracefs. I could implement these as dummy callbacks, or the logic could be changed so these callbacks can be optional. Which is the right approach?
--- include/linux/kprobes.h | 10 + include/linux/trace_events.h | 1 + kernel/trace/trace_kprobe.c | 355 +++++++++++++++++++++++++++++++++-- kernel/trace/trace_probe.c | 25 ++- kernel/trace/trace_probe.h | 7 +- kernel/trace/trace_uprobe.c | 2 +- 6 files changed, 380 insertions(+), 20 deletions(-)
include/linux/kprobes.h +10 -0
@@ -162,6 +162,16 @@ struct kretprobe {struct kretprobe_holder *rph; }; +struct keventprobe { +/* tracepoint system */ +const char *event_system; + +/* tracepoint event */ +const char *event_name; + +struct trace_event_call *tp; +}; +struct kretprobe_instance {union {struct freelist_node freelist;
include/linux/trace_events.h +1 -0
@@ -634,6 +634,7 @@ enum event_trigger_type {ETT_EVENT_ENABLE = (1 <<3 ),ETT_EVENT_HIST = (1 <<4 ),ETT_HIST_ENABLE = (1 <<5 ), + ETT_EVENT_KPROBE = (1 << 6), };extern int filter_match_preds (struct event_filter *filter,void *rec);
kernel/trace/trace_kprobe.c +342 -13
438 lines not on this page · raw message
kernel/trace/trace_probe.c +21 -4
@@ -227,12 +227,12 @@ int traceprobe_split_symbol_offset(char *symbol, long *offset)/* @buf must has MAX_EVENT_NAME_LEN size */ int traceprobe_parse_event_name (const char **pevent,const char **pgroup, -char *buf,int offset) +char *buf,int offset,int delim) {const char *slash, *event = *pevent;int len; - slash =strchr (event,'/' ); + slash =strchr (event, delim);if (slash) {if (slash == event) {trace_probe_log_err (offset,NO_GROUP_NAME );
@@ -316,6 +316,13 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t, code->op =FETCH_OP_ARG ; code->param = (unsigned int )param -1 ;#endif + }else if (flags &TPARG_FL_TPOINT ) { +if (code->data) +return -EFAULT ; + code->data =kstrdup (arg,GFP_KERNEL ); +if (!code->data) +return -ENOMEM ; + code->op =FETCH_OP_TP_ARG ; }else goto inval_var;
@@ -633,14 +640,15 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size, !strcmp(parg->type->name, "ustring")) {if (code->op !=FETCH_OP_DEREF && code->op !=FETCH_OP_UDEREF && code->op !=FETCH_OP_IMM && code->op !=FETCH_OP_COMM && - code->op !=FETCH_OP_DATA ) { + code->op !=FETCH_OP_DATA && code->op !=FETCH_OP_TP_ARG ) {trace_probe_log_err (offset + (t ? (t - arg) :0 ),BAD_STRING ); ret = -EINVAL ; goto fail; }if ((code->op ==FETCH_OP_IMM || code->op ==FETCH_OP_COMM || - code->op ==FETCH_OP_DATA ) || parg->count) { + code->op ==FETCH_OP_DATA ) || code->op ==FETCH_OP_TP_ARG || + parg->count) {/* * IMM, DATA and COMM is pointing actual address, those * must be kept, and if parg->count != 0, this is an
@@ -985,6 +993,15 @@ void trace_probe_cleanup(struct trace_probe *tp)trace_probe_unlink (tp); } +void trace_event_probe_cleanup (struct keventprobe *ep) +{ +kfree (ep->event_name); +kfree (ep->event_system); +if (ep->tp) +module_put (ep->tp->mod); +kfree (ep); +} +int trace_probe_init (struct trace_probe *tp,const char *event,const char *group,bool alloc_filter) {
kernel/trace/trace_probe.h +5 -2
@@ -102,6 +102,7 @@ enum fetch_op {FETCH_OP_MOD_BF ,/* Bitfield: .basesize, .lshift, .rshift */ // Stage 5 (loop) op FETCH_OP_LP_ARRAY ,/* Array: .param = loop count */ +FETCH_OP_TP_ARG ,/* Trace Point argument */ FETCH_OP_END , FETCH_NOP_SYMBOL, /* Unresolved Symbol holder */ };
@@ -330,6 +331,7 @@ static inline bool trace_probe_has_single_file(struct trace_probe *tp)int trace_probe_init (struct trace_probe *tp,const char *event,const char *group,bool alloc_filter);void trace_probe_cleanup (struct trace_probe *tp); +void trace_event_probe_cleanup (struct keventprobe *ep);int trace_probe_append (struct trace_probe *tp,struct trace_probe *to);void trace_probe_unlink (struct trace_probe *tp);int trace_probe_register_event_call (struct trace_probe *tp);
@@ -351,7 +353,8 @@ int trace_probe_create(const char *raw_command, int (*createfn)(int, const char#define TPARG_FL_RETURN BIT(0)#define TPARG_FL_KERNEL BIT(1)#define TPARG_FL_FENTRY BIT(2) -#define TPARG_FL_MASK GENMASK(2, 0) +#define TPARG_FL_TPOINT BIT(3) +#define TPARG_FL_MASK GENMASK(3, 0)extern int traceprobe_parse_probe_arg (struct trace_probe *tp,int i,char *arg,unsigned int flags);
@@ -361,7 +364,7 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);extern int traceprobe_split_symbol_offset (char *symbol,long *offset);int traceprobe_parse_event_name (const char **pevent,const char **pgroup, -char *buf,int offset); +char *buf,int offset,int delim);extern int traceprobe_set_print_fmt (struct trace_probe *tp,bool is_return);
kernel/trace/trace_uprobe.c +1 -1
@@ -641,7 +641,7 @@ static int __trace_uprobe_create(int argc, const char **argv)trace_probe_log_set_index (0 );if (event) { ret =traceprobe_parse_event_name (&event, &group, buf, - event - argv[0 ]); + event - argv[0 ],'/' );if (ret) goto fail_address_parse; }else {
Signature · 1 line
2.30.2
Steven Rostedt
maintainerPATCH 2/2 tracing: Update to the event probes
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
--- include/linux/kprobes.h | 5 +- include/linux/trace_events.h | 3 + kernel/trace/trace.h | 5 + kernel/trace/trace_kprobe.c | 248 ++++++++++++++++++++++++++++------- kernel/trace/trace_probe.c | 4 +- 5 files changed, 218 insertions(+), 47 deletions(-)
include/linux/kprobes.h +4 -1
@@ -162,6 +162,8 @@ struct kretprobe {struct kretprobe_holder *rph; }; +struct trace_kprobe ; +struct keventprobe {/* tracepoint system */ const char *event_system;
@@ -169,7 +171,8 @@ struct keventprobe {/* tracepoint event */ const char *event_name; -struct trace_event_call *tp; +struct trace_event_call *event; +struct trace_kprobe *tk; };struct kretprobe_instance {
include/linux/trace_events.h +3 -0
@@ -312,6 +312,7 @@ enum {TRACE_EVENT_FL_TRACEPOINT_BIT ,TRACE_EVENT_FL_KPROBE_BIT ,TRACE_EVENT_FL_UPROBE_BIT , + TRACE_EVENT_FL_EPROBE_BIT, };/*
@@ -323,6 +324,7 @@ enum {* TRACEPOINT - Event is a tracepoint * KPROBE - Event is a kprobe * UPROBE - Event is a uprobe +* EPROBE - Event is an event probe */ enum {TRACE_EVENT_FL_FILTERED = (1 <<TRACE_EVENT_FL_FILTERED_BIT ),
@@ -332,6 +334,7 @@ enum {TRACE_EVENT_FL_TRACEPOINT = (1 <<TRACE_EVENT_FL_TRACEPOINT_BIT ),TRACE_EVENT_FL_KPROBE = (1 <<TRACE_EVENT_FL_KPROBE_BIT ),TRACE_EVENT_FL_UPROBE = (1 <<TRACE_EVENT_FL_UPROBE_BIT ), + TRACE_EVENT_FL_EPROBE = (1 << TRACE_EVENT_FL_EPROBE_BIT), };#define TRACE_EVENT_FL_UKPROBE (TRACE_EVENT_FL_KPROBE | TRACE_EVENT_FL_UPROBE)
kernel/trace/trace.h +5 -0
@@ -1547,9 +1547,14 @@ static inline int register_trigger_hist_enable_disable_cmds(void) { return 0; }extern int register_trigger_cmds (void );extern void clear_event_triggers (struct trace_array *tr); +enum { +EVENT_TRIGGER_FL_PROBE =BIT (0 ), +}; +struct event_trigger_data {unsigned long count;int ref; +int flags;struct event_trigger_ops *ops;struct event_command *cmd_ops;struct event_filter __rcu *filter;
kernel/trace/trace_kprobe.c +204 -44
387 lines not on this page · raw message
kernel/trace/trace_probe.c +2 -2
@@ -997,8 +997,8 @@ void trace_event_probe_cleanup(struct keventprobe *ep) {kfree (ep->event_name);kfree (ep->event_system); -if (ep->tp) -module_put (ep->tp->mod); +if (ep->event) +module_put (ep->event->mod);kfree (ep); }
Signature · 1 line
2.30.2
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Update to the event probes
Quoted 25 lines · On Thu, 15 Jul 2021 23:26:34 -0400 Steven Rostedt <[email protected]> wrote:
From: "Steven Rostedt (VMware)" <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>--- include/linux/kprobes.h | 5 +- include/linux/trace_events.h | 3 + kernel/trace/trace.h | 5 + kernel/trace/trace_kprobe.c | 248 ++++++++++++++++++++++++++++------- kernel/trace/trace_probe.c | 4 +- 5 files changed, 218 insertions(+), 47 deletions(-)include/linux/kprobes.h +2
-0@@ -162,6 +162,8 @@ struct kretprobe {struct kretprobe_holder *rph; }; +struct trace_kprobe ; +struct keventprobe {/* tracepoint system */ const char *event_system;
include/linux/kprobes.h struct keventprobe { · line 174
Quoted 5 lines
include/linux/kprobes.h +1
-1@@ -169,7 +171,8 @@ struct keventprobe {/* tracepoint event */ const char *event_name; -struct trace_event_call *tp; +struct trace_event_call *event;
I renamed "tp" to "event" as "tp" is used elsewhere for something that is not the same as a trace_event_call, which makes it confusing. Try to keep names consistent, for referencing the same type.
Quoted 25 lines
include/linux/kprobes.h +1
-0+struct trace_kprobe *tk; };struct kretprobe_instance {include/linux/trace_events.h +2
-0@@ -312,6 +312,7 @@ enum {TRACE_EVENT_FL_TRACEPOINT_BIT ,TRACE_EVENT_FL_KPROBE_BIT ,TRACE_EVENT_FL_UPROBE_BIT , + TRACE_EVENT_FL_EPROBE_BIT, };/* @@ -323,6 +324,7 @@ enum {* TRACEPOINT - Event is a tracepoint * KPROBE - Event is a kprobe * UPROBE - Event is a uprobe +* EPROBE - Event is an event probe */ enum {TRACE_EVENT_FL_FILTERED = (1 <<TRACE_EVENT_FL_FILTERED_BIT ),
include/linux/trace_events.h enum { · line 337
Quoted 4 lines
include/linux/trace_events.h +1
-0@@ -332,6 +334,7 @@ enum {TRACE_EVENT_FL_TRACEPOINT = (1 <<TRACE_EVENT_FL_TRACEPOINT_BIT ),TRACE_EVENT_FL_KPROBE = (1 <<TRACE_EVENT_FL_KPROBE_BIT ),TRACE_EVENT_FL_UPROBE = (1 <<TRACE_EVENT_FL_UPROBE_BIT ), + TRACE_EVENT_FL_EPROBE = (1 << TRACE_EVENT_FL_EPROBE_BIT),
I added the EPROBE but haven't used it yet. There are locations that this will make a difference, more below.
Quoted 27 lines
include/linux/trace_events.h +0
-0};#define TRACE_EVENT_FL_UKPROBE (TRACE_EVENT_FL_KPROBE | TRACE_EVENT_FL_UPROBE)kernel/trace/trace.h +5
-0@@ -1547,9 +1547,14 @@ static inline int register_trigger_hist_enable_disable_cmds(void) { return 0; }extern int register_trigger_cmds (void );extern void clear_event_triggers (struct trace_array *tr); +enum { +EVENT_TRIGGER_FL_PROBE =BIT (0 ), +}; +struct event_trigger_data {unsigned long count;int ref; +int flags;struct event_trigger_ops *ops;struct event_command *cmd_ops;struct event_filter __rcu *filter;kernel/trace/trace_kprobe.c +0
-0
kernel/trace/trace_kprobe.c static struct trace_kprobe *alloc_event_kprobe(const char *group, · line 342
Quoted 4 lines
kernel/trace/trace_kprobe.c +1
-0@@ -339,6 +339,7 @@ static struct trace_kprobe *alloc_event_kprobe(const char *group, tk->ep->event_system =kstrdup (sys_name,GFP_KERNEL );if (!tk->ep->event_system) goto error; + tk->ep->tk = tk;
We have access to the "ep" from the trigger, but we also need access to the tk structure as well, as it holds information on the probe args. Thus I had it point back. But this might be able to be passed by the data structure I have below. I added that later, so this may not be necessary. But instead of passing down the ep for creating the event probe, we would need to pass down the tk and save that.
kernel/trace/trace_kprobe.c static struct trace_kprobe *find_trace_kprobe(const char *event, · line 372
Quoted 11 lines
kernel/trace/trace_kprobe.c +4
-0tk->rp.maxactive = maxactive; ret =trace_probe_init (&tk->tp, event, group, false);@@ -365,6 +366,11 @@ static struct trace_kprobe *find_trace_kprobe(const char *event,return NULL ; } +struct eprobe_data { +struct keventprobe *ep; +struct trace_event_file *file; +};
And here's that data structure I was talking about. Maybe record the 'tk' instead of the 'ep'?
kernel/trace/trace_kprobe.c static int eprobe_trigger_print(struct seq_file *m, · line 397
Quoted 13 lines
kernel/trace/trace_kprobe.c +6
-0+static int eprobe_trigger_init (struct event_trigger_ops *ops,struct event_trigger_data *data) {@@ -384,11 +390,126 @@ static int eprobe_trigger_print(struct seq_file *m,return 0 ; } +static unsigned long get_event_field (struct fetch_insn *code,void *rec) +{ +struct ftrace_event_field *field = code->data; +unsigned long val; +void *addr;
This only handles numbers (and pointers). It doesn't handle string or arrays (dynamic or static). We have to think about what we are going to do with them.
Quoted 33 lines
kernel/trace/trace_kprobe.c +33
-0+ + addr = rec + field->offset; + +switch (field->size) { +case 1 : +if (field->is_signed) + val = *(char *)addr; +else + val = *(unsigned char *)addr; +break ; +case 2 : +if (field->is_signed) + val = *(short *)addr; +else + val = *(unsigned short *)addr; +break ; +case 4 : +if (field->is_signed) + val = *(int *)addr; +else + val = *(unsigned int *)addr; +break ; +default : +if (field->is_signed) + val = *(long *)addr; +else + val = *(unsigned long *)addr; +break ; + } +return val; +} + +static int get_eprobe_size (struct trace_probe *tp,void *rec)
kernel/trace/trace_kprobe.c static int eprobe_trigger_print(struct seq_file *m, · line 440
Quoted 10 lines
kernel/trace/trace_kprobe.c +10
-0+{ +struct probe_arg *arg; +int i, len, ret =0 ; + +for (i =0 ; i < tp->nr_args; i++) { + arg = tp->args + i; +if (unlikely (arg->dynamic)) { +unsigned long val; + + val =get_event_field (arg->code, rec);
I found that the process_fetch_insn_bottom() did all the kprobe magic against the val, and didn't require the regs (which the process_fetch_insn()) did. Hence, just get the data from the field and use that.
kernel/trace/trace_kprobe.c static int eprobe_trigger_print(struct seq_file *m, · line 453
Quoted 13 lines
kernel/trace/trace_kprobe.c +13
-0+ len =process_fetch_insn_bottom (arg->code +1 , val,NULL ,NULL ); +if (len >0 ) + ret += len; + } + } + +return ret; +} + +static inline void +store_event_args (void *data,struct trace_probe *tp,void *rec, +int header_size,int maxlen) +{
This function was basically a copy of "store_trace_args()" from trace_probe_tmpl.h, but instead of messing with regs, I used the get_event_field(). We should probably add a way to consolidate the two.
Quoted 19 lines
kernel/trace/trace_kprobe.c +19
-0+struct probe_arg *arg; +unsigned long val; +void *base = data - header_size; +void *dyndata = data + tp->size; +u32 *dl;/* Data location */ +int ret, i; + +for (i =0 ; i < tp->nr_args; i++) { + arg = tp->args + i; + dl = data + arg->offset; +/* Point the dynamic data area if needed */ +if (unlikely (arg->dynamic)) + *dl =make_data_loc (maxlen, dyndata - base); + val =get_event_field (arg->code, rec); + ret =process_fetch_insn_bottom (arg->code +1 , val, dl, base); +if (unlikely (ret <0 && arg->dynamic)) { + *dl =make_data_loc (0 , dyndata - base); + }else { + dyndata += ret;
kernel/trace/trace_kprobe.c static int eprobe_trigger_print(struct seq_file *m, · line 391
Quoted 10 lines
kernel/trace/trace_kprobe.c +5
-1+ maxlen -= ret; + } + } +} +static void eprobe_trigger_func (struct event_trigger_data *data,struct trace_buffer *buffer,void *rec,struct ring_buffer_event *rbe) { -/* ToDo */
This was basically a copy of __kprobe_trace_func() which is in the same file.
Probably consolidate the two as well?
Quoted 35 lines
kernel/trace/trace_kprobe.c +31
-0+struct kprobe_trace_entry_head *entry; +struct eprobe_data *edata = data->private_data; +struct keventprobe *ep = edata->ep; +struct trace_event_file *trace_file = edata->file; +struct trace_kprobe *tk = ep->tk; +struct trace_event_call *call =trace_probe_event_call (&tk->tp); +struct trace_event_buffer fbuffer; +int dsize; + +if (trace_trigger_soft_disabled (edata->file)) +return ; + + fbuffer.trace_ctx =tracing_gen_ctx (); + fbuffer.trace_file = trace_file; + + dsize =get_eprobe_size (&tk->tp, rec); + + fbuffer.event = +trace_event_buffer_lock_reserve (&fbuffer.buffer, trace_file, + call->event.type, +sizeof (*entry) + tk->tp.size + dsize, + fbuffer.trace_ctx); +if (!fbuffer.event) +return ; + + fbuffer.regs =0 ; + entry = fbuffer.entry =ring_buffer_event_data (fbuffer.event); + entry->ip = (unsigned long )tk->rp.kp.addr; +store_event_args (&entry[1 ], &tk->tp, rec,sizeof (*entry), dsize); + +trace_event_buffer_commit (&fbuffer); }static struct event_trigger_ops eprobe_trigger_ops = {
kernel/trace/trace_kprobe.c static struct event_trigger_ops *eprobe_trigger_get_ops(char *cmd, · line 550
Quoted 5 lines
kernel/trace/trace_kprobe.c +1
-1@@ -426,9 +547,9 @@ static struct event_trigger_ops *eprobe_trigger_get_ops(char *cmd, }static struct event_command event_trigger_cmd = { - .name ="kprobe" , + .name ="eprobe" ,
Not sure if the name mattered, but decided to change it.
kernel/trace/trace_kprobe.c static struct event_trigger_ops *eprobe_trigger_get_ops(char *cmd, · line 552
Quoted 3 lines
kernel/trace/trace_kprobe.c +1
-1.trigger_type = ETT_EVENT_KPROBE, - .flags = EVENT_CMD_FL_POST_TRIGGER | EVENT_CMD_FL_NEEDS_REC, + .flags = EVENT_CMD_FL_NEEDS_REC,
"POST_TRIGGER" is called after the event is committed, which means you lose out on all the event fields. That is, POST_TRIGGER is actually mutually exclusive with NEEDS_REC.
kernel/trace/trace_kprobe.c static struct event_command event_trigger_cmd = { · line 562
Quoted 10 lines
kernel/trace/trace_kprobe.c +2
-1.func = eprobe_trigger_cmd_func, .reg = eprobe_trigger_reg_func, .unreg = eprobe_trigger_unreg_func,@@ -437,56 +558,87 @@ static struct event_command event_trigger_cmd = { .set_filter = NULL, }; -static int new_eprobe_trigger (struct event_trigger_data **trigger) +static struct event_trigger_data * +new_eprobe_trigger (struct keventprobe *ep,struct trace_event_file *file)
The code is cleaner if we return the allocated trigger instead of doing all the logic of the passed in pointer.
Quoted 6 lines
kernel/trace/trace_kprobe.c +0
-5{ -int ret =0 ; - - *trigger =kzalloc (sizeof (struct event_trigger_data ),GFP_KERNEL ); -if (!(*trigger)) { - ret = -ENOMEM ;
kernel/trace/trace_kprobe.c static struct event_command event_trigger_cmd = {
Quoted 10 lines
kernel/trace/trace_kprobe.c +9
-1- goto error; +struct event_trigger_data *trigger; +struct eprobe_data *edata; + + edata =kzalloc (sizeof (*edata),GFP_KERNEL ); + trigger =kzalloc (sizeof (*trigger),GFP_KERNEL ); +if (!trigger || !edata) { +kfree (edata); +kfree (trigger); + return ERR_PTR(-ENOMEM)];
This is one of the "magic" things that the kernel does. The ERR_PTR() set the -ENOMEM int a value that looks like a pointer but is at a location where the pointer is not valid. And the return code can detect that the pointer sent back is not a valid pointer but an error occurred.
Quoted 23 lines
kernel/trace/trace_kprobe.c +10
-8} - (*trigger)->count =-1 ; - (*trigger)->ops = &eprobe_trigger_ops; - (*trigger)->cmd_ops = &event_trigger_cmd; + trigger->flags =EVENT_TRIGGER_FL_PROBE ; + trigger->count =-1 ; + trigger->ops = &eprobe_trigger_ops; + trigger->cmd_ops = &event_trigger_cmd; -INIT_LIST_HEAD (&(*trigger)->list); -RCU_INIT_POINTER ((*trigger)->filter,NULL ); +INIT_LIST_HEAD (&trigger->list); +RCU_INIT_POINTER (trigger->filter,NULL ); -return ret; + edata->ep = ep; + edata->file = file; + trigger->private_data = edata; -error: -return ret; +return trigger;
kernel/trace/trace_kprobe.c static struct event_command event_trigger_cmd = { · line 594
Quoted 10 lines
kernel/trace/trace_kprobe.c +3
-3} -static int enable_eprobe (struct keventprobe *ep,struct trace_array *tr) +static int enable_eprobe (struct keventprobe *ep, +struct trace_event_file *eprobe_file) { -struct trace_event_file *target_event;struct event_trigger_data *trigger; -int ret; +struct trace_event_file *file;
I used "file" for trace_event_file as that's a common variable that is used for it. I was confused by what "target_event" was, as that's a new name.
kernel/trace/trace_kprobe.c static struct event_command event_trigger_cmd = { · line 601
Quoted 12 lines
kernel/trace/trace_kprobe.c +5
-5+struct trace_array *tr = eprobe_file->tr; - target_event =find_event_file (tr, ep->event_system, ep->event_name); -if (!target_event) + file =find_event_file (tr, ep->event_system, ep->event_name); +if (!file)return -ENOENT ; - ret =new_eprobe_trigger (&trigger); -if (!ret) -return ret; + trigger =new_eprobe_trigger (ep, eprobe_file); +if (IS_ERR (trigger))
IS_ERR() is the macro that tests if a pointer is actually an error that was created by ERR_PTR().
kernel/trace/trace_kprobe.c static struct event_command event_trigger_cmd = { · line 602
Quoted 1 line
kernel/trace/trace_kprobe.c +1
-0+return PTR_ERR (trigger);
And PTR_ERR() turns that pointer back to the error code. In this case -ENOMEM.
Quoted 5 lines
kernel/trace/trace_kprobe.c +1
-2-list_add_tail_rcu (&trigger->list, &target_event->triggers); +list_add_tail_rcu (&trigger->list, &file->triggers); -trace_event_trigger_enable_disable (target_event,1 );
kernel/trace/trace_kprobe.c static struct event_command event_trigger_cmd = { · line 614
Quoted 10 lines
kernel/trace/trace_kprobe.c +4
-1+trace_event_trigger_enable_disable (file,1 ); +update_cond_flag (file);return 0 ; } -static int disable_eprobe (struct keventprobe *ep) +static int disable_eprobe (struct keventprobe *ep, +struct trace_array *tr) {
This was similar to what you did with enable_probe. I just passed more around to be able to do the disabling.
Quoted 32 lines
kernel/trace/trace_kprobe.c +23
-1+struct event_trigger_data *trigger; +struct trace_event_file *file; +struct eprobe_data *edata; + + file =find_event_file (tr, ep->event_system, ep->event_name); +if (!file) +return -ENOENT ; + +list_for_each_entry (trigger, &file->triggers, list) { +if (!(trigger->flags &EVENT_TRIGGER_FL_PROBE )) +continue ; + edata = trigger->private_data; +if (edata->ep == ep) +break ; + } +if (list_entry_is_head (trigger, &file->triggers, list)) +return -ENODEV ; + +list_del_rcu (&trigger->list); + +trace_event_trigger_enable_disable (file,0 ); +update_cond_flag (file);return 0 ; }static inline int __enable_trace_kprobe (struct trace_kprobe *tk, -struct trace_array *tr) +struct trace_event_file *file) {int ret =0 ;
kernel/trace/trace_kprobe.c static inline int __enable_trace_kprobe(struct trace_kprobe *tk, · line 649
Quoted 5 lines
kernel/trace/trace_kprobe.c +1
-1@@ -494,7 +646,7 @@ static inline int __enable_trace_kprobe(struct trace_kprobe *tk,if (trace_kprobe_is_return (tk)) ret =enable_kretprobe (&tk->rp);else if (trace_kprobe_is_event (tk)) - ret =enable_eprobe (tk->ep, tr); + ret =enable_eprobe (tk->ep, file);
If we want the data descriptor to hold the tk, then we need to pass tk instead of ep here.
Quoted 50 lines
kernel/trace/trace_kprobe.c +6
-5else ret =enable_kprobe (&tk->rp.kp); }@@ -502,7 +654,8 @@ static inline int __enable_trace_kprobe(struct trace_kprobe *tk,return ret; } -static void __disable_trace_kprobe (struct trace_probe *tp) +static void __disable_trace_kprobe (struct trace_probe *tp, +struct trace_event_file *file) {struct trace_probe *pos;struct trace_kprobe *tk;@@ -514,7 +667,7 @@ static void __disable_trace_kprobe(struct trace_probe *tp)if (trace_kprobe_is_return (tk))disable_kretprobe (&tk->rp);else if (trace_kprobe_is_event (tk)) -disable_eprobe (tk->ep); +disable_eprobe (tk->ep, file->tr);else disable_kprobe (&tk->rp.kp); }@@ -552,7 +705,7 @@ static int enable_trace_kprobe(struct trace_event_call *call, tk = container_of(pos, struct trace_kprobe, tp);if (trace_kprobe_has_gone (tk))continue ; - ret =__enable_trace_kprobe (tk, file->tr); + ret =__enable_trace_kprobe (tk, file);if (ret)break ; enabled = true;@@ -561,7 +714,7 @@ static int enable_trace_kprobe(struct trace_event_call *call,if (ret) {/* Failed to enable one of them. Roll back all */ if (enabled) -__disable_trace_kprobe (tp); +__disable_trace_kprobe (tp, file);if (file)trace_probe_remove_file (tp, file);else @@ -594,7 +747,7 @@ static int disable_trace_kprobe(struct trace_event_call *call,trace_probe_clear_flag (tp,TP_FLAG_PROFILE );if (!trace_probe_is_enabled (tp)) -__disable_trace_kprobe (tp); +__disable_trace_kprobe (tp, file); out:if (file)
kernel/trace/trace_kprobe.c static int trace_eprobe_tp_find(struct trace_kprobe *tk) · line 1050
Quoted 5 lines
kernel/trace/trace_kprobe.c +1
-1@@ -894,7 +1047,7 @@ static int trace_eprobe_tp_find(struct trace_kprobe *tk)mutex_lock (&event_mutex);list_for_each_entry (tp_event, &ftrace_events, list) { -if (!(tp_event->flags &TRACE_EVENT_FL_TRACEPOINT )) +if (tp_event->flags &TRACE_EVENT_FL_IGNORE_ENABLE )
If we want to connect to synthetic events or kprobes, we need to check all, but we should probably not attach an event probe to another event probe. That could cause issues (infinite recursion?). I did not add a check for that.
Quoted 39 lines
kernel/trace/trace_kprobe.c +4
-3continue ;if (!tp_event->class->system ||strcmp (tk->ep->event_system, tp_event->class->system))@@ -907,7 +1060,7 @@ static int trace_eprobe_tp_find(struct trace_kprobe *tk) ret = -ENODEV ;break ; } - tk->ep->tp = tp_event; + tk->ep->event = tp_event; ret =0 ;break ; }@@ -922,7 +1075,7 @@ static int trace_eprobe_tp_arg_find(struct trace_kprobe *tk, int i)struct ftrace_event_field *field;struct list_head *head; - head =trace_get_fields (tk->ep->tp); + head =trace_get_fields (tk->ep->event);list_for_each_entry (field, head, link) {if (!strcmp (parg->code->data, field->name)) {kfree (parg->code->data);@@ -941,6 +1094,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])unsigned int flags =TPARG_FL_KERNEL |TPARG_FL_TPOINT ;const char *sys_event =NULL , *sys_name =NULL ;struct trace_kprobe *tk =NULL ; +struct trace_event_call *call;char buf1[MAX_EVENT_NAME_LEN ];char buf2[MAX_EVENT_NAME_LEN ];char *tmp =NULL ;@@ -950,7 +1104,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])if (argc <2 )return -ECANCELED ; -trace_probe_log_init ("trace_kprobe" , argc, argv); +trace_probe_log_init ("event_probe" , argc, argv); event =strchr (&argv[0 ][1 ],':' );if (event) {
kernel/trace/trace_kprobe.c static int __trace_eprobe_create(int argc, const char *argv[]) · line 1165
Quoted 5 lines
kernel/trace/trace_kprobe.c +2
-0@@ -1007,19 +1161,25 @@ static int __trace_eprobe_create(int argc, const char *argv[]) ret =traceprobe_set_print_fmt (&tk->tp, false);if (ret <0 ) goto error; + +mutex_lock (&event_mutex);
Make sure you enable "PROVE_LOCKING" in your .config file. As lockdep blew up on me when I ran your original patch, because register_kprobe_event() and dyn_event_add() require that the event_mutex is held.
Quoted 29 lines
kernel/trace/trace_kprobe.c +8
-4ret =register_kprobe_event (tk);if (ret) - goto error; + goto out_unlock; - ret =dyn_event_add (&tk->devent); -if (ret) - goto error; +/* Reassign to a EPROBE */ + call =trace_probe_event_call (&tk->tp); + call->flags =TRACE_EVENT_FL_EPROBE ; + ret =dyn_event_add (&tk->devent); +out_unlock: +mutex_unlock (&event_mutex);return ret; parse_error: ret = -EINVAL ; error: +free_trace_kprobe (tk);return ret; }kernel/trace/trace_probe.c +0
-0
kernel/trace/trace_probe.c void trace_event_probe_cleanup(struct keventprobe *ep) · line 1004
Quoted 10 lines
kernel/trace/trace_probe.c +2
-2@@ -997,8 +997,8 @@ void trace_event_probe_cleanup(struct keventprobe *ep) {kfree (ep->event_name);kfree (ep->event_system); -if (ep->tp) -module_put (ep->tp->mod); +if (ep->event) +module_put (ep->event->mod);kfree (ep); }
As we discussed, we need to hide event probe triggers from the list in the trigger files. I'll let you look at that. It currently uses "list_empty(file->triggers)" to decide to print the instructions when there are no triggers. But instead, we may need to add a counter for the normal triggers (ignoring the event probe triggers) and perhaps even open code the seq_list, so that it will skip these as well.
Also, we must make it where writing into the event's trigger files does not touch these triggers (as we saw that will crash the system!).
Looking forward to the RFC patch :-)
-- Steve
Steven Rostedt
maintainerRe: [PATCH 2/2] tracing: Update to the event probes
Quoted 2 lines · On Fri, 16 Jul 2021 00:04:06 -0400 Steven Rostedt <[email protected]> wrote:
Also, we must make it where writing into the event's trigger files does not touch these triggers (as we saw that will crash the system!).
Tzvetomir,
I know I was able to crash the kernel before, but I do not remember how I did it, and trying various things, I have not been able to crash it with these patches.
I may continue to try a little more, otherwise, I'll just stop and go ahead and review your latest patch.
-- Steve