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) [off-list ref] wrote:

quoted

quoted

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.