[PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[Ihor Solodrai]

Information about USDT argument size is implicitly stored in
__bpf_usdt_arg_spec, but currently it's not accessbile to BPF programs
that use USDT.

Implement bpf_sdt_arg_size() that returns the size of an USDT argument
in bytes.

Factor out __bpf_usdt_arg_spec() routine from bpf_usdt_arg(). It
searches for arg_spec given ctx and arg_num.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 tools/lib/bpf/usdt.bpf.h | 59 ++++++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
index b811f754939f..6041271c5e4e 100644
--- a/tools/lib/bpf/usdt.bpf.h
+++ b/tools/lib/bpf/usdt.bpf.h
@@ -108,19 +108,12 @@ int bpf_usdt_arg_cnt(struct pt_regs *ctx)
 	return spec->arg_cnt;
 }
 
-/* Fetch USDT argument #*arg_num* (zero-indexed) and put its value into *res.
- * Returns 0 on success; negative error, otherwise.
- * On error *res is guaranteed to be set to zero.
- */
-__weak __hidden
-int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
+/* Validate ctx and arg_num, if ok set arg_spec pointer */
+static __always_inline
+int __bpf_usdt_arg_spec(struct pt_regs *ctx, __u64 arg_num, struct __bpf_usdt_arg_spec **arg_spec)
 {
 	struct __bpf_usdt_spec *spec;
-	struct __bpf_usdt_arg_spec *arg_spec;
-	unsigned long val;
-	int err, spec_id;
-
-	*res = 0;
+	int spec_id;
 
 	spec_id = __bpf_usdt_spec_id(ctx);
 	if (spec_id < 0)
@@ -136,7 +129,49 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
 	if (arg_num >= spec->arg_cnt)
 		return -ENOENT;
 
-	arg_spec = &spec->args[arg_num];
+	*arg_spec = &spec->args[arg_num];
+
+	return 0;
+}
+
+/* Returns the size in bytes of the #*arg_num* (zero-indexed) USDT argument.
+ * Returns negative error if argument is not found or arg_num is invalid.
+ */
+static __always_inline
+int bpf_usdt_arg_size(struct pt_regs *ctx, __u64 arg_num)
+{
+	struct __bpf_usdt_arg_spec *arg_spec;
+	int err;
+
+	err = __bpf_usdt_arg_spec(ctx, arg_num, &arg_spec);
+	if (err)
+		return err;
+
+	/* arg_spec->arg_bitshift = 64 - arg_sz * 8
+	 * so: arg_sz = (64 - arg_spec->arg_bitshift) / 8
+	 * Do a bitshift instead of a division to avoid
+	 * "unsupported signed division" error.
+	 */
+	return (64 - arg_spec->arg_bitshift) >> 3;
+}
+
+/* Fetch USDT argument #*arg_num* (zero-indexed) and put its value into *res.
+ * Returns 0 on success; negative error, otherwise.
+ * On error *res is guaranteed to be set to zero.
+ */
+__weak __hidden
+int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
+{
+	struct __bpf_usdt_arg_spec *arg_spec;
+	unsigned long val;
+	int err;
+
+	*res = 0;
+
+	err = __bpf_usdt_arg_spec(ctx, arg_num, &arg_spec);
+	if (err)
+		return err;
+
 	switch (arg_spec->arg_type) {
 	case BPF_USDT_ARG_CONST:
 		/* Arg is just a constant ("-4@$-9" in USDT arg spec).
-- 
2.48.1

[PATCH bpf-next 2/2] selftests/bpf: test bpf_usdt_arg_size() function

[Ihor Solodrai]

Update usdt tests to also check for correct behavior of
bpf_usdt_arg_size().

Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 tools/testing/selftests/bpf/prog_tests/usdt.c | 11 ++++++++++-
 tools/testing/selftests/bpf/progs/test_usdt.c | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
index 56ed1eb9b527..495d66414b57 100644
--- a/tools/testing/selftests/bpf/prog_tests/usdt.c
+++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
@@ -45,7 +45,7 @@ static void subtest_basic_usdt(void)
 	LIBBPF_OPTS(bpf_usdt_opts, opts);
 	struct test_usdt *skel;
 	struct test_usdt__bss *bss;
-	int err;
+	int err, i;
 
 	skel = test_usdt__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "skel_open"))
@@ -75,6 +75,7 @@ static void subtest_basic_usdt(void)
 	ASSERT_EQ(bss->usdt0_cookie, 0xcafedeadbeeffeed, "usdt0_cookie");
 	ASSERT_EQ(bss->usdt0_arg_cnt, 0, "usdt0_arg_cnt");
 	ASSERT_EQ(bss->usdt0_arg_ret, -ENOENT, "usdt0_arg_ret");
+	ASSERT_EQ(bss->usdt0_arg_size, -ENOENT, "usdt0_arg_size");
 
 	/* auto-attached usdt3 gets default zero cookie value */
 	ASSERT_EQ(bss->usdt3_cookie, 0, "usdt3_cookie");
@@ -86,6 +87,9 @@ static void subtest_basic_usdt(void)
 	ASSERT_EQ(bss->usdt3_args[0], 1, "usdt3_arg1");
 	ASSERT_EQ(bss->usdt3_args[1], 42, "usdt3_arg2");
 	ASSERT_EQ(bss->usdt3_args[2], (uintptr_t)&bla, "usdt3_arg3");
+	ASSERT_EQ(bss->usdt3_arg_sizes[0], 4, "usdt3_arg1_size");
+	ASSERT_EQ(bss->usdt3_arg_sizes[1], 8, "usdt3_arg2_size");
+	ASSERT_EQ(bss->usdt3_arg_sizes[2], 8, "usdt3_arg3_size");
 
 	/* auto-attached usdt12 gets default zero cookie value */
 	ASSERT_EQ(bss->usdt12_cookie, 0, "usdt12_cookie");
@@ -104,6 +108,11 @@ static void subtest_basic_usdt(void)
 	ASSERT_EQ(bss->usdt12_args[10], nums[idx], "usdt12_arg11");
 	ASSERT_EQ(bss->usdt12_args[11], t1.y, "usdt12_arg12");
 
+	int usdt12_expected_arg_sizes[12] = { 4, 4, 8, 8, 4, 8, 8, 8, 4, 2, 2, 1 };
+
+	for (i = 0; i < 12; i++)
+		ASSERT_EQ(bss->usdt12_arg_sizes[i], usdt12_expected_arg_sizes[i], "usdt12_arg_size");
+
 	/* trigger_func() is marked __always_inline, so USDT invocations will be
 	 * inlined in two different places, meaning that each USDT will have
 	 * at least 2 different places to be attached to. This verifies that
diff --git a/tools/testing/selftests/bpf/progs/test_usdt.c b/tools/testing/selftests/bpf/progs/test_usdt.c
index 505aab9a5234..096488f47fbc 100644
--- a/tools/testing/selftests/bpf/progs/test_usdt.c
+++ b/tools/testing/selftests/bpf/progs/test_usdt.c
@@ -11,6 +11,7 @@ int usdt0_called;
 u64 usdt0_cookie;
 int usdt0_arg_cnt;
 int usdt0_arg_ret;
+int usdt0_arg_size;
 
 SEC("usdt")
 int usdt0(struct pt_regs *ctx)
@@ -26,6 +27,7 @@ int usdt0(struct pt_regs *ctx)
 	usdt0_arg_cnt = bpf_usdt_arg_cnt(ctx);
 	/* should return -ENOENT for any arg_num */
 	usdt0_arg_ret = bpf_usdt_arg(ctx, bpf_get_prandom_u32(), &tmp);
+	usdt0_arg_size = bpf_usdt_arg_size(ctx, bpf_get_prandom_u32());
 	return 0;
 }
 
@@ -34,6 +36,7 @@ u64 usdt3_cookie;
 int usdt3_arg_cnt;
 int usdt3_arg_rets[3];
 u64 usdt3_args[3];
+int usdt3_arg_sizes[3];
 
 SEC("usdt//proc/self/exe:test:usdt3")
 int usdt3(struct pt_regs *ctx)
@@ -50,12 +53,15 @@ int usdt3(struct pt_regs *ctx)
 
 	usdt3_arg_rets[0] = bpf_usdt_arg(ctx, 0, &tmp);
 	usdt3_args[0] = (int)tmp;
+	usdt3_arg_sizes[0] = bpf_usdt_arg_size(ctx, 0);
 
 	usdt3_arg_rets[1] = bpf_usdt_arg(ctx, 1, &tmp);
 	usdt3_args[1] = (long)tmp;
+	usdt3_arg_sizes[1] = bpf_usdt_arg_size(ctx, 1);
 
 	usdt3_arg_rets[2] = bpf_usdt_arg(ctx, 2, &tmp);
 	usdt3_args[2] = (uintptr_t)tmp;
+	usdt3_arg_sizes[2] = bpf_usdt_arg_size(ctx, 2);
 
 	return 0;
 }
@@ -64,12 +70,15 @@ int usdt12_called;
 u64 usdt12_cookie;
 int usdt12_arg_cnt;
 u64 usdt12_args[12];
+int usdt12_arg_sizes[12];
 
 SEC("usdt//proc/self/exe:test:usdt12")
 int BPF_USDT(usdt12, int a1, int a2, long a3, long a4, unsigned a5,
 		     long a6, __u64 a7, uintptr_t a8, int a9, short a10,
 		     short a11, signed char a12)
 {
+	int i;
+
 	if (my_pid != (bpf_get_current_pid_tgid() >> 32))
 		return 0;
 
@@ -90,6 +99,11 @@ int BPF_USDT(usdt12, int a1, int a2, long a3, long a4, unsigned a5,
 	usdt12_args[9] = a10;
 	usdt12_args[10] = a11;
 	usdt12_args[11] = a12;
+
+	bpf_for(i, 0, 12) {
+		usdt12_arg_sizes[i] = bpf_usdt_arg_size(ctx, i);
+	}
+
 	return 0;
 }
 
-- 
2.48.1

Re: [PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[Andrii Nakryiko]

On Thu, Feb 20, 2025 at 1:59 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>
> Information about USDT argument size is implicitly stored in
> __bpf_usdt_arg_spec, but currently it's not accessbile to BPF programs
> that use USDT.
>
> Implement bpf_sdt_arg_size() that returns the size of an USDT argument
> in bytes.
>
> Factor out __bpf_usdt_arg_spec() routine from bpf_usdt_arg(). It
> searches for arg_spec given ctx and arg_num.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> ---
>  tools/lib/bpf/usdt.bpf.h | 59 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index b811f754939f..6041271c5e4e 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -108,19 +108,12 @@ int bpf_usdt_arg_cnt(struct pt_regs *ctx)
>         return spec->arg_cnt;
>  }
>
> -/* Fetch USDT argument #*arg_num* (zero-indexed) and put its value into *res.
> - * Returns 0 on success; negative error, otherwise.
> - * On error *res is guaranteed to be set to zero.
> - */
> -__weak __hidden
> -int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> +/* Validate ctx and arg_num, if ok set arg_spec pointer */
> +static __always_inline
> +int __bpf_usdt_arg_spec(struct pt_regs *ctx, __u64 arg_num, struct __bpf_usdt_arg_spec **arg_spec)
>  {
>         struct __bpf_usdt_spec *spec;
> -       struct __bpf_usdt_arg_spec *arg_spec;
> -       unsigned long val;
> -       int err, spec_id;
> -
> -       *res = 0;
> +       int spec_id;
>
>         spec_id = __bpf_usdt_spec_id(ctx);
>         if (spec_id < 0)
> @@ -136,7 +129,49 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>         if (arg_num >= spec->arg_cnt)
>                 return -ENOENT;
>
> -       arg_spec = &spec->args[arg_num];
> +       *arg_spec = &spec->args[arg_num];
> +
> +       return 0;
> +}
> +
> +/* Returns the size in bytes of the #*arg_num* (zero-indexed) USDT argument.
> + * Returns negative error if argument is not found or arg_num is invalid.
> + */
> +static __always_inline
> +int bpf_usdt_arg_size(struct pt_regs *ctx, __u64 arg_num)
> +{
> +       struct __bpf_usdt_arg_spec *arg_spec;
> +       int err;
> +
> +       err = __bpf_usdt_arg_spec(ctx, arg_num, &arg_spec);

let's not extract this into a separate function. I don't particularly
like the out parameter, and no need to add another function that could
be used by users. There isn't much duplication, I think it's fine if
we just copy/paste few lines of code.

pw-bot: cr

> +       if (err)
> +               return err;
> +
> +       /* arg_spec->arg_bitshift = 64 - arg_sz * 8
> +        * so: arg_sz = (64 - arg_spec->arg_bitshift) / 8
> +        * Do a bitshift instead of a division to avoid
> +        * "unsupported signed division" error.
> +        */
> +       return (64 - arg_spec->arg_bitshift) >> 3;

arg_bitshift is stored as char (which could be signed), so that's why
you were getting signed division, just cast to unsigned and keep
division:

return (64 - (unsigned)arg_spec->arg_bitshift) / 8;

> +}
> +
> +/* Fetch USDT argument #*arg_num* (zero-indexed) and put its value into *res.
> + * Returns 0 on success; negative error, otherwise.
> + * On error *res is guaranteed to be set to zero.
> + */
> +__weak __hidden
> +int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> +{
> +       struct __bpf_usdt_arg_spec *arg_spec;
> +       unsigned long val;
> +       int err;
> +
> +       *res = 0;
> +
> +       err = __bpf_usdt_arg_spec(ctx, arg_num, &arg_spec);
> +       if (err)
> +               return err;
> +
>         switch (arg_spec->arg_type) {
>         case BPF_USDT_ARG_CONST:
>                 /* Arg is just a constant ("-4@$-9" in USDT arg spec).
> --
> 2.48.1
>

Re: [PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[Ihor Solodrai]

On 2/24/25 2:05 PM, Andrii Nakryiko wrote:
> On Thu, Feb 20, 2025 at 1:59 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>>
>> Information about USDT argument size is implicitly stored in
>> __bpf_usdt_arg_spec, but currently it's not accessbile to BPF programs
>> that use USDT.
>>
>> Implement bpf_sdt_arg_size() that returns the size of an USDT argument
>> in bytes.
>>
>> Factor out __bpf_usdt_arg_spec() routine from bpf_usdt_arg(). It
>> searches for arg_spec given ctx and arg_num.
>>
>> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
>> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>> ---
>>  tools/lib/bpf/usdt.bpf.h | 59 ++++++++++++++++++++++++++++++++--------
>>  1 file changed, 47 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
>> index b811f754939f..6041271c5e4e 100644
>> --- a/tools/lib/bpf/usdt.bpf.h
>> +++ b/tools/lib/bpf/usdt.bpf.h
>> @@ -108,19 +108,12 @@ int bpf_usdt_arg_cnt(struct pt_regs *ctx)
>>         return spec->arg_cnt;
>>  }
>>
>> -/* Fetch USDT argument #*arg_num* (zero-indexed) and put its value into *res.
>> - * Returns 0 on success; negative error, otherwise.
>> - * On error *res is guaranteed to be set to zero.
>> - */
>> -__weak __hidden
>> -int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>> +/* Validate ctx and arg_num, if ok set arg_spec pointer */
>> +static __always_inline
>> +int __bpf_usdt_arg_spec(struct pt_regs *ctx, __u64 arg_num, struct __bpf_usdt_arg_spec **arg_spec)
>>  {
>>         struct __bpf_usdt_spec *spec;
>> -       struct __bpf_usdt_arg_spec *arg_spec;
>> -       unsigned long val;
>> -       int err, spec_id;
>> -
>> -       *res = 0;
>> +       int spec_id;
>>
>>         spec_id = __bpf_usdt_spec_id(ctx);
>>         if (spec_id < 0)
>> @@ -136,7 +129,49 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>>         if (arg_num >= spec->arg_cnt)
>>                 return -ENOENT;
>>
>> -       arg_spec = &spec->args[arg_num];
>> +       *arg_spec = &spec->args[arg_num];
>> +
>> +       return 0;
>> +}
>> +
>> +/* Returns the size in bytes of the #*arg_num* (zero-indexed) USDT argument.
>> + * Returns negative error if argument is not found or arg_num is invalid.
>> + */
>> +static __always_inline
>> +int bpf_usdt_arg_size(struct pt_regs *ctx, __u64 arg_num)
>> +{
>> +       struct __bpf_usdt_arg_spec *arg_spec;
>> +       int err;
>> +
>> +       err = __bpf_usdt_arg_spec(ctx, arg_num, &arg_spec);
>
> let's not extract this into a separate function. I don't particularly
> like the out parameter, and no need to add another function that could
> be used by users. There isn't much duplication, I think it's fine if
> we just copy/paste few lines of code.

Ok. I wasn't sure what is the tolerance level for copy-pasting in this
context.

>
> pw-bot: cr
>
>> +       if (err)
>> +               return err;
>> +
>> +       /* arg_spec->arg_bitshift = 64 - arg_sz * 8
>> +        * so: arg_sz = (64 - arg_spec->arg_bitshift) / 8
>> +        * Do a bitshift instead of a division to avoid
>> +        * "unsupported signed division" error.
>> +        */
>> +       return (64 - arg_spec->arg_bitshift) >> 3;
>
> arg_bitshift is stored as char (which could be signed), so that's why
> you were getting signed division, just cast to unsigned and keep
> division:
>
> return (64 - (unsigned)arg_spec->arg_bitshift) / 8;

I haven't realized that. Thanks.

>
>> +}
>> +
>> +/* Fetch USDT argument #*arg_num* (zero-indexed) and put its value into *res.
>> + * Returns 0 on success; negative error, otherwise.
>> + * On error *res is guaranteed to be set to zero.
>> + */
>> +__weak __hidden
>> +int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
>> +{
>> +       struct __bpf_usdt_arg_spec *arg_spec;
>> +       unsigned long val;
>> +       int err;
>> +
>> +       *res = 0;
>> +
>> +       err = __bpf_usdt_arg_spec(ctx, arg_num, &arg_spec);
>> +       if (err)
>> +               return err;
>> +
>>         switch (arg_spec->arg_type) {
>>         case BPF_USDT_ARG_CONST:
>>                 /* Arg is just a constant ("-4@$-9" in USDT arg spec).
>> --
>> 2.48.1
>>

Re: [PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[Ihor Solodrai]

On 2/24/25 2:05 PM, Andrii Nakryiko wrote:

> [...]
>
> arg_bitshift is stored as char (which could be signed), so that's why
> you were getting signed division, just cast to unsigned and keep
> division:
>
> return (64 - (unsigned)arg_spec->arg_bitshift) / 8;

As it turns out, this doesn't work either. Presumably because
(64 - (u8)x) is still a signed int.

This works, however:

    return (unsigned char)(64 - arg_spec->arg_bitshift) / 8;

>
>> [...]

Re: [PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[Andrii Nakryiko]

On Mon, Feb 24, 2025 at 3:15 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>
> On 2/24/25 2:05 PM, Andrii Nakryiko wrote:
>
> > [...]
> >
> > arg_bitshift is stored as char (which could be signed), so that's why
> > you were getting signed division, just cast to unsigned and keep
> > division:
> >
> > return (64 - (unsigned)arg_spec->arg_bitshift) / 8;
>
> As it turns out, this doesn't work either. Presumably because
> (64 - (u8)x) is still a signed int.

hm... ok, surprising, but fine

>
> This works, however:
>
>     return (unsigned char)(64 - arg_spec->arg_bitshift) / 8;
>


nit: just unsigned (int), BPF doesn't have single-byte division
anyways, so it will be upconverted to 32-bit (or 64-bit for noalu32).
So let's be bold and use 32-bits here ;)

> >
> >> [...]

Re: [PATCH bpf-next 1/2] libbpf: implement bpf_usdt_arg_size BPF function

[Ihor Solodrai]

On 2/24/25 3:25 PM, Andrii Nakryiko wrote:
> On Mon, Feb 24, 2025 at 3:15 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>>
>> On 2/24/25 2:05 PM, Andrii Nakryiko wrote:
>>
>>> [...]
>>>
>>> arg_bitshift is stored as char (which could be signed), so that's why
>>> you were getting signed division, just cast to unsigned and keep
>>> division:
>>>
>>> return (64 - (unsigned)arg_spec->arg_bitshift) / 8;
>>
>> As it turns out, this doesn't work either. Presumably because
>> (64 - (u8)x) is still a signed int.
>
> hm... ok, surprising, but fine
>
>>
>> This works, however:
>>
>>     return (unsigned char)(64 - arg_spec->arg_bitshift) / 8;
>>
>
>
> nit: just unsigned (int), BPF doesn't have single-byte division
> anyways, so it will be upconverted to 32-bit (or 64-bit for noalu32).
> So let's be bold and use 32-bits here ;)

oh

I specified a type because:

    $ ./scripts/checkpatch.pl /tmpfs/0001-libbpf-implement-bpf_usdt_arg_size-BPF-function.patch 
    WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
    #56: FILE: tools/lib/bpf/usdt.bpf.h:140:
    +	return (64 - (unsigned)arg_spec->arg_bitshift) / 8;

(unsigned int) it is then

>
>>>
>>>> [...]