![]() |
|
Date: Fri, 03 Mar 2023 07:03:19 +0300 From: Alexey Izbyshev <izbyshev@...ras.ru> To: musl@...ts.openwall.com Subject: Re: [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll On 2023-03-03 04:19, Rich Felker wrote: > On Thu, Mar 02, 2023 at 08:16:56AM +0300, Alexey Izbyshev wrote: >> After migration to 64-bit time_t in struct timespec, passing it to >> SYS_ppoll on arches where the syscall expects the 32-bit version of >> the struct became wrong and results in overlaying 64-bit tv_sec over >> the >> whole expected struct. In this case, tv_nsec is completely ignored, >> and >> interpretation of tv_sec depends on endianness. Because its value in >> the >> case of poll fits into 32 bits, on little endian arches tv_sec is >> interpreted as the correct number of seconds and zero nanoseconds, so >> the original timeout is effectively rounded down to seconds. On big >> endian arches, tv_sec is interpreted as nanoseconds (and zero >> seconds), >> so the original timeout is effectively divided by 10^9 and rounded >> down >> to nanoseconds. >> >> The only in-tree affected arch is or1k, which is big endian. > > Ah, because it's the only one that lacks SYS_poll. I guess that's why > this bug went unnoticed so long -- all the other 32-bit archs have > SYS_poll. > > FWIW any rv32 folks using out-of-tree patches are probably suffering > from this. The LE behavior is quite bad too. Pretty much all poll > timeouts are <1s, so it looks like the bug will make these poll calls > hammer 100% cpu. > I don't think riscv32 is affected. In the kernel: $ git grep __ARCH_WANT_TIME32_SYSCALLS arch/ arch/arc/include/uapi/asm/unistd.h:#define __ARCH_WANT_TIME32_SYSCALLS arch/arm64/include/uapi/asm/unistd.h:#define __ARCH_WANT_TIME32_SYSCALLS arch/csky/include/uapi/asm/unistd.h:#define __ARCH_WANT_TIME32_SYSCALLS arch/hexagon/include/uapi/asm/unistd.h:#define __ARCH_WANT_TIME32_SYSCALLS arch/nios2/include/uapi/asm/unistd.h:#define __ARCH_WANT_TIME32_SYSCALLS arch/openrisc/include/uapi/asm/unistd.h:#define __ARCH_WANT_TIME32_SYSCALLS So it seems that riscv32 doesn't have time32 ppoll at all, and https://www.openwall.com/lists/musl/2020/09/04/2 confirms this by only defining __NR_ppoll_time64. To be affected, an arch needs to be both old enough to have time32 syscalls and new enough not to have SYS_poll. >> Fix this by passing the timeout in the array of the type actually >> expected by the kernel for SYS_ppoll, as done in other time64-related >> code. >> --- >> src/select/poll.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/src/select/poll.c b/src/select/poll.c >> index c84c8a99..d1caefcc 100644 >> --- a/src/select/poll.c >> +++ b/src/select/poll.c >> @@ -8,8 +8,14 @@ int poll(struct pollfd *fds, nfds_t n, int timeout) >> #ifdef SYS_poll >> return syscall_cp(SYS_poll, fds, n, timeout); >> #else >> - return syscall_cp(SYS_ppoll, fds, n, timeout>=0 ? >> - &((struct timespec){ .tv_sec = timeout/1000, >> - .tv_nsec = timeout%1000*1000000 }) : 0, 0, _NSIG/8); >> + time_t s = timeout>=0 ? timeout/1000 : 0; >> + long ns = timeout>=0 ? timeout%1000*1000000 : 0; >> +#ifdef SYS_ppoll_time64 >> + if (SYS_ppoll == SYS_ppoll_time64) >> + return syscall_cp(SYS_ppoll_time64, fds, n, >> + timeout>=0 ? ((long long[]){s, ns}) : 0, 0, _NSIG/8); >> +#endif >> + return syscall_cp(SYS_ppoll, fds, n, >> + timeout>=0 ? ((long[]){s, ns}) : 0, 0, _NSIG/8); >> #endif >> } >> -- >> 2.39.1 > > Is there a reason you duplicated the timeout>=0 conditions, s and ns > setup, etc. other than to parallel other functions? I'm not sure it > makes sense here -- it certainly makes things a lot more verbose, and > I think it makes it harder to read (confusion over why things are > being done this way without the context of other functions where the > need can be seen). Since timeout is an int, there's no need for ever > storing it in a time_t (which is normally done for CLAMPing). > No, the reason is as you say: to parallel other code (ppoll.c in particular) after "evaluating" IS32BIT(s) to true. So the patch is optimized for readers familiar with how time64 support is done in musl. I agree that it might seem arcane out of context. > The following is kinda ugly but makes it obvious what the actual code > path possibilities are: > > return syscall_cp(SYS_ppoll, fds, n, timeout>=0 ? (( > #if SYS_ppoll_time64 == SYS_ppoll > long > #endif > long[]){timeout/1000, timeout%1000*1000000}) : 0, 0, _NSIG/8); > > a less hideous (maybe? not sure ;) way to do that might be: > > #if SYS_ppoll_time64 == SYS_ppoll > typedef long long timeout_t[2]; > #else > typedef long timeout_t[2]; > #endif > return syscall_cp(SYS_ppoll, fds, n, timeout>=0 ? > ((timeout_t){{timeout/1000, timeout%1000*1000000}}) : > 0, 0, _NSIG/8); > > or similar. Anyone else have opinions on this bikeshed? > The second option looks fine to me. Thanks, Alexey
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.