Message-ID: <8a527e65-8677-43be-8c8d-ffc5d351f8fb@arctic-alpaca.de>
Date: Thu, 11 Jul 2024 07:23:00 +0200
From: Julian Schindel <mail@...tic-alpaca.de>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: Magnus Karlsson <magnus.karlsson@...il.com>, bpf@...r.kernel.org,
 Björn Töpel <bjorn@...nel.org>,
 Magnus Karlsson <magnus.karlsson@...el.com>,
 Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
 Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org
Subject: Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

On 11.07.24 05:48, Stanislav Fomichev wrote:
> On 07/10, Julian Schindel wrote:
>> 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@...tic-alpaca.de> 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
> Awesome, thanks for confirming! I guess for now you can work it around
> by having an explicit padding field and setting it to zero?

Yes,the issue isn't blocking for me.
> For a long-term fix, I'm leaning towards adding new umem flag as
> a signal to the kernel to interpret this as a tx_metadata_len. But
> this is gonna break any existing users that set this value. Hopefully
> should not be a lot of them since it is a pretty recent functionality.
>
> I'm also gonna sprinkle some compile time asserts to make sure we can extend
> xdp_umem_reg in the future without hitting the same issue again. I'm a
> bit spoiled by sys_bpf which takes care of enforcing the padding being
> zero.

Sounds good to me, I cannot think of any non-breaking solution.
Thank you for taking care of the issue!

>
> Magnus, any better ideas?

Powered by blists - more mailing lists