Re: [PATCH v5 0/2] Return EADDRNOTAVAIL when func matches several symbols during kprobe creation

[Francis Laniel <flaniel@linux.microsoft.com>]

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@goodmis.org> wrote:
> > On Thu, 19 Oct 2023 21:18:43 +0900
> > 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org> 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@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! I should have thought about it nonetheless but I will take more 
care in the future!

> Thank you,
> 
> > -- Steve

Best regards.