klibc - Feb 2019 - [PATCH 2/2] ia64: Fix sigaction struct layout and function implementation

Commit 8418552 ("[klibc] ia64: Fix shared build") missed this use of
the
GP register, although the code appears to have been dubious anyway,
assuming the address of errno was the first thing pointed to by GP.
---
 usr/klibc/arch/ia64/vfork.S | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/usr/klibc/arch/ia64/vfork.S b/usr/klibc/arch/ia64/vfork.S
index 7e76a71..e513188 100644
--- a/usr/klibc/arch/ia64/vfork.S
+++ b/usr/klibc/arch/ia64/vfork.S
@@ -26,16 +26,12 @@ vfork:
 	mov	r15=__NR_clone
 	mov	out0=CLONE_VM|CLONE_VFORK|SIGCHLD
 	mov     out1=0
-	;;
 	break 0x100000      // Do the syscall
-	;;
-	addl	r15=0,r1
 	cmp.eq  p7,p6 = -1,r10
 	;;
-	ld8	r14=[r15]
+(p7)	movl r14 = errno
 	;;
 (p7)	st4	[r14]=r8
-	;;
 (p7)	mov	r8=-1
 	br.ret.sptk.many b0
 	.endp vfork
-- 
1.8.5.3

Commit 8418552 ("[klibc] ia64: Fix shared build") switched to the
function descriptor-less ABI. However, when passing a function pointer
to the kernel for signal handling, it always expects a descriptor, and
-mno-pic does not set any ELF flags that the kernel could detect. Thus
to fix this regression we must construct descriptors solely for passing
to/from the kernel.

Despite this, usr/dash/static/sh -c "usr/utils/static/true; exit" was
still seen to hang during the test suite. This is because sa_mask and
sa_flags were round the wrong way (matching the old sigaction syscall
rather than the newer rt_sigaction syscall, despite the former not
existing on ia64), causing the kernel to think SA_NOCLDWAIT was set.
This bug appears to have been present since the ia64 port was first
committed.
---
 usr/include/arch/ia64/klibc/archsignal.h |  3 ++-
 usr/klibc/sigaction.c                    | 45 +++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/usr/include/arch/ia64/klibc/archsignal.h
b/usr/include/arch/ia64/klibc/archsignal.h
index fbc961b..c52c41a 100644
--- a/usr/include/arch/ia64/klibc/archsignal.h
+++ b/usr/include/arch/ia64/klibc/archsignal.h
@@ -22,8 +22,9 @@ struct sigaction {
 		__sighandler_t _sa_handler;
 		void (*_sa_sigaction) (int, struct siginfo *, void *);
 	} _u;
-	sigset_t sa_mask;
 	int sa_flags;
+	int __pad0;
+	sigset_t sa_mask;
 };
 
 #define sa_handler      _u._sa_handler
diff --git a/usr/klibc/sigaction.c b/usr/klibc/sigaction.c
index 19a8a54..e4901ab 100644
--- a/usr/klibc/sigaction.c
+++ b/usr/klibc/sigaction.c
@@ -19,13 +19,30 @@ __extern int __rt_sigaction(int, const struct sigaction *,
struct sigaction *,
 			    size_t);
 #endif
 
+#ifdef __ia64__
+/* We use -mno-pic so our function pointers are straight to the function entry
+   point, but the kernel always expects a descriptor. Thus we create a fake
+   descriptor for each possible signal, update it, and pass that to the kernel
+   instead (the descriptor must remain valid after returning from sigaction
+   until it is replaced). */
+struct {
+	uintptr_t entry;
+	uintptr_t gp;
+} signal_descriptors[_NSIG];
+#endif
+
 int sigaction(int sig, const struct sigaction *act, struct sigaction *oact)
 {
 	int rv;
 
-#if _KLIBC_NEEDS_SA_RESTORER
+#if _KLIBC_NEEDS_SA_RESTORER || defined(__ia64__)
 	struct sigaction sa;
+#endif
+#ifdef __ia64__
+	uintptr_t old_entry;
+#endif
 
+#if _KLIBC_NEEDS_SA_RESTORER
 	if (act && !(act->sa_flags & SA_RESTORER)) {
 		sa = *act;
 		act = &sa;
@@ -37,6 +54,26 @@ int sigaction(int sig, const struct sigaction *act, struct
sigaction *oact)
 	}
 #endif
 
+#ifdef __ia64__
+	if (sig < 0 || sig >= _NSIG) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (oact) {
+		old_entry = signal_descriptors[sig].entry;
+	}
+
+	if (act && act->sa_handler != SIG_IGN && act->sa_handler
!= SIG_DFL) {
+		sa = *act;
+		act = &sa;
+
+		signal_descriptors[sig].entry = (uintptr_t)sa.sa_handler;
+
+		sa.sa_handler = (__sighandler_t)(uintptr_t)&signal_descriptors[sig];
+	}
+#endif
+
 #if _KLIBC_USE_RT_SIG
 # ifdef __sparc__
 	{
@@ -61,5 +98,11 @@ int sigaction(int sig, const struct sigaction *act, struct
sigaction *oact)
 	}
 #endif
 
+#ifdef __ia64__
+	if (oact && oact->sa_handler != SIG_IGN &&
oact->sa_handler != SIG_DFL) {
+		oact->sa_handler = (__sighandler_t)old_entry;
+	}
+#endif
+
 	return rv;
 }
-- 
1.8.5.3

On Sat, 2019-02-02 at 01:35 +0000, James Clarke wrote:
[...]
> --- a/usr/klibc/sigaction.c
> +++ b/usr/klibc/sigaction.c
> @@ -19,13 +19,30 @@ __extern int __rt_sigaction(int, const struct sigaction
*, struct sigaction *,
>  			    size_t);
>  #endif
>  
> +#ifdef __ia64__
> +/* We use -mno-pic so our function pointers are straight to the function
entry
> +   point, but the kernel always expects a descriptor. Thus we create a
fake
> +   descriptor for each possible signal, update it, and pass that to the
kernel
> +   instead (the descriptor must remain valid after returning from
sigaction
> +   until it is replaced). */
> +struct {
> +	uintptr_t entry;
> +	uintptr_t gp;
> +} signal_descriptors[_NSIG];
> +#endif
This should be declared static.

[...]
> +#ifdef __ia64__
> +	if (sig < 0 || sig >= _NSIG) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
> +	if (oact) {
> +		old_entry = signal_descriptors[sig].entry;
> +	}
> +
> +	if (act && act->sa_handler != SIG_IGN &&
act->sa_handler != SIG_DFL) {
> +		sa = *act;
> +		act = &sa;
> +
> +		signal_descriptors[sig].entry = (uintptr_t)sa.sa_handler;
What if sigaction() races with delivery of the signal, so it arrives
between here and the system call?  At the very least this assignment
needs to be made atomic.

Also, what if the system call fails?  In that case the signal handler
must never be called, so we can't do the assignment until after the
call returns.  It seems like we would have to block the signal
temporarily.

Are you sure it's worth the trouble to avoid using function
descriptors?

Ben.
> +		sa.sa_handler = (__sighandler_t)(uintptr_t)&signal_descriptors[sig];
> +	}
> +#endif
> +
>  #if _KLIBC_USE_RT_SIG
>  # ifdef __sparc__
>  	{
> @@ -61,5 +98,11 @@ int sigaction(int sig, const struct sigaction *act,
struct sigaction *oact)
>  	}
>  #endif
>  
> +#ifdef __ia64__
> +	if (oact && oact->sa_handler != SIG_IGN &&
oact->sa_handler != SIG_DFL) {
> +		oact->sa_handler = (__sighandler_t)old_entry;
> +	}
> +#endif
> +
>  	return rv;
>  }
-- 
Ben Hutchings
Knowledge is power.  France is bacon.