Commit e3c3bf2b58 introduce use of pselect6, but has following disadvantages:
* Use of userspace types in args67 structure - it breaks, for example, configs when 32-bit uClibc-ng compiled against 64-bit kernel. Syscall will always return EINVAL. We must use __kernel_* types and __SYSCALL_SIGSET_T_SIZE.
* It have excess checks for NSEC_PER_SEC. Original code from select() implementation has struct timeval => struct timespec conversion, kernel select() syscall implementation do the same. But none of libc versions (glibc, eglibc, musl) I know, perform similar checks for pselect() - there is no structure fields conversions, just struct timespec through all the calls. To have such checks in uClibc-ng we need one example, at least.
* It is possible to avoid extra userspace reads from kernel code if sigmask == NULL. I suggest to do it, for a few bytes cost.
* Commit didn't add test case to testsuite.
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com --- libc/sysdeps/linux/common/pselect.c | 67 ++++++++++++------------------------- test/unistd/tst-pselect.c | 51 ++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 45 deletions(-) create mode 100644 test/unistd/tst-pselect.c
diff --git a/libc/sysdeps/linux/common/pselect.c b/libc/sysdeps/linux/common/pselect.c index 3f1dd28..fbe85b7 100644 --- a/libc/sysdeps/linux/common/pselect.c +++ b/libc/sysdeps/linux/common/pselect.c @@ -31,55 +31,32 @@ static int __NC(pselect)(int nfds, fd_set *readfds, fd_set *writefds, const sigset_t *sigmask) { #ifdef __NR_pselect6 -#define NSEC_PER_SEC 1000000000L - struct timespec _ts, *ts = 0; - if (timeout) { - /* The Linux kernel can in some situations update the timeout value. - * We do not want that so use a local variable. - */ + /* The Linux kernel can in some situations update the timeout value. + * We do not want that so use a local variable. + */ + struct timespec _ts; + + if (timeout != NULL) { _ts = *timeout; + timeout = &_ts; + } + /* Note: the system call expects 7 values but on most architectures + we can only pass in 6 directly. If there is an architecture with + support for more parameters a new version of this file needs to + be created. */ + struct { + __kernel_ulong_t ss; + __kernel_size_t ss_len; + } data;
- /* GNU extension: allow for timespec values where the sub-sec - * field is equal to or more than 1 second. The kernel will - * reject this on us, so take care of the time shift ourself. - * Some applications (like readline and linphone) do this. - * See 'clarification on select() type calls and invalid timeouts' - * on the POSIX general list for more information. - */ - if (_ts.tv_nsec >= NSEC_PER_SEC) { - _ts.tv_sec += _ts.tv_nsec / NSEC_PER_SEC; - _ts.tv_nsec %= NSEC_PER_SEC; - } - - ts = &_ts; + if (sigmask != NULL) { + data.ss = (__kernel_ulong_t) sigmask; + data.ss_len = __SYSCALL_SIGSET_T_SIZE; + + sigmask = (void *)&data; }
- /* The pselect6 syscall API is strange. It wants a 7th arg to be - * the sizeof(*sigmask). However syscalls with > 6 arguments aren't - * supported on linux. So arguments 6 and 7 are stuffed in a struct - * and a pointer to that struct is passed as the 6th argument to - * the syscall. - * Glibc stuffs arguments 6 and 7 in a ulong[2]. Linux reads - * them as if there were a struct { sigset_t*; size_t } in - * userspace. There woudl be trouble if userspace and the kernel are - * compiled differently enough that size_t isn't the same as ulong, - * but not enough to trigger the compat layer in linux. I can't - * think of such a case, so I'm using linux's struct. - * Furthermore Glibc sets the sigsetsize to _NSIG/8. However linux - * checks for sizeof(sigset_t), which internally is a ulong array. - * This means that if _NSIG isn't a multiple of BITS_PER_LONG then - * linux will refuse glibc's value. So I prefer sizeof(sigset_t) for - * the value of sigsetsize. - */ - struct { - const sigset_t *sigmask; - size_t sigsetsize; - } args67 = { - sigmask, - sizeof(sigset_t), - }; - - return INLINE_SYSCALL(pselect6, 6, nfds, readfds, writefds, exceptfds, ts, &args67); + return INLINE_SYSCALL(pselect6, 6, nfds, readfds, writefds, exceptfds, timeout, sigmask); #else struct timeval tval; int retval; diff --git a/test/unistd/tst-pselect.c b/test/unistd/tst-pselect.c new file mode 100644 index 0000000..cab9451 --- /dev/null +++ b/test/unistd/tst-pselect.c @@ -0,0 +1,51 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <errno.h> +#include <signal.h> +#include <sys/types.h> +#include <sys/select.h> + +// our SIGALRM handler +void handler(int signum) { + (void)signum; + puts("got signal\n"); +} + +static int +do_test (void) +{ + int rc; + sigset_t wait_mask, mask_sigchld; + struct sigaction act; + + // block SIGALRM. We want to handle it only when we're ready + sigemptyset(&mask_sigchld); + sigaddset(&mask_sigchld, SIGALRM); + sigprocmask(SIG_BLOCK, &mask_sigchld, &wait_mask); + sigdelset(&wait_mask, SIGALRM); + + // register a signal handler so we can see when the signal arrives + memset(&act, 0, sizeof(act)); + sigemptyset(&act.sa_mask); // just in case an empty set isn't all 0's (total paranoia) + act.sa_handler = handler; + sigaction(SIGALRM, &act, NULL); + + // send ourselves a SIGARLM. It will pend until we unblock that signal in pselect() + printf("sending ourselves a signal\n"); + kill(getpid(), SIGALRM); + + printf("signal is pending; calling pselect()\n"); + rc = pselect(0, NULL, NULL, NULL, NULL, &wait_mask); + if (rc != -1 || errno != EINTR) { + int e = errno; + printf("pselect() returned %d, errno %d (%s)\n", rc, e, strerror(e)); + exit(1); + } + + return 0; +} + +#define TEST_FUNCTION do_test () +#include <test-skeleton.c>
Thanks for cleaning up my code. It's much better now.
I didn't know the userspace names of kernel types. I didn't think a 32bit userspace calling a 64bit kernel would break because there's a compat_sys_pselect6() in linux.
-N
On Sun, Dec 27, 2015 at 9:46 AM, Leonid Lisovskiy lly.dev@gmail.com wrote:
Commit e3c3bf2b58 introduce use of pselect6, but has following disadvantages:
Use of userspace types in args67 structure - it breaks, for example, configs when 32-bit uClibc-ng compiled against 64-bit kernel. Syscall will always return EINVAL. We must use __kernel_* types and __SYSCALL_SIGSET_T_SIZE.
It have excess checks for NSEC_PER_SEC. Original code from select() implementation has struct timeval => struct timespec conversion, kernel select() syscall implementation do the same. But none of libc versions (glibc, eglibc, musl) I know, perform similar checks for pselect() - there is no structure fields conversions, just struct timespec through all the calls. To have such checks in uClibc-ng we need one example, at least.
It is possible to avoid extra userspace reads from kernel code if sigmask == NULL. I suggest to do it, for a few bytes cost.
Commit didn't add test case to testsuite.
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com
libc/sysdeps/linux/common/pselect.c | 67 ++++++++++++------------------------- test/unistd/tst-pselect.c | 51 ++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 45 deletions(-) create mode 100644 test/unistd/tst-pselect.c
diff --git a/libc/sysdeps/linux/common/pselect.c b/libc/sysdeps/linux/common/pselect.c index 3f1dd28..fbe85b7 100644 --- a/libc/sysdeps/linux/common/pselect.c +++ b/libc/sysdeps/linux/common/pselect.c @@ -31,55 +31,32 @@ static int __NC(pselect)(int nfds, fd_set *readfds, fd_set *writefds, const sigset_t *sigmask) { #ifdef __NR_pselect6 -#define NSEC_PER_SEC 1000000000L
struct timespec _ts, *ts = 0;if (timeout) {/* The Linux kernel can in some situations update thetimeout value.
* We do not want that so use a local variable.*/
/* The Linux kernel can in some situations update the timeoutvalue.
* We do not want that so use a local variable.*/struct timespec _ts;if (timeout != NULL) { _ts = *timeout;timeout = &_ts;}/* Note: the system call expects 7 values but on most architectureswe can only pass in 6 directly. If there is an architecturewith
support for more parameters a new version of this file needs tobe created. */struct {__kernel_ulong_t ss;__kernel_size_t ss_len;} data;
/* GNU extension: allow for timespec values where thesub-sec
* field is equal to or more than 1 second. The kernel will* reject this on us, so take care of the time shiftourself.
* Some applications (like readline and linphone) do this.* See 'clarification on select() type calls and invalidtimeouts'
* on the POSIX general list for more information.*/if (_ts.tv_nsec >= NSEC_PER_SEC) {_ts.tv_sec += _ts.tv_nsec / NSEC_PER_SEC;_ts.tv_nsec %= NSEC_PER_SEC;}ts = &_ts;
if (sigmask != NULL) {data.ss = (__kernel_ulong_t) sigmask;data.ss_len = __SYSCALL_SIGSET_T_SIZE;sigmask = (void *)&data; }
/* The pselect6 syscall API is strange. It wants a 7th arg to be* the sizeof(*sigmask). However syscalls with > 6 arguments aren't* supported on linux. So arguments 6 and 7 are stuffed in a struct* and a pointer to that struct is passed as the 6th argument to* the syscall.* Glibc stuffs arguments 6 and 7 in a ulong[2]. Linux reads* them as if there were a struct { sigset_t*; size_t } in* userspace. There woudl be trouble if userspace and the kernelare
* compiled differently enough that size_t isn't the same as ulong,* but not enough to trigger the compat layer in linux. I can't* think of such a case, so I'm using linux's struct.* Furthermore Glibc sets the sigsetsize to _NSIG/8. However linux* checks for sizeof(sigset_t), which internally is a ulong array.* This means that if _NSIG isn't a multiple of BITS_PER_LONG then* linux will refuse glibc's value. So I prefer sizeof(sigset_t)for
* the value of sigsetsize.*/struct {const sigset_t *sigmask;size_t sigsetsize;} args67 = {sigmask,sizeof(sigset_t),};return INLINE_SYSCALL(pselect6, 6, nfds, readfds, writefds,exceptfds, ts, &args67);
return INLINE_SYSCALL(pselect6, 6, nfds, readfds, writefds,exceptfds, timeout, sigmask); #else struct timeval tval; int retval; diff --git a/test/unistd/tst-pselect.c b/test/unistd/tst-pselect.c new file mode 100644 index 0000000..cab9451 --- /dev/null +++ b/test/unistd/tst-pselect.c @@ -0,0 +1,51 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <errno.h> +#include <signal.h> +#include <sys/types.h> +#include <sys/select.h>
+// our SIGALRM handler +void handler(int signum) {
(void)signum;puts("got signal\n");+}
+static int +do_test (void) +{
int rc;sigset_t wait_mask, mask_sigchld;struct sigaction act;// block SIGALRM. We want to handle it only when we're readysigemptyset(&mask_sigchld);sigaddset(&mask_sigchld, SIGALRM);sigprocmask(SIG_BLOCK, &mask_sigchld, &wait_mask);sigdelset(&wait_mask, SIGALRM);// register a signal handler so we can see when the signal arrivesmemset(&act, 0, sizeof(act));sigemptyset(&act.sa_mask); // just in case an empty set isn't all0's (total paranoia)
act.sa_handler = handler;sigaction(SIGALRM, &act, NULL);// send ourselves a SIGARLM. It will pend until we unblock thatsignal in pselect()
printf("sending ourselves a signal\n");kill(getpid(), SIGALRM);printf("signal is pending; calling pselect()\n");rc = pselect(0, NULL, NULL, NULL, NULL, &wait_mask);if (rc != -1 || errno != EINTR) {int e = errno;printf("pselect() returned %d, errno %d (%s)\n", rc, e,strerror(e));
exit(1);}return 0;+}
+#define TEST_FUNCTION do_test ()
+#include <test-skeleton.c>
1.8.5.6
Hi Leonid, Leonid Lisovskiy wrote,
Commit e3c3bf2b58 introduce use of pselect6, but has following disadvantages:
Use of userspace types in args67 structure - it breaks, for example, configs when 32-bit uClibc-ng compiled against 64-bit kernel. Syscall will always return EINVAL. We must use __kernel_* types and __SYSCALL_SIGSET_T_SIZE.
It have excess checks for NSEC_PER_SEC. Original code from select() implementation has struct timeval => struct timespec conversion, kernel select() syscall implementation do the same. But none of libc versions (glibc, eglibc, musl) I know, perform similar checks for pselect() - there is no structure fields conversions, just struct timespec through all the calls. To have such checks in uClibc-ng we need one example, at least.
It is possible to avoid extra userspace reads from kernel code if sigmask == NULL. I suggest to do it, for a few bytes cost.
Commit didn't add test case to testsuite.
Signed-off-by: Leonid Lisovskiy lly.dev@gmail.com
Applied and pushed, thx Waldemar
-
Leonid Lisovskiy -
Nic Dade -
Waldemar Brodkorb