On Fri, Feb 17, 2023 at 11:10:29PM +0000, Alan Maguire wrote:
> It has been observed [1] that the recent dwarves changes
> that skip BTF encoding for functions that have optimized-out
> parameters are too aggressive, leading to missing kfuncs
> which generate warnings and a BPF selftest failure.
>
> Here a different approach is used; we observe that
> just because a function does not _use_ a parameter,
> it does not mean it was not passed to it. What we
> are really keen to detect are cases where the calling
> conventions are violated such that a function will
> not have parameter values in the expected registers.
> In such cases, tracing and kfunc behaviour will be
> unstable. We are not worried about parameters being
> optimized out, provided that optimization does not
> lead to other parameters being passed via
> unexpected registers.
>
> So this series attempts to detect such cases by
> examining register (DW_OP_regX) values for
> parameters where available; if these match
> expectations, the function is deemed safe to add to
> BTF, even if parameters are optimized out.
>
> Using this approach, the only functions that
> BTF generation is suppressed for are
>
> 1. those with parameters that violate calling
> conventions where present; and
> 2. those which have multiple inconsistent prototypes.
>
> With these changes, running pahole on a gcc-built
> vmlinux skips
>
> - 1164 functions due to multiple inconsistent function
> prototypes. Most of these are "."-suffixed optimized
> fuctions.
> - 331 functions due to unexpected register usage
>
> For a clang-built kernel, the numbers are
>
> - 539 functions with inconsistent prototypes are skipped
> - 209 functions with unexpected register usage are skipped
>
> One complication is that functions that are passed
> structs (or typedef structs) can use multiple registers
> to pass those structures. Examples include
> bpf_lsm_socket_getpeersec_stream() (passing a typedef
> struct sockptr_t) and the bpf_testmod_test_struct_arg_1
> function in bpf_testmod. Because multiple registers
> are used to represent the structure, this throws
> off expectations for any subsequent parameter->register
> mappings. To handle this, simply exempt functions
> that have struct (or typedef struct) parameters from
> our register checks.
>
> Note to test this series on bpf-next, the following
> commit should be reverted (reverting the revert
> so that the flags are added to BTF encoding when
> using pahole v1.25):
>
> commit 1f5dfcc78ab4 ("Revert "bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25"")
>
> With these changes we also see tracing_struct now pass:
>
> $ sudo ./test_progs -t tracing_struct
> #233 tracing_struct:OK
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
>
> Further testing is needed - along with support for additional
> parameter index -> DWARF reg for more platforms.
>
> Future work could also add annotations for optimized-out
> parameters via BTF tags to help guide tracing.
>
> [1] https://lore.kernel.org/bpf/CAADnVQ+hfQ9LEmEFXneB7hm17NvRniXSShrHLaM-1BrguLjLQw@xxxxxxxxxxxxxx/
>
> Alan Maguire (4):
> dwarf_loader: mark functions that do not use expected registers for
> params
> btf_encoder: exclude functions with unexpected param register use not
> optimizations
> pahole: update descriptions for btf_gen_optimized,
> skip_encoding_btf_inconsistent_proto
> pahole: update man page for options also
changes look good, but I don't have that much insight into dwarf part of
the code
anyway I tested on x86 with gcc and clang bpf selftests and it looks good,
and duplicate functions are gone
Tested-by: Jiri Olsa <jolsa@xxxxxxxxxx>
thanks,
jirka
>
> btf_encoder.c | 24 +++++++---
> dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++---
> dwarves.h | 5 +++
> man-pages/pahole.1 | 4 +-
> pahole.c | 4 +-
> 5 files changed, 129 insertions(+), 17 deletions(-)
>
> --
> 2.31.1
>
