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

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

[Alexey Izbyshev]

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.

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

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

[Rich Felker]

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

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

[Alexey Izbyshev]

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