netdev - Re: [PATCH 1/8] perf/kprobe: Add support to create multiple probes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Nov 2021 10:43:09 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Steven Rostedt <rostedt@...dmis.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Martin KaFai Lau <kafai@...com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [PATCH 1/8] perf/kprobe: Add support to create multiple probes

On Sun, 28 Nov 2021 23:34:13 +0100
Jiri Olsa <jolsa@...hat.com> 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@...nel.org>