From f481172b53390f9bb0be491ef00da20687b92b84 Mon Sep 17 00:00:00 2001 From: NymanRobin Date: Mon, 4 Mar 2024 12:49:13 +0200 Subject: [PATCH] Convert atomic.Value to atomic.Pointer[T] --- pkg/log/log.go | 4 ++-- pkg/sentry/kernel/rseq.go | 4 ++-- pkg/sentry/kernel/seccomp.go | 6 +++--- pkg/sentry/kernel/task.go | 12 +++++------- pkg/sentry/kernel/task_clone.go | 4 ++-- pkg/sentry/kernel/task_log.go | 12 +++++++----- pkg/sentry/kernel/task_start.go | 1 - pkg/sentry/kernel/thread_group.go | 4 ++-- pkg/sentry/pgalloc/pgalloc.go | 18 +++++++++--------- pkg/sentry/pgalloc/save_restore.go | 2 +- pkg/sync/seqatomic/seqatomic_test.go | 11 ++++++----- 11 files changed, 39 insertions(+), 39 deletions(-) diff --git a/pkg/log/log.go b/pkg/log/log.go index af95fb327..581aa77c7 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -250,11 +250,11 @@ func (l *BasicLogger) SetLevel(level Level) { var logMu sync.Mutex // log is the default logger. -var log atomic.Value +var log atomic.Pointer[BasicLogger] // Log retrieves the global logger. func Log() *BasicLogger { - return log.Load().(*BasicLogger) + return log.Load() } // SetTarget sets the log target. diff --git a/pkg/sentry/kernel/rseq.go b/pkg/sentry/kernel/rseq.go index 709b1f165..033cdb060 100644 --- a/pkg/sentry/kernel/rseq.go +++ b/pkg/sentry/kernel/rseq.go @@ -133,7 +133,7 @@ func (t *Task) ClearRSeq(addr hostarch.Addr, length, signature uint32) error { // OldRSeqCriticalRegion returns a copy of t's thread group's current // old restartable sequence. func (t *Task) OldRSeqCriticalRegion() OldRSeqCriticalRegion { - return *t.tg.oldRSeqCritical.Load().(*OldRSeqCriticalRegion) + return *t.tg.oldRSeqCritical.Load() } // SetOldRSeqCriticalRegion replaces t's thread group's old restartable @@ -387,7 +387,7 @@ func (t *Task) rseqAddrInterrupt() { // Preconditions: The caller must be running on the task goroutine. func (t *Task) oldRSeqInterrupt() { - r := t.tg.oldRSeqCritical.Load().(*OldRSeqCriticalRegion) + r := t.tg.oldRSeqCritical.Load() if ip := t.Arch().IP(); r.CriticalSection.Contains(hostarch.Addr(ip)) { t.Debugf("Interrupted rseq critical section at %#x; restarting at %#x", ip, r.Restart) t.Arch().SetIP(uintptr(r.Restart)) diff --git a/pkg/sentry/kernel/seccomp.go b/pkg/sentry/kernel/seccomp.go index ea5f48a9a..f0d7659ec 100644 --- a/pkg/sentry/kernel/seccomp.go +++ b/pkg/sentry/kernel/seccomp.go @@ -141,7 +141,7 @@ func (t *Task) checkSeccompSyscall(sysno int32, args arch.SyscallArguments, ip h func (t *Task) evaluateSyscallFilters(sysno int32, args arch.SyscallArguments, ip hostarch.Addr) uint32 { ret := uint32(linux.SECCOMP_RET_ALLOW) - ts := t.seccomp.Load().(*taskSeccomp) + ts := t.seccomp.Load() if ts == nil { return ret } @@ -280,7 +280,7 @@ func (t *Task) AppendSyscallFilter(p bpf.Program, syncAll bool) error { totalLength := p.Length() newSeccomp := &taskSeccomp{} - if ts := t.seccomp.Load().(*taskSeccomp); ts != nil { + if ts := t.seccomp.Load(); ts != nil { for _, f := range ts.filters { totalLength += f.Length() + 4 } @@ -313,7 +313,7 @@ func (t *Task) AppendSyscallFilter(p bpf.Program, syncAll bool) error { // seccomp syscall filtering mode, appropriate for both prctl(PR_GET_SECCOMP) // and /proc/[pid]/status. func (t *Task) SeccompMode() int { - if ts := t.seccomp.Load().(*taskSeccomp); ts != nil && len(ts.filters) > 0 { + if ts := t.seccomp.Load(); ts != nil && len(ts.filters) > 0 { return linux.SECCOMP_MODE_FILTER } return linux.SECCOMP_MODE_NONE diff --git a/pkg/sentry/kernel/task.go b/pkg/sentry/kernel/task.go index 4b0a131a4..12ffae9d8 100644 --- a/pkg/sentry/kernel/task.go +++ b/pkg/sentry/kernel/task.go @@ -411,7 +411,7 @@ type Task struct { // logPrefix is a string containing the task's thread ID in the root PID // namespace, and is prepended to log messages emitted by Task.Infof etc. - logPrefix atomic.Value `state:"nosave"` + logPrefix atomic.Pointer[string] `state:"nosave"` // traceContext and traceTask are both used for tracing, and are // updated along with the logPrefix in updateInfoLocked. @@ -450,12 +450,10 @@ type Task struct { // seccomp contains all seccomp-bpf syscall filters applicable to the task. // The type of the atomic is *taskSeccomp. - // Writing needs to be protected by the signal mutex. Note that due to - // atomic.Value limitations (atomic.Value.Store(nil) panics), a nil - // seccomp is always represented as a typed nil (i.e. (*taskSeccomp)(nil)). + // Writing needs to be protected by the signal mutex. // // seccomp is owned by the task goroutine. - seccomp atomic.Value `state:".(*taskSeccomp)"` + seccomp atomic.Pointer[taskSeccomp] `state:".(*taskSeccomp)"` // If cleartid is non-zero, treat it as a pointer to a ThreadID in the // task's virtual address space; when the task exits, set the pointed-to @@ -622,7 +620,7 @@ func (t *Task) loadPtraceTracer(tracer *Task) { } func (t *Task) saveSeccomp() *taskSeccomp { - return t.seccomp.Load().(*taskSeccomp) + return t.seccomp.Load() } func (t *Task) loadSeccomp(seccompData *taskSeccomp) { @@ -632,7 +630,7 @@ func (t *Task) loadSeccomp(seccompData *taskSeccomp) { // afterLoad is invoked by stateify. func (t *Task) afterLoad() { t.updateInfoLocked() - if ts := t.seccomp.Load().(*taskSeccomp); ts != nil { + if ts := t.seccomp.Load(); ts != nil { ts.populateCache(t) } t.interruptChan = make(chan struct{}, 1) diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go index 5ebf0b3e8..6d6b56b36 100644 --- a/pkg/sentry/kernel/task_clone.go +++ b/pkg/sentry/kernel/task_clone.go @@ -323,12 +323,12 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) { // "If fork/clone and execve are allowed by @prog, any child processes will // be constrained to the same filters and system call ABI as the parent." - // Documentation/prctl/seccomp_filter.txt - if ts := t.seccomp.Load().(*taskSeccomp); ts != nil { + if ts := t.seccomp.Load(); ts != nil { seccompCopy := ts.copy() seccompCopy.populateCache(nt) nt.seccomp.Store(seccompCopy) } else { - nt.seccomp.Store((*taskSeccomp)(nil)) + nt.seccomp.Store(nil) } if args.Flags&linux.CLONE_VFORK != 0 { nt.vforkParent = t diff --git a/pkg/sentry/kernel/task_log.go b/pkg/sentry/kernel/task_log.go index 16500ac8b..85db6e7e9 100644 --- a/pkg/sentry/kernel/task_log.go +++ b/pkg/sentry/kernel/task_log.go @@ -37,21 +37,21 @@ const ( // Infof logs an formatted info message by calling log.Infof. func (t *Task) Infof(fmt string, v ...any) { if log.IsLogging(log.Info) { - log.InfofAtDepth(1, t.logPrefix.Load().(string)+fmt, v...) + log.InfofAtDepth(1, *t.logPrefix.Load()+fmt, v...) } } // Warningf logs a warning string by calling log.Warningf. func (t *Task) Warningf(fmt string, v ...any) { if log.IsLogging(log.Warning) { - log.WarningfAtDepth(1, t.logPrefix.Load().(string)+fmt, v...) + log.WarningfAtDepth(1, *t.logPrefix.Load()+fmt, v...) } } // Debugf creates a debug string that includes the task ID. func (t *Task) Debugf(fmt string, v ...any) { if log.IsLogging(log.Debug) { - log.DebugfAtDepth(1, t.logPrefix.Load().(string)+fmt, v...) + log.DebugfAtDepth(1, *t.logPrefix.Load()+fmt, v...) } } @@ -196,9 +196,11 @@ func (t *Task) updateInfoLocked() { pid := t.tg.pidns.tgids[t.tg] tid := t.tg.pidns.tids[t] if rootPID == pid && rootTID == tid { - t.logPrefix.Store(fmt.Sprintf("[% 4d:% 4d] ", pid, tid)) + prefix := fmt.Sprintf("[% 4d:% 4d] ", pid, tid) + t.logPrefix.Store(&prefix) } else { - t.logPrefix.Store(fmt.Sprintf("[% 4d(%4d):% 4d(%4d)] ", rootPID, pid, rootTID, tid)) + prefix := fmt.Sprintf("[% 4d(%4d):% 4d(%4d)] ", rootPID, pid, rootTID, tid) + t.logPrefix.Store(&prefix) } t.rebuildTraceContext(rootTID) diff --git a/pkg/sentry/kernel/task_start.go b/pkg/sentry/kernel/task_start.go index c5cb5411a..f4423f411 100644 --- a/pkg/sentry/kernel/task_start.go +++ b/pkg/sentry/kernel/task_start.go @@ -176,7 +176,6 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error) t.netns = cfg.NetworkNamespace t.creds.Store(cfg.Credentials) t.endStopCond.L = &t.tg.signalHandlers.mu - t.seccomp.Store((*taskSeccomp)(nil)) // We don't construct t.blockingTimer until Task.run(); see that function // for justification. diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index dbd0f6f15..8896b9ac3 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -238,7 +238,7 @@ type ThreadGroup struct { execed bool // oldRSeqCritical is the thread group's old rseq critical region. - oldRSeqCritical atomic.Value `state:".(*OldRSeqCriticalRegion)"` + oldRSeqCritical atomic.Pointer[OldRSeqCriticalRegion] `state:".(*OldRSeqCriticalRegion)"` // tty is the thread group's controlling terminal. If nil, there is no // controlling terminal. @@ -289,7 +289,7 @@ func (k *Kernel) NewThreadGroup(pidns *PIDNamespace, sh *SignalHandlers, termina // saveOldRSeqCritical is invoked by stateify. func (tg *ThreadGroup) saveOldRSeqCritical() *OldRSeqCriticalRegion { - return tg.oldRSeqCritical.Load().(*OldRSeqCriticalRegion) + return tg.oldRSeqCritical.Load() } // loadOldRSeqCritical is invoked by stateify. diff --git a/pkg/sentry/pgalloc/pgalloc.go b/pkg/sentry/pgalloc/pgalloc.go index ee72c3f9f..84632084d 100644 --- a/pkg/sentry/pgalloc/pgalloc.go +++ b/pkg/sentry/pgalloc/pgalloc.go @@ -153,7 +153,7 @@ type MemoryFile struct { // operations. This allows MemoryFile.MapInternal to avoid locking in the // common case where chunk mappings already exist. mappingsMu mappingsMutex - mappings atomic.Value + mappings atomic.Pointer[[]uintptr] // destroyed is set by Destroy to instruct the reclaimer goroutine to // release resources and exit. destroyed is protected by mu. @@ -351,7 +351,7 @@ func NewMemoryFile(file *os.File, opts MemoryFileOpts) (*MemoryFile, error) { file: file, evictable: make(map[EvictableMemoryUser]*evictableMemoryUserInfo), } - f.mappings.Store(make([]uintptr, 0)) + f.mappings.Store(&[]uintptr{}) f.reclaimCond.L = &f.mu if f.opts.DelayedEviction == DelayedEvictionEnabled && f.opts.UseHostMemcgPressure { @@ -567,10 +567,10 @@ func (f *MemoryFile) allocate(length uint64, opts *AllocOpts) (memmap.FileRange, } f.fileSize = newFileSize f.mappingsMu.Lock() - oldMappings := f.mappings.Load().([]uintptr) + oldMappings := *f.mappings.Load() newMappings := make([]uintptr, newFileSize>>chunkShift) copy(newMappings, oldMappings) - f.mappings.Store(newMappings) + f.mappings.Store(&newMappings) f.mappingsMu.Unlock() } @@ -962,7 +962,7 @@ func (f *MemoryFile) MapInternal(fr memmap.FileRange, at hostarch.AccessType) (s // forEachMappingSlice invokes fn on a sequence of byte slices that // collectively map all bytes in fr. func (f *MemoryFile) forEachMappingSlice(fr memmap.FileRange, fn func([]byte)) error { - mappings := f.mappings.Load().([]uintptr) + mappings := *f.mappings.Load() for chunkStart := fr.Start &^ chunkMask; chunkStart < fr.End; chunkStart += chunkSize { chunk := int(chunkStart >> chunkShift) m := atomic.LoadUintptr(&mappings[chunk]) @@ -991,7 +991,7 @@ func (f *MemoryFile) getChunkMapping(chunk int) ([]uintptr, uintptr, error) { defer f.mappingsMu.Unlock() // Another thread may have replaced f.mappings altogether due to file // expansion. - mappings := f.mappings.Load().([]uintptr) + mappings := *f.mappings.Load() // Another thread may have already mapped the chunk. if m := mappings[chunk]; m != 0 { return mappings, m, nil @@ -1395,7 +1395,7 @@ func (f *MemoryFile) runReclaim() { f.file = nil f.mappingsMu.Lock() defer f.mappingsMu.Unlock() - mappings := f.mappings.Load().([]uintptr) + mappings := *f.mappings.Load() for i, m := range mappings { if m != 0 { _, _, errno := unix.Syscall(unix.SYS_MUNMAP, m, chunkSize, 0) @@ -1404,8 +1404,8 @@ func (f *MemoryFile) runReclaim() { } } } - // Similarly, invalidate f.mappings. (atomic.Value.Store(nil) panics.) - f.mappings.Store([]uintptr{}) + // Similarly, invalidate f.mappings + f.mappings.Store(nil) f.mu.Unlock() // This must be called without holding f.mu to avoid circular lock diff --git a/pkg/sentry/pgalloc/save_restore.go b/pkg/sentry/pgalloc/save_restore.go index 625e2bed1..c28fc91f5 100644 --- a/pkg/sentry/pgalloc/save_restore.go +++ b/pkg/sentry/pgalloc/save_restore.go @@ -139,7 +139,7 @@ func (f *MemoryFile) LoadFrom(ctx context.Context, r wire.Reader) error { return err } newMappings := make([]uintptr, f.fileSize>>chunkShift) - f.mappings.Store(newMappings) + f.mappings.Store(&newMappings) if _, err := state.Load(ctx, r, &f.usage); err != nil { return err } diff --git a/pkg/sync/seqatomic/seqatomic_test.go b/pkg/sync/seqatomic/seqatomic_test.go index 2c4568b07..332520a08 100644 --- a/pkg/sync/seqatomic/seqatomic_test.go +++ b/pkg/sync/seqatomic/seqatomic_test.go @@ -118,14 +118,15 @@ func BenchmarkSeqAtomicTryLoadIntUncontended(b *testing.B) { } // For comparison: -func BenchmarkAtomicValueLoadIntUncontended(b *testing.B) { - var a atomic.Value +func BenchmarkAtomicPointerLoadIntUncontended(b *testing.B) { + var a atomic.Pointer[int] const want = 42 - a.Store(int(want)) + value := int(want) + a.Store(&value) b.RunParallel(func(pb *testing.PB) { for pb.Next() { - if got := a.Load().(int); got != want { - b.Fatalf("atomic.Value.Load: got %v, wanted %v", got, want) + if got := a.Load(); *got != want { + b.Fatalf("atomic.Pointer[int].Load: got %v, wanted %v", got, want) } } })