All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alan Maguire <alan.maguire@oracle.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: martin.lau@linux.dev, ast@kernel.org, andrii@kernel.org,
	tony.ambardar@gmail.com, alexis.lothore@bootlin.com,
	eddyz87@gmail.com, song@kernel.org, yonghong.song@linux.dev,
	john.fastabend@gmail.com, kpsingh@kernel.org, sdf@fomichev.me,
	haoluo@google.com, jolsa@kernel.org, mykolal@fb.com,
	bpf@vger.kernel.org, dwarves@vger.kernel.org
Subject: Re: [RFC bpf-next 0/3] bpf: handle 0-sized structs properly
Date: Thu, 15 May 2025 11:56:01 +0100	[thread overview]
Message-ID: <8faae89d-3515-480c-9abe-4d0e7514e41b@oracle.com> (raw)
In-Reply-To: <CAEf4BzZfFixwy4vQG8jrUBtAOUFx=t1KG2F+AtKPVNCsMz0vQw@mail.gmail.com>

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




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


  parent reply	other threads:[~2025-05-15 10:56 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-08 13:22 [RFC bpf-next 0/3] bpf: handle 0-sized structs properly Alan Maguire
2025-05-08 13:22 ` [RFC bpf-next 1/3] libbpf: update BPF_PROG2() to handle empty structs Alan Maguire
2025-05-08 13:45   ` Alan Maguire
2025-05-08 13:22 ` [RFC bpf-next 2/3] bpf: allow 0-sized structs as function parameters Alan Maguire
2025-05-08 13:22 ` [RFC bpf-next 3/3] selftests/bpf: add 0-length struct testing to tracing_struct tests Alan Maguire
2025-05-09 18:40 ` [RFC bpf-next 0/3] bpf: handle 0-sized structs properly Andrii Nakryiko
2025-05-12  9:17   ` Tony Ambardar
2025-05-14 10:30     ` Alan Maguire
2025-05-14 16:22       ` Andrii Nakryiko
2025-05-15  8:02         ` Tony Ambardar
2025-05-15 16:23           ` Andrii Nakryiko
2025-05-15 10:56   ` Alan Maguire [this message]
2025-05-20  8:59     ` Alan Maguire
2025-05-21  0:58       ` Tony Ambardar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8faae89d-3515-480c-9abe-4d0e7514e41b@oracle.com \
    --to=alan.maguire@oracle.com \
    --cc=alexis.lothore@bootlin.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=dwarves@vger.kernel.org \
    --cc=eddyz87@gmail.com \
    --cc=haoluo@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=mykolal@fb.com \
    --cc=sdf@fomichev.me \
    --cc=song@kernel.org \
    --cc=tony.ambardar@gmail.com \
    --cc=yonghong.song@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.