Message-ID: <CAJ8uoz2AMN1y0kXqR2-uguPVHObekM0654M71GBzM4D1LmsMBg@mail.gmail.com>
Date: Thu, 11 Jul 2024 10:11:03 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: Julian Schindel <mail@...tic-alpaca.de>, 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 Thu, 11 Jul 2024 at 05:48, Stanislav Fomichev <sdf@...ichev.me> 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?
>
> 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.
>
> Magnus, any better ideas?

Unfortunately not. This is a pest or cholera kind of choice. I agree
with you that the least bad choice here is to break it for the users
of this new functionality as they should be fewer than all the
existing users that do not use the Tx metadata functionality. Really
appreciate your suggestions to put some compile time asserts in there
to make sure we do not make the same mistake again. Thanks.

Powered by blists - more mailing lists