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

syscall, internal/poll: accept4-to-accept fallback removal broke Go code on Synology DSM 6.2 ARM devices #57333

Closed
bradfitz opened this issue Dec 15, 2022 · 10 comments
Closed

syscall, internal/poll: accept4-to-accept fallback removal broke Go code on Synology DSM 6.2 ARM devices #57333

bradfitz opened this issue Dec 15, 2022 · 10 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge

Comments

@bradfitz
Copy link
Contributor

We've started getting bug reports that our application is broken on older ARM-based Synology devices. SyncThing is also hitting it in syncthing/syncthing#8325.

In #45964 it was decided to require Linux 2.6.32 at minimum because that seemed like a popular cut-off as seen in the wild (due to Synology DSM 6.2) as seen by data I'd earlier collected.

Then some Go patches landed to simplify things based on that new minimum:

Because packages in the Synology Package Center don't update often (it's like quarters to a year, not days like in the iOS/Google app stores), people are just hitting this now.

As @sfanous says in syncthing/syncthing#8325 (comment):

Did some more digging, it seems accept4 for ARM was added in 2.6.36.
....

So it seems like maybe we should revert that 6705191 accept4->accept fallback if we truly want to say that 2.6.32 is our Linux minimum?

Oh, but I see at https://github.com/golang/go/wiki/MinimumRequirements that it says:

Kernel version 2.6.32 or later. [This depends on architecture though, we need to have specific builder for this.] Linux/ARMv5 requires much newer kernels, at least v3.1 (for __kuser_cmpxchg64).

... and sure enough, some of these older Synology boxes experiencing problems are ARMv5 (e.g. Linux ds212j 2.6.32.12 #25556 Thu Jul 1 14:25:48 CST 2021 armv5tel GNU/Linux synology_88f6281_212j) so maybe accept4 would just be the first of many problems they'd hit. I suppose I could try to run the Go std tests on one of these old boxes, but it's a bespoke userspace so would require some machinery running elsewhere.

/cc @tklauser @ianlancetaylor @knyar (who dig much of this digging)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 15, 2022
@bradfitz bradfitz changed the title internal/poll: accept4-to-accept fallback removal broke Go code on Synology DSM 6.2 ARM devices syscall, internal/poll: accept4-to-accept fallback removal broke Go code on Synology DSM 6.2 ARM devices Dec 15, 2022
@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 15, 2022 •
edited

Does Go actually use __kuser_cmpxchg64? I see:

// Linux/ARM atomic operations.

// Because there is so much variation in ARM devices,
// the Linux kernel provides an appropriate compare-and-swap
// implementation at address 0xffff0fc0.  Caller sets:
//      R0 = old value
//      R1 = new value
//      R2 = addr
//      LR = return address
// The function returns with CS true if the swap happened.
// http://lxr.linux.no/linux+v2.6.37.2/arch/arm/kernel/entry-armv.S#L850
// On older kernels (before 2.6.24) the function can incorrectly
// report a conflict, so we have to double-check the compare ourselves
// and retry if necessary.
//
// https://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b49c0f24cf6744a3f4fd09289fe7cade349dead5
//
TEXT cas<>(SB),NOSPLIT,$0
        MOVW    $0xffff0fc0, R15 // R15 is hardware PC.

TEXT ·Cas(SB),NOSPLIT|NOFRAME,$0
        MOVB    runtime·goarm(SB), R11
        CMP     $7, R11
        BLT     2(PC)
        JMP     ·armcas(SB)
        JMP     kernelcas<>(SB)

TEXT kernelcas<>(SB),NOSPLIT,$0
        MOVW    ptr+0(FP), R2
        // trigger potential paging fault here,
        // because we don't know how to traceback through __kuser_cmpxchg
        MOVW    (R2), R0
        MOVW    old+4(FP), R0

@ianlancetaylor
Copy link
Contributor

Looks like https://go.dev/cl/93637 (from 2018) removed the use of __kuser_cmpxchg64, saying "On linux/arm, 64-bit CAS kernel helper is dropped, as we're trying to move away from kernel helpers."

@ianlancetaylor
Copy link
Contributor

The Linux man page says that accept4 was added in kernel version 2.6.28. However, I do see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21d93e2e29722d7832f61cc56d73fb953ee6578e which says "sys_accept4() was added in kernel 2.6.28, but ARM was not updated to include it." So, yeah. I think we need to revert CL 346849. CC @tklauser .

@ianlancetaylor
Copy link
Contributor

@gopherbot Please open backport issues.

We thought that we could require support for accept4 on Linux when we updated the minimum kernel version to 2.6.32, but it turns out that on 32-bit ARM accept4 was only added in kernel version 2.6.36.

This was referenced Dec 15, 2022
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #57338 (for 1.18), #57339 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@tklauser
Copy link
Member

The Linux man page says that accept4 was added in kernel version 2.6.28. However, I do see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21d93e2e29722d7832f61cc56d73fb953ee6578e which says "sys_accept4() was added in kernel 2.6.28, but ARM was not updated to include it." So, yeah. I think we need to revert CL 346849. CC @tklauser .

I agree and I think we'd need to revert https://go.dev/cl/422375 (dropping the accept fallback from internal/poll), https://go.dev/cl/410737 (removing syscall.accept for linux/loong64), https://go.dev/cl/386415 (removing syscall.accept for all Linux platforms except loong64) in addition.

@ianlancetaylor
Copy link
Contributor

I'm working on a patch, rather than a set of reverts.

@gopherbot
Copy link

Change https://go.dev/cl/457995 mentions this issue: syscall, internal/poll: fall back to accept on linux-arm

@gopherbot
Copy link

Change https://go.dev/cl/457996 mentions this issue: [release-branch.go1.19] syscall, internal/poll: fall back to accept on linux-arm

@gopherbot
Copy link

Change https://go.dev/cl/457997 mentions this issue: [release-branch.go1.18] syscall, internal/poll: fall back to accept on linux-arm

This was referenced Dec 16, 2022
Closed
Closed
bradfitz pushed a commit to tailscale/go that referenced this issue Dec 19, 2022
@ianlancetaylor @bradfitz
[release-branch.go1.19] syscall, internal/poll: fall back to accept o…
2f7f1ad 
…n linux-arm

Our minimum Linux version is 2.6.32, and the accept4 system call was
introduced in 2.6.28, so we use accept4 everywhere. Unfortunately,
it turns out that the accept4 system call was only added to
linux-arm in 2.6.36, so for linux-arm only we need to try the accept4
system call and then fall back to accept if it doesn't work.

The code we use on linux-arm is the code we used in Go 1.17.
On non-arm platforms we continue using the simpler code introduced
in Go 1.18.

Adding accept4 to the ARM Linux kernel was:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21d93e2e29722d7832f61cc56d73fb953ee6578e

For golang#57333
Fixes golang#57339

Cherry picked from commit 8d809f950986db3162ec62aa9ebb490a2d20f621
at https://go-review.googlesource.com/c/go/+/457996 which is due
to be merged into Go's release-branch.go1.19.

Change-Id: I6680cb54dd4d3514a6887dda8906e6708c64459d
This was referenced Dec 19, 2022
Merged
Closed
bradfitz pushed a commit to tailscale/go that referenced this issue Dec 20, 2022
@ianlancetaylor @bradfitz
[release-branch.go1.19] syscall, internal/poll: fall back to accept o…
dc0ce63 
…n linux-arm

Our minimum Linux version is 2.6.32, and the accept4 system call was
introduced in 2.6.28, so we use accept4 everywhere. Unfortunately,
it turns out that the accept4 system call was only added to
linux-arm in 2.6.36, so for linux-arm only we need to try the accept4
system call and then fall back to accept if it doesn't work.

The code we use on linux-arm is the code we used in Go 1.17.
On non-arm platforms we continue using the simpler code introduced
in Go 1.18.

Adding accept4 to the ARM Linux kernel was:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21d93e2e29722d7832f61cc56d73fb953ee6578e

For golang#57333
Fixes golang#57339

Cherry picked from commit 8d809f950986db3162ec62aa9ebb490a2d20f621
at https://go-review.googlesource.com/c/go/+/457996 which is due
to be merged into Go's release-branch.go1.19.

Change-Id: I6680cb54dd4d3514a6887dda8906e6708c64459d
gopherbot pushed a commit that referenced this issue Dec 21, 2022
@ianlancetaylor @gopherbot
[release-branch.go1.18] syscall, internal/poll: fall back to accept o…
856ec21 
…n linux-arm

Our minimum Linux version is 2.6.32, and the accept4 system call was
introduced in 2.6.28, so we use accept4 everywhere. Unfortunately,
it turns out that the accept4 system call was only added to
linux-arm in 2.6.36, so for linux-arm only we need to try the accept4
system call and then fall back to accept if it doesn't work.

The code we use on linux-arm is the code we used in Go 1.17.
On non-arm platforms we continue using the simpler code introduced
in Go 1.18.

Adding accept4 to the ARM Linux kernel was:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21d93e2e29722d7832f61cc56d73fb953ee6578e

For #57333
Fixes #57338

Change-Id: I6680cb54dd4d3514a6887dda8906e6708c64459d
Reviewed-on: https://go-review.googlesource.com/c/go/+/457997
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Dec 21, 2022
@ianlancetaylor @gopherbot
[release-branch.go1.19] syscall, internal/poll: fall back to accept o…
41eb70a 
…n linux-arm

Our minimum Linux version is 2.6.32, and the accept4 system call was
introduced in 2.6.28, so we use accept4 everywhere. Unfortunately,
it turns out that the accept4 system call was only added to
linux-arm in 2.6.36, so for linux-arm only we need to try the accept4
system call and then fall back to accept if it doesn't work.

The code we use on linux-arm is the code we used in Go 1.17.
On non-arm platforms we continue using the simpler code introduced
in Go 1.18.

Adding accept4 to the ARM Linux kernel was:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=21d93e2e29722d7832f61cc56d73fb953ee6578e

For #57333
Fixes #57339

Change-Id: I6680cb54dd4d3514a6887dda8906e6708c64459d
Reviewed-on: https://go-review.googlesource.com/c/go/+/457996
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dswarbrick dswarbrick mentioned this issue Dec 28, 2022
Closed
@AlBundy33 AlBundy33 mentioned this issue Dec 29, 2022
Closed
@jadbaz jadbaz mentioned this issue Dec 29, 2022
Closed
@johnsonjh johnsonjh mentioned this issue Jan 5, 2023
Closed
@golang golang locked and limited conversation to collaborators Dec 16, 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
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bradfitz @tklauser @ianlancetaylor @gopherbot