mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <>
Subject: Re: [musl] [PATCH] poll: fix timespec kernel ABI mismatch on 32-bit arches without SYS_poll
Date: Thu, 2 Mar 2023 20:19:46 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

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[]){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];
	typedef long timeout_t[2];
	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?


  reply	other threads:[~2023-03-03  1:20 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 [this message]
2023-03-03  4:03   ` Alexey Izbyshev

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:

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

  git send-email \ \ \ \

* 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

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).