On Thu, 19 Oct 2023 09:51:04 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Thu, 19 Oct 2023 21:18:43 +0900
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> 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: stable@xxxxxxxxxxxxxxx
>
> 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) <mhiramat@xxxxxxxxxx>
