Message-ID: <Zo9WCnMFSs775MSd@mini-arch>
Date: Wed, 10 Jul 2024 20:48:26 -0700
From: Stanislav Fomichev <sdf@...ichev.me>
To: Julian Schindel <mail@...tic-alpaca.de>
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 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?

Powered by blists - more mailing lists