Re: [RFC PATCH bpf-next 02/12] libbpf: Introduce bpf SDT probe macros

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardcoded named labels in inline assembly break compilation during routine compiler optimizations.
- [High] Missing `static` keyword on dummy type-checking variables causes linker errors.
- [High] Missing `"memory"` clobber in inline assembly allows the compiler to reorder memory operations past the probe site.
- [Medium] Macro local variable names (`__a0`, `__a1`, etc.) are insufficiently obfuscated, leading to variable shadowing and trace data corruption.
- [Low] BPF subsystem multi-line comment style violation.
--

commit 225a1b2d44558cc189a9e559d3f30514eab101a7
Author: Xu Kuohai <xukuohai@huawei.com>

libbpf: Introduce bpf SDT probe macros

Introduce macros to declare and define bpf SDT probes. The declaration is
used to generate BTF ID of FUNC_PROTO for each probe site, so the probe
sites can be validated by the kernel, and observer programs can be attached
to the FUNC_PROTO like normal tracing programs.

> diff --git a/tools/lib/bpf/bpf_sdt.h b/tools/lib/bpf/bpf_sdt.h
> new file mode 100644
> index 0000000000000..23f84460aa368
> --- /dev/null
> +++ b/tools/lib/bpf/bpf_sdt.h

[ ... ]

> +#define BPF_SDT_DECLARE1(name, t0)					\
> +	___sdt_decl_attr(name, 1)					\
> +	static void (*const __sdt_type_##name)(t0);			\
> +	___sdt_chk_attr t0 __sdt_chk_##name##_0

[Severity: High]
Could this missing static keyword cause linker errors?

If a developer places BPF_SDT_DECLARE<N> in a shared header included in
multiple translation units, modern compilers defaulting to -fno-common might
throw multiple strong definition errors for these dummy variables since they
lack the static keyword.

[ ... ]

> +/* probe macros, 0-5 arguments
> + *
> + * Each probe emits a NOP (goto +0) in the program stream plus a
> + * .bpf_sdt_notes entry.  The entry begins with the ___sdt_jt_<name>

[Severity: Low]
This isn't a bug, but the BPF subsystem multi-line comment style requires
the opening /* to be on its own line.

> + * label so that libbpf can locate each entry boundary at load time.
> + * One BPF_SDT_PROBE<N> invocation per probe name is required; invoking
> + * the same name twice produces a duplicate symbol.
> + */
> +#define BPF_SDT_PROBE(name)						\
> +({									\
> +	asm volatile(							\
> +		"0:\n"							\
> +		"goto +0\n"						\
> +		".pushsection .bpf_sdt_notes,\"\",@progbits\n"		\
> +		"___sdt_jt_" #name ":\n"				\

[Severity: High]
Does this inline assembly block need a "memory" clobber?

Without it, could the compiler reorder memory operations past the probe
site, causing attached observer programs to read stale or uncommitted state
when triggered?

[Severity: High]
Can the fixed label ___sdt_jt_ #name cause a build failure?

If the compiler unrolls a loop containing this macro or inlines an
__always_inline caller multiple times, it seems the assembler would
encounter multiple identical labels and fail with a redefinition error.

[ ... ]

> +#define BPF_SDT_PROBE2(name, a, b)					\
> +({									\
> +	_Static_assert(__builtin_types_compatible_p(typeof(a),		\
> +			typeof(__sdt_chk_##name##_0)),			\
> +			"BPF_SDT_PROBE2: arg0 type mismatch");		\
> +	_Static_assert(sizeof(typeof(a)) <= 8,				\
> +			"BPF_SDT_PROBE2: arg0 too large, use pointer");	\
> +	_Static_assert(__builtin_types_compatible_p(typeof(b),		\
> +			typeof(__sdt_chk_##name##_1)),			\
> +			"BPF_SDT_PROBE2: arg1 type mismatch");		\
> +	_Static_assert(sizeof(typeof(b)) <= 8,				\
> +			"BPF_SDT_PROBE2: arg1 too large, use pointer");	\
> +	typeof(a) __a0 = (a);						\
> +	typeof(b) __a1 = (b);						\

[Severity: Medium]
Can these local variable names silently shadow caller variables?

If a caller passes a variable named __a0 as the second argument (b), does
the declaration of __a1 capture the macro's newly declared __a0 instead of
the caller's variable, resulting in the probe recording the first argument
twice?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782571533.git.xukuohai@huawei.com?part=2