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, 16:51:04 EEST Steven Rostedt a écrit :
> 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.

Thank you for the suggestion!
I updated the commit message and I am about to send v6!

> And yes, it should have:
> 
> Cc: stable@vger.kernel.org
> 
> which is how to mark something for stable, and

I will for sure remember about it for future contributions! Thank you!

> Fixes: ...
> 
> To the commit that caused the bug.
> 
> -- Steve

Best regards.