Re: [PATCH RFC bpf-next 1/4] bpf: add struct largest member size in func model

[Xu Kuohai <xukuohai@huaweicloud.com>]

On 4/21/2025 12:02 AM, Alexis Lothoré wrote:
> Hi Xu,
> 
> On Thu Apr 17, 2025 at 4:10 PM CEST, Xu Kuohai wrote:
>> On 4/17/2025 3:14 PM, Alexis Lothoré wrote:
>>> Hi Andrii,
>>>
>>> On Wed Apr 16, 2025 at 11:24 PM CEST, Andrii Nakryiko wrote:
>>>> On Fri, Apr 11, 2025 at 1:32 PM Alexis Lothoré (eBPF Foundation)
>>>> <alexis.lothore@bootlin.com> wrote:
> 
> [...]
> 
>>>> I might be missing something, but how can the *size* of the field be
>>>> used to calculate that argument's *alignment*? i.e., I don't
>>>> understand why arg_largest_member_size needs to be calculated instead
>>>> of arg_largest_member_alignment...
>>>
>>> Indeed I initially checked whether I could return directly some alignment
>>> info from btf, but it then involves the alignment computation in the btf
>>> module. Since there could be minor differences between architectures about
>>> alignment requirements, I though it would be better to in fact keep alignment
>>> computation out of the btf module. For example, I see that 128 bits values
>>> are aligned on 16 bytes on ARM64, while being aligned on 8 bytes on S390.
>>>
>>> And since for ARM64, all needed alignments are somehow derived from size
>>> (it is either directly size for fundamental types, or alignment of the
>>> largest member for structs, which is then size of largest member),
>>> returning the size seems to be enough to allow the JIT side to compute
>>> alignments.
>>>
>>
>> Not exactly. The compiler's "packed" and "alignment" attributes cause a
>> structure to be aligned differently from its natural alignment.
>>
>> For example, with the following three structures:
>>
>> struct s0 {
>>       __int128 x;
>> };
>>
>> struct s1 {
>>       __int128 x;
>> } __attribute__((packed));
>>
>> struct s2 {
>>       __int128 x;
>> } __attribute__((aligned(64)));
>>
>> Even though the largest member size is the same, s0 will be aligned to 16
>> bytes, s1 and s2 are not aligned the same way. s1 has no alignment due to
>> the "packed" attribute, while s2 will be aligned to 64 bytes.
>>
>> When these three structures are passed as function arguments, they will be
>> located on different positions on the stack.
>>
>> For the following three functions:
>>
>> int f0(__int128 a, __int128 b, __int128 c, int64_t d, __int128 e, int64_t f, struct s0 g);
>> int f1(__int128 a, __int128 b, __int128 c, int64_t d, __int128 e, int64_t f, struct s1 g);
>> int f2(__int128 a, __int128 b, __int128 c, int64_t d, __int128 e, int64_t f, struct s2 g);
>>
>> g will be located at sp+32 in f0, sp + 24 in f1, and some 64-byte aligned
>> stack address in f2.
> 
> Ah, thanks for those clear examples, I completely overlooked this
> possibility. And now that you mention it, I feel a bit dumb because I now
> remember that you mentioned this in Puranjay's series...
> 
> I took a quick look at the x86 JIT compiler for reference, and saw no code
> related to this specific case neither. So I searched in the kernel for
> actual functions taking struct arguments by value AND being declared with some
> packed or aligned attribute. I only found a handful of those, and none
> seems to take enough arguments to have the corresponding struct passed on the
> stack. So rather than supporting this very specific case, I am tempted
> to just return an error for now during trampoline creation if we detect such
> structure (and then the JIT compiler can keep using data size to compute
> alignment, now that it is sure not to receive custom alignments). Or am I
> missing some actual cases involving those very specific alignments ?
> 

How can we reliably 'detect' the case? If a function has such a parameter
but we fail to detect it, the BPF trampoline will pass an incorrect value
to the function, which is also unacceptable.

> Thanks,
> 
> Alexis
>