Re: [RFC bpf-next 0/3] bpf: handle 0-sized structs properly

[Alan Maguire <alan.maguire@oracle.com>]

On 15/05/2025 11:56, Alan Maguire wrote:
> On 09/05/2025 19:40, Andrii Nakryiko wrote:
>> On Thu, May 8, 2025 at 6:22 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>
>>> When testing v1 of [1] we noticed that functions with 0-sized structs
>>> as parameters were not part of BTF encoding; this was fixed in v2.
>>> However we need to make sure we handle such zero-sized structs
>>> correctly since they confound the calling convention expectations -
>>> no registers are used for the empty struct so this has knock-on effects
>>> for subsequent register-parameter matching.
>>
>> Do you have a list (or at least an example) of the function we are
>> talking about, just curious to see what's that.
>>
>> The question I have is whether it's safe to assume that regardless of
>> architecture we can assume that zero-sized struct has no effect on
>> register allocation (which would seem logical, but is that true for
>> all ABIs).
>>
> 
> I've been investigating this a bit, specifically in the context of s390
> where we saw the test failure. The actual kernel function where I first
> observed the zero-sized struct in practice is
> 
> static int __io_run_local_work(struct io_ring_ctx *ctx, io_tw_token_t
> tw, int min_events, int max_events);
> 
> In s390 DWARF, we see the following representation for it:
> 
>  <1><6f7f788>: Abbrev Number: 104 (DW_TAG_subprogram)
>     <6f7f789>   DW_AT_name        : (indirect string, offset: 0x2c47f5):
> __io_run_local_work
>     <6f7f78d>   DW_AT_decl_file   : 1
>     <6f7f78e>   DW_AT_decl_line   : 1301
>     <6f7f790>   DW_AT_decl_column : 12
>     <6f7f791>   DW_AT_prototyped  : 1
>     <6f7f791>   DW_AT_type        : <0x6f413a2>
>     <6f7f795>   DW_AT_low_pc      : 0x99c850
>     <6f7f79d>   DW_AT_high_pc     : 0x2b2
>     <6f7f7a5>   DW_AT_frame_base  : 1 byte block: 9c
> (DW_OP_call_frame_cfa)
>     <6f7f7a7>   DW_AT_GNU_all_call_sites: 1
>     <6f7f7a7>   DW_AT_sibling     : <0x6f802e6>
>  <2><6f7f7ab>: Abbrev Number: 53 (DW_TAG_formal_parameter)
>     <6f7f7ac>   DW_AT_name        : ctx
>     <6f7f7b0>   DW_AT_decl_file   : 1
>     <6f7f7b1>   DW_AT_decl_line   : 1301
>     <6f7f7b3>   DW_AT_decl_column : 52
>     <6f7f7b4>   DW_AT_type        : <0x6f6882b>
>     <6f7f7b8>   DW_AT_location    : 0x2babcbe (location list)
>     <6f7f7bc>   DW_AT_GNU_locviews: 0x2babcac
>  <2><6f7f7c0>: Abbrev Number: 135 (DW_TAG_formal_parameter)
>     <6f7f7c2>   DW_AT_name        : tw
>     <6f7f7c5>   DW_AT_decl_file   : 1
>     <6f7f7c6>   DW_AT_decl_line   : 1301
>     <6f7f7c8>   DW_AT_decl_column : 71
>     <6f7f7c9>   DW_AT_type        : <0x6f6833e>
>     <6f7f7cd>   DW_AT_location    : 2 byte block: 73 0  (DW_OP_breg3
> (r3): 0)
> 
> 
> ..i.e. we are using the expected calling-convention register (r3) here
> for the zero-sized struct parameter.
> 
> Contrast this with x86_64 and aarch64, where regardless of -O level we
> appear to use an offset from the frame ptr to reference the zero-sized
> struct. As a result the next parameter after the zero-sized struct uses
> the next available calling-convention register (%rdi if the zero-sized
> struct is the first arg, %rsi if it was the second etc) that was unused
> by the zero-sized struct parameter.
> 
> I don't see anything in the ABI specs which covers this scenario
> exactly; I suspect the 0-sized object handling in cases other than s390
> is just using the usual > register size aggregate object handling
> (passing a large struct as a parameter), and in s390 it's not.
> 
> So long story short, we may need to take an arch-specific approach here
> unfortunately. Great that CI flagged this as an issue too!
> 
> Alan
> 
> 

I discussed this with Jose, and the gcc behaviour with zero-sized
structs varies a bit between architectures. Given that complexity, my
inclination would be to class functions with 0-sized struct parameters
as having inconsistent representations. They can then be tackled by
adding location info on a per-site basis later as part of the
inline-related work. For now we would just not emit BTF for them, since
without that site-specific analysis we can't be sure from function
signature alone where parameters are stored. In practice this means
leaving one function out of kernel BTF.

So long story short, I think it might make sense to withdraw this series
for now and see if we can tweak Tony's patch to class functions with
0-sized parameters as inconsistent as the v1 version did, meaning they
don't get a BTF representation. Thanks!

Alan

> 
> 
>> BTW, while looking at patch #2, I noticed that
>> btf_distill_func_proto() disallows functions returning
>> struct-by-value, which seems overly aggressive, at least for structs
>> of up to 8 bytes. So maybe if we can validate that both cases are not
>> introducing any new quirks across all supported architectures, we can
>> solve both limitations?
>>
>> P.S., oh, and s390x selftest (test_struct_args) isn't happy, please check.
>>
>>
>>>
>>> Patch 1 updates BPF_PROG2() to handle the zero-sized struct case.
>>> Patch 2 makes 0-sized structs a special case, allowing them to exist
>>> as parameter representations in BTF without failing verification.
>>> Patch 3 is a selftest that ensures the parameters after the 0-sized
>>> struct are represented correctly.
>>>
>>> [1] https://lore.kernel.org/dwarves/20250502070318.1561924-1-tony.ambardar@gmail.com/
>>>
>>> Alan Maguire (3):
>>>   libbpf: update BPF_PROG2() to handle empty structs
>>>   bpf: allow 0-sized structs as function parameters
>>>   selftests/bpf: add 0-length struct testing to tracing_struct tests
>>>
>>>  kernel/bpf/btf.c                                     |  2 +-
>>>  tools/lib/bpf/bpf_tracing.h                          |  6 ++++--
>>>  .../selftests/bpf/prog_tests/tracing_struct.c        |  2 ++
>>>  tools/testing/selftests/bpf/progs/tracing_struct.c   | 11 +++++++++++
>>>  tools/testing/selftests/bpf/test_kmods/bpf_testmod.c | 12 ++++++++++++
>>>  5 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> --
>>> 2.39.3
>>>
> 
>