Messages in this thread | | | From | Cong Wang <> | Date | Mon, 18 Nov 2019 20:59:34 -0800 | Subject | Re: [PATCH v3] tracing: Introduce trace event injection |
| |
On Tue, Nov 12, 2019 at 8:31 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > Again, sorry for the late review. Things have just been crazy :-/
I know, the only problem is even I myself almost forget how the code works, I have to spend some time to warm up. :-(
> > +config TRACE_EVENT_INJECT > > + bool "Trace event injection" > > + depends on TRACING > > + default n > > No need for the above "default n", that's the default.
Yeah, I know you want to save one line, let's do it!
> > +static int > > +parse_field(char *str, struct trace_event_call *call, > > + struct ftrace_event_field **pf, u64 *pv) > > +{ > > + struct ftrace_event_field *field; > > + char *field_name; > > + int s, i = 0; > > + char *p; > > + int len; > > + u64 val; > > + > > + if (!str[i]) > > + return 0; > > + /* First find the field to associate to */ > > + while (isspace(str[i])) > > + i++; > > + s = i; > > + while (isalnum(str[i]) || str[i] == '_') > > + i++; > > + len = i - s; > > + if (!len) > > + return -EINVAL; > > + > > + field_name = kmemdup_nul(str + s, len, GFP_KERNEL); > > + if (!field_name) > > + return -ENOMEM; > > + field = trace_find_event_field(call, field_name); > > + kfree(field_name); > > + if (!field) > > + return -ENOENT; > > + > > + *pf = field; > > + p = strchr(str + i, '='); > > Hmm, the above here will parse out: > > "field some garbarge = one" > > It will skip the some garbage, and jump straight to 1.
Good catch! Let's write a test case for it and fix it accordingly.
> > > + if (!p) > > + return -EINVAL; > > + i = p + 1 - str; > > + while (isspace(str[i])) > > + i++; > > + s = i; > > + if (isdigit(str[i]) || str[i] == '-') { > > + char *num, c; > > + int ret; > > + > > + /* Make sure the field is not a string */ > > + if (is_string_field(field)) > > + return -EINVAL; > > + > > + if (str[i] == '-') > > + i++; > > + > > + /* We allow 0xDEADBEEF */ > > + while (isalnum(str[i])) > > + i++; > > + num = str + s; > > + c = str[i]; > > Probably should verify that c is whitespace or '\0'.
Hmm, I think we should be more strict on errors. Let's me write a test case for this too.
> > > + str[i] = '\0'; > > + /* Make sure it is a value */ > > + if (field->is_signed) > > + ret = kstrtoll(num, 0, &val); > > + else > > + ret = kstrtoull(num, 0, &val); > > + str[i] = c; > > + if (ret) > > + return ret; > > + > > + *pv = val; > > + return i; > > + } else if (str[i] == '\'' || str[i] == '"') { > > + char q = str[i]; > > + > > + /* Make sure the field is OK for strings */ > > + if (!is_string_field(field)) > > + return -EINVAL; > > + > > + for (i++; str[i]; i++) { > > + if (str[i] == '\\' && str[i + 1]) { > > + i++; > > + continue; > > + } > > + if (str[i] == q) > > + break; > > + } > > + if (!str[i]) > > + return -EINVAL; > > + > > + /* Skip quotes */ > > + s++; > > + len = i - s; > > + if (len >= MAX_FILTER_STR_VAL) > > + return -EINVAL; > > + > > + *pv = (unsigned long)(str + s); > > + str[i] = 0; > > + /* go past the last quote */ > > + i++; > > + return i; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int trace_get_entry_size(struct trace_event_call *call) > > +{ > > + struct ftrace_event_field *field; > > + struct list_head *head; > > + int size = 0; > > + > > + head = trace_get_fields(call); > > + list_for_each_entry(field, head, link) { > > + if (field->size + field->offset > size) > > + size = field->size + field->offset; > > + } > > + > > + return size; > > +} > > + > > +static void *trace_alloc_entry(struct trace_event_call *call, int *size) > > +{ > > + int entry_size = trace_get_entry_size(call); > > + struct ftrace_event_field *field; > > + struct list_head *head; > > + void *entry = NULL; > > + > > + /* We need an extra '\0' at the end. */ > > + entry = kzalloc(entry_size + 1, GFP_KERNEL); > > + if (!entry) > > + return NULL; > > + > > + head = trace_get_fields(call); > > + list_for_each_entry(field, head, link) { > > + if (!is_string_field(field)) > > + continue; > > + if (field->filter_type == FILTER_STATIC_STRING) > > + continue; > > + if (field->filter_type == FILTER_DYN_STRING) { > > + u32 *str_item; > > + int str_len = 0; > > + int str_loc = entry_size & 0xffff; > > + > > + str_item = (u32 *)(entry + field->offset); > > + *str_item = (str_len << 16) | str_loc; > > str_len is always zero here, right? Might as well just make this > > *str_item = str_loc; >
Hmm, I don't even remember whether I forgot to set str_len to some value or simply we can just remove it...
Sorry for it, you know this code was written more than 6 months ago...
> > > + } else { > > + char **paddr; > > + > > + paddr = (char **)(entry + field->offset); > > + *paddr = ""; > > Hmm, I know this is a default pointer to a sting, but perhaps we should > make it point to NULL, and not a string defined by this function?
Again, I don't remember why but I guess it might be intentionally reserved to empty string.
> > > + } > > + } > > + > > + *size = entry_size + 1; > > + return entry; > > +} > > + > > +#define INJECT_STRING "STATIC STRING CAN NOT BE INJECTED" > > + > > +/* Caller is responsible to free the *pentry. */ > > +static int parse_entry(char *str, struct trace_event_call *call, void **pentry) > > +{ > > + struct ftrace_event_field *field; > > + unsigned long irq_flags; > > + void *entry = NULL; > > + int entry_size; > > + u64 val; > > + int len; > > + > > + entry = trace_alloc_entry(call, &entry_size); > > + *pentry = entry; > > + if (!entry) > > + return -ENOMEM; > > + > > + local_save_flags(irq_flags); > > + tracing_generic_entry_update(entry, call->event.type, irq_flags, > > + preempt_count()); > > + > > + while ((len = parse_field(str, call, &field, &val)) > 0) { > > + if (is_function_field(field)) > > Why not allow injecting function fields?
I don't quite remember, but I think it is probably due to I don't have such a use case for testing rasdaemon. Do you have any use case on your mind?
> > > + return -EINVAL; > > + > > + if (is_string_field(field)) { > > + char *addr = (char *)(unsigned long) val; > > + > > + if (field->filter_type == FILTER_STATIC_STRING) { > > + strlcpy(entry + field->offset, addr, field->size); > > + } else if (field->filter_type == FILTER_DYN_STRING) { > > + int str_len = strlen(addr) + 1; > > + int str_loc = entry_size & 0xffff; > > + u32 *str_item; > > + > > + entry_size += str_len; > > + *pentry = krealloc(entry, entry_size, GFP_KERNEL); > > + entry = *pentry; > > + if (!entry) > > + return -ENOMEM; > > The above leaks memory, as if *pentry is NULL then the old entry is not > freed. The above needs to be: > > *pentry = krealloc(entry, entry_size, GFP_KERNEL); > if (!*pentry) { > kfree(entry); > return -ENOMEM; > } > entry = *pentry;
Good catch!
> > > + > > + strlcpy(entry + (entry_size - str_len), addr, str_len); > > + str_item = (u32 *)(entry + field->offset); > > + *str_item = (str_len << 16) | str_loc; > > + } else { > > + char **paddr; > > + > > + /* TODO: can we find the constant string? */ > > probably not ;-)
Ok, I thought you want to keep it, then let's remove it.
Thanks!
| |