other: 0.956 graphic: 0.948 semantic: 0.942 device: 0.928 boot: 0.925 socket: 0.924 vnc: 0.922 KVM: 0.911 network: 0.881 [Qemu-devel] [BUG] Inappropriate size of target_sigset_t Hello, Peter, Laurent, While working on another problem yesterday, I think I discovered a long-standing bug in QEMU Linux user mode: our target_sigset_t structure is eight times smaller as it should be! In this code segment from syscalls_def.h: #ifdef TARGET_MIPS #define TARGET_NSIG 128 #else #define TARGET_NSIG 64 #endif #define TARGET_NSIG_BPW TARGET_ABI_BITS #define TARGET_NSIG_WORDS (TARGET_NSIG / TARGET_NSIG_BPW) typedef struct { abi_ulong sig[TARGET_NSIG_WORDS]; } target_sigset_t; ... TARGET_ABI_BITS should be replaced by eight times smaller constant (in fact, semantically, we need TARGET_ABI_BYTES, but it is not defined) (what is needed is actually "a byte per signal" in target_sigset_t, and we allow "a bit per signal"). All this probably sounds to you like something impossible, since this code is in QEMU "since forever", but I checked everything, and the bug seems real. I wish you can prove me wrong. I just wanted to let you know about this, given the sensitive timing of current softfreeze, and the fact that I won't be able to do more investigation on this in coming weeks, since I am busy with other tasks, but perhaps you can analyze and do something which you consider appropriate. Yours, Aleksandar Le 03/07/2019 à 21:46, Aleksandar Markovic a écrit : > Hello, Peter, Laurent, > > While working on another problem yesterday, I think I discovered a > long-standing bug in QEMU Linux user mode: our target_sigset_t structure is > eight times smaller as it should be! > > In this code segment from syscalls_def.h: > > #ifdef TARGET_MIPS > #define TARGET_NSIG 128 > #else > #define TARGET_NSIG 64 > #endif > #define TARGET_NSIG_BPW TARGET_ABI_BITS > #define TARGET_NSIG_WORDS (TARGET_NSIG / TARGET_NSIG_BPW) > > typedef struct { > abi_ulong sig[TARGET_NSIG_WORDS]; > } target_sigset_t; > > ... TARGET_ABI_BITS should be replaced by eight times smaller constant (in > fact, semantically, we need TARGET_ABI_BYTES, but it is not defined) (what is > needed is actually "a byte per signal" in target_sigset_t, and we allow "a > bit per signal"). TARGET_NSIG is divided by TARGET_ABI_BITS which gives you the number of abi_ulong words we need in target_sigset_t. > All this probably sounds to you like something impossible, since this code is > in QEMU "since forever", but I checked everything, and the bug seems real. I > wish you can prove me wrong. > > I just wanted to let you know about this, given the sensitive timing of > current softfreeze, and the fact that I won't be able to do more > investigation on this in coming weeks, since I am busy with other tasks, but > perhaps you can analyze and do something which you consider appropriate. If I compare with kernel, it looks good: In Linux: arch/mips/include/uapi/asm/signal.h #define _NSIG 128 #define _NSIG_BPW (sizeof(unsigned long) * 8) #define _NSIG_WORDS (_NSIG / _NSIG_BPW) typedef struct { unsigned long sig[_NSIG_WORDS]; } sigset_t; _NSIG_BPW is 8 * 8 = 64 on MIPS64 or 4 * 8 = 32 on MIPS In QEMU: TARGET_NSIG_BPW is TARGET_ABI_BITS which is TARGET_LONG_BITS which is 64 on MIPS64 and 32 on MIPS. I think there is no problem. Thanks, Laurent From: Laurent Vivier > If I compare with kernel, it looks good: > ... > I think there is no problem. Sure, thanks for such fast response - again, I am glad if you are right. However, for some reason, glibc (and musl too) define sigset_t differently than kernel. Please take a look. I am not sure if this is covered fine in our code. Yours, Aleksandar > Thanks, > Laurent On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic wrote: > > From: Laurent Vivier > > If I compare with kernel, it looks good: > > ... > > I think there is no problem. > > Sure, thanks for such fast response - again, I am glad if you are right. > However, for some reason, glibc (and musl too) define sigset_t differently > than kernel. Please take a look. I am not sure if this is covered fine in our > code. Yeah, the libc definitions of sigset_t don't match the kernel ones (this is for obscure historical reasons IIRC). We're providing implementations of the target syscall interface, so our target_sigset_t should be the target kernel's version (and the target libc's version doesn't matter to us). On the other hand we will be using the host libc version, I think, so a little caution is required and it's possible we have some bugs in our code. thanks -- PMM > From: Peter Maydell > > On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic wrote: > > > > From: Laurent Vivier > > > If I compare with kernel, it looks good: > > > ... > > > I think there is no problem. > > > > Sure, thanks for such fast response - again, I am glad if you are right. > > However, for some reason, glibc (and musl too) define sigset_t differently > > than kernel. Please take a look. I am not sure if this is covered fine in > > our code. > > Yeah, the libc definitions of sigset_t don't match the > kernel ones (this is for obscure historical reasons IIRC). > We're providing implementations of the target > syscall interface, so our target_sigset_t should be the > target kernel's version (and the target libc's version doesn't > matter to us). On the other hand we will be using the > host libc version, I think, so a little caution is required > and it's possible we have some bugs in our code. OK, I gather than this is not something that requires our immediate attention (for 4.1), but we can analyze it later on. Thanks for response!! Sincerely, Aleksandar > thanks > -- PMM Le 03/07/2019 à 22:28, Peter Maydell a écrit : > On Wed, 3 Jul 2019 at 21:20, Aleksandar Markovic wrote: > > > > From: Laurent Vivier > >> If I compare with kernel, it looks good: > >> ... > >> I think there is no problem. > > > > Sure, thanks for such fast response - again, I am glad if you are right. > > However, for some reason, glibc (and musl too) define sigset_t differently > > than kernel. Please take a look. I am not sure if this is covered fine in > > our code. > > Yeah, the libc definitions of sigset_t don't match the > kernel ones (this is for obscure historical reasons IIRC). > We're providing implementations of the target > syscall interface, so our target_sigset_t should be the > target kernel's version (and the target libc's version doesn't > matter to us). On the other hand we will be using the > host libc version, I think, so a little caution is required > and it's possible we have some bugs in our code. It's why we need host_to_target_sigset_internal() and target_to_host_sigset_internal() that translates bits and bytes between guest kernel interface and host libc interface. void host_to_target_sigset_internal(target_sigset_t *d, const sigset_t *s) { int i; target_sigemptyset(d); for (i = 1; i <= TARGET_NSIG; i++) { if (sigismember(s, i)) { target_sigaddset(d, host_to_target_signal(i)); } } } void target_to_host_sigset_internal(sigset_t *d, const target_sigset_t *s) { int i; sigemptyset(d); for (i = 1; i <= TARGET_NSIG; i++) { if (target_sigismember(s, i)) { sigaddset(d, target_to_host_signal(i)); } } } Thanks, Laurent Hi Aleksandar, On Wed, Jul 3, 2019 at 12:48 PM Aleksandar Markovic wrote: > #define TARGET_NSIG_BPW TARGET_ABI_BITS > #define TARGET_NSIG_WORDS (TARGET_NSIG / TARGET_NSIG_BPW) > > typedef struct { > abi_ulong sig[TARGET_NSIG_WORDS]; > } target_sigset_t; > > ... TARGET_ABI_BITS should be replaced by eight times smaller constant (in > fact, > semantically, we need TARGET_ABI_BYTES, but it is not defined) (what is needed > is actually "a byte per signal" in target_sigset_t, and we allow "a bit per > signal"). Why do we need a byte per target signal, if the functions in linux-user/signal.c operate with bits? -- Thanks. -- Max > Why do we need a byte per target signal, if the functions in > linux-user/signal.c > operate with bits? Max, I did not base my findings on code analysis, but on dumping size/offsets of elements of some structures, as they are emulated in QEMU, and in real systems. So, I can't really answer your question. Yours, Aleksandar > -- > Thanks. > -- Max