Re: [PATCH 4/4] tracing/eprobes: Have event probes be consistent with kprobes and uprobes

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 19 Aug 2022 21:40:39 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Currently, if a symbol "@" is attempted to be used with an event probe
> (eprobes), it will cause a NULL pointer dereference crash.
> 
> Both kprobes and uprobes can reference data other than the main registers.
> Such as immediate address, symbols and the current task name. Have eprobes
> do the same thing.
> 
> For "comm", if "comm" is used and the event being attached to does not
> have the "comm" field, then make it the "$comm" that kprobes has. This is
> consistent to the way histograms and filters work.

Hmm, I think I would better allow user to use $COMM to get comm string
for kprobe/uprobe event users too. (There are many special variables...)
Anyway, this looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you!

> 
> Cc: stable@vger.kernel.org
> Fixes: 7491e2c44278 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_eprobe.c | 67 +++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index a1d3423ab74f..63218a541217 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -227,6 +227,7 @@ static int trace_eprobe_tp_arg_update(struct trace_eprobe *ep, int i)
>  	struct probe_arg *parg = &ep->tp.args[i];
>  	struct ftrace_event_field *field;
>  	struct list_head *head;
> +	int ret = -ENOENT;
>  
>  	head = trace_get_fields(ep->event);
>  	list_for_each_entry(field, head, link) {
> @@ -236,9 +237,17 @@ static int trace_eprobe_tp_arg_update(struct trace_eprobe *ep, int i)
>  			return 0;
>  		}
>  	}
> +
> +	/* Argument no found on event. But allow for comm and COMM to be used */
> +	if (strcmp(parg->code->data, "COMM") == 0 ||
> +	    strcmp(parg->code->data, "comm") == 0) {
> +		parg->code->op = FETCH_OP_COMM;
> +		ret = 0;
> +	}
> +
>  	kfree(parg->code->data);
>  	parg->code->data = NULL;
> -	return -ENOENT;
> +	return ret;
>  }
>  
>  static int eprobe_event_define_fields(struct trace_event_call *event_call)
> @@ -363,16 +372,38 @@ static unsigned long get_event_field(struct fetch_insn *code, void *rec)
>  
>  static int get_eprobe_size(struct trace_probe *tp, void *rec)
>  {
> +	struct fetch_insn *code;
>  	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)) {
> +		if (arg->dynamic) {
>  			unsigned long val;
>  
> -			val = get_event_field(arg->code, rec);
> -			len = process_fetch_insn_bottom(arg->code + 1, val, NULL, NULL);
> +			code = arg->code;
> + retry:
> +			switch (code->op) {
> +			case FETCH_OP_TP_ARG:
> +				val = get_event_field(code, rec);
> +				break;
> +			case FETCH_OP_IMM:
> +				val = code->immediate;
> +				break;
> +			case FETCH_OP_COMM:
> +				val = (unsigned long)current->comm;
> +				break;
> +			case FETCH_OP_DATA:
> +				val = (unsigned long)code->data;
> +				break;
> +			case FETCH_NOP_SYMBOL:	/* Ignore a place holder */
> +				code++;
> +				goto retry;
> +			default:
> +				continue;
> +			}
> +			code++;
> +			len = process_fetch_insn_bottom(code, val, NULL, NULL);
>  			if (len > 0)
>  				ret += len;
>  		}
> @@ -390,8 +421,28 @@ process_fetch_insn(struct fetch_insn *code, void *rec, void *dest,
>  {
>  	unsigned long val;
>  
> -	val = get_event_field(code, rec);
> -	return process_fetch_insn_bottom(code + 1, val, dest, base);
> + retry:
> +	switch (code->op) {
> +	case FETCH_OP_TP_ARG:
> +		val = get_event_field(code, rec);
> +		break;
> +	case FETCH_OP_IMM:
> +		val = code->immediate;
> +		break;
> +	case FETCH_OP_COMM:
> +		val = (unsigned long)current->comm;
> +		break;
> +	case FETCH_OP_DATA:
> +		val = (unsigned long)code->data;
> +		break;
> +	case FETCH_NOP_SYMBOL:	/* Ignore a place holder */
> +		code++;
> +		goto retry;
> +	default:
> +		return -EILSEQ;
> +	}
> +	code++;
> +	return process_fetch_insn_bottom(code, val, dest, base);
>  }
>  NOKPROBE_SYMBOL(process_fetch_insn)
>  
> @@ -866,6 +917,10 @@ static int trace_eprobe_tp_update_arg(struct trace_eprobe *ep, const char *argv[
>  			trace_probe_log_err(0, BAD_ATTACH_ARG);
>  	}
>  
> +	/* Handle symbols "@" */
> +	if (!ret)
> +		ret = traceprobe_update_arg(&ep->tp.args[i]);
> +
>  	return ret;
>  }
>  
> -- 
> 2.35.1


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>