[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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.
