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@xxxxxxxxxx>
>> ---
>> 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.
