[PATCH bpf-next] selftests/bpf: Fix uprobe consumer test (again)

[PATCH bpf-next] selftests/bpf: Fix uprobe consumer test (again)

[Jiri Olsa]

The new uprobe changes bring bit some new behaviour that we need
to reflect in the consumer test.

There's special case when we have one of the existing uretprobes removed
and at the same time we're adding the other uretprobe. In this case we get
hit on the new uretprobe consumer only if there was already another uprobe
existing so the uprobe object stayed valid for uprobe return instance.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/uprobe_multi_test.c    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index 619b31cd24a1..545b91385749 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -873,10 +873,21 @@ static int consumer_test(struct uprobe_multi_consumers *skel,
 			 * which means one of the 'return' uprobes was alive when probe was hit:
 			 *
 			 *   idxs: 2/3 uprobe return in 'installed' mask
+			 *
+			 * There's special case when we have one of the existing uretprobes removed
+			 * and at the same time we're adding the other uretprobe. In this case we get
+			 * hit on the new uretprobe consumer only if there was already another uprobe
+			 * existing so the uprobe object stayed valid for uprobe return instance.
 			 */
 			unsigned long had_uretprobes  = before & 0b1100; /* is uretprobe installed */
+			unsigned long b = before >> 2, a = after >> 2;
+			bool hit = true;
+
+			/* Match for following a/b cases: 01/10 10/01 */
+			if ((a ^ b) == 0b11)
+				hit = before & 0b11;
 
-			if (had_uretprobes && test_bit(idx, after))
+			if (hit && had_uretprobes && test_bit(idx, after))
 				val++;
 			fmt = "idx 2/3: uretprobe";
 		}
-- 
2.47.0

Re: [PATCH bpf-next] selftests/bpf: Fix uprobe consumer test (again)

[Andrii Nakryiko]

On Wed, Nov 6, 2024 at 2:40 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The new uprobe changes bring bit some new behaviour that we need

needs some proofreading, not sure what you were trying to say

> to reflect in the consumer test.
>
> There's special case when we have one of the existing uretprobes removed

see below, I don't like how special that case seems. It's actually not
that special, we just have a rule under which uretprobe instance
survives before->after transition, and we can express that pretty
clearly and explicitly.

pw-bot: cr

> and at the same time we're adding the other uretprobe. In this case we get
> hit on the new uretprobe consumer only if there was already another uprobe
> existing so the uprobe object stayed valid for uprobe return instance.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/uprobe_multi_test.c    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> index 619b31cd24a1..545b91385749 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> @@ -873,10 +873,21 @@ static int consumer_test(struct uprobe_multi_consumers *skel,
>                          * which means one of the 'return' uprobes was alive when probe was hit:
>                          *
>                          *   idxs: 2/3 uprobe return in 'installed' mask
> +                        *
> +                        * There's special case when we have one of the existing uretprobes removed
> +                        * and at the same time we're adding the other uretprobe. In this case we get
> +                        * hit on the new uretprobe consumer only if there was already another uprobe
> +                        * existing so the uprobe object stayed valid for uprobe return instance.
>                          */
>                         unsigned long had_uretprobes  = before & 0b1100; /* is uretprobe installed */
> +                       unsigned long b = before >> 2, a = after >> 2;
> +                       bool hit = true;
> +
> +                       /* Match for following a/b cases: 01/10 10/01 */
> +                       if ((a ^ b) == 0b11)
> +                               hit = before & 0b11;
>
> -                       if (had_uretprobes && test_bit(idx, after))
> +                       if (hit && had_uretprobes && test_bit(idx, after))

I found these changes very hard to reason about (not because of bit
manipulations, but due to very specific 01/10 requirement, which seems
too specific). So I came up with this:

    bool uret_stays = before & after & 0b1100;
    bool uret_survives = (before & 0b1100) && (after & 0b1100) &&
(before & 0b0011);

    if ((uret_stays || uret_survives) && test_bit(idx, after))
        val++;

The idea being that uretprobe under test either stayed from before to
after (uret_stays + test_bit) or uretprobe instance survived and we
have uretprobe active in after (uret_survives + test_bit).

uret_survives just states that uretprobe survives if there are *any*
uretprobes both before and after (overlapping or not, doesn't matter)
and uprobe was attached before.

Does it make sense? Can you incorporate that into v2, if you agree?


>                                 val++;
>                         fmt = "idx 2/3: uretprobe";
>                 }
> --
> 2.47.0
>

Re: [PATCH bpf-next] selftests/bpf: Fix uprobe consumer test (again)

[Jiri Olsa]

On Wed, Nov 06, 2024 at 04:26:11PM -0800, Andrii Nakryiko wrote:
> On Wed, Nov 6, 2024 at 2:40 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The new uprobe changes bring bit some new behaviour that we need
> 
> needs some proofreading, not sure what you were trying to say
> 
> > to reflect in the consumer test.
> >
> > There's special case when we have one of the existing uretprobes removed
> 
> see below, I don't like how special that case seems. It's actually not
> that special, we just have a rule under which uretprobe instance
> survives before->after transition, and we can express that pretty
> clearly and explicitly.
> 
> pw-bot: cr
> 
> > and at the same time we're adding the other uretprobe. In this case we get
> > hit on the new uretprobe consumer only if there was already another uprobe
> > existing so the uprobe object stayed valid for uprobe return instance.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/uprobe_multi_test.c    | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > index 619b31cd24a1..545b91385749 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -873,10 +873,21 @@ static int consumer_test(struct uprobe_multi_consumers *skel,
> >                          * which means one of the 'return' uprobes was alive when probe was hit:
> >                          *
> >                          *   idxs: 2/3 uprobe return in 'installed' mask
> > +                        *
> > +                        * There's special case when we have one of the existing uretprobes removed
> > +                        * and at the same time we're adding the other uretprobe. In this case we get
> > +                        * hit on the new uretprobe consumer only if there was already another uprobe
> > +                        * existing so the uprobe object stayed valid for uprobe return instance.
> >                          */
> >                         unsigned long had_uretprobes  = before & 0b1100; /* is uretprobe installed */
> > +                       unsigned long b = before >> 2, a = after >> 2;
> > +                       bool hit = true;
> > +
> > +                       /* Match for following a/b cases: 01/10 10/01 */
> > +                       if ((a ^ b) == 0b11)
> > +                               hit = before & 0b11;
> >
> > -                       if (had_uretprobes && test_bit(idx, after))
> > +                       if (hit && had_uretprobes && test_bit(idx, after))
> 
> I found these changes very hard to reason about (not because of bit
> manipulations, but due to very specific 01/10 requirement, which seems
> too specific). So I came up with this:
> 
>     bool uret_stays = before & after & 0b1100;
>     bool uret_survives = (before & 0b1100) && (after & 0b1100) &&
> (before & 0b0011);
> 
>     if ((uret_stays || uret_survives) && test_bit(idx, after))
>         val++;
> 
> The idea being that uretprobe under test either stayed from before to
> after (uret_stays + test_bit) or uretprobe instance survived and we
> have uretprobe active in after (uret_survives + test_bit).
> 
> uret_survives just states that uretprobe survives if there are *any*
> uretprobes both before and after (overlapping or not, doesn't matter)
> and uprobe was attached before.
> 
> Does it make sense? Can you incorporate that into v2, if you agree?

ok, seems easier.. will send v2

thanks,
jirka