Re: [PATCH v7 08/10] tracing: Add a probe that attaches to trace events

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Steve,

Thanks for updating.

On Thu, 19 Aug 2021 00:13:29 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> +static bool find_event_probe(const char *group, const char *event)
> +{
> +	struct trace_eprobe *ep;
> +	struct dyn_event *ev;
> +	bool ret = false;
> +
> +	/*
> +	 * Must grab the event_mutex to prevent the list from being modified
> +	 * by other probes. But the event_probe being only created via the
> +	 * dynamic_events file, is only added under the dyn_event_ops_mutex,
> +	 * which is currently held. There is no race between this check and
> +	 * adding the new probe.

This is not correct, as I said in the previous mail. The dynamic event has
2 lists, one is for the "kind of" dynamic event (dyn_event_ops), and
the other one is for the dynamic events itself. The "dyn_event_ops_mutex"
is protecting only "dyn_event_ops", and the dynamic event list is ptotected
by the "event_mutex". (This is described in the trace_dynevent.c)
So holding event_mutex is correct.

> +	 */
> +	mutex_lock(&event_mutex);
> +	for_each_dyn_event(ev) {
> +		if (ev->ops != &eprobe_dyn_event_ops)
> +			continue;
> +		ep = to_trace_eprobe(ev);
> +		if (strcmp(ep->tp.event->class.system, group) == 0 &&
> +		    strcmp(ep->tp.event->call.name, event) == 0) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +	mutex_lock(&event_mutex);
> +
> +	return ret;
> +}
> +
> +static int __trace_eprobe_create(int argc, const char *argv[])
> +{
> +	/*
> +	 * Argument syntax:
> +	 *      e[:[GRP/]ENAME] SYSTEM.EVENT [FETCHARGS]
> +	 * Fetch args:
> +	 *  <name>=$<field>[:TYPE]
> +	 */
> +	const char *event = NULL, *group = EPROBE_EVENT_SYSTEM;
> +	const char *sys_event = NULL, *sys_name = NULL;
> +	struct trace_event_call *event_call;
> +	struct trace_eprobe *ep = NULL;
> +	char buf1[MAX_EVENT_NAME_LEN];
> +	char buf2[MAX_EVENT_NAME_LEN];
> +	int ret = 0;
> +	int i;
> +
> +	if (argc < 2 || argv[0][0] != 'e')
> +		return -ECANCELED;
> +
> +	trace_probe_log_init("event_probe", argc, argv);
> +
> +	event = strchr(&argv[0][1], ':');
> +	if (event) {
> +		event++;
> +		ret = traceprobe_parse_event_name(&event, &group, buf1,
> +						  event - argv[0]);
> +		if (ret)
> +			goto parse_error;
> +	} else {
> +		strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN);
> +		sanitize_event_name(buf1);
> +		event = buf1;
> +	}
> +	if (!is_good_name(event) || !is_good_name(group))
> +		goto parse_error;
> +
> +	/* Check if the name already exists */
> +	if (find_event_probe(group, event))
> +		return -EEXIST;

Hmm, there is a window between checking the name confliction here, ...

> +
> +	sys_event = argv[1];
> +	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
> +					  sys_event - argv[1]);
> +	if (ret || !sys_name)
> +		goto parse_error;
> +	if (!is_good_name(sys_event) || !is_good_name(sys_name))
> +		goto parse_error;
> +
> +	mutex_lock(&event_mutex);
> +	event_call = find_and_get_event(sys_name, sys_event);
> +	ep = alloc_event_probe(group, event, event_call, argc - 2);
> +	mutex_unlock(&event_mutex);
> +
> +	if (IS_ERR(ep)) {
> +		ret = PTR_ERR(ep);
> +		/* This must return -ENOMEM, else there is a bug */
> +		WARN_ON_ONCE(ret != -ENOMEM);
> +		goto error;	/* We know ep is not allocated */
> +	}
> +
> +	argc -= 2; argv += 2;
> +	/* parse arguments */
> +	for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
> +		trace_probe_log_set_index(i + 2);
> +		ret = trace_eprobe_tp_update_arg(ep, argv, i);
> +		if (ret)
> +			goto error;
> +	}
> +	ret = traceprobe_set_print_fmt(&ep->tp, PROBE_PRINT_EVENT);
> +	if (ret < 0)
> +		goto error;
> +	init_trace_eprobe_call(ep);
> +	mutex_lock(&event_mutex);
> +	ret = trace_probe_register_event_call(&ep->tp);
> +	if (ret) {
> +		mutex_unlock(&event_mutex);
> +		goto error;
> +	}

... and register it here.

Between the existance check and the registration, someone can register
same name event probe. So I recommend you to do it as;

static int register_event_probe(ep)
{
	init_trace_eprobe_call(ep);
	mutex_lock(&event_mutex);
	if (find_event_probe(group, event))
		ret = -EEXIST;
		goto out;
	}

	ret = trace_probe_register_event_call(&ep->tp);
	if (ret)
		goto out;
	ret = dyn_event_add(&ep->devent, &ep->tp.event->call);
	mutex_unlock(&event_mutex);
out:
	return ret;
}

Anyway, I will send a patch for fixing related issue. If you don't care
the name collision between eprobes or other events, you can just apply it.
Then trace_probe_register_event_call() will reject the same name event.


Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>