lkml.org 
[lkml]   [2021]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 08/10] tracing: Add a probe that attaches to trace events
    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>

    \
     
     \ /
      Last update: 2021-08-19 12:24    [W:5.134 / U:0.316 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site