mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Alexey Izbyshev <izbyshev@ispras.ru>
To: musl@lists.openwall.com
Subject: Re: [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll
Date: Fri, 03 Mar 2023 07:03:19 +0300	[thread overview]
Message-ID: <5ddc2eba8b12af91d12d938aebb80967@ispras.ru> (raw)
In-Reply-To: <20230303011946.GM4163@brightrain.aerifal.cx>

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

      reply	other threads:[~2023-03-03  4:03 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-02  5:16 Alexey Izbyshev
2023-03-03  1:19 ` Rich Felker
2023-03-03  4:03   ` Alexey Izbyshev [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5ddc2eba8b12af91d12d938aebb80967@ispras.ru \
    --to=izbyshev@ispras.ru \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).