Use moby/sys for signal parsing #3213

katiewasnothere · 2021-09-14T02:14:24Z

This PR addresses the issues mentioned in the discussion containerd/containerd#5931 around signal parsing. This PR updates runc to use the moby/sys signal's package. This package supports portable signal parsing.

This PR additionally changes the use of several instances where the golang unix package is used in favor of the syscall package. This allows for the potential use of additional systemd supported signals as discussed containerd/containerd#5693.

katiewasnothere · 2021-09-14T17:44:05Z

@thaJeztah and @kzys fyi

thaJeztah · 2021-09-14T21:45:17Z

kill.go

@@ -53,16 +53,16 @@ signal to the init process of the "ubuntu01" container:
 	},
 }

-func parseSignal(rawSignal string) (unix.Signal, error) {
+func parseSignal(rawSignal string) (syscall.Signal, error) {


I think we can keep this as unix.Signal. Given that the syscall package is deprecated / frozen, and this is all Linux-only code (so we don't have to consider (e.g.) Windows; unix.Signal is an alias for syscall.Signal, so I don't think keeping it should be a problem; https://github.com/golang/sys/blob/751e447fb3d0a97f584890476adddc1d56307388/unix/aliases.go#L13-L14

Sure, done!

cyphar

Can you please split the commits up into at least two commits:

Commit updating golang.org/x/sys. (Is this necessary for moby/sys/signal to work by the way? Also I'm confused why dependabot isn't updating those for us... Maybe because they don't have versions?) and adding github.com/moby/sys/signal.
Commit switching to ParseSignal.

kolyshkin · 2021-09-15T22:02:03Z

Also I'm confused why dependabot isn't updating those for us... Maybe because they don't have versions?

Exactly.

kolyshkin · 2021-09-15T22:09:44Z

Commit updating golang.org/x/sys. (Is this necessary for moby/sys/signal to work by the way?

Probably not, but since it requires v0.0.0-20210630005230-0f9fa26af87c, and we currently require v0.0.0-20210426230700-d19ff857e887, it makes sense to update (I'm not sure if the update should be to the latest version though, probably 20210630... is sufficient).

Indeed, go get github.com/moby/sys/signal@latest updates golang.org/x/sys from v0.0.0-20210426230700-d19ff857e887 to v0.0.0-20210630005230-0f9fa26af87c in our go.mod, and this is what should be done in this PR.

Separating this into two commits is a bit problematic though, because the above go get does not make any sense without actual use of the module.

thaJeztah · 2021-09-16T14:37:05Z

kill.go

-	if !strings.HasPrefix(sig, "SIG") {
-		sig = "SIG" + sig


For other reviewers; I was initially thinking we had to keep this (make sure it has a SIG prefix), or if this had to be moved to signal.ParseSignal, but looks like that's already the case; signal.ParseSignal() normalises by stripping the SIG prefix (if present);

https://github.com/moby/sys/blob/1a1d2afe36340e8eac900548192ab7a43ac584c8/signal/signal.go#L46

signal, ok := SignalMap[strings.TrimPrefix(strings.ToUpper(rawSignal), "SIG")]

Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>

katiewasnothere · 2021-09-16T19:01:35Z

Separating this into two commits is a bit problematic though, because the above go get does not make any sense without actual use of the module.

@cyphar I've update the PR to pull in the golang.org/x/sys version that comes along with pulling in moby/sys/signal. Are you okay with keeping this in the same commit per @kolyshkin suggestion above?

kolyshkin

NACK

I see two problems with this:

We add a whole new package which does the same job as unix.SignalNum, the only simplification is we don't have to add SIG prefix and call strings.ToUpper(). Is it worth it?
moby/sys/signal package fills in the SignalMap during initialization time, which means once we include it, every runc invocation incurs the overhead of doing so, thereas unix.SignalNum does that initialization lazily (when first called), meaning there is no overhead unless you actually call it.

Overall, we have very little to gain and something to lose, so I'd rather not merge this.

katiewasnothere · 2021-09-20T22:45:24Z

We can update moby/sys/signal to lazy initialize if that's preferable @kolyshkin

However, per @kzys's comment below:

moby/signal is slightly "opinionated" than runc or golang.org/x/sys regarding SIGRTMIN handling. The actual value is depending on libc (musl and glibc have the different numeric value). Pushing that to golang.org/x/sys may be hard.

Because of that, I'm bit hesitant to have the logic in runc. runc is the reference implementation and other implementations may need to do what runc does to be compatible with, even the conversation logic is not covered by the OCI Runtime Spec. As a result, we may make the container world somewhat incompatible with musl-based Linux distros such as Alpine.

It sounds like this PR is problematic. More input would be great to understand how to best support SIGRTMIN signals.

cyphar · 2021-09-20T23:46:56Z

@cyphar I've update the PR to pull in the golang.org/x/sys version that comes along with pulling in moby/sys/signal. Are you okay with keeping this in the same commit per @kolyshkin suggestion above?

That's fine, just as long as there is a separate commit for the vendor changes and the code change to make potential cherry-picking and bisecting slightly less painful in the future. 😸

moby/signal is slightly "opinionated" than runc or golang.org/x/sys regarding SIGRTMIN handling. The actual value is depending on libc (musl and glibc have the different numeric value). Pushing that to golang.org/x/sys may be hard.

Ah the wonders of nptl(7) -- I completely forgot about this snag and didn't realise (though I guess it makes sense in retrospect) that glibc and musl would have different sets of realtime signals. It's also great that glibc decided to use the bottom two rather than the top two (if they'd used the top two we wouldn't have this brilliant incompatibility -- though maybe they expected that linux might add new RT signals in the future so bottom two is better than two in the middle).

Yeah this makes this issue quite bad because which signal we should send now somewhat depends on what libc the container process is using which is going to be non-trivial (bordering on impossible) to detect reliably. But on the other hand we definitely should support sending real-time signals because many programs use them (one example is systemd -- rather than using SIGPWR they use SIGRT13 as their shutdown signal -- which as an aside was a very fun thing for the LXC folks to grapple with). We currently support this by just supporting numerical signals but that doesn't help users from separate operating systems who might not know how the numerical signals work for their container...

Yeah this is a bit of a doozy, I might need to think about this one for a bit...

kolyshkin · 2021-09-22T19:42:46Z

As runc is exclusive to Linux, the cross-platform discussion in containerd/containerd#5931 is not really relevant.

In other words, what this PR adds is an ability to specify SIGRTMIN+x or SIGRTMAX-x (where x is a number) signals, and even that is problematic due to musl vs glibc mentioned above in in #3213 (comment)

By the way, I just found one more issue with those SIGRT signal names -- different sources name them differently.

kill (the bash built-in) uses SIGRTMIN to SIGRTMIN+15, then goes to SIGRTMAX-14 to SIGRTMAX:

[kir@kir-rhat runc]$ kill -l
.....
31) SIGSYS	34) SIGRTMIN	35) SIGRTMIN+1	36) SIGRTMIN+2	37) SIGRTMIN+3
38) SIGRTMIN+4	39) SIGRTMIN+5	40) SIGRTMIN+6	41) SIGRTMIN+7	42) SIGRTMIN+8
43) SIGRTMIN+9	44) SIGRTMIN+10	45) SIGRTMIN+11	46) SIGRTMIN+12	47) SIGRTMIN+13
48) SIGRTMIN+14	49) SIGRTMIN+15	50) SIGRTMAX-14	51) SIGRTMAX-13	52) SIGRTMAX-12
53) SIGRTMAX-11	54) SIGRTMAX-10	55) SIGRTMAX-9	56) SIGRTMAX-8	57) SIGRTMAX-7
58) SIGRTMAX-6	59) SIGRTMAX-5	60) SIGRTMAX-4	61) SIGRTMAX-3	62) SIGRTMAX-2
63) SIGRTMAX-1	64) SIGRTMAX	
[kir@kir-rhat runc]$ echo $BASH_VERSION 
5.1.0(1)-release

moby/sys/signal pkg does the same (for Linux):
https://github.com/moby/sys/blob/daaeddaaf61cbd0a2b8ade6b3c6f1f6f92604c17/signal/signal_linux.go#L52-L82
kill (from util-linux) uses and understands RTx, RTMIN+x, RTMAX-x syntaxes:

[kir@kir-rhat runc]$ /bin/kill -l
HUP INT QUIT ILL TRAP ABRT IOT BUS FPE KILL USR1 SEGV USR2 PIPE ALRM
TERM STKFLT CHLD CLD CONT STOP TSTP TTIN TTOU URG XCPU XFSZ VTALRM PROF
WINCH IO POLL PWR SYS RT<N> RTMIN+<N> RTMAX-<N>
[kir@kir-rhat runc]$ rpm -qf /bin/kill
util-linux-2.36.2-1.fc34.x86_64

busybox kill lists RTMIN and RTMAX, understands RTMIN+x and RTMAX-x syntax:

[kir@kir-rhat runc]$ busybox kill -l
...
31) SYS
32) RTMIN
64) RTMAX
[kir@kir-rhat rrbf]$ strace -ekill busybox kill -RTMIN+2 1
kill(1, SIGRT_2)                        = -1 EPERM (Operation not permitted)
...
[kir@kir-rhat rrbf]$ strace -ekill busybox kill -RTMAX-2 1
kill(1, SIGRT_30)                       = -1 EPERM (Operation not permitted)
...
[kir@kir-rhat runc]$ [kir@kir-rhat runc]$ busybox --help
BusyBox v1.33.1 (2021-05-06 18:08:02 UTC) multi-call binary.
...

strace (see above) lists these signals as SIGRTMIN, SIGRT_1 (== SIGRTMIN+1), and so on up to SIGRT_32 (not SIGRTMAX!). Note that this RT_x notation is not the same as bash's kill RTx!
systemd(1) man page lists RTMIN+N (up to RTMIN+28):

       SIGRTMIN+0
           Enters default mode, starts the default.target unit. This is mostly equivalent to systemctl isolate default.target.

....

       SIGRTMIN+27, SIGRTMIN+28
           Sets the log target to "console" on SIGRTMIN+27 (or "kmsg" on SIGRTMIN+28), in a fashion equivalent to systemd.log_target=console (or systemd.log_target=kmsg on SIGRTMIN+28) on
           the kernel command line.
....

kill (the zsh built-in) is interesting -- it does not list or accept any SIGRT notations.
kill (the dash built-in) is the same as bash above.

kolyshkin · 2021-09-22T20:09:11Z

It's worse than that actually:

kernel, busybox and strace have RTMIN as 32 (actually busybox is configurable, see https://git.busybox.net/busybox/tree/libbb/u_signal_names.c#n16, maybe strace is also configurable);
glibc, bash, /bin/kill, moby/sys/signal etc. have rtmin as 34;
musl have rtmin as 35.

Given all that 🤹🏻, we should probably not try to interpret SIGRT names at all, or we will be wrong.

kzys · 2021-09-22T20:34:09Z

What a mess... 🤦

FYI strace is checking SIGRTMIN on asm/signal.h apparently.

https://github.com/strace/strace/blob/d2b1a5d79476f1df2adc1465ac53eefb85a52f50/configure.ac#L550-L558

katiewasnothere · 2021-09-23T03:45:34Z

I think this can be closed then and discussion can continue on containerd/containerd#5931 for cross platform signal handling in containerd

thaJeztah · 2021-09-23T09:56:15Z

What a mess... 🤦

Yes, quite so. Can't get my head around how that's an actual "api"

I think this can be closed then and discussion can continue on containerd/containerd#5931 for cross platform signal handling in containerd

Yes, we can continue the discussion there, and try to find a solution. After all, conversion needs to happen somewhere. Personally I'd still like it to be as close to where it's used (runc / OCI runtime if possible), but yes, we'd need to resolve how the conversion should be handled.

kolyshkin · 2021-09-23T17:05:45Z

Can't get my head around how that's an actual "api"

Yeah, this is so flawed. Apparently, whoever wrote this did not accounted for having different programs compiled with different versions of libc (or maybe even without libc, like busybox).

I found one more piece of info, it's signal(7) man page, sub-section "Real-time signals". Here's a relevant quote:

   Real-time signals
       Starting  with  version  2.2, Linux supports real-time signals as originally defined in
       the POSIX.1b real-time extensions (and now included in  POSIX.1-2001).   The  range  of
       supported   real-time   signals  is  defined  by  the  macros  SIGRTMIN  and  SIGRTMAX.
       POSIX.1-2001 requires that an implementation  support  at  least  _POSIX_RTSIG_MAX  (8)
       real-time signals.

       The Linux kernel supports a range of 33 different real-time signals, numbered 32 to 64.
       However, the glibc POSIX threads implementation internally uses two (for NPTL) or three
       (for LinuxThreads) real-time signals (see pthreads(7)), and adjusts the value of SIGRT‐
       MIN suitably (to 34 or 35).  Because the range of available  real-time  signals  varies
       according  to  the  glibc threading implementation (and this variation can occur at run
       time according to the available kernel and glibc), and indeed the  range  of  real-time
       signals  varies  across  UNIX systems, programs should never refer to real-time signals
       using hard-coded numbers, but instead should always refer to  real-time  signals  using
       the  notation  SIGRTMIN+n,  and include suitable (run-time) checks that SIGRTMIN+n does
       not exceed SIGRTMAX.

       Unlike standard signals, real-time signals have no predefined meanings: the entire  set
       of real-time signals can be used for application-defined purposes.

katiewasnothere force-pushed the update_signal_use branch 2 times, most recently from 2237a3a to 2b53c12 Compare September 14, 2021 17:31

thaJeztah reviewed Sep 14, 2021

View reviewed changes

katiewasnothere force-pushed the update_signal_use branch from 2b53c12 to d708ae6 Compare September 14, 2021 21:59

katiewasnothere mentioned this pull request Sep 14, 2021

Update runc kill wrapper to take a string for signal containerd/go-runc#78

Open

cyphar requested changes Sep 15, 2021

View reviewed changes

thaJeztah reviewed Sep 16, 2021

View reviewed changes

Update runc to use portable signal parsing and handling

a319490

Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>

katiewasnothere force-pushed the update_signal_use branch from d708ae6 to a319490 Compare September 16, 2021 18:58

kolyshkin requested changes Sep 20, 2021

View reviewed changes

katiewasnothere closed this Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use moby/sys for signal parsing #3213

Use moby/sys for signal parsing #3213

katiewasnothere commented Sep 14, 2021

katiewasnothere commented Sep 14, 2021

thaJeztah Sep 14, 2021

katiewasnothere Sep 14, 2021

cyphar left a comment

kolyshkin commented Sep 15, 2021 •

edited

kolyshkin commented Sep 15, 2021

thaJeztah Sep 16, 2021

katiewasnothere commented Sep 16, 2021

kolyshkin left a comment

katiewasnothere commented Sep 20, 2021

cyphar commented Sep 20, 2021

kolyshkin commented Sep 22, 2021

kolyshkin commented Sep 22, 2021

kzys commented Sep 22, 2021

katiewasnothere commented Sep 23, 2021

thaJeztah commented Sep 23, 2021

kolyshkin commented Sep 23, 2021

Use moby/sys for signal parsing #3213

Use moby/sys for signal parsing #3213

Conversation

katiewasnothere commented Sep 14, 2021

katiewasnothere commented Sep 14, 2021

thaJeztah Sep 14, 2021

Choose a reason for hiding this comment

katiewasnothere Sep 14, 2021

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

kolyshkin commented Sep 15, 2021 • edited

kolyshkin commented Sep 15, 2021

thaJeztah Sep 16, 2021

Choose a reason for hiding this comment

katiewasnothere commented Sep 16, 2021

kolyshkin left a comment

Choose a reason for hiding this comment

katiewasnothere commented Sep 20, 2021

cyphar commented Sep 20, 2021

kolyshkin commented Sep 22, 2021

kolyshkin commented Sep 22, 2021

kzys commented Sep 22, 2021

katiewasnothere commented Sep 23, 2021

thaJeztah commented Sep 23, 2021

kolyshkin commented Sep 23, 2021

kolyshkin commented Sep 15, 2021 •

edited