* [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll @ 2023-03-02 5:16 Alexey Izbyshev 2023-03-03 1:19 ` Rich Felker 0 siblings, 1 reply; 3+ messages in thread From: Alexey Izbyshev @ 2023-03-02 5:16 UTC (permalink / raw) To: musl 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. 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll 2023-03-02 5:16 [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll Alexey Izbyshev @ 2023-03-03 1:19 ` Rich Felker 2023-03-03 4:03 ` Alexey Izbyshev 0 siblings, 1 reply; 3+ messages in thread From: Rich Felker @ 2023-03-03 1:19 UTC (permalink / raw) To: musl 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. > 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). 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? Rich ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll 2023-03-03 1:19 ` Rich Felker @ 2023-03-03 4:03 ` Alexey Izbyshev 0 siblings, 0 replies; 3+ messages in thread From: Alexey Izbyshev @ 2023-03-03 4:03 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-03-03 4:03 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-02 5:16 [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll Alexey Izbyshev 2023-03-03 1:19 ` Rich Felker 2023-03-03 4:03 ` Alexey Izbyshev
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).