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]
Message-ID: <ZOJ2W4O75BYP1uML@krava>
Date:   Sun, 20 Aug 2023 22:23:55 +0200
From:   Jiri Olsa <olsajiri@...il.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Francis Laniel <flaniel@...ux.microsoft.com>,
        linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        linux-trace-kernel@...r.kernel.org,
        Song Liu <songliubraving@...com>, bpf@...r.kernel.org
Subject: Re: [RFC PATCH v1 1/1] tracing/kprobe: Add multi-probe support for
 'perf_kprobe' PMU

On Sat, Aug 19, 2023 at 10:11:05AM +0900, Masami Hiramatsu wrote:
> Hi Francis,
> (Cc: Song Liu and BPF ML)
> 
> On Fri, 18 Aug 2023 20:12:11 +0200
> Francis Laniel <flaniel@...ux.microsoft.com> wrote:
> 
> > Hi.
> > 
> > Le vendredi 18 août 2023, 15:05:37 CEST Masami Hiramatsu a écrit :
> > > On Thu, 17 Aug 2023 13:06:20 +0200
> > > 
> > > Francis Laniel <flaniel@...ux.microsoft.com> wrote:
> > > > Hi.
> > > > 
> > > > Le jeudi 17 août 2023, 09:50:57 CEST Masami Hiramatsu a écrit :
> > > > > Hi,
> > > > > 
> > > > > On Wed, 16 Aug 2023 18:35:17 +0200
> > > > > 
> > > > > Francis Laniel <flaniel@...ux.microsoft.com> wrote:
> > > > > > When using sysfs, it is possible to create kprobe for several kernel
> > > > > > functions sharing the same name, but of course with different
> > > > > > addresses,
> > > > > > by writing their addresses in kprobe_events file.
> > > > > > 
> > > > > > When using PMU, if only the symbol name is given, the event will be
> > > > > > created for the first address which matches the symbol, as returned by
> > > > > > kallsyms_lookup_name().
> > > > > 
> > > > > Do you mean probing the same name symbols? Yes, it is intended behavior,
> > > > > since it is not always true that the same name function has the same
> > > > > prototype (it is mostly true but is not ensured), it is better to leave
> > > > > user to decide which one is what you want to probe.
> > > > 
> > > > This is what I meant.
> > > > I also share your mind regarding leaving the users deciding which one they
> > > > want to probe but in my case (which I agree is a bit a corner one) it
> > > > leaded me to misunderstanding as the PMU kprobe was only added to the
> > > > first ntfs_file_write_iter() which is not the one for ntfs3.
> > > 
> > > Hmm, OK. I think in that case (multiple same-name symbols exist) the default
> > > behavior is rejecting with error message. And optionally, it will probe all
> > > or them like your patch.
> > 
> > I am not sure to understand.
> > Can you please precise the default behavior of which software component?
> 
> I meant that the behavior of the kprobe-events via /sys/kernel/tracing.
> But your patch is for the other interface for perf as kprobe-event PMU.
> In that case, I think we should CC to other users like BPF because
> this may change the expected behavior.

it does not break bpf tests, but of course we don't have such use case, but I
think should make this optional not to potentionaly break existing users,
because you get more probes than you currently ask for

would be great to have some kind of tests for this as well

SNIP

> > > > > > +		/*
> > > > > > +		 * alloc_trace_kprobe() first considers symbol name, so we set
> > > > > > +		 * this to NULL to allocate this kprobe on the given address.
> > > > > > +		 */
> > > > > > +		tk_same_name = alloc_trace_kprobe(KPROBE_EVENT_SYSTEM, event,
> > > > > > +						  (void *)address, NULL, offs,
> > > > > > +						  0 /* maxactive */,
> > > > > > +						  0 /* nargs */, is_return);
> > > > > > +
> > > > > > +		if (IS_ERR(tk_same_name)) {
> > > > > > +			ret = -ENOMEM;
> > > > > > +			goto error_free;
> > > > > > +		}
> > > > > > +
> > > > > > +		init_trace_event_call(tk_same_name);
> > > > > > +
> > > > > > +		if (traceprobe_set_print_fmt(&tk_same_name->tp, ptype) < 0) {
> > > > > > +			ret = -ENOMEM;
> > > > > > +			goto error_free;
> > > > > > +		}
> > > > > > +
> > > > > > +		ret = append_trace_kprobe(tk_same_name, tk);
> > > > > > +		if (ret)
> > > > > > +			goto error_free;

this seems tricky if offs is specified, because IIUC that will most
likely fail in the __register_trace_kprobe/register_kprobe call inside
the append_trace_kprobe ... should we allow this just for offs == 0 ?

jirka

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ