On 10.07.24 06:45, Stanislav Fomichev wrote:
> On 07/09, Julian Schindel wrote:
>> On 09.07.24 11:23, Magnus Karlsson wrote:
>>> On Sun, 7 Jul 2024 at 17:06, Julian Schindel <mail@xxxxxxxxxxxxxxxx> wrote:
>>>> Hi,
>>>>
>>>> [...]
>>> Thank you for reporting this Julian. This seems to be a bug. If I
>>> check the value of sizeof(struct xdp_umem_reg_v2), I get 32 bytes too
>>> on my system, compiling with gcc 11.4. I am not a compiler guy so do
>>> not know what the rules are for padding structs, but I read the
>>> following from [0]:
>>>
>>> "Pad the entire struct to a multiple of 64-bits if the structure
>>> contains 64-bit types - the structure size will otherwise differ on
>>> 32-bit versus 64-bit. Having a different structure size hurts when
>>> passing arrays of structures to the kernel, or if the kernel checks
>>> the structure size, which e.g. the drm core does."
>>>
>>> I compiled for 64-bits and I believe you did too, but we still get
>>> this padding.
>> Yes, I did also compile for 64-bits. If I understood the resource you
>> linked correctly, the compiler automatically adding padding to align to
>> 64-bit boundaries is expected for 64-bit platforms:
>>
>> "[...] 32-bit platforms don’t necessarily align 64-bit values to 64-bit
>> boundaries, but 64-bit platforms do. So we always need padding to the
>> natural size to get this right."
>>> What is sizeof(struct xdp_umem_reg) for you before the
>>> patch that added tx_metadata_len?
>> I would expect this to be the same as sizeof(struct xdp_umem_reg_v2)
>> after the patch. I'm not sure how to check this with different kernel
>> versions.
>>
>> Maybe the following code helps show all the sizes
>> of xdp_umem_reg[_v1/_v2] on my system (compiled with "gcc test.c -o
>> test" using gcc 14.1.1):
>>
>> #include <stdio.h>
>> #include <sys/types.h>
>>
>> typedef __uint32_t __u32;
>> typedef __uint64_t __u64;
>>
>> struct xdp_umem_reg_v1 {
>> __u64 addr; /* Start of packet data area */
>> __u64 len; /* Length of packet data area */
>> __u32 chunk_size;
>> __u32 headroom;
>> };
>>
>> struct xdp_umem_reg_v2 {
>> __u64 addr; /* Start of packet data area */
>> __u64 len; /* Length of packet data area */
>> __u32 chunk_size;
>> __u32 headroom;
>> __u32 flags;
>> };
>>
>> struct xdp_umem_reg {
>> __u64 addr; /* Start of packet data area */
>> __u64 len; /* Length of packet data area */
>> __u32 chunk_size;
>> __u32 headroom;
>> __u32 flags;
>> __u32 tx_metadata_len;
>> };
>>
>> int main() {
>> printf("__u32: \t\t\t %lu\n", sizeof(__u32));
>> printf("__u64: \t\t\t %lu\n", sizeof(__u64));
>> printf("xdp_umem_reg_v1: \t %lu\n", sizeof(struct xdp_umem_reg_v1));
>> printf("xdp_umem_reg_v2: \t %lu\n", sizeof(struct xdp_umem_reg_v2));
>> printf("xdp_umem_reg: \t\t %lu\n", sizeof(struct xdp_umem_reg));
>> }
>>
>> Running "./test" produced this output:
>>
>> __u32: 4
>> __u64: 8
>> xdp_umem_reg_v1: 24
>> xdp_umem_reg_v2: 32
>> xdp_umem_reg: 32
>>> [0]: https://www.kernel.org/doc/html/v5.4/ioctl/botching-up-ioctls.html
> Hmm, true, this means our version check won't really work :-/ I don't
> see a good way to solve it without breaking the uapi. We can either
> add some new padding field to xdp_umem_reg to make it larger than _v2.
> Or we can add a new flag to signify the presence of tx_metadata_len
> and do the validation based on that.
>
> Btw, what are you using to setup umem? Looking at libxsk, it does
> `memset(&mr, 0, sizeof(mr));` which should clear the padding as well.
I'm using "setsockopt" directly with Rust bindings and the C
representation of Rust structs [1]. I'm guessing the compiler is not
zeroing the padding, which is why I encountered the issue.
[1]:
https://doc.rust-lang.org/reference/type-layout.html#the-c-representation
