[RFC dwarves 0/4] dwarves: change BTF encoding skip logic for functions

[RFC dwarves 0/4] dwarves: change BTF encoding skip logic for functions

[Alan Maguire]

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@mail.gmail.com/

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

 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

[RFC dwarves 1/4] dwarf_loader: mark functions that do not use expected registers for params

[Alan Maguire]

Calling conventions dictate which registers are used for
function parameters.

When a function is optimized however, we need to ensure that
the non-optimized parameters do not violate expectations about
register use as this would violate expectations for tracing.
At CU initialization, create a mapping from parameter index
to expected DW_OP_reg, and use it to validate parameters
match with expectations.  A parameter which is passed via
the stack, as a constant, or uses an unexpected register,
violates these expectations and it (and the associated
function) are marked as having unexpected register mapping.

Note though that there is as exception here that needs to
be handled; when a (typedef) struct is passed as a parameter,
it can use multiple registers so will throw off later register
expectations.  Exempt functions that have unexpected
register usage _and_ struct parameters (examples are found
in the "tracing_struct" test).

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++---
 dwarves.h      |   5 +++
 2 files changed, 109 insertions(+), 5 deletions(-)

diff --git a/dwarf_loader.c b/dwarf_loader.c
index acdb68d..014e130 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -1022,6 +1022,51 @@ static int arch__nr_register_params(const GElf_Ehdr *ehdr)
 	return 0;
 }
 
+/* map from parameter index (0 for first, ...) to expected DW_OP_reg.
+ * This will allow us to identify cases where optimized-out parameters
+ * interfere with expectations about register contents on function
+ * entry.
+ */
+static void arch__set_register_params(const GElf_Ehdr *ehdr, struct cu *cu)
+{
+	memset(cu->register_params, -1, sizeof(cu->register_params));
+
+	switch (ehdr->e_machine) {
+	case EM_S390:
+		/* https://github.com/IBM/s390x-abi/releases/download/v1.6/lzsabi_s390x.pdf */
+		cu->register_params[0] = DW_OP_reg2;	// %r2
+		cu->register_params[1] = DW_OP_reg3;	// %r3
+		cu->register_params[2] = DW_OP_reg4;	// %r4
+		cu->register_params[3] = DW_OP_reg5;	// %r5
+		cu->register_params[4] = DW_OP_reg6;	// %r6
+		return;
+	case EM_X86_64:
+		/* //en.wikipedia.org/wiki/X86_calling_conventions#System_V_AMD64_ABI */
+		cu->register_params[0] = DW_OP_reg5;	// %rdi
+		cu->register_params[1] = DW_OP_reg4;	// %rsi
+		cu->register_params[2] = DW_OP_reg1;	// %rdx
+		cu->register_params[3] = DW_OP_reg2;	// %rcx
+		cu->register_params[4] = DW_OP_reg8;	// %r8
+		cu->register_params[5] = DW_OP_reg9;	// %r9
+		return;
+	case EM_ARM:
+		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers */
+	case EM_AARCH64:
+		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers */
+		cu->register_params[0] = DW_OP_reg0;
+		cu->register_params[1] = DW_OP_reg1;
+		cu->register_params[2] = DW_OP_reg2;
+		cu->register_params[3] = DW_OP_reg3;
+		cu->register_params[4] = DW_OP_reg4;
+		cu->register_params[5] = DW_OP_reg5;
+		cu->register_params[6] = DW_OP_reg6;
+		cu->register_params[7] = DW_OP_reg7;
+		return;
+	default:
+		return;
+	}
+}
+
 static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 					struct conf_load *conf, int param_idx)
 {
@@ -1075,18 +1120,28 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
 		if (parm->has_loc &&
 		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
 			loc.exprlen != 0) {
+			int expected_reg = cu->register_params[param_idx];
 			Dwarf_Op *expr = loc.expr;
 
 			switch (expr->atom) {
 			case DW_OP_reg0 ... DW_OP_reg31:
+				/* mark parameters that use an unexpected
+				 * register to hold a parameter; these will
+				 * be problematic for users of BTF as they
+				 * violate expectations about register
+				 * contents.
+				 */
+				if (expected_reg >= 0 && expected_reg != expr->atom)
+					parm->unexpected_reg = 1;
+				break;
 			case DW_OP_breg0 ... DW_OP_breg31:
 				break;
 			default:
-				parm->optimized = 1;
+				parm->unexpected_reg = 1;
 				break;
 			}
 		} else if (has_const_value) {
-			parm->optimized = 1;
+			parm->unexpected_reg = 1;
 		}
 	}
 
@@ -2302,13 +2357,17 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu)
 			pos->tag.type = dtype->tag->type;
 			/* share location information between parameter and
 			 * abstract origin; if neither have location, we will
-			 * mark the parameter as optimized out.
+			 * mark the parameter as optimized out.  Also share
+			 * info regarding unexpected register use for
+			 * parameters.
 			 */
 			if (pos->has_loc)
 				opos->has_loc = pos->has_loc;
 
 			if (pos->optimized)
 				opos->optimized = pos->optimized;
+			if (pos->unexpected_reg)
+				opos->unexpected_reg = pos->unexpected_reg;
 			continue;
 		}
 
@@ -2584,6 +2643,27 @@ out:
 	return 0;
 }
 
+static bool param__is_struct(struct cu *cu, struct tag *tag)
+{
+	const struct dwarf_tag *dtag = tag->priv;
+	struct dwarf_tag *dtype = dwarf_cu__find_type_by_ref(cu->priv, &dtag->type);
+	struct tag *type;
+
+	if (!dtype)
+		return false;
+	type = dtype->tag;
+
+	switch (type->tag) {
+	case DW_TAG_structure_type:
+		return true;
+	case DW_TAG_typedef:
+		/* handle "typedef struct" */
+		return param__is_struct(cu, type);
+	default:
+		return false;
+	}
+}
+
 static int cu__resolve_func_ret_types_optimized(struct cu *cu)
 {
 	struct ptr_table *pt = &cu->functions_table;
@@ -2593,6 +2673,7 @@ static int cu__resolve_func_ret_types_optimized(struct cu *cu)
 		struct tag *tag = pt->entries[i];
 		struct parameter *pos;
 		struct function *fn = tag__function(tag);
+		bool has_unexpected_reg = false, has_struct_param = false;
 
 		/* mark function as optimized if parameter is, or
 		 * if parameter does not have a location; at this
@@ -2600,12 +2681,29 @@ static int cu__resolve_func_ret_types_optimized(struct cu *cu)
 		 * abstract origins for cases where a parameter
 		 * location is not stored in the original function
 		 * parameter tag.
+		 *
+		 * Also mark functions which, due to optimization,
+		 * use an unexpected register for a parameter.
+		 * Exception is functions which have a struct
+		 * as a parameter, as multiple registers may
+		 * be used to represent it, throwing off register
+		 * to parameter mapping.
 		 */
 		ftype__for_each_parameter(&fn->proto, pos) {
-			if (pos->optimized || !pos->has_loc) {
+			if (pos->optimized || !pos->has_loc)
 				fn->proto.optimized_parms = 1;
-				break;
+
+			if (pos->unexpected_reg)
+				has_unexpected_reg = true;
+		}
+		if (has_unexpected_reg) {
+			ftype__for_each_parameter(&fn->proto, pos) {
+				has_struct_param = param__is_struct(cu, &pos->tag);
+				if (has_struct_param)
+					break;
 			}
+			if (!has_struct_param)
+				fn->proto.unexpected_reg = 1;
 		}
 
 		if (tag == NULL || tag->type != 0)
@@ -2917,6 +3015,7 @@ static int cu__set_common(struct cu *cu, struct conf_load *conf,
 
 	cu->little_endian = ehdr.e_ident[EI_DATA] == ELFDATA2LSB;
 	cu->nr_register_params = arch__nr_register_params(&ehdr);
+	arch__set_register_params(&ehdr, cu);
 	return 0;
 }
 
diff --git a/dwarves.h b/dwarves.h
index 24a1909..5074cf8 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -234,6 +234,8 @@ struct debug_fmt_ops {
 	bool		   has_alignment_info;
 };
 
+#define ARCH_MAX_REGISTER_PARAMS	8
+
 /*
  * unspecified_type: If this CU has a DW_TAG_unspecified_type, as BTF doesn't have a representation for this
  * 		     and thus we need to check functions returning this to convert it to void.
@@ -265,6 +267,7 @@ struct cu {
 	uint8_t		 uses_global_strings:1;
 	uint8_t		 little_endian:1;
 	uint8_t		 nr_register_params;
+	int		 register_params[ARCH_MAX_REGISTER_PARAMS];
 	uint16_t	 language;
 	unsigned long	 nr_inline_expansions;
 	size_t		 size_inline_expansions;
@@ -812,6 +815,7 @@ struct parameter {
 	struct tag tag;
 	const char *name;
 	uint8_t optimized:1;
+	uint8_t unexpected_reg:1;
 	uint8_t has_loc:1;
 };
 
@@ -834,6 +838,7 @@ struct ftype {
 	uint16_t	 nr_parms;
 	uint8_t		 unspec_parms:1; /* just one bit is needed */
 	uint8_t		 optimized_parms:1;
+	uint8_t		 unexpected_reg:1;
 	uint8_t		 processed:1;
 	uint8_t		 inconsistent_proto:1;
 };
-- 
2.31.1

[RFC dwarves 2/4] btf_encoder: exclude functions with unexpected param register use not optimizations

[Alan Maguire]

A key problem with the existing approach of excluding functions
with optimized-out parameters is that it does not distinguish
between cases where a parameter was passed but not used versus
cases where a parameter is not passed or used.  The latter
is the only problematic case as for the former case, the
expected register states at function call time are as
they would be absent optimization.  Ideally call-site analysis
could tell us what actually happens when a function is called,
but in practice we use a different method to exclude functions
from BTF representation.  If a function clearly violates the
calling-convention expectations, where such a violation
means "parameter X does not use the expected register
as specified in calling conventions", it is excluded.

Note that we continue to mark functions which have optimized-out
parameters as this is good to know, but only actively exclude
functions with unexpected register usage for parameters or
multiple inconsistent function prototypes.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index ea5b47b..da776f4 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -871,14 +871,16 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
 
 		/* If saving and we find an existing entry, we want to merge
 		 * observations across both functions, checking that the
-		 * "seen optimized parameters" and "inconsistent prototype"
-		 * status is reflected in the func entry.
+		 * "seen optimized parameters", "inconsistent prototype"
+		 * and "unexpected register" status is reflected in the
+		 * the func entry.
 		 * If the entry is new, record encoder state required
 		 * to add the local function later (encoder + type_id_off)
 		 * such that we can add the function later.
 		 */
 		existing->proto.optimized_parms |= fn->proto.optimized_parms;
-		if (!existing->proto.optimized_parms && !existing->proto.inconsistent_proto &&
+		existing->proto.unexpected_reg |= fn->proto.unexpected_reg;
+		if (!existing->proto.unexpected_reg && !existing->proto.inconsistent_proto &&
 		     !funcs__match(encoder, func, fn))
 			existing->proto.inconsistent_proto = 1;
 	} else {
@@ -940,20 +942,26 @@ static void btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
 			if (!other_fn)
 				continue;
 			fn->proto.optimized_parms |= other_fn->proto.optimized_parms;
+			fn->proto.unexpected_reg |= other_fn->proto.unexpected_reg;
 			if (other_fn->proto.inconsistent_proto)
 				fn->proto.inconsistent_proto = 1;
-			if (!fn->proto.optimized_parms && !fn->proto.inconsistent_proto &&
+			if (!fn->proto.unexpected_reg && !fn->proto.inconsistent_proto &&
 			    !funcs__match(encoder, func, other_fn))
 				fn->proto.inconsistent_proto = 1;
 			other_fn->proto.processed = 1;
 		}
-		if (fn->proto.optimized_parms || fn->proto.inconsistent_proto) {
+		/* do not exclude functions with optimized-out parameters; they
+		 * may still be _called_ with the right parameter values, they
+		 * just do not _use_ them.  Only exclude functions with
+		 * unexpected register use or multiple inconsistent prototypes.
+		 */
+		if (fn->proto.unexpected_reg || fn->proto.inconsistent_proto) {
 			if (encoder->verbose) {
 				const char *name = function__name(fn);
 
 				printf("skipping addition of '%s'(%s) due to %s\n",
 				       name, fn->alias ?: name,
-				       fn->proto.optimized_parms ? "optimized-out parameters" :
+				       fn->proto.unexpected_reg ? "unexpected register used for parameter" :
 								   "multiple inconsistent function prototypes");
 			}
 		} else {
@@ -1856,7 +1864,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
 						printf("matched function '%s' with '%s'%s\n",
 						       name, func->name,
 						       fn->proto.optimized_parms ?
-						       ", has optimized-out parameters" : "");
+						       ", has optimized-out parameters" :
+						       fn->proto.unexpected_reg ? ", has unexpected register use by params" :
+						       "");
 					fn->alias = func->name;
 				}
 			}
-- 
2.31.1

[RFC dwarves 3/4] pahole: update descriptions for btf_gen_optimized, skip_encoding_btf_inconsistent_proto

[Alan Maguire]

Semantics are changed such that we do not skip for optimized-out parameters,
rather unexpected register use.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 pahole.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pahole.c b/pahole.c
index 53dfb07..6fc4ed6 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1647,12 +1647,12 @@ static const struct argp_option pahole__options[] = {
 	{
 		.name = "btf_gen_optimized",
 		.key  = ARGP_btf_gen_optimized,
-		.doc  = "Generate BTF for functions with optimization-related suffixes (.isra, .constprop).  BTF will only be generated if a function does not optimize out parameters."
+		.doc  = "Generate BTF for functions with optimization-related suffixes (.isra, .constprop)."
 	},
 	{
 		.name = "skip_encoding_btf_inconsistent_proto",
 		.key = ARGP_skip_encoding_btf_inconsistent_proto,
-		.doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or have optimized-out parameters."
+		.doc = "Skip functions that have multiple inconsistent function prototypes sharing the same name, or that use unexpected registers for parameter values."
 	},
 	{
 		.name = NULL,
-- 
2.31.1

[RFC dwarves 4/4] pahole: update man page for options also

[Alan Maguire]

...in line with changes made to skip logic.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 man-pages/pahole.1 | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index bfa20dc..c1b48de 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -231,8 +231,7 @@ Do not encode type tags in BTF.
 
 .TP
 .B \-\-skip_encoding_btf_inconsistent_proto
-Do not encode functions with multiple inconsistent prototypes or optimized-out
-parameters.
+Do not encode functions with multiple inconsistent prototypes or unexpected register use for their parameters, where the registers used do not match calling conventions.
 
 .TP
 .B \-j, \-\-jobs=N
@@ -269,7 +268,6 @@ information has float types.
 .TP
 .B \-\-btf_gen_optimized
 Generate BTF for functions with optimization-related suffixes (.isra, .constprop).
-BTF will only be generated if a function does not optimize out parameters.
 
 .TP
 .B \-\-btf_gen_all
-- 
2.31.1

Re: [RFC dwarves 0/4] dwarves: change BTF encoding skip logic for functions

[Arnaldo Carvalho de Melo]

Em Fri, Feb 17, 2023 at 11:10:29PM +0000, Alan Maguire escreveu:
> 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.

This sounds sensible at first sight, I've applied the patches so that it
can go thru further testing on the libbpf CI, for now its just on the
'next' branch, the testing I did so far:

make allmodconfig + enablig BTF on bpf-next reverting that revert so
that we use the new options,

Now I'm building and booting a kernel with a fedora-ish config without
using the new options and then with them (reverting the revert) to
compare the tools/testing/selftests/bpf/ ./test-progs output
before/after.

Its an extended holiday down here, so I'll be spotty but want to get
this moving forward,

Thanks!

- Arnaldo
 
> 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@mail.gmail.com/
> 
> 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
> 
>  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
> 

-- 

- Arnaldo

Re: [RFC dwarves 0/4] dwarves: change BTF encoding skip logic for functions

[Arnaldo Carvalho de Melo]

Em Sat, Feb 18, 2023 at 11:12:50AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Feb 17, 2023 at 11:10:29PM +0000, Alan Maguire escreveu:
> > 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.
> 
> This sounds sensible at first sight, I've applied the patches so that it
> can go thru further testing on the libbpf CI, for now its just on the
> 'next' branch, the testing I did so far:
> 
> make allmodconfig + enablig BTF on bpf-next reverting that revert so
> that we use the new options,
> 
> Now I'm building and booting a kernel with a fedora-ish config without
> using the new options and then with them (reverting the revert) to
> compare the tools/testing/selftests/bpf/ ./test-progs output
> before/after.

-Summary: 274/1609 PASSED, 24 SKIPPED, 17 FAILED
+Summary: 276/1612 PASSED, 24 SKIPPED, 15 FAILED

Well, we're passing _more_ tests :-)

And no messages about that kernel module, etc. Will redo with clang as
time permits.

- Arnaldo
 
> Its an extended holiday down here, so I'll be spotty but want to get
> this moving forward,
> 
> Thanks!
> 
> - Arnaldo
>  
> > 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@mail.gmail.com/
> > 
> > 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
> > 
> >  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
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [RFC dwarves 0/4] dwarves: change BTF encoding skip logic for functions

[Jiri Olsa]

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@mail.gmail.com/
> 
> 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@kernel.org>

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
> 

Re: [RFC dwarves 1/4] dwarf_loader: mark functions that do not use expected registers for params

[Alexei Starovoitov]

On Fri, Feb 17, 2023 at 11:10:30PM +0000, Alan Maguire wrote:
> Calling conventions dictate which registers are used for
> function parameters.
> 
> When a function is optimized however, we need to ensure that
> the non-optimized parameters do not violate expectations about
> register use as this would violate expectations for tracing.
> At CU initialization, create a mapping from parameter index
> to expected DW_OP_reg, and use it to validate parameters
> match with expectations.  A parameter which is passed via
> the stack, as a constant, or uses an unexpected register,
> violates these expectations and it (and the associated
> function) are marked as having unexpected register mapping.
> 
> Note though that there is as exception here that needs to
> be handled; when a (typedef) struct is passed as a parameter,
> it can use multiple registers so will throw off later register
> expectations.  Exempt functions that have unexpected
> register usage _and_ struct parameters (examples are found
> in the "tracing_struct" test).
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++---
>  dwarves.h      |   5 +++
>  2 files changed, 109 insertions(+), 5 deletions(-)
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index acdb68d..014e130 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -1022,6 +1022,51 @@ static int arch__nr_register_params(const GElf_Ehdr *ehdr)
>  	return 0;
>  }
>  
> +/* map from parameter index (0 for first, ...) to expected DW_OP_reg.
> + * This will allow us to identify cases where optimized-out parameters
> + * interfere with expectations about register contents on function
> + * entry.
> + */
> +static void arch__set_register_params(const GElf_Ehdr *ehdr, struct cu *cu)
> +{
> +	memset(cu->register_params, -1, sizeof(cu->register_params));
> +
> +	switch (ehdr->e_machine) {
> +	case EM_S390:
> +		/* https://github.com/IBM/s390x-abi/releases/download/v1.6/lzsabi_s390x.pdf */
> +		cu->register_params[0] = DW_OP_reg2;	// %r2
> +		cu->register_params[1] = DW_OP_reg3;	// %r3
> +		cu->register_params[2] = DW_OP_reg4;	// %r4
> +		cu->register_params[3] = DW_OP_reg5;	// %r5
> +		cu->register_params[4] = DW_OP_reg6;	// %r6
> +		return;
> +	case EM_X86_64:
> +		/* //en.wikipedia.org/wiki/X86_calling_conventions#System_V_AMD64_ABI */
> +		cu->register_params[0] = DW_OP_reg5;	// %rdi
> +		cu->register_params[1] = DW_OP_reg4;	// %rsi
> +		cu->register_params[2] = DW_OP_reg1;	// %rdx
> +		cu->register_params[3] = DW_OP_reg2;	// %rcx
> +		cu->register_params[4] = DW_OP_reg8;	// %r8
> +		cu->register_params[5] = DW_OP_reg9;	// %r9
> +		return;
> +	case EM_ARM:
> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers */
> +	case EM_AARCH64:
> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers */
> +		cu->register_params[0] = DW_OP_reg0;
> +		cu->register_params[1] = DW_OP_reg1;
> +		cu->register_params[2] = DW_OP_reg2;
> +		cu->register_params[3] = DW_OP_reg3;
> +		cu->register_params[4] = DW_OP_reg4;
> +		cu->register_params[5] = DW_OP_reg5;
> +		cu->register_params[6] = DW_OP_reg6;
> +		cu->register_params[7] = DW_OP_reg7;
> +		return;
> +	default:
> +		return;
> +	}
> +}
> +
>  static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  					struct conf_load *conf, int param_idx)
>  {
> @@ -1075,18 +1120,28 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>  		if (parm->has_loc &&
>  		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
>  			loc.exprlen != 0) {
> +			int expected_reg = cu->register_params[param_idx];
>  			Dwarf_Op *expr = loc.expr;
>  
>  			switch (expr->atom) {
>  			case DW_OP_reg0 ... DW_OP_reg31:
> +				/* mark parameters that use an unexpected
> +				 * register to hold a parameter; these will
> +				 * be problematic for users of BTF as they
> +				 * violate expectations about register
> +				 * contents.
> +				 */
> +				if (expected_reg >= 0 && expected_reg != expr->atom)
> +					parm->unexpected_reg = 1;
> +				break;

Overall I guess it's a step forward, since it addresses the immediate issue,
but probably too fragile long term.

Your earlier example:
 __bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked)

had
0x0891dabe:     DW_TAG_formal_parameter
                  DW_AT_location        (indexed (0x7a) loclist = 0x00f50eb1:
                     [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI
                     [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX
                     [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI
                     [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX
                     [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)

0x0891dad4:     DW_TAG_formal_parameter
                  DW_AT_location        (indexed (0x7b) loclist = 0x00f50eda:
                     [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX
                     [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX
                     [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX)
                  DW_AT_name    ("acked")

Both args will fail above check. If I'm reading above code correctly.
It checks that every reg in DW_AT_location matches ?
Or just first ?

>  			case DW_OP_breg0 ... DW_OP_breg31:
>  				break;
>  			default:
> -				parm->optimized = 1;
> +				parm->unexpected_reg = 1;
>  				break;
>  			}
>  		} else if (has_const_value) {
> -			parm->optimized = 1;
> +			parm->unexpected_reg = 1;

Is this part too restrictive as well?
Just because one arg is constant it doesn't mean that the calling convention
is not correct for this and other args.

Re: [RFC dwarves 1/4] dwarf_loader: mark functions that do not use expected registers for params

[Alan Maguire]

On 20/02/2023 19:03, Alexei Starovoitov wrote:
> On Fri, Feb 17, 2023 at 11:10:30PM +0000, Alan Maguire wrote:
>> Calling conventions dictate which registers are used for
>> function parameters.
>>
>> When a function is optimized however, we need to ensure that
>> the non-optimized parameters do not violate expectations about
>> register use as this would violate expectations for tracing.
>> At CU initialization, create a mapping from parameter index
>> to expected DW_OP_reg, and use it to validate parameters
>> match with expectations.  A parameter which is passed via
>> the stack, as a constant, or uses an unexpected register,
>> violates these expectations and it (and the associated
>> function) are marked as having unexpected register mapping.
>>
>> Note though that there is as exception here that needs to
>> be handled; when a (typedef) struct is passed as a parameter,
>> it can use multiple registers so will throw off later register
>> expectations.  Exempt functions that have unexpected
>> register usage _and_ struct parameters (examples are found
>> in the "tracing_struct" test).
>>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>>  dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++---
>>  dwarves.h      |   5 +++
>>  2 files changed, 109 insertions(+), 5 deletions(-)
>>
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index acdb68d..014e130 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -1022,6 +1022,51 @@ static int arch__nr_register_params(const GElf_Ehdr *ehdr)
>>  	return 0;
>>  }
>>  
>> +/* map from parameter index (0 for first, ...) to expected DW_OP_reg.
>> + * This will allow us to identify cases where optimized-out parameters
>> + * interfere with expectations about register contents on function
>> + * entry.
>> + */
>> +static void arch__set_register_params(const GElf_Ehdr *ehdr, struct cu *cu)
>> +{
>> +	memset(cu->register_params, -1, sizeof(cu->register_params));
>> +
>> +	switch (ehdr->e_machine) {
>> +	case EM_S390:
>> +		/* https://github.com/IBM/s390x-abi/releases/download/v1.6/lzsabi_s390x.pdf */
>> +		cu->register_params[0] = DW_OP_reg2;	// %r2
>> +		cu->register_params[1] = DW_OP_reg3;	// %r3
>> +		cu->register_params[2] = DW_OP_reg4;	// %r4
>> +		cu->register_params[3] = DW_OP_reg5;	// %r5
>> +		cu->register_params[4] = DW_OP_reg6;	// %r6
>> +		return;
>> +	case EM_X86_64:
>> +		/* //en.wikipedia.org/wiki/X86_calling_conventions#System_V_AMD64_ABI */
>> +		cu->register_params[0] = DW_OP_reg5;	// %rdi
>> +		cu->register_params[1] = DW_OP_reg4;	// %rsi
>> +		cu->register_params[2] = DW_OP_reg1;	// %rdx
>> +		cu->register_params[3] = DW_OP_reg2;	// %rcx
>> +		cu->register_params[4] = DW_OP_reg8;	// %r8
>> +		cu->register_params[5] = DW_OP_reg9;	// %r9
>> +		return;
>> +	case EM_ARM:
>> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers */
>> +	case EM_AARCH64:
>> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers */
>> +		cu->register_params[0] = DW_OP_reg0;
>> +		cu->register_params[1] = DW_OP_reg1;
>> +		cu->register_params[2] = DW_OP_reg2;
>> +		cu->register_params[3] = DW_OP_reg3;
>> +		cu->register_params[4] = DW_OP_reg4;
>> +		cu->register_params[5] = DW_OP_reg5;
>> +		cu->register_params[6] = DW_OP_reg6;
>> +		cu->register_params[7] = DW_OP_reg7;
>> +		return;
>> +	default:
>> +		return;
>> +	}
>> +}
>> +
>>  static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>  					struct conf_load *conf, int param_idx)
>>  {
>> @@ -1075,18 +1120,28 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>  		if (parm->has_loc &&
>>  		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
>>  			loc.exprlen != 0) {
>> +			int expected_reg = cu->register_params[param_idx];
>>  			Dwarf_Op *expr = loc.expr;
>>  
>>  			switch (expr->atom) {
>>  			case DW_OP_reg0 ... DW_OP_reg31:
>> +				/* mark parameters that use an unexpected
>> +				 * register to hold a parameter; these will
>> +				 * be problematic for users of BTF as they
>> +				 * violate expectations about register
>> +				 * contents.
>> +				 */
>> +				if (expected_reg >= 0 && expected_reg != expr->atom)
>> +					parm->unexpected_reg = 1;
>> +				break;
> 
> Overall I guess it's a step forward, since it addresses the immediate issue,
> but probably too fragile long term.
> 
> Your earlier example:
>  __bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> 
> had
> 0x0891dabe:     DW_TAG_formal_parameter
>                   DW_AT_location        (indexed (0x7a) loclist = 0x00f50eb1:
>                      [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI
>                      [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX
>                      [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI
>                      [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX
>                      [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
> 
> 0x0891dad4:     DW_TAG_formal_parameter
>                   DW_AT_location        (indexed (0x7b) loclist = 0x00f50eda:
>                      [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX
>                      [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX
>                      [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX)
>                   DW_AT_name    ("acked")
> 
> Both args will fail above check. If I'm reading above code correctly.
> It checks that every reg in DW_AT_location matches ?

It checks location info for those that have it; so in this case the location
lists specify rdi on entry for the first parameter (sk)


0x068a0f3b:     DW_TAG_formal_parameter
                  DW_AT_location        (indexed (0x74) loclist = 0x00a4c5a0:
                     [0xffffffff81b87849, 0xffffffff81b87866): DW_OP_reg5 RDI
                     [0xffffffff81b87866, 0xffffffff81b87899): DW_OP_reg3 RBX
                     [0xffffffff81b87899, 0xffffffff81b878a0): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
                  DW_AT_name    ("sk")
                  DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
                  DW_AT_decl_line       (446)
                  DW_AT_type    (0x06886461 "sock *")


no location info for the second (ack):

0x068a0f47:     DW_TAG_formal_parameter
                  DW_AT_name    ("ack")
                  DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
                  DW_AT_decl_line       (446)
                  DW_AT_type    (0x06886451 "u32")

...so matching it is skipped, and rdx as the first element in the location list
for the third parameter (acked):

0x068a0f52:     DW_TAG_formal_parameter
                  DW_AT_location        (indexed (0x75) loclist = 0x00a4c5bb:
                     [0xffffffff81b87849, 0xffffffff81b87884): DW_OP_reg1 RDX
                     [0xffffffff81b87884, 0xffffffff81b87890): DW_OP_reg0 RAX
                     [0xffffffff81b87890, 0xffffffff81b87898): DW_OP_reg1 RDX)
                  DW_AT_name    ("acked")
                  DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
                  DW_AT_decl_line       (446)
                  DW_AT_type    (0x06886451 "u32")


So this would be okay using the register-checking approach.

> Or just first ?
> 
>>  			case DW_OP_breg0 ... DW_OP_breg31:
>>  				break;
>>  			default:
>> -				parm->optimized = 1;
>> +				parm->unexpected_reg = 1;
>>  				break;
>>  			}
>>  		} else if (has_const_value) {
>> -			parm->optimized = 1;
>> +			parm->unexpected_reg = 1;
> 
> Is this part too restrictive as well?
> Just because one arg is constant it doesn't mean that the calling convention
> is not correct for this and other args.
> 

Great catch; this part is wrong; should just be parm->optimized = 1 as it
was before.

Re: [RFC dwarves 1/4] dwarf_loader: mark functions that do not use expected registers for params

[Alexei Starovoitov]

On Mon, Feb 20, 2023 at 07:42:15PM +0000, Alan Maguire wrote:
> On 20/02/2023 19:03, Alexei Starovoitov wrote:
> > On Fri, Feb 17, 2023 at 11:10:30PM +0000, Alan Maguire wrote:
> >> Calling conventions dictate which registers are used for
> >> function parameters.
> >>
> >> When a function is optimized however, we need to ensure that
> >> the non-optimized parameters do not violate expectations about
> >> register use as this would violate expectations for tracing.
> >> At CU initialization, create a mapping from parameter index
> >> to expected DW_OP_reg, and use it to validate parameters
> >> match with expectations.  A parameter which is passed via
> >> the stack, as a constant, or uses an unexpected register,
> >> violates these expectations and it (and the associated
> >> function) are marked as having unexpected register mapping.
> >>
> >> Note though that there is as exception here that needs to
> >> be handled; when a (typedef) struct is passed as a parameter,
> >> it can use multiple registers so will throw off later register
> >> expectations.  Exempt functions that have unexpected
> >> register usage _and_ struct parameters (examples are found
> >> in the "tracing_struct" test).
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >> ---
> >>  dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++---
> >>  dwarves.h      |   5 +++
> >>  2 files changed, 109 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/dwarf_loader.c b/dwarf_loader.c
> >> index acdb68d..014e130 100644
> >> --- a/dwarf_loader.c
> >> +++ b/dwarf_loader.c
> >> @@ -1022,6 +1022,51 @@ static int arch__nr_register_params(const GElf_Ehdr *ehdr)
> >>  	return 0;
> >>  }
> >>  
> >> +/* map from parameter index (0 for first, ...) to expected DW_OP_reg.
> >> + * This will allow us to identify cases where optimized-out parameters
> >> + * interfere with expectations about register contents on function
> >> + * entry.
> >> + */
> >> +static void arch__set_register_params(const GElf_Ehdr *ehdr, struct cu *cu)
> >> +{
> >> +	memset(cu->register_params, -1, sizeof(cu->register_params));
> >> +
> >> +	switch (ehdr->e_machine) {
> >> +	case EM_S390:
> >> +		/* https://github.com/IBM/s390x-abi/releases/download/v1.6/lzsabi_s390x.pdf */
> >> +		cu->register_params[0] = DW_OP_reg2;	// %r2
> >> +		cu->register_params[1] = DW_OP_reg3;	// %r3
> >> +		cu->register_params[2] = DW_OP_reg4;	// %r4
> >> +		cu->register_params[3] = DW_OP_reg5;	// %r5
> >> +		cu->register_params[4] = DW_OP_reg6;	// %r6
> >> +		return;
> >> +	case EM_X86_64:
> >> +		/* //en.wikipedia.org/wiki/X86_calling_conventions#System_V_AMD64_ABI */
> >> +		cu->register_params[0] = DW_OP_reg5;	// %rdi
> >> +		cu->register_params[1] = DW_OP_reg4;	// %rsi
> >> +		cu->register_params[2] = DW_OP_reg1;	// %rdx
> >> +		cu->register_params[3] = DW_OP_reg2;	// %rcx
> >> +		cu->register_params[4] = DW_OP_reg8;	// %r8
> >> +		cu->register_params[5] = DW_OP_reg9;	// %r9
> >> +		return;
> >> +	case EM_ARM:
> >> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers */
> >> +	case EM_AARCH64:
> >> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers */
> >> +		cu->register_params[0] = DW_OP_reg0;
> >> +		cu->register_params[1] = DW_OP_reg1;
> >> +		cu->register_params[2] = DW_OP_reg2;
> >> +		cu->register_params[3] = DW_OP_reg3;
> >> +		cu->register_params[4] = DW_OP_reg4;
> >> +		cu->register_params[5] = DW_OP_reg5;
> >> +		cu->register_params[6] = DW_OP_reg6;
> >> +		cu->register_params[7] = DW_OP_reg7;
> >> +		return;
> >> +	default:
> >> +		return;
> >> +	}
> >> +}
> >> +
> >>  static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> >>  					struct conf_load *conf, int param_idx)
> >>  {
> >> @@ -1075,18 +1120,28 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
> >>  		if (parm->has_loc &&
> >>  		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
> >>  			loc.exprlen != 0) {
> >> +			int expected_reg = cu->register_params[param_idx];
> >>  			Dwarf_Op *expr = loc.expr;
> >>  
> >>  			switch (expr->atom) {
> >>  			case DW_OP_reg0 ... DW_OP_reg31:
> >> +				/* mark parameters that use an unexpected
> >> +				 * register to hold a parameter; these will
> >> +				 * be problematic for users of BTF as they
> >> +				 * violate expectations about register
> >> +				 * contents.
> >> +				 */
> >> +				if (expected_reg >= 0 && expected_reg != expr->atom)
> >> +					parm->unexpected_reg = 1;
> >> +				break;
> > 
> > Overall I guess it's a step forward, since it addresses the immediate issue,
> > but probably too fragile long term.
> > 
> > Your earlier example:
> >  __bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked)
> > 
> > had
> > 0x0891dabe:     DW_TAG_formal_parameter
> >                   DW_AT_location        (indexed (0x7a) loclist = 0x00f50eb1:
> >                      [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI
> >                      [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX
> >                      [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI
> >                      [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX
> >                      [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
> > 
> > 0x0891dad4:     DW_TAG_formal_parameter
> >                   DW_AT_location        (indexed (0x7b) loclist = 0x00f50eda:
> >                      [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX
> >                      [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX
> >                      [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX)
> >                   DW_AT_name    ("acked")
> > 
> > Both args will fail above check. If I'm reading above code correctly.
> > It checks that every reg in DW_AT_location matches ?
> 
> It checks location info for those that have it; so in this case the location
> lists specify rdi on entry for the first parameter (sk)
> 
> 
> 0x068a0f3b:     DW_TAG_formal_parameter
>                   DW_AT_location        (indexed (0x74) loclist = 0x00a4c5a0:
>                      [0xffffffff81b87849, 0xffffffff81b87866): DW_OP_reg5 RDI
>                      [0xffffffff81b87866, 0xffffffff81b87899): DW_OP_reg3 RBX
>                      [0xffffffff81b87899, 0xffffffff81b878a0): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
>                   DW_AT_name    ("sk")
>                   DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
>                   DW_AT_decl_line       (446)
>                   DW_AT_type    (0x06886461 "sock *")
> 
> 
> no location info for the second (ack):
> 
> 0x068a0f47:     DW_TAG_formal_parameter
>                   DW_AT_name    ("ack")
>                   DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
>                   DW_AT_decl_line       (446)
>                   DW_AT_type    (0x06886451 "u32")
> 
> ...so matching it is skipped, and rdx as the first element in the location list
> for the third parameter (acked):
> 
> 0x068a0f52:     DW_TAG_formal_parameter
>                   DW_AT_location        (indexed (0x75) loclist = 0x00a4c5bb:
>                      [0xffffffff81b87849, 0xffffffff81b87884): DW_OP_reg1 RDX
>                      [0xffffffff81b87884, 0xffffffff81b87890): DW_OP_reg0 RAX
>                      [0xffffffff81b87890, 0xffffffff81b87898): DW_OP_reg1 RDX)
>                   DW_AT_name    ("acked")
>                   DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
>                   DW_AT_decl_line       (446)
>                   DW_AT_type    (0x06886451 "u32")
> 
> 
> So this would be okay using the register-checking approach.

I meant in all that it's not clear that first and only first location info is used.
Is that the behavior of attr_location() ?


> > Or just first ?
> > 
> >>  			case DW_OP_breg0 ... DW_OP_breg31:
> >>  				break;
> >>  			default:
> >> -				parm->optimized = 1;
> >> +				parm->unexpected_reg = 1;
> >>  				break;
> >>  			}
> >>  		} else if (has_const_value) {
> >> -			parm->optimized = 1;
> >> +			parm->unexpected_reg = 1;
> > 
> > Is this part too restrictive as well?
> > Just because one arg is constant it doesn't mean that the calling convention
> > is not correct for this and other args.
> > 
> 
> Great catch; this part is wrong; should just be parm->optimized = 1 as it
> was before.

Will this fix change the stats you've quoted earlier ?

"
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
"

How does it compare before/after ?
iirc there were ~2500 functions skipped in vmlinux-gcc and now it's down to 1164+331 ?

Re: [RFC dwarves 1/4] dwarf_loader: mark functions that do not use expected registers for params

[Alan Maguire]

On 20/02/2023 22:30, Alexei Starovoitov wrote:
> On Mon, Feb 20, 2023 at 07:42:15PM +0000, Alan Maguire wrote:
>> On 20/02/2023 19:03, Alexei Starovoitov wrote:
>>> On Fri, Feb 17, 2023 at 11:10:30PM +0000, Alan Maguire wrote:
>>>> Calling conventions dictate which registers are used for
>>>> function parameters.
>>>>
>>>> When a function is optimized however, we need to ensure that
>>>> the non-optimized parameters do not violate expectations about
>>>> register use as this would violate expectations for tracing.
>>>> At CU initialization, create a mapping from parameter index
>>>> to expected DW_OP_reg, and use it to validate parameters
>>>> match with expectations.  A parameter which is passed via
>>>> the stack, as a constant, or uses an unexpected register,
>>>> violates these expectations and it (and the associated
>>>> function) are marked as having unexpected register mapping.
>>>>
>>>> Note though that there is as exception here that needs to
>>>> be handled; when a (typedef) struct is passed as a parameter,
>>>> it can use multiple registers so will throw off later register
>>>> expectations.  Exempt functions that have unexpected
>>>> register usage _and_ struct parameters (examples are found
>>>> in the "tracing_struct" test).
>>>>
>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>> ---
>>>>  dwarf_loader.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++---
>>>>  dwarves.h      |   5 +++
>>>>  2 files changed, 109 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>>> index acdb68d..014e130 100644
>>>> --- a/dwarf_loader.c
>>>> +++ b/dwarf_loader.c
>>>> @@ -1022,6 +1022,51 @@ static int arch__nr_register_params(const GElf_Ehdr *ehdr)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +/* map from parameter index (0 for first, ...) to expected DW_OP_reg.
>>>> + * This will allow us to identify cases where optimized-out parameters
>>>> + * interfere with expectations about register contents on function
>>>> + * entry.
>>>> + */
>>>> +static void arch__set_register_params(const GElf_Ehdr *ehdr, struct cu *cu)
>>>> +{
>>>> +	memset(cu->register_params, -1, sizeof(cu->register_params));
>>>> +
>>>> +	switch (ehdr->e_machine) {
>>>> +	case EM_S390:
>>>> +		/* https://github.com/IBM/s390x-abi/releases/download/v1.6/lzsabi_s390x.pdf */
>>>> +		cu->register_params[0] = DW_OP_reg2;	// %r2
>>>> +		cu->register_params[1] = DW_OP_reg3;	// %r3
>>>> +		cu->register_params[2] = DW_OP_reg4;	// %r4
>>>> +		cu->register_params[3] = DW_OP_reg5;	// %r5
>>>> +		cu->register_params[4] = DW_OP_reg6;	// %r6
>>>> +		return;
>>>> +	case EM_X86_64:
>>>> +		/* //en.wikipedia.org/wiki/X86_calling_conventions#System_V_AMD64_ABI */
>>>> +		cu->register_params[0] = DW_OP_reg5;	// %rdi
>>>> +		cu->register_params[1] = DW_OP_reg4;	// %rsi
>>>> +		cu->register_params[2] = DW_OP_reg1;	// %rdx
>>>> +		cu->register_params[3] = DW_OP_reg2;	// %rcx
>>>> +		cu->register_params[4] = DW_OP_reg8;	// %r8
>>>> +		cu->register_params[5] = DW_OP_reg9;	// %r9
>>>> +		return;
>>>> +	case EM_ARM:
>>>> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers */
>>>> +	case EM_AARCH64:
>>>> +		/* https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers */
>>>> +		cu->register_params[0] = DW_OP_reg0;
>>>> +		cu->register_params[1] = DW_OP_reg1;
>>>> +		cu->register_params[2] = DW_OP_reg2;
>>>> +		cu->register_params[3] = DW_OP_reg3;
>>>> +		cu->register_params[4] = DW_OP_reg4;
>>>> +		cu->register_params[5] = DW_OP_reg5;
>>>> +		cu->register_params[6] = DW_OP_reg6;
>>>> +		cu->register_params[7] = DW_OP_reg7;
>>>> +		return;
>>>> +	default:
>>>> +		return;
>>>> +	}
>>>> +}
>>>> +
>>>>  static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>>  					struct conf_load *conf, int param_idx)
>>>>  {
>>>> @@ -1075,18 +1120,28 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu,
>>>>  		if (parm->has_loc &&
>>>>  		    attr_location(die, &loc.expr, &loc.exprlen) == 0 &&
>>>>  			loc.exprlen != 0) {
>>>> +			int expected_reg = cu->register_params[param_idx];
>>>>  			Dwarf_Op *expr = loc.expr;
>>>>  
>>>>  			switch (expr->atom) {
>>>>  			case DW_OP_reg0 ... DW_OP_reg31:
>>>> +				/* mark parameters that use an unexpected
>>>> +				 * register to hold a parameter; these will
>>>> +				 * be problematic for users of BTF as they
>>>> +				 * violate expectations about register
>>>> +				 * contents.
>>>> +				 */
>>>> +				if (expected_reg >= 0 && expected_reg != expr->atom)
>>>> +					parm->unexpected_reg = 1;
>>>> +				break;
>>>
>>> Overall I guess it's a step forward, since it addresses the immediate issue,
>>> but probably too fragile long term.
>>>
>>> Your earlier example:
>>>  __bpf_kfunc void tcp_reno_cong_avoid(struct sock *sk, u32 ack, u32 acked)
>>>
>>> had
>>> 0x0891dabe:     DW_TAG_formal_parameter
>>>                   DW_AT_location        (indexed (0x7a) loclist = 0x00f50eb1:
>>>                      [0xffffffff82031185, 0xffffffff8203119e): DW_OP_reg5 RDI
>>>                      [0xffffffff8203119e, 0xffffffff820311cc): DW_OP_reg3 RBX
>>>                      [0xffffffff820311cc, 0xffffffff820311d1): DW_OP_reg5 RDI
>>>                      [0xffffffff820311d1, 0xffffffff820311d2): DW_OP_reg3 RBX
>>>                      [0xffffffff820311d2, 0xffffffff820311d8): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
>>>
>>> 0x0891dad4:     DW_TAG_formal_parameter
>>>                   DW_AT_location        (indexed (0x7b) loclist = 0x00f50eda:
>>>                      [0xffffffff82031185, 0xffffffff820311bc): DW_OP_reg1 RDX
>>>                      [0xffffffff820311bc, 0xffffffff820311c8): DW_OP_reg0 RAX
>>>                      [0xffffffff820311c8, 0xffffffff820311d1): DW_OP_reg1 RDX)
>>>                   DW_AT_name    ("acked")
>>>
>>> Both args will fail above check. If I'm reading above code correctly.
>>> It checks that every reg in DW_AT_location matches ?
>>
>> It checks location info for those that have it; so in this case the location
>> lists specify rdi on entry for the first parameter (sk)
>>
>>
>> 0x068a0f3b:     DW_TAG_formal_parameter
>>                   DW_AT_location        (indexed (0x74) loclist = 0x00a4c5a0:
>>                      [0xffffffff81b87849, 0xffffffff81b87866): DW_OP_reg5 RDI
>>                      [0xffffffff81b87866, 0xffffffff81b87899): DW_OP_reg3 RBX
>>                      [0xffffffff81b87899, 0xffffffff81b878a0): DW_OP_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value)
>>                   DW_AT_name    ("sk")
>>                   DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
>>                   DW_AT_decl_line       (446)
>>                   DW_AT_type    (0x06886461 "sock *")
>>
>>
>> no location info for the second (ack):
>>
>> 0x068a0f47:     DW_TAG_formal_parameter
>>                   DW_AT_name    ("ack")
>>                   DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
>>                   DW_AT_decl_line       (446)
>>                   DW_AT_type    (0x06886451 "u32")
>>
>> ...so matching it is skipped, and rdx as the first element in the location list
>> for the third parameter (acked):
>>
>> 0x068a0f52:     DW_TAG_formal_parameter
>>                   DW_AT_location        (indexed (0x75) loclist = 0x00a4c5bb:
>>                      [0xffffffff81b87849, 0xffffffff81b87884): DW_OP_reg1 RDX
>>                      [0xffffffff81b87884, 0xffffffff81b87890): DW_OP_reg0 RAX
>>                      [0xffffffff81b87890, 0xffffffff81b87898): DW_OP_reg1 RDX)
>>                   DW_AT_name    ("acked")
>>                   DW_AT_decl_file       ("/home/opc/src/clang/bpf-next/net/ipv4/tcp_cong.c")
>>                   DW_AT_decl_line       (446)
>>                   DW_AT_type    (0x06886451 "u32")
>>
>>
>> So this would be okay using the register-checking approach.
> 
> I meant in all that it's not clear that first and only first location info is used.
> Is that the behavior of attr_location() ?
>

I did a bit of additional debugging here; you're right, attr_location() does not
work for location lists so they are skipped. It uses dwarf_getlocation(), but
if we use dwarf_getlocations() (added in elfutils 0.157), single locations and 
location lists will be supported, and in the case of location lists, the first
expression is examined. See [1] for the details of the fix.

> 
>>> Or just first ?
>>>
>>>>  			case DW_OP_breg0 ... DW_OP_breg31:
>>>>  				break;
>>>>  			default:
>>>> -				parm->optimized = 1;
>>>> +				parm->unexpected_reg = 1;
>>>>  				break;
>>>>  			}
>>>>  		} else if (has_const_value) {
>>>> -			parm->optimized = 1;
>>>> +			parm->unexpected_reg = 1;
>>>
>>> Is this part too restrictive as well?
>>> Just because one arg is constant it doesn't mean that the calling convention
>>> is not correct for this and other args.
>>>
>>
>> Great catch; this part is wrong; should just be parm->optimized = 1 as it
>> was before.
> 
> Will this fix change the stats you've quoted earlier ?
> 
> "
> 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
>

This changes to

- 1084 functions skipped due to inconsistent function prototypes,
  597 of these are not "."-suffixed functions.
- 255 functions skipped due to unexpected register usage, of which
  only 16 are not "."-suffixed functions.

Overall it's 1339 functions, which is slightly less than
the 1495 we skipped before.
 
> For a clang-built kernel, the numbers are
> 
> - 539 functions with inconsistent prototypes are skipped
> - 209 functions with unexpected register usage are skipped
> "
> 
> How does it compare before/after ?
> iirc there were ~2500 functions skipped in vmlinux-gcc and now it's down to 1164+331 ?
> 
For clang we see

- 539 functions skipped due to inconsistent prototypes (unchanged)
- 272 functions skipped due to unexpected register usage

The increase here appears to be a result of supporting location
lists so we can do more checking on which registers parameters use.

For many of the 272 we see %r15 unexpectedly being used for boolean 
parameters. In some cases the __cold attribute is applied to functions
and they end up using atypical registers. And in some cases -
for example ZSTD_execSequenceEnd() - we see a stack-passed
parameter leading to later parameters using unexpected registers.
In other cases we see the fact that a function does not use
a parameter, resulting in unexpected register use for other
parameters  - __ddebug_add_module() is an example of this, where
the second parameter is unused in the code, so the third uses %rsi.
These are I think the cases we'd really like to catch.

I've sent a small series of fixups to address these issues [1].

Testing indicates we see no issues with missing BTF ids, and the
tracing_struct test still passes.

[1] https://lore.kernel.org/bpf/1676994522-1557-1-git-send-email-alan.maguire@oracle.com/