On Sun, 28 Nov 2021 23:34:13 +0100
Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> > > + if (!tk_old) {
> > > + ret = -EINVAL;
> > > + goto error;
> > > + }
> > > +
> > > + /* Append to existing event */
> > > + ret = trace_probe_append(&tk->tp, &tk_old->tp);
> > > + if (ret)
> > > + goto error;
> > > +
> > > + /* Register k*probe */
> > > + ret = __register_trace_kprobe(tk);
> > > + if (ret)
> > > + goto error;
> >
> > If "appended" probe failed to register, it must be "unlinked" from
> > the first one and goto error to free the trace_kprobe.
> >
> > if (ret) {
> > trace_probe_unlink(&tk->tp);
> > goto error;
> > }
> >
> > See append_trace_kprobe() for details.
>
> so there's goto error jumping to:
>
> error:
> free_trace_kprobe(tk);
>
> that calls:
> trace_probe_cleanup
> -> trace_probe_unlink
>
> that should do it, right?
Ah, OK. Clean up all the kprobe events in this function. Then it's good.
>
> >
> > > +
> > > + return trace_probe_event_call(&tk->tp);
> > > + }
> > > +
> > > init_trace_event_call(tk);
> > >
> > > ptype = trace_kprobe_is_return(tk) ?
> > > @@ -1841,6 +1868,8 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
> > >
> > > void destroy_local_trace_kprobe(struct trace_event_call *event_call)
> > > {
> > > + struct trace_probe_event *event;
> > > + struct trace_probe *pos, *tmp;
> > > struct trace_kprobe *tk;
> > >
> > > tk = trace_kprobe_primary_from_call(event_call);
> > > @@ -1852,9 +1881,15 @@ void destroy_local_trace_kprobe(struct trace_event_call *event_call)
> > > return;
> > > }
> > >
> > > - __unregister_trace_kprobe(tk);
> > > + event = tk->tp.event;
> > > + list_for_each_entry_safe(pos, tmp, &event->probes, list) {
> > > + list_for_each_entry_safe(pos, tmp, &event->probes, list) {
> > > + list_del_init(&pos->list);
> > > + __unregister_trace_kprobe(tk);
> > > + __free_trace_kprobe(tk);
> > > + }
> > >
> > > - free_trace_kprobe(tk);
> > > + trace_probe_event_free(event);
> >
> > Actually, each probe already allocated the trace_probe events (which are not
> > used if it is appended). Thus you have to use trace_probe_unlink(&tk->tp) in
> > the above loop.
> >
> > list_for_each_entry_safe(pos, tmp, &event->probes, list) {
> > list_for_each_entry_safe(pos, tmp, &event->probes, list) {
> > __unregister_trace_kprobe(tk);
> > trace_probe_unlink(&tk->tp); /* This will call trace_probe_event_free() internally */
> > free_trace_kprobe(tk);
> > }
>
> so calling trace_probe_event_free inside this loop is a problem,
> because the loop iterates that trace_probe_event's probes list,
> and last probe removed will trigger trace_probe_event_free, that
> will free the list we iterate.. and we go down ;-)
Oops, right. So in this case, you are looping on the all probes
on an event, so event is referred outside of loop.
OK, I got it.
In the ftrace kprobe-event, this loop cursor is done by dynevent,
so this problem doesn't occur. But the BPF is only using the
trace_event, thus this special routine is needed.
Could you add such comment on your loop?
Thank you,
>
> so that's why I added new free function '__free_trace_kprobe'
> that frees everything as free_trace_kprobe, but does not call
> trace_probe_unlink
>
> event = tk->tp.event;
> list_for_each_entry_safe(pos, tmp, &event->probes, list) {
> list_for_each_entry_safe(pos, tmp, &event->probes, list) {
> list_del_init(&pos->list);
> __unregister_trace_kprobe(tk);
> __free_trace_kprobe(tk);
> }
>
> trace_probe_event_free(event);
>
> and there's trace_probe_event_free(event) to make the final free
>
> thanks,
> jirka
>
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
