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

sync/atomic, runtime/internal/atomic: LoadUint32 SEGFAULTs on ARM when loading from read-only memory #23777

Closed
paulzhol opened this issue Feb 11, 2018 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@paulzhol
Copy link
Member

paulzhol commented Feb 11, 2018

package main

import (
	"os"
	"sync/atomic"
	"syscall"
	"testing"
	"unsafe"
)

func TestMmapAtomicLoad(t *testing.T) {
	b, err := syscall.Mmap(-1, 0, os.Getpagesize(), syscall.PROT_EXEC|syscall.PROT_READ, syscall.MAP_ANON|syscall.MAP_PRIVATE)
	if err != nil {
		t.Fatal(err)
	}
	defer syscall.Munmap(b)

	val := atomic.LoadUint32((*uint32)(unsafe.Pointer(&b[0])))
	_ = val
}

GOOS=linux GOARM=7 RPi3:

./atomic_test -test.v
=== RUN   TestMmapAtomicLoad
unexpected fault address 0xb6e7e000
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x2 addr=0xb6e7e000 pc=0xffff0fcc]

goroutine 5 [running]:
runtime.throw(0x11effd, 0x5)
        /usr/local/go1.9/src/runtime/panic.go:605 +0x70 fp=0x10d25f48 sp=0x10d25f3c pc=0x3a8b0
runtime.sigpanic()
        /usr/local/go1.9/src/runtime/signal_unix.go:374 +0x1cc fp=0x10d25f6c sp=0x10d25f48 pc=0x4deb0
sync/atomic.LoadUint32(0xb6e7e000, 0x127b08)
        /usr/local/go1.9/src/sync/atomic/asm_linux_arm.s:172 +0x14 fp=0x10d25f74 sp=0x10d25f70 pc=0x11500
command-line-arguments.TestMmapAtomicLoad(0x10d8a090)
        /tmp/atomic_test.go:18 +0xfc fp=0x10d25fb8 sp=0x10d25f74 pc=0xe5f4c
testing.tRunner(0x10d8a090, 0x127868)
        /usr/local/go1.9/src/testing/testing.go:746 +0xb0 fp=0x10d25fe4 sp=0x10d25fb8 pc=0xb3a58
runtime.goexit()
        /usr/local/go1.9/src/runtime/asm_arm.s:971 +0x4 fp=0x10d25fe4 sp=0x10d25fe4 pc=0x64900
created by testing.(*T).Run
        /usr/local/go1.9/src/testing/testing.go:789 +0x258

goroutine 1 [chan receive]:
testing.(*T).Run(0x10d8a000, 0x12131f, 0x12, 0x127868, 0xb3a04)
        /usr/local/go1.9/src/testing/testing.go:790 +0x270
testing.runTests.func1(0x10d8a000)
        /usr/local/go1.9/src/testing/testing.go:1004 +0x50
testing.tRunner(0x10d8a000, 0x10d33eec)
        /usr/local/go1.9/src/testing/testing.go:746 +0xb0
testing.runTests(0x10d0a050, 0x1aec38, 0x1, 0x1, 0x8)
        /usr/local/go1.9/src/testing/testing.go:1002 +0x26c
testing.(*M).Run(0x10d33f8c, 0x1)
        /usr/local/go1.9/src/testing/testing.go:921 +0xe0
main.main()
        command-line-arguments/_test/_testmain.go:44 +0xa4

GOOS=freebsd GOARM=7 Allwiner A20:

 ./atomic_test -test.v
=== RUN   TestMmapAtomicLoad
unexpected fault address 0x50442000
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x2 addr=0x50442000 pc=0x11494]

goroutine 18 [running]:
runtime.throw(0x11f4fc, 0x5)
	/usr/home/paulzhol/go1.9/src/runtime/panic.go:605 +0x70 fp=0x303bcf4c sp=0x303bcf40 pc=0x3a218
runtime.sigpanic()
	/usr/home/paulzhol/go1.9/src/runtime/signal_unix.go:374 +0x1cc fp=0x303bcf70 sp=0x303bcf4c pc=0x4d794
sync/atomic.LoadUint32(0x50442000, 0x127a78)
	/usr/home/paulzhol/go1.9/src/sync/atomic/asm_freebsd_arm.s:62 +0x8 fp=0x303bcf74 sp=0x303bcf74 pc=0x11494
command-line-arguments.TestMmapAtomicLoad(0x30424090)
	/tmp/atomic_test.go:18 +0xfc fp=0x303bcfb8 sp=0x303bcf74 pc=0xe6490
testing.tRunner(0x30424090, 0x1277d8)
	/usr/home/paulzhol/go1.9/src/testing/testing.go:746 +0xb0 fp=0x303bcfe4 sp=0x303bcfb8 pc=0xb3f9c
runtime.goexit()
	/usr/home/paulzhol/go1.9/src/runtime/asm_arm.s:971 +0x4 fp=0x303bcfe4 sp=0x303bcfe4 pc=0x64244
created by testing.(*T).Run
	/usr/home/paulzhol/go1.9/src/testing/testing.go:789 +0x258

goroutine 1 [chan receive]:
testing.(*T).Run(0x30424000, 0x121633, 0x12, 0x1277d8, 0xb3f48)
	/usr/home/paulzhol/go1.9/src/testing/testing.go:790 +0x270
testing.runTests.func1(0x30424000)
	/usr/home/paulzhol/go1.9/src/testing/testing.go:1004 +0x50
testing.tRunner(0x30424000, 0x303cfeec)
	/usr/home/paulzhol/go1.9/src/testing/testing.go:746 +0xb0
testing.runTests(0x30404020, 0x1aec40, 0x1, 0x1, 0x8)
	/usr/home/paulzhol/go1.9/src/testing/testing.go:1002 +0x26c
testing.(*M).Run(0x303cff8c, 0x1)
	/usr/home/paulzhol/go1.9/src/testing/testing.go:921 +0xe0
main.main()
	command-line-arguments/_test/_testmain.go:44 +0xa4

The cause is the STREX instruction which attempts to write into a protected area used in the implementation of LoadUint32 (implemented with Xadd(&addr,0) ).
The motivation for using atomic.Load on a read-only address is explained here.

@bradfitz bradfitz added this to the Go1.11 milestone Feb 11, 2018
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 11, 2018
@bradfitz
Copy link
Contributor

/cc @aclements @randall77

@randall77
Copy link
Contributor

Any reason we're not just doing a load followed by a DMB?

@paulzhol
Copy link
Member Author

Could it be due to ARMv6 / ARMv7 support? DMB is only available on ARMv7.
Although I've seen FreeBSD use this:
#define dmb() __asm __volatile("mcr p15, 0, %0, c7, c10, 5" : : "r" (0) : "memory") for ARMv6.
Also the armcas already does check runtime·goarm(SB) before issuing DMB.

@gopherbot
Copy link

Change https://golang.org/cl/93615 mentions this issue: runtime/internal/atomic, sync/atomic: unify memory barrier macros

@gopherbot
Copy link

Change https://golang.org/cl/94275 mentions this issue: runtime/internal/atomic: don't use Cas in atomic.Load on ARM

@golang golang locked and limited conversation to collaborators Apr 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants