From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 29483 invoked from network); 3 Mar 2023 04:03:40 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 3 Mar 2023 04:03:40 -0000 Received: (qmail 23692 invoked by uid 550); 3 Mar 2023 04:03:36 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 23660 invoked from network); 3 Mar 2023 04:03:36 -0000 DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru 8DC3944C1006 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1677816199; bh=+MufF4e1aNZ4jHfsTcaJohCN/l6jC0HA/KHGNPY23dU=; h=Date:From:To:Subject:Reply-To:In-Reply-To:References:From; b=dLbYeJHf9T2BTb26n5u08rU3Fk1geAzvPyyPpi8Z/2Cqrp/ppDJ4XKmdwmEJ7fOth REn4mu5I0PetyTtye7iB8+YsVKoEfzNbhnAu20ItUylNVl/ZyR2SiP9yzOgpmzCdnS i0gpXmGqyyY0q+2mp1YutPAqjYYPjtLKzzyIHaVc= MIME-Version: 1.0 Date: Fri, 03 Mar 2023 07:03:19 +0300 From: Alexey Izbyshev To: musl@lists.openwall.com Mail-Followup-To: musl@lists.openwall.com In-Reply-To: <20230303011946.GM4163@brightrain.aerifal.cx> References: <20230302051656.260369-1-izbyshev@ispras.ru> <20230303011946.GM4163@brightrain.aerifal.cx> User-Agent: Roundcube Webmail/1.4.4 Message-ID: <5ddc2eba8b12af91d12d938aebb80967@ispras.ru> X-Sender: izbyshev@ispras.ru Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [musl] [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