Re: [PATCH] linux-user/syscall: zero-init msghdr in do_sendrecvmsg_locked

[Laurent Vivier]

Le 20/06/2021 à 17:09, Kenta Iwasaki a écrit :
> No worries, though I would also like to apologize as I find that the 
> explanation I gave in my last
> email was a little all over the place.
> 
> To clarify my last e-mail, I believe the current msghdr struct layout in QEMU 
> is libc-agnostic, but
> not kernel agnostic. Rather, the current msghdr struct layout defined in QEMU 
> follows the definition
> of the Linux kernel's own struct layout of msghdr, which isn't compliant with 
> POSIX.
> 
> Zeroing the bytes in the patch I provided, which is a trick used in musl libc 
> as well to be both
> kernel-agnostic and libc-agnostic, would make QEMU's struct layout definition 
> of msghdr be both
> kernel-agnostic to all kernels adhering to the POSIX standard, and 
> libc-agnostic as well.

Yes, linux-user is not kernel agnostic. It's why I would prefer to define a 
"host_msghdr"  and use
it as a parameter to the host outgoing syscall (we already define 
"target_msghdr" for target
incoming syscall). host_msghdr would be a copy of kernel user_msghdr.

But what do you mean by "all kernels": do we have linux kernels that use a 
different msghdr
definition (POSIX one)?

Thanks,
Laurent

> Best regards,
> Kenta Iwasaki
> 
> On Sun, 20 Jun 2021 at 23:56, Laurent Vivier <laurent@vivier.eu 
> <mailto:laurent@vivier.eu>> wrote:
> 
>     Le 16/05/2021 à 14:57, Kenta Iwasaki a écrit :
>     > Sure,
>     >
>     > The bytes of `msghdr` need to be cleared because the `msghdr` struct 
> layout specified in QEMU
>     > appears to generalize between the definitions of `msghdr` across 
> different libc's and kernels. To
>     > appropriately generalize `msghdr` across libc's and kernels would 
> either:
>     >
>     > 1. require specializing code in do_sendrecvmsg_locked() for each 
> individual libc and kernel
>     version, or
>     > 2. zeroing out all bytes of `msghdr`, b/c certain libc or kernel 
> versions may misinterpret the
>     > undefined padding bytes that come from misalignment in the struct as 
> actual syscall params.
>     >
>     > The patch I provided would be going for route #2, given that it's a 
> simpler fix for the underlying
>     > problem for the short term.
>     >
>     > What I believe is the background behind why the struct layout has been 
> a problem is because, since
>     > the beginning, the Linux kernel has always specified the layout of 
> `msghdr` differently from
>     POSIX.
>     > Given that this implies incompatibility between kernels on how `msghdr` 
> is specified,
>     different libc
>     > projects such as musl and glibc provide different workarounds by laying 
> out `msghdr` differently
>     > amongst one another.
>     >
>     > A few projects running tests/applications through QEMU have been bitten 
> by this, and a
>     solution that
>     > one of the projects discovered was that patching QEMU to 
> zero-initialize the bytes msghdr the same
>     > way my patch does allow for compatibility between different `msghdr` 
> layouts across glibc,
>     musl, and
>     > the Linux kernel:
>     
> https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360
>     
> <https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360>
>     > 
> <https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360
>     
> <https://github.com/void-linux/void-packages/issues/23557#issuecomment-718392360>>
>     >
>     > For some additional useful context, here's a link pointing changes musl 
> libc made to laying out
>     > `msghdr` b/c of Linux incorrectly laying out `msghdr` against the POSIX 
> standard:
>     > 
> http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64
>     
> <http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64>
>     > 
> <http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64
>     
> <http://git.musl-libc.org/cgit/musl/commit/?id=7168790763cdeb794df52be6e3b39fbb021c5a64>>
>     >
>     > Also, here is a link to the `msghdr` struct layout in musl libc's 
> bleeding edge branch as of
>     > now: https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22
>     <https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22>
>     > <https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22
>     <https://git.musl-libc.org/cgit/musl/tree/include/sys/socket.h#n22>>
>     >
>     > As for my rationale for sending in this patch, it is because I'm 
> currently implementing
>     > cross-platform networking in the standard library for the Zig 
> programming language, and have run
>     > into this exact same problem with EMSGSIZE being returned by sendmsg() 
> when tests are run through
>     > QEMU on x86_64-linux-musl.
>     >
>     > My discussions with the Zig community about it alongside debug logs 
> regarding the exact parameters
>     > being fed to the sendmsg() syscall can be found
>     > here: https://github.com/ziglang/zig/pull/8750#issuecomment-841641576
>     <https://github.com/ziglang/zig/pull/8750#issuecomment-841641576>
>     > <https://github.com/ziglang/zig/pull/8750#issuecomment-841641576
>     <https://github.com/ziglang/zig/pull/8750#issuecomment-841641576>>
>     >
>     > Hope this gives enough context about the problem and patch, but please 
> do let me know if there is
>     > any more information that I could provide which would help.
> 
>     Thank you for the explanation and sorry for the delay.
> 
>     As we use directly the kernel syscall rather than the libc wrapper we 
> should not use here the msghdr
>     definition from the libc but the one from the kernel.
> 
>     The one we receive is also the one from the kernel as we trap the kernel 
> syscall not the libc call.
> 
>     So the code should be libc-agnostic...
> 
>     The reference implementation in our case is 'struct user_msghdr' from the 
> kernel, and we need to
>     duplicate it in QEMU to be able to use it.
> 
>     Thanks,
>     LAurent
>