Re: [PATCH v5 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation
On Thu, 19 Oct 2023 09:51:04 -0400
Steven Rostedt wrote:
> On Thu, 19 Oct 2023 21:18:43 +0900
> Masami Hiramatsu (Google) wrote:
>
> > > So why is this adding stable? (and as Greg's form letter states, that's
> > > not
> > > how you do that)
> > >
> > > I don't see this as a fix but a new feature.
> >
> > I asked him to make this a fix since the current kprobe event' behavior is
> > somewhat strange. It puts the probe on only the "first symbol" if user
> > specifies a symbol name which has multiple instances. In this case, the
> > actual probe address can not be solved by name. User must specify the
> > probe address by unique name + offset. Unless, it can put a probe on
> > unexpected address, especially if it specifies non-unique symbol + offset,
> > the address may NOT be the instruction boundary.
> > To avoid this issue, it should check the given symbol is unique.
> >
>
> OK, so what is broken is that when you add a probe to a function that has
> multiple names, it will attach to the first one and not necessarily the one
> you want.
>
> The change log needs to be more explicit in what the "bug" is. It does
> state this in a round about way, but it is written in a way that it doesn't
> stand out.
>
> Previously to this commit, if func matches several symbols, a kprobe,
> being either sysfs or PMU, would only be installed for the first
> matching address. This could lead to some misunderstanding when some
> BPF code was never called because it was attached to a function which
> was indeed not called, because the effectively called one has no
> kprobes attached.
>
> So, this commit returns EADDRNOTAVAIL when func matches several
> symbols. This way, user needs to use address to remove the ambiguity.
>
>
> What it should say is:
>
> When a kprobe is attached to a function that's name is not unique (is
> static and shares the name with other functions in the kernel), the
> kprobe is attached to the first function it finds. This is a bug as the
> function that it is attaching to is not necessarily the one that the
> user wants to attach to.
>
> Instead of blindly picking a function to attach to what is ambiguous,
> error with EADDRNOTAVAIL to let the user know that this function is not
> unique, and that the user must use another unique function with an
> address offset to get to the function they want to attach to.
>
Great!, yes this looks good to me too.
> And yes, it should have:
>
> Cc: sta...@vger.kernel.org
>
> which is how to mark something for stable, and
>
> Fixes: ...
>
> To the commit that caused the bug.
Yes, this should be the first one.
Fixes: 413d37d1eb69 ("tracing: Add kprobe-based event tracer")
Thank you,
>
> -- Steve
--
Masami Hiramatsu (Google)
Re: [PATCH v5 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation
On Thu, 19 Oct 2023 21:18:43 +0900 Masami Hiramatsu (Google) wrote: > > So why is this adding stable? (and as Greg's form letter states, that's not > > how you do that) > > > > I don't see this as a fix but a new feature. > > I asked him to make this a fix since the current kprobe event' behavior is > somewhat strange. It puts the probe on only the "first symbol" if user > specifies a symbol name which has multiple instances. In this case, the > actual probe address can not be solved by name. User must specify the > probe address by unique name + offset. Unless, it can put a probe on > unexpected address, especially if it specifies non-unique symbol + offset, > the address may NOT be the instruction boundary. > To avoid this issue, it should check the given symbol is unique. > OK, so what is broken is that when you add a probe to a function that has multiple names, it will attach to the first one and not necessarily the one you want. The change log needs to be more explicit in what the "bug" is. It does state this in a round about way, but it is written in a way that it doesn't stand out. Previously to this commit, if func matches several symbols, a kprobe, being either sysfs or PMU, would only be installed for the first matching address. This could lead to some misunderstanding when some BPF code was never called because it was attached to a function which was indeed not called, because the effectively called one has no kprobes attached. So, this commit returns EADDRNOTAVAIL when func matches several symbols. This way, user needs to use address to remove the ambiguity. What it should say is: When a kprobe is attached to a function that's name is not unique (is static and shares the name with other functions in the kernel), the kprobe is attached to the first function it finds. This is a bug as the function that it is attaching to is not necessarily the one that the user wants to attach to. Instead of blindly picking a function to attach to what is ambiguous, error with EADDRNOTAVAIL to let the user know that this function is not unique, and that the user must use another unique function with an address offset to get to the function they want to attach to. And yes, it should have: Cc: sta...@vger.kernel.org which is how to mark something for stable, and Fixes: ... To the commit that caused the bug. -- Steve
Re: [PATCH v5 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation
On Wed, 18 Oct 2023 13:00:42 -0400 Steven Rostedt wrote: > On Wed, 18 Oct 2023 17:40:28 +0300 > Francis Laniel wrote: > > > Changes since: > > v1: > > * Use EADDRNOTAVAIL instead of adding a new error code. > > * Correct also this behavior for sysfs kprobe. > > v2: > > * Count the number of symbols corresponding to function name and return > > EADDRNOTAVAIL if higher than 1. > > * Return ENOENT if above count is 0, as it would be returned later by > > while > > registering the kprobe. > > v3: > > * Check symbol does not contain ':' before testing its uniqueness. > > * Add a selftest to check this is not possible to install a kprobe for a > > non > > unique symbol. > > v5: > > * No changes, just add linux-stable as recipient. > > So why is this adding stable? (and as Greg's form letter states, that's not > how you do that) > > I don't see this as a fix but a new feature. I asked him to make this a fix since the current kprobe event' behavior is somewhat strange. It puts the probe on only the "first symbol" if user specifies a symbol name which has multiple instances. In this case, the actual probe address can not be solved by name. User must specify the probe address by unique name + offset. Unless, it can put a probe on unexpected address, especially if it specifies non-unique symbol + offset, the address may NOT be the instruction boundary. To avoid this issue, it should check the given symbol is unique. Thank you, > > -- Steve -- Masami Hiramatsu (Google)
Re: [PATCH v5 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation
Hi! Le mercredi 18 octobre 2023, 20:00:42 EEST Steven Rostedt a écrit : > On Wed, 18 Oct 2023 17:40:28 +0300 > > Francis Laniel wrote: > > Changes since: > > v1: > > * Use EADDRNOTAVAIL instead of adding a new error code. > > * Correct also this behavior for sysfs kprobe. > > > > v2: > > * Count the number of symbols corresponding to function name and return > > EADDRNOTAVAIL if higher than 1. > > * Return ENOENT if above count is 0, as it would be returned later by > > while > > registering the kprobe. > > > > v3: > > * Check symbol does not contain ':' before testing its uniqueness. > > * Add a selftest to check this is not possible to install a kprobe for a > > non unique symbol. > > > > v5: > > * No changes, just add linux-stable as recipient. > > So why is this adding stable? (and as Greg's form letter states, that's not > how you do that) Oops! Really sorry for this, I will correct everything for the next version! > > I don't see this as a fix but a new feature. You mean I should add a "Fix:" in the commit description? > > -- Steve Best regards.
