Simplify signal handling code in libthr by removing use of SYS_sigreturn

[dg612_cam.ac.uk]

Summary

The use of SYS_sigreturn is unnecessary here.

If handle_signal is called when a signal is delivered, it can just return normally back to sigcode which will call sigreturn anyway.

In case handle_signal is called by check_deferred_signal, using setcontext is better than SYS_sigreturn because that is the correct system call to pair with the earlier getcontext.

[jhb]

I think this is correct (and had discussed this with Dapeng previously), but probably best to see what @kib thinks.

[kib]

Can you upload the full context for the diff?

I tend to think that the removal of sigreturn() from handle_signal on the sync signal delivery path is fine. But I do not like the use of setcontext() for deferred delivery. Yes, sigreturn() vs. setcontext() should have not semantic difference when context is correct. But for incorrect sigreturn's we do provide different error handling, including generation of the signals with right si_info (removing this changes the ABI subtly), and verbose printouts. Also I like the idea that signal handling ends up with sigreturn() (twice).

I am curious why do you think that this change makes the code cleaner.

[dg612_cam.ac.uk]

Updated diff to use sigreturn instead of setcontext during deferred signal handling.

[kib]

Please upload patches with full context.

lib/libthr/thread/thr_sig.c

400

syscall(SYS_sigreturn) was used there not frivolously. Potentially the sigreturn symbol needs to be resolved there, and this might deadlock if rtld bind_lock is already owned. Look at _thr_rtld_init() how it is done (and I am not sure that it worth the trouble to issue this syscall on libthr startup).

[dg612_cam.ac.uk]

In D44893#1026733, @kib wrote:

I am curious why do you think that this change makes the code cleaner.

I think the change makes the control flow easier to follow, as it is more well-bracketed than the current implementation. For sync signals, control returns naturally to where the signal is initiated i.e., the sigcode. For deferred signals, putting the getcontext/sigreturn pair in the same function highlights that non-standard control flow is happening.

[dg612_cam.ac.uk]

Use syscall(SYS_sigreturn) instead of sigreturn.

[dg612_cam.ac.uk]

lib/libthr/thread/thr_sig.c

400

syscall(SYS_sigreturn) was used there not frivolously. Potentially the sigreturn symbol needs to be resolved there, and this might deadlock if rtld bind_lock is already owned. Look at _thr_rtld_init() how it is done (and I am not sure that it worth the trouble to issue this syscall on libthr startup).

@jhb, @brooks, and I have been discussing about removing uses of syscall(2) in the base system because it complicates security mechanisms that intercept and audit system calls (e.g. compartmentalisation on CheriBSD). It seems that, at least in libthr, syscall(2) is only used to pre-resolve symbols that cannot be called with the effect of a no-op.

One workaround I see is to define a function pointer in libthr pointing to these syscalls so that they are force-resolved when libthr is loaded.

[imp]

lib/libthr/thread/thr_sig.c

312

I think this is better...
the arm64 contexts have grown in complexity as new features were added in an ABI compatible way, so copying them like this gets tricky... It's no wonder that POSiX deleted these interfaces in 2008...

[dg612_cam.ac.uk]

@imp I wonder if you could help me commit this patch, since I don't have push rights to FreeBSD. Thanks!

[imp]

In D44893#1036933, @dg612_cam.ac.uk wrote:

@imp I wonder if you could help me commit this patch, since I don't have push rights to FreeBSD. Thanks!

You bet! This was the other commit of yours that I had on my radar to test, but ETOOMANYINTERRUPTIONS. I'll commit after lunch since it looks ready to my eye, kib@ seems happy with it and you think it's ready.

[arichardson]

It looks like removing this call to memcpy might actually also fix the TSan issue I tried to work around in https://reviews.freebsd.org/D28536. Might be time to try run the TSan testsuite again.