From ec44093c9777fb873a4a400a950ff695a0d8c137 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Mon, 18 Apr 2022 17:27:50 -0700 Subject: [PATCH] introduce atomicbitops 32-bit types Part of a series of changes that will end with prohibiting use of sync/atomic (u)int32 functions. See cl/440484071 for more details. PiperOrigin-RevId: 442673296 --- pkg/atomicbitops/32b_32bit.go | 201 +++++++++++++++++++++ pkg/atomicbitops/32b_64bit.go | 201 +++++++++++++++++++++ pkg/atomicbitops/BUILD | 2 + pkg/atomicbitops/aligned_32bit_unsafe.go | 6 + pkg/atomicbitops/aligned_64bit.go | 6 + pkg/atomicbitops/aligned_test.go | 16 ++ pkg/atomicbitops/atomicbitops.go | 24 ++- pkg/atomicbitops/atomicbitops_amd64.s | 8 +- pkg/atomicbitops/atomicbitops_arm64.s | 8 +- pkg/atomicbitops/atomicbitops_noasm.go | 28 ++- pkg/atomicbitops/atomicbitops_test.go | 13 +- pkg/sentry/platform/kvm/bluepill_fault.go | 8 +- pkg/sentry/platform/kvm/bluepill_unsafe.go | 7 +- pkg/sentry/platform/kvm/kvm_test.go | 5 +- pkg/sentry/platform/kvm/machine.go | 28 +-- pkg/sentry/platform/kvm/machine_unsafe.go | 2 +- pkg/usermem/bytes_io_unsafe.go | 8 +- 17 files changed, 508 insertions(+), 63 deletions(-) create mode 100644 pkg/atomicbitops/32b_32bit.go create mode 100644 pkg/atomicbitops/32b_64bit.go diff --git a/pkg/atomicbitops/32b_32bit.go b/pkg/atomicbitops/32b_32bit.go new file mode 100644 index 000000000..51853403f --- /dev/null +++ b/pkg/atomicbitops/32b_32bit.go @@ -0,0 +1,201 @@ +// Copyright 2022 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build arm || mips || mipsle || 386 +// +build arm mips mipsle 386 + +package atomicbitops + +import ( + "sync/atomic" + + "gvisor.dev/gvisor/pkg/sync" +) + +// Note that this file is *identical* to 32b_64bit.go, as go_stateify gets +// confused about build tags if these are not separated. + +// LINT.IfChange + +// Int32 is an atomic int32. +// +// The default value is zero. +// +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// +// +stateify savable +type Int32 struct { + _ sync.NoCopy + value int32 +} + +// FromInt32 returns an Int32 initialized to value v. +//go:nosplit +func FromInt32(v int32) Int32 { + return Int32{value: v} +} + +// Load is analogous to atomic.LoadInt32. +//go:nosplit +func (i *Int32) Load() int32 { + return atomic.LoadInt32(&i.value) +} + +// RacyLoad is analogous to reading an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (i *Int32) RacyLoad() int32 { + return i.value +} + +// Store is analogous to atomic.StoreInt32. +//go:nosplit +func (i *Int32) Store(v int32) { + atomic.StoreInt32(&i.value, v) +} + +// RacyStore is analogous to setting an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// +//go:nosplit +func (i *Int32) RacyStore(v int32) { + i.value = v +} + +// Add is analogous to atomic.AddInt32. +//go:nosplit +func (i *Int32) Add(v int32) int32 { + return atomic.AddInt32(&i.value, v) +} + +// RacyAdd is analogous to adding to an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (i *Int32) RacyAdd(v int32) int32 { + i.value += v + return i.value +} + +// Swap is analogous to atomic.SwapInt32. +//go:nosplit +func (i *Int32) Swap(v int32) int32 { + return atomic.SwapInt32(&i.value, v) +} + +// CompareAndSwap is analogous to atomic.CompareAndSwapInt32. +//go:nosplit +func (i *Int32) CompareAndSwap(oldVal, newVal int32) bool { + return atomic.CompareAndSwapInt32(&i.value, oldVal, newVal) +} + +//go:nosplit +func (i *Int32) ptr() *int32 { + return &i.value +} + +// Uint32 is an atomic uint32. +// +// See aligned_unsafe.go in this directory for justification. +// +// +stateify savable +type Uint32 struct { + _ sync.NoCopy + value uint32 +} + +// FromUint32 returns an Uint32 initialized to value v. +//go:nosplit +func FromUint32(v uint32) Uint32 { + return Uint32{value: v} +} + +// Load is analogous to atomic.LoadUint32. +//go:nosplit +func (u *Uint32) Load() uint32 { + return atomic.LoadUint32(&u.value) +} + +// RacyLoad is analogous to reading an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (u *Uint32) RacyLoad() uint32 { + return u.value +} + +// Store is analogous to atomic.StoreUint32. +//go:nosplit +func (u *Uint32) Store(v uint32) { + atomic.StoreUint32(&u.value, v) +} + +// RacyStore is analogous to setting an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (u *Uint32) RacyStore(v uint32) { + u.value = v +} + +// Add is analogous to atomic.AddUint32. +//go:nosplit +func (u *Uint32) Add(v uint32) uint32 { + return atomic.AddUint32(&u.value, v) +} + +// RacyAdd is analogous to adding to an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (u *Uint32) RacyAdd(v uint32) uint32 { + u.value += v + return u.value +} + +// Swap is analogous to atomic.SwapUint32. +//go:nosplit +func (u *Uint32) Swap(v uint32) uint32 { + return atomic.SwapUint32(&u.value, v) +} + +// CompareAndSwap is analogous to atomic.CompareAndSwapUint32. +//go:nosplit +func (u *Uint32) CompareAndSwap(oldVal, newVal uint32) bool { + return atomic.CompareAndSwapUint32(&u.value, oldVal, newVal) +} + +//go:nosplit +func (u *Uint32) ptr() *uint32 { + return &u.value +} + +// LINT.ThenChange(32b_64bit.go) diff --git a/pkg/atomicbitops/32b_64bit.go b/pkg/atomicbitops/32b_64bit.go new file mode 100644 index 000000000..92878c334 --- /dev/null +++ b/pkg/atomicbitops/32b_64bit.go @@ -0,0 +1,201 @@ +// Copyright 2022 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build !arm && !mips && !mipsle && !386 +// +build !arm,!mips,!mipsle,!386 + +package atomicbitops + +import ( + "sync/atomic" + + "gvisor.dev/gvisor/pkg/sync" +) + +// Note that this file is *identical* to 32b_32bit.go, as go_stateify gets +// confused about build tags if these are not separated. + +// LINT.IfChange + +// Int32 is an atomic int32. +// +// The default value is zero. +// +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// +// +stateify savable +type Int32 struct { + _ sync.NoCopy + value int32 +} + +// FromInt32 returns an Int32 initialized to value v. +//go:nosplit +func FromInt32(v int32) Int32 { + return Int32{value: v} +} + +// Load is analogous to atomic.LoadInt32. +//go:nosplit +func (i *Int32) Load() int32 { + return atomic.LoadInt32(&i.value) +} + +// RacyLoad is analogous to reading an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (i *Int32) RacyLoad() int32 { + return i.value +} + +// Store is analogous to atomic.StoreInt32. +//go:nosplit +func (i *Int32) Store(v int32) { + atomic.StoreInt32(&i.value, v) +} + +// RacyStore is analogous to setting an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// +//go:nosplit +func (i *Int32) RacyStore(v int32) { + i.value = v +} + +// Add is analogous to atomic.AddInt32. +//go:nosplit +func (i *Int32) Add(v int32) int32 { + return atomic.AddInt32(&i.value, v) +} + +// RacyAdd is analogous to adding to an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (i *Int32) RacyAdd(v int32) int32 { + i.value += v + return i.value +} + +// Swap is analogous to atomic.SwapInt32. +//go:nosplit +func (i *Int32) Swap(v int32) int32 { + return atomic.SwapInt32(&i.value, v) +} + +// CompareAndSwap is analogous to atomic.CompareAndSwapInt32. +//go:nosplit +func (i *Int32) CompareAndSwap(oldVal, newVal int32) bool { + return atomic.CompareAndSwapInt32(&i.value, oldVal, newVal) +} + +//go:nosplit +func (i *Int32) ptr() *int32 { + return &i.value +} + +// Uint32 is an atomic uint32. +// +// See aligned_unsafe.go in this directory for justification. +// +// +stateify savable +type Uint32 struct { + _ sync.NoCopy + value uint32 +} + +// FromUint32 returns an Uint32 initialized to value v. +//go:nosplit +func FromUint32(v uint32) Uint32 { + return Uint32{value: v} +} + +// Load is analogous to atomic.LoadUint32. +//go:nosplit +func (u *Uint32) Load() uint32 { + return atomic.LoadUint32(&u.value) +} + +// RacyLoad is analogous to reading an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (u *Uint32) RacyLoad() uint32 { + return u.value +} + +// Store is analogous to atomic.StoreUint32. +//go:nosplit +func (u *Uint32) Store(v uint32) { + atomic.StoreUint32(&u.value, v) +} + +// RacyStore is analogous to setting an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (u *Uint32) RacyStore(v uint32) { + u.value = v +} + +// Add is analogous to atomic.AddUint32. +//go:nosplit +func (u *Uint32) Add(v uint32) uint32 { + return atomic.AddUint32(&u.value, v) +} + +// RacyAdd is analogous to adding to an atomic value without using +// synchronization. +// +// It may be helpful to document why a racy operation is permitted. +// +//go:nosplit +func (u *Uint32) RacyAdd(v uint32) uint32 { + u.value += v + return u.value +} + +// Swap is analogous to atomic.SwapUint32. +//go:nosplit +func (u *Uint32) Swap(v uint32) uint32 { + return atomic.SwapUint32(&u.value, v) +} + +// CompareAndSwap is analogous to atomic.CompareAndSwapUint32. +//go:nosplit +func (u *Uint32) CompareAndSwap(oldVal, newVal uint32) bool { + return atomic.CompareAndSwapUint32(&u.value, oldVal, newVal) +} + +//go:nosplit +func (u *Uint32) ptr() *uint32 { + return &u.value +} + +// LINT.ThenChange(32b_32bit.go) diff --git a/pkg/atomicbitops/BUILD b/pkg/atomicbitops/BUILD index 10abe2451..993946e43 100644 --- a/pkg/atomicbitops/BUILD +++ b/pkg/atomicbitops/BUILD @@ -5,6 +5,8 @@ package(licenses = ["notice"]) go_library( name = "atomicbitops", srcs = [ + "32b_32bit.go", + "32b_64bit.go", "aligned_32bit_unsafe.go", "aligned_64bit.go", "atomicbitops.go", diff --git a/pkg/atomicbitops/aligned_32bit_unsafe.go b/pkg/atomicbitops/aligned_32bit_unsafe.go index 65f02b0be..60063fc1a 100644 --- a/pkg/atomicbitops/aligned_32bit_unsafe.go +++ b/pkg/atomicbitops/aligned_32bit_unsafe.go @@ -27,6 +27,9 @@ import ( // Int64 is an atomic int64 that is guaranteed to be 64-bit // aligned, even on 32-bit systems. // +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// // Per https://golang.org/pkg/sync/atomic/#pkg-note-BUG: // // "On ARM, 386, and 32-bit MIPS, it is the caller's responsibility to arrange @@ -121,6 +124,9 @@ func (i *Int64) CompareAndSwap(oldVal, newVal int64) bool { // Uint64 is an atomic uint64 that is guaranteed to be 64-bit // aligned, even on 32-bit systems. // +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// // Per https://golang.org/pkg/sync/atomic/#pkg-note-BUG: // // "On ARM, 386, and 32-bit MIPS, it is the caller's responsibility to arrange diff --git a/pkg/atomicbitops/aligned_64bit.go b/pkg/atomicbitops/aligned_64bit.go index f04d7a64b..f3754bce8 100644 --- a/pkg/atomicbitops/aligned_64bit.go +++ b/pkg/atomicbitops/aligned_64bit.go @@ -29,6 +29,9 @@ import ( // // The default value is zero. // +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// // See aligned_32bit_unsafe.go in this directory for justification. // // +stateify savable @@ -113,6 +116,9 @@ func (i *Int64) ptr() *int64 { // aligned, even on 32-bit systems. On most architectures, it's just a regular // uint64. // +// Don't add fields to this struct. It is important that it remain the same +// size as its builtin analogue. +// // See aligned_unsafe.go in this directory for justification. // // +stateify savable diff --git a/pkg/atomicbitops/aligned_test.go b/pkg/atomicbitops/aligned_test.go index 886efb772..31e4532bd 100644 --- a/pkg/atomicbitops/aligned_test.go +++ b/pkg/atomicbitops/aligned_test.go @@ -16,6 +16,7 @@ package atomicbitops import ( "testing" + "unsafe" ) func TestAtomiciInt64(t *testing.T) { @@ -33,3 +34,18 @@ func TestAtomicUint64(t *testing.T) { }{} v.v64.Add(1) } + +func TestSize(t *testing.T) { + if size := unsafe.Sizeof(Int32{}); size != 4 { + t.Errorf("Int32 should be 4 bytes in size, but is %d bytes", size) + } + if size := unsafe.Sizeof(Uint32{}); size != 4 { + t.Errorf("Uint32 should be 4 bytes in size, but is %d bytes", size) + } + if size := unsafe.Sizeof(Int64{}); size != 8 { + t.Errorf("Int32 should be 8 bytes in size, but is %d bytes", size) + } + if size := unsafe.Sizeof(Uint64{}); size != 8 { + t.Errorf("Int32 should be 8 bytes in size, but is %d bytes", size) + } +} diff --git a/pkg/atomicbitops/atomicbitops.go b/pkg/atomicbitops/atomicbitops.go index 63ab8c3cc..696f0108e 100644 --- a/pkg/atomicbitops/atomicbitops.go +++ b/pkg/atomicbitops/atomicbitops.go @@ -22,17 +22,33 @@ package atomicbitops // AndUint32 atomically applies bitwise AND operation to *addr with val. -func AndUint32(addr *uint32, val uint32) +func AndUint32(addr *Uint32, val uint32) { + andUint32(&addr.value, val) +} + +func andUint32(addr *uint32, val uint32) // OrUint32 atomically applies bitwise OR operation to *addr with val. -func OrUint32(addr *uint32, val uint32) +func OrUint32(addr *Uint32, val uint32) { + orUint32(&addr.value, val) +} + +func orUint32(addr *uint32, val uint32) // XorUint32 atomically applies bitwise XOR operation to *addr with val. -func XorUint32(addr *uint32, val uint32) +func XorUint32(addr *Uint32, val uint32) { + xorUint32(&addr.value, val) +} + +func xorUint32(addr *uint32, val uint32) // CompareAndSwapUint32 is like sync/atomic.CompareAndSwapUint32, but returns // the value previously stored at addr. -func CompareAndSwapUint32(addr *uint32, old, new uint32) uint32 +func CompareAndSwapUint32(addr *Uint32, old, new uint32) uint32 { + return compareAndSwapUint32(&addr.value, old, new) +} + +func compareAndSwapUint32(addr *uint32, old, new uint32) uint32 // AndUint64 atomically applies bitwise AND operation to *addr with val. func AndUint64(addr *Uint64, val uint64) { diff --git a/pkg/atomicbitops/atomicbitops_amd64.s b/pkg/atomicbitops/atomicbitops_amd64.s index 5df9134ea..e6b4e56a9 100644 --- a/pkg/atomicbitops/atomicbitops_amd64.s +++ b/pkg/atomicbitops/atomicbitops_amd64.s @@ -16,28 +16,28 @@ #include "textflag.h" -TEXT ·AndUint32(SB),NOSPLIT,$0-12 +TEXT ·andUint32(SB),NOSPLIT,$0-12 MOVQ addr+0(FP), BX MOVL val+8(FP), AX LOCK ANDL AX, 0(BX) RET -TEXT ·OrUint32(SB),NOSPLIT,$0-12 +TEXT ·orUint32(SB),NOSPLIT,$0-12 MOVQ addr+0(FP), BX MOVL val+8(FP), AX LOCK ORL AX, 0(BX) RET -TEXT ·XorUint32(SB),NOSPLIT,$0-12 +TEXT ·xorUint32(SB),NOSPLIT,$0-12 MOVQ addr+0(FP), BX MOVL val+8(FP), AX LOCK XORL AX, 0(BX) RET -TEXT ·CompareAndSwapUint32(SB),NOSPLIT,$0-20 +TEXT ·compareAndSwapUint32(SB),NOSPLIT,$0-20 MOVQ addr+0(FP), DI MOVL old+8(FP), AX MOVL new+12(FP), DX diff --git a/pkg/atomicbitops/atomicbitops_arm64.s b/pkg/atomicbitops/atomicbitops_arm64.s index cffa38347..600c591cf 100644 --- a/pkg/atomicbitops/atomicbitops_arm64.s +++ b/pkg/atomicbitops/atomicbitops_arm64.s @@ -16,7 +16,7 @@ #include "textflag.h" -TEXT ·AndUint32(SB),NOSPLIT,$0-12 +TEXT ·andUint32(SB),NOSPLIT,$0-12 MOVD ptr+0(FP), R0 MOVW val+8(FP), R1 again: @@ -26,7 +26,7 @@ again: CBNZ R3, again RET -TEXT ·OrUint32(SB),NOSPLIT,$0-12 +TEXT ·orUint32(SB),NOSPLIT,$0-12 MOVD ptr+0(FP), R0 MOVW val+8(FP), R1 again: @@ -36,7 +36,7 @@ again: CBNZ R3, again RET -TEXT ·XorUint32(SB),NOSPLIT,$0-12 +TEXT ·xorUint32(SB),NOSPLIT,$0-12 MOVD ptr+0(FP), R0 MOVW val+8(FP), R1 again: @@ -46,7 +46,7 @@ again: CBNZ R3, again RET -TEXT ·CompareAndSwapUint32(SB),NOSPLIT,$0-20 +TEXT ·compareAndSwapUint32(SB),NOSPLIT,$0-20 MOVD addr+0(FP), R0 MOVW old+8(FP), R1 MOVW new+12(FP), R2 diff --git a/pkg/atomicbitops/atomicbitops_noasm.go b/pkg/atomicbitops/atomicbitops_noasm.go index 6ed68ebec..db8ca46f3 100644 --- a/pkg/atomicbitops/atomicbitops_noasm.go +++ b/pkg/atomicbitops/atomicbitops_noasm.go @@ -17,51 +17,49 @@ package atomicbitops -import ( - "sync/atomic" -) +import "sync/atomic" //go:nosplit -func AndUint32(addr *uint32, val uint32) { +func AndUint32(addr *Uint32, val uint32) { for { - o := atomic.LoadUint32(addr) + o := addr.Load() n := o & val - if atomic.CompareAndSwapUint32(addr, o, n) { + if atomic.CompareAndSwapUint32(&addr.value, o, n) { break } } } //go:nosplit -func OrUint32(addr *uint32, val uint32) { +func OrUint32(addr *Uint32, val uint32) { for { - o := atomic.LoadUint32(addr) + o := addr.Load() n := o | val - if atomic.CompareAndSwapUint32(addr, o, n) { + if atomic.CompareAndSwapUint32(&addr.value, o, n) { break } } } //go:nosplit -func XorUint32(addr *uint32, val uint32) { +func XorUint32(addr *Uint32, val uint32) { for { - o := atomic.LoadUint32(addr) + o := addr.Load() n := o ^ val - if atomic.CompareAndSwapUint32(addr, o, n) { + if atomic.CompareAndSwapUint32(&addr.value, o, n) { break } } } //go:nosplit -func CompareAndSwapUint32(addr *uint32, old, new uint32) (prev uint32) { +func CompareAndSwapUint32(addr *Uint32, old, new uint32) (prev uint32) { for { - prev = atomic.LoadUint32(addr) + prev = addr.Load() if prev != old { return } - if atomic.CompareAndSwapUint32(addr, old, new) { + if atomic.CompareAndSwapUint32(&addr.value, old, new) { return } } diff --git a/pkg/atomicbitops/atomicbitops_test.go b/pkg/atomicbitops/atomicbitops_test.go index 0f908175b..a00230109 100644 --- a/pkg/atomicbitops/atomicbitops_test.go +++ b/pkg/atomicbitops/atomicbitops_test.go @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +checkalignedignore package atomicbitops import ( @@ -23,20 +24,20 @@ import ( const iterations = 100 -func detectRaces32(val, target uint32, fn func(*uint32, uint32)) bool { +func detectRaces32(val, target uint32, fn func(*Uint32, uint32)) bool { runtime.GOMAXPROCS(100) for n := 0; n < iterations; n++ { - x := val + x := FromUint32(val) var wg sync.WaitGroup for i := uint32(0); i < 32; i++ { wg.Add(1) - go func(a *uint32, i uint32) { + go func(a *Uint32, i uint32) { defer wg.Done() fn(a, uint32(1<