xdp/xsk.c: Possible bug in xdp_umem_reg version check

xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Julian Schindel]

Hi,

I hope this is the correct way to ask about this issue, I haven't used
the kernel mailing list before.

Between different compilations of an AF_XDP project, I encountered
"random" EINVAL errors when calling setsockopt XDP_UMEM_REG with the
same parameter.

I think this might be caused by this patch:
https://lore.kernel.org/all/20231127190319.1190813-2-sdf@google.com/
It added "tx_metadata_len" to the "xdp_umem_reg" struct.
In the  "xsk_setsockopt" code in xdp/xsk.c, the provided "optlen" is
checked against the length of "xdp_umem_reg_v2" and "xdp_umem_reg" to
check which version of "xdp_umem_reg", the user supplied.

At least on my machine (x86_64, Fedora 40, 6.9.7), these two structs
have the same size (32 bytes) due to the compiler adding padding to
"xdp_umem_reg_v2". This means if the user supplies "xdp_umem_reg_v2", it
is falsely treated as "xdp_umem_reg".

I'm not sure whether there is some implicit struct packing happening or
whether this is indeed a bug.

Best regards,
Julian

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Magnus Karlsson]

On Sun, 7 Jul 2024 at 17:06, Julian Schindel <mail@arctic-alpaca.de> wrote:
>
> Hi,
>
> I hope this is the correct way to ask about this issue, I haven't used
> the kernel mailing list before.
>
> Between different compilations of an AF_XDP project, I encountered
> "random" EINVAL errors when calling setsockopt XDP_UMEM_REG with the
> same parameter.
>
> I think this might be caused by this patch:
> https://lore.kernel.org/all/20231127190319.1190813-2-sdf@google.com/
> It added "tx_metadata_len" to the "xdp_umem_reg" struct.
> In the  "xsk_setsockopt" code in xdp/xsk.c, the provided "optlen" is
> checked against the length of "xdp_umem_reg_v2" and "xdp_umem_reg" to
> check which version of "xdp_umem_reg", the user supplied.
>
> At least on my machine (x86_64, Fedora 40, 6.9.7), these two structs
> have the same size (32 bytes) due to the compiler adding padding to
> "xdp_umem_reg_v2". This means if the user supplies "xdp_umem_reg_v2", it
> is falsely treated as "xdp_umem_reg".
>
> I'm not sure whether there is some implicit struct packing happening or
> whether this is indeed a bug.

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. What is sizeof(struct xdp_umem_reg) for you before the
patch that added tx_metadata_len?

[0]: https://www.kernel.org/doc/html/v5.4/ioctl/botching-up-ioctls.html

> Best regards,
> Julian
>
>

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Julian Schindel]

On 09.07.24 11:23, Magnus Karlsson wrote:
> On Sun, 7 Jul 2024 at 17:06, Julian Schindel <mail@arctic-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
>> Best regards,
>> Julian

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Stanislav Fomichev]

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@arctic-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.

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Julian Schindel]

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@arctic-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

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Stanislav Fomichev]

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@arctic-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?

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Julian Schindel]

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@arctic-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?

Re: xdp/xsk.c: Possible bug in xdp_umem_reg version check

[Magnus Karlsson]

On Thu, 11 Jul 2024 at 05:48, Stanislav Fomichev <sdf@fomichev.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@arctic-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.