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.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 9016 invoked from network); 3 Mar 2023 01:20:04 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 3 Mar 2023 01:20:04 -0000 Received: (qmail 25803 invoked by uid 550); 3 Mar 2023 01:20:00 -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 25769 invoked from network); 3 Mar 2023 01:19:59 -0000 Date: Thu, 2 Mar 2023 20:19:46 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20230303011946.GM4163@brightrain.aerifal.cx> References: <20230302051656.260369-1-izbyshev@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230302051656.260369-1-izbyshev@ispras.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll 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