Re: [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll

[Rich Felker <dalias@libc.org>]

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