Hi!
Le jeudi 19 octobre 2023, 18:07:08 EEST Masami Hiramatsu a écrit :
> 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! I should have thought about it nonetheless but I will take more
care in the future!
> Thank you,
>
> > -- Steve
Best regards.
