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.

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

x/sys/unix: OpenBSD mmap syscall number (unix.SYS_MMAP) obsolete #59661

Open
dusaint opened this issue Apr 16, 2023 · 8 comments
Open

x/sys/unix: OpenBSD mmap syscall number (unix.SYS_MMAP) obsolete #59661

dusaint opened this issue Apr 16, 2023 · 8 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-OpenBSD
Milestone

Comments

@dusaint
Copy link

dusaint commented Apr 16, 2023

On 2021-12-23, a new syscall for mmap was introduced:

49	STD NOLOCK	{ void *sys_mmap(void *addr, size_t len, int prot, \
			    int flags, int fd, off_t pos); }

At the same time, the existing mmap syscall was renamed from sys_mmap to sys_pad_mmap (since it contains a now-obsolete argument for padding).

See sys/kern/syscalls.master:

openbsd/src@1d60349#diff-e8c6a075c6e1240e27216779540b66f3db43b14a3b3615c2ecb7a111faa54504

On 2023-02-11, the original mmap syscall was retired entirely:

openbsd/src@8c7f5cc

With the release of OpenBSD 7.3 on 2023-04-10, any go program that uses mmap on OpenBSD will crash with SIGSYS.

For example, here's a snippet from kdump of a failing run of gotosocial:

 83839 gotosocial CALL  (via syscall) #197 (obsolete pad_mmap)()
 83839 gotosocial PSIG  SIGSYS caught handler=0x4682c0 mask=0<>
 83839 gotosocial RET   #197 (obsolete pad_mmap) -1 errno 78 Function not implemented

This is a bug both in this project and in modernc.org/libc. Here, the constant unix.SYS_MMAP needs to be changed from 197 to 49. I'll also file a bug in that project for them to switch both the number and how they're invoking syscall.

@ianlancetaylor ianlancetaylor changed the title OpenBSD mmap syscall number (unix.SYS_MMAP) obsolete x/sys/unix: OpenBSD mmap syscall number (unix.SYS_MMAP) obsolete Apr 17, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 17, 2023
@gopherbot gopherbot added this to the Unreleased milestone Apr 17, 2023
@ianlancetaylor ianlancetaylor added OS-OpenBSD NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Apr 17, 2023
@ianlancetaylor ianlancetaylor modified the milestones: Unreleased, Backlog Apr 17, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/openbsd

@landryb
Copy link

landryb commented Apr 17, 2023

Fwiw, influxdb 1.8.10 on openbsd 7.3 had this issue (crashing with SIGSYS at startup, with a goroutine trace pointing at sys_mmap), and upgrading the x/sys module to 0.5.0 fixed it. cf https://marc.info/?l=openbsd-ports&m=168166545112152&w=2

so maybe it's already fixed 'somewhere' in the go ecosystem and projects need to update their dependencies ?

@sthen
Copy link

sthen commented Apr 17, 2023

There are two related problems. One is that older versions of x/sys use syscall() itself, this has been replaced with calls to libc wrappers. The other is that the syscall tables are many (openbsd) releases out of date and in particular the old ones with "pad" arguments have recently been retired - golang/sys@3086868 synced some tables but not syscalls. Fixing that will partly involve syncing the table but will also involve fixing the call parameters.

There is an additional twist in that openbsd is considering blocking syscall() completely. It has not yet been done though the linker now prints a warning when it's used. If that's done then syscall access will become impossible except through libc.

@dusaint
Copy link
Author

dusaint commented Apr 18, 2023

FWIW, just calling unix.mmap works as expected -- the SIGSYS I saw came from another package's use of the unix.SYS_MMAP constant. I haven't tested the other changed syscalls, but wouldn't be surprised if they follow the same pattern.

@wilmhub
Copy link

wilmhub commented Apr 20, 2023

As a side effect bootstrapping go by building Go 1.4 from source is no longer possible on OpenBSD 7.3. During the build of cmd/go go_bootstrap is crashing with SIGSYS, due to a mmap call.

@4a6f656c
Copy link
Contributor

As a side effect bootstrapping go by building Go 1.4 from source is no longer possible on OpenBSD 7.3. During the build of cmd/go go_bootstrap is crashing with SIGSYS, due to a mmap call.

The minimum bootstrap for Go 1.20 is Go 1.17.3:

https://go.dev/doc/go1.20#bootstrap

As such, there is no effort being made to keep Go 1.4 working on OpenBSD.

@4a6f656c
Copy link
Contributor

This is a bug both in this project and in modernc.org/libc. Here, the constant unix.SYS_MMAP needs to be changed from 197 to 49. I'll also file a bug in that project for them to switch both the number and how they're invoking syscall.

The short version is - I do not believe there is any way to fix this safely. It is not just the syscall number that changed, but also the arguments that are being passed to the syscall. This means that updating SYS_MMAP to the new value will break existing code on some platforms, and it would mean silently misbehaving code instead of a SIGSYS. This is why they're marked as deprecated:

https://github.com/golang/sys/blob/master/unix/zsysnum_openbsd_386.go#L9

Any code that wants to make system calls on OpenBSD should use the libc stubs directly, which the likes of unix.Mmap() does, avoiding the issue entirely. As also noted by @sthen, it is highly likely that the syscall(2) system call will be removed in future versions of OpenBSD - so even if the SYS_MMAP value was changed, it would only be kicking the can slightly further down the road.

One issue worth noting is that x/sys/unix.Mmap does not provide any way to pass in an address, which restricts some operations:

#56123

If this is needed then calling mmap(2) via libc stubs would need to be handled outside of golang.org/x/sys/unix.

@dusaint
Copy link
Author

dusaint commented Apr 22, 2023

This is a bug both in this project and in modernc.org/libc. Here, the constant unix.SYS_MMAP needs to be changed from 197 to 49. I'll also file a bug in that project for them to switch both the number and how they're invoking syscall.

The short version is - I do not believe there is any way to fix this safely. It is not just the syscall number that changed, but also the arguments that are being passed to the syscall. This means that updating SYS_MMAP to the new value will break existing code on some platforms, and it would mean silently misbehaving code instead of a SIGSYS. This is why they're marked as deprecated:

https://github.com/golang/sys/blob/master/unix/zsysnum_openbsd_386.go#L9

Any code that wants to make system calls on OpenBSD should use the libc stubs directly, which the likes of unix.Mmap() does, avoiding the issue entirely. As also noted by @sthen, it is highly likely that the syscall(2) system call will be removed in future versions of OpenBSD - so even if the SYS_MMAP value was changed, it would only be kicking the can slightly further down the road.

It would indeed be better for downstream packages to just call unix.MMap() or use some other libc stub. But that isn't really the concern here. That unix.SYS_MMAP is now an incorrect value is, and it's reasonable to fix that without worrying overmuch about how projects are going to use it. If source-level backward compatibility is a goal of this project, then perhaps name the updated constant something like unix.SYS_MMAP2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-OpenBSD
Projects
None yet
Development

No branches or pull requests

7 participants