Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/sys/unix: Sigset_t follows libc instead of kernel definition on Linux #55349

Closed
lmb opened this issue Sep 22, 2022 · 18 comments
Closed

x/sys/unix: Sigset_t follows libc instead of kernel definition on Linux #55349

lmb opened this issue Sep 22, 2022 · 18 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@lmb
Copy link
Contributor

lmb commented Sep 22, 2022

Sigset_t in x/sys/unix is defined as follows for amd64:

type Sigset_t struct {
	Val [16]uint64
}

In total, it has space for 1024 signals. However, Linux syscalls expect a sigset_t like so:

#define _NSIG		64

#define _NSIG_BPW	64

#define _NSIG_WORDS	(_NSIG / _NSIG_BPW)

typedef struct {
	unsigned long sig[_NSIG_WORDS];
} sigset_t;

This becomes a problem for syscalls that require sizeof(sigset_t) to be passed, for example rt_sigprocmask:

int syscall(SYS_rt_sigprocmask, int how, const kernel_sigset_t *set,
                       kernel_sigset_t *oldset, size_t sigsetsize)

Passing unsafe.Sizeof(unix.Sigset_t{}) will return EINVAL.

This stackoverflow answer suggests that the discrepancy is due to glibc defining a larger than necessary sigset_t: https://unix.stackexchange.com/a/399356

From my POV, x/sys/unix should follow the kernel layout, not the libc one. I think this hasn't been a problem so far since all existing users of Sigset_t only take a pointer, so a larger than necessary structure is not a problem. Is there something that would break if we changed the definition?
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 22, 2022
@gopherbot gopherbot added this to the Unreleased milestone Sep 22, 2022
@lmb
Copy link
Contributor Author

lmb commented Sep 22, 2022

The runtime has https://github.com/golang/go/blob/d11c58eedbee29b4912dd50508b36ad15ebb739e/src/runtime/os_linux_generic.go and friends to deal with this. Maybe it makes sense to just lift that code into sys/unix?

@cherrymui
Copy link
Member

cc @ianlancetaylor @bradfitz @tklauser

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 22, 2022
@ianlancetaylor
Copy link
Contributor

I'm not sure there is a good answer here. What should happen if the kernel changes the number of signals?. Arguably people calling unix.Syscall(unix.SYS_RT_SIGPROGMASK, ...) need to know what they are doing, and not try to use Sigset_t.

@lmb
Copy link
Contributor Author

lmb commented Sep 23, 2022

I'm not sure there is a good answer here. What should happen if the kernel changes the number of signals?

I think that would only work if the kernel started to support a sigset_t smaller than the in kernel sigset_t at the same time. Otherwise user space would break, which is against Linux policy.

From the sys/unix perspective this would mean making the struct larger and recompiling.

@lmb
Copy link
Contributor Author

lmb commented Sep 23, 2022

Maybe we can copy what was done for Signalfd? I could add RtSigprocmask() like so:

https://cs.opensource.google/go/x/sys/+/fb04ddd9:unix/syscall_linux.go;l=1944

We omit the size argument from the wrapper and pass the size ourselves. Passing NSIG/8 is kind of a hack but I guess it works well enough, and this avoids changing sigset_t.

Maybe in addition export NSIG so that implementations of sigaddset can return an error if trying to set an unsupported signal?

@ianlancetaylor
Copy link
Contributor

I don't see why userspace would break if the kernel increased the number of signals. As far as I know the reason that the size is passed to the system call is to permit the kernel to increase the signal count. The kernel would simply continue to accept the smaller size.

@ianlancetaylor
Copy link
Contributor

I agree that if add Sigprocmask then it ought to work like Signalfd. And perhaps that is the way to go. Normally the Go runtime should be in charge of the signal mask, but I suppose there are special cases where it is helpful for Go code to control it temporarily. Actually it shouldn't be Sigprocmask, it should be Pthread_sigmask, although I think they may be the same on Linux.

I don't see a reason to export NSIG, that just seems to set us up for problems in the unlikely event that it ever changes. Note that just recompiling with a new version of x/sys/unix doesn't really help because programs would need to run on both old and new kernels.

@lmb
Copy link
Contributor Author

lmb commented Sep 23, 2022

The kernel would simply continue to accept the smaller size.

Yes, that's what I was trying to say. I suspect we're talking past each other with this sigset_t size stuff, but adding a rt_sigprocmask wrapper would solve my problem without having to dive deeper here :)

I suppose there are special cases where it is helpful for Go code to control it temporarily.

Yes, we need it for #55243.

Actually it shouldn't be Sigprocmask, it should be Pthread_sigmask, although I think they may be the same on Linux.

This would be the first Pthread* syscall to be exported so I did some digging:

Is it OK if I add PthreadSigmask for Linux only to get started?

@ianlancetaylor
Copy link
Contributor

According to POSIX sigprocmask is not defined in a multi-threaded process, so using sigprocmask seems clearly wrong.

It's fine to add PthreadSigmask for Linux only as long as the API can in principle be implemented on any Unix system. Thanks.

lmb added a commit to lmb/sys that referenced this issue Sep 26, 2022
@lmb
unix: add PthreadSigmask
beb6663 
Add a syscall wrapper for SYS_RT_SIGPROCMASK and export it as PthreadSigmask.
The latter is defined by POSIX and can therefore be implemented by Darwin, etc.
later on.

Follow the approach used by Signalfd of passing _C__NSIG/8 as sigsetsize. This
avoids exporting _C__NSIG and allows the syscall to work with the current
definition of Sigset_t, which doesn't match the kernel definition of Sigset_t.

Updates golang/go#55349
@lmb lmb mentioned this issue Sep 26, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/434555 mentions this issue: unix: add PthreadSigmask

lmb added a commit to lmb/sys that referenced this issue Sep 26, 2022
@lmb
unix: add PthreadSigmask for Linux
295e0e8 
Add a syscall wrapper for SYS_RT_SIGPROCMASK and export it as
PthreadSigmask. The latter is defined by POSIX and can therefore
be implemented by Darwin, etc. later on.

Follow the approach used by Signalfd of passing _C__NSIG/8 as
sigsetsize. This avoids exporting _C__NSIG and allows the syscall
to work with the current definition of Sigset_t, which doesn't
match the kernel definition of Sigset_t.

Updates golang/go#55349
@gopherbot
Copy link

Change https://go.dev/cl/435095 mentions this issue: unix: add PthreadSigmask for Linux

@cherrymui
Copy link
Member

I think the name PthreadSigmask is a bit confusing. It sounds like it is using the pthread library (the C library), but it doesn't?

@lmb
Copy link
Contributor Author

lmb commented Sep 27, 2022

I found it confusing too, but I believe it's the correct name. pthread is a standard, not a library: https://man7.org/linux/man-pages/man7/pthreads.7.html Some of pthreads can be implemented in user space, some of it in kernel space (see my links for Darwin where pthread_sigmask is an actual syscall). There are even distinct implementations on Linux of pthreads, for example nptl and musl.

@ianlancetaylor
Copy link
Contributor

pthread_sigmask is the POSIX name (https://pubs.opengroup.org/onlinepubs/009696899/functions/pthread_sigmask.html). I agree that it's not a great name but I don't know of a better one.

@cherrymui
Copy link
Member

Okay, thanks. Then I guess PthreadSigmask is probably okay.

gopherbot pushed a commit to golang/sys that referenced this issue Sep 27, 2022
@lmb @gopherbot
unix: add PthreadSigmask for Linux
d9d178b 
Add a syscall wrapper for SYS_RT_SIGPROCMASK and export it as
PthreadSigmask. The latter is defined by POSIX and can therefore
be implemented by Darwin, etc. later on.

Follow the approach used by Signalfd of passing _C__NSIG/8 as
sigsetsize. This avoids exporting _C__NSIG and allows the syscall
to work with the current definition of Sigset_t, which doesn't
match the kernel definition of Sigset_t.

Updates golang/go#55349

Change-Id: I49dc93366a7d316d820b0c25ecdef2ebb584634b
Reviewed-on: https://go-review.googlesource.com/c/sys/+/435095
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/435775 mentions this issue: unix: add SIG_BLOCK and friends for Linux

@lmb
Copy link
Contributor Author

lmb commented Sep 28, 2022

Sorry for sending another CL, I hadn't realised that the SIG_* constants used by PthreadSigmask use arch specific values.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 28, 2022
gopherbot pushed a commit to golang/sys that referenced this issue Sep 28, 2022
@lmb @gopherbot
unix: add SIG_BLOCK and friends for Linux
c57c793 
The recently added PthreadSigmask syscall expects a parameter
how that specifies which action to perform on the thread's signal
set. The value of this parameter differs between arches. Add the
SIG_* constants to have a portable source.

Updates golang/go#55349

Change-Id: I7cb42d7c2c42e9b3d22123e8de74960121d89313
Reviewed-on: https://go-review.googlesource.com/c/sys/+/435775
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@lmb
Copy link
Contributor Author

lmb commented Sep 28, 2022

Thanks very much everyone!

@lmb lmb closed this as completed Sep 28, 2022
@golang golang locked and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@lmb @dmitshur @ianlancetaylor @gopherbot @cherrymui