Merge release-20250820.0-33-g1941bc68e (automated)

gvisor-bot · gvisor-bot · commit c958f002d313 · 2025-08-28T07:00:13.000Z
diff --git a/pkg/sentry/kernel/fs_context.go b/pkg/sentry/kernel/fs_context.go
@@ -61,6 +61,20 @@ func NewFSContext(root, cwd vfs.VirtualDentry, umask uint) *FSContext {
 	return &f
 }
 
+// destroy destroys the FSContext.
+//
+// Preconditions: f must have no refcount.
+func (f *FSContext) destroy(ctx context.Context) {
+	// Hold f.mu so that we don't race with RootDirectory() and
+	// WorkingDirectory().
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.root.DecRef(ctx)
+	f.root = vfs.VirtualDentry{}
+	f.cwd.DecRef(ctx)
+	f.cwd = vfs.VirtualDentry{}
+}
+
 // DecRef implements RefCounter.DecRef.
 //
 // When f reaches zero references, DecRef will be called on both root and cwd
@@ -71,15 +85,7 @@ func NewFSContext(root, cwd vfs.VirtualDentry, umask uint) *FSContext {
 // proc files or other mechanisms.
 func (f *FSContext) DecRef(ctx context.Context) {
 	f.FSContextRefs.DecRef(func() {
-		// Hold f.mu so that we don't race with RootDirectory() and
-		// WorkingDirectory().
-		f.mu.Lock()
-		defer f.mu.Unlock()
-
-		f.root.DecRef(ctx)
-		f.root = vfs.VirtualDentry{}
-		f.cwd.DecRef(ctx)
-		f.cwd = vfs.VirtualDentry{}
+		f.destroy(ctx)
 	})
 }
 
@@ -202,7 +208,7 @@ func (f *FSContext) checkAndPreventSharingOutsideTG(tg *ThreadGroup) bool {
 
 	tgCount := int64(0)
 	tg.ForEachTask(func(t *Task) bool {
-		if t.fsContext == f {
+		if t.FSContext() == f {
 			tgCount++
 		}
 		return true
@@ -233,3 +239,20 @@ func (f *FSContext) share() bool {
 	f.IncRef()
 	return true
 }
+
+// unshareFromTask removes the FSContext f from the given Task t and replaces it with newF.
+// It returns a bool indicating whether f needs to be destroyed.
+
+// This func operates without compromising a concurrent checkAndPreventSharingOutsideTG(): t's
+// association with f is severed atomically by holding f.mu, allowing the concurrent func to
+// correctly ascribe extra ref counts to tasks outside of t's thread group.
+//
+// Preconditions: The caller must be on the task goroutine and must hold t.mu.
+func (f *FSContext) unshareFromTask(t *Task, newF *FSContext) bool {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	t.fsContext.Store(newF)
+	destroy := false
+	f.FSContextRefs.DecRef(func() { destroy = true })
+	return destroy
+}
diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go
@@ -1993,9 +1993,9 @@ func (k *Kernel) ReplaceFSContextRoots(ctx context.Context, oldRoot vfs.VirtualD
 	k.tasks.mu.RLock()
 	oldRootDecRefs := 0
 	k.tasks.forEachTaskLocked(func(t *Task) {
-		t.mu.Lock()
+		t.mu.Lock() // To prevent t's task goroutine from unsharing its fsContext from under us.
 		defer t.mu.Unlock()
-		if fsc := t.fsContext; fsc != nil {
+		if fsc := t.FSContext(); fsc != nil {
 			fsc.mu.Lock()
 			defer fsc.mu.Unlock()
 			if fsc.root == oldRoot {
diff --git a/pkg/sentry/kernel/kernel_state.go b/pkg/sentry/kernel/kernel_state.go
@@ -67,6 +67,16 @@ func (t *Task) loadSeccomp(_ context.Context, seccompData *taskSeccomp) {
 	t.seccomp.Store(seccompData)
 }
 
+// saveFsContext is invoked by stateify.
+func (t *Task) saveFsContext() *FSContext {
+	return t.fsContext.Load()
+}
+
+// loadFsContext is invoked by stateify.
+func (t *Task) loadFsContext(_ context.Context, fsContext *FSContext) {
+	t.fsContext.Store(fsContext)
+}
+
 // saveAppCPUClockLast is invoked by stateify.
 func (tg *ThreadGroup) saveAppCPUClockLast() *Task {
 	return tg.appCPUClockLast.Load()
diff --git a/pkg/sentry/kernel/kernel_state_autogen.go b/pkg/sentry/kernel/kernel_state_autogen.go
@@ -1506,6 +1506,9 @@ func (t *Task) beforeSave() {}
 // +checklocksignore
 func (t *Task) StateSave(stateSinkObject state.Sink) {
 	t.beforeSave()
+	var fsContextValue *FSContext
+	fsContextValue = t.saveFsContext()
+	stateSinkObject.SaveValue(27, fsContextValue)
 	var vforkParentValue *Task
 	vforkParentValue = t.saveVforkParent()
 	stateSinkObject.SaveValue(29, vforkParentValue)
@@ -1542,7 +1545,6 @@ func (t *Task) StateSave(stateSinkObject state.Sink) {
 	stateSinkObject.Save(24, &t.k)
 	stateSinkObject.Save(25, &t.containerID)
 	stateSinkObject.Save(26, &t.image)
-	stateSinkObject.Save(27, &t.fsContext)
 	stateSinkObject.Save(28, &t.fdTable)
 	stateSinkObject.Save(30, &t.exitState)
 	stateSinkObject.Save(31, &t.exitTracerNotified)
@@ -1617,7 +1619,6 @@ func (t *Task) StateLoad(ctx context.Context, stateSourceObject state.Source) {
 	stateSourceObject.Load(24, &t.k)
 	stateSourceObject.Load(25, &t.containerID)
 	stateSourceObject.Load(26, &t.image)
-	stateSourceObject.Load(27, &t.fsContext)
 	stateSourceObject.Load(28, &t.fdTable)
 	stateSourceObject.Load(30, &t.exitState)
 	stateSourceObject.Load(31, &t.exitTracerNotified)
@@ -1661,6 +1662,7 @@ func (t *Task) StateLoad(ctx context.Context, stateSourceObject state.Source) {
 	stateSourceObject.Load(71, &t.Origin)
 	stateSourceObject.Load(72, &t.onDestroyAction)
 	stateSourceObject.Load(73, &t.execveCredsMutexOwner)
+	stateSourceObject.LoadValue(27, new(*FSContext), func(y any) { t.loadFsContext(ctx, y.(*FSContext)) })
 	stateSourceObject.LoadValue(29, new(*Task), func(y any) { t.loadVforkParent(ctx, y.(*Task)) })
 	stateSourceObject.LoadValue(35, new(*Task), func(y any) { t.loadPtraceTracer(ctx, y.(*Task)) })
 	stateSourceObject.LoadValue(52, new(*taskSeccomp), func(y any) { t.loadSeccomp(ctx, y.(*taskSeccomp)) })
diff --git a/pkg/sentry/kernel/task.go b/pkg/sentry/kernel/task.go
@@ -291,8 +291,9 @@ type Task struct {
 
 	// fsContext is the task's filesystem context.
 	//
-	// fsContext is protected by mu, and is owned by the task goroutine.
-	fsContext *FSContext
+	// fsContext is protected by mu, and is owned by the task goroutine. It can be read without
+	// synchronization using FSContext().
+	fsContext atomic.Pointer[FSContext] `state:".(*FSContext)"`
 
 	// fdTable is the task's file descriptor table.
 	//
@@ -750,7 +751,7 @@ func (t *Task) SyscallRestartBlock() SyscallRestartBlock {
 func (t *Task) IsChrooted() bool {
 	realRoot := t.mountNamespace.Root(t)
 	defer realRoot.DecRef(t)
-	root := t.fsContext.RootDirectory()
+	root := t.FSContext().RootDirectory()
 	defer root.DecRef(t)
 	return root != realRoot
 }
@@ -766,10 +767,11 @@ func (t *Task) TaskImage() *TaskImage {
 // FSContext returns t's FSContext. FSContext does not take an additional
 // reference on the returned FSContext.
 //
-// Precondition: The caller must be running on the task goroutine, or t.mu must
-// be locked.
+// Precondition: No synchronization is needed for just read access. If the caller instead intends to
+// modify the contents of the FSContext, it must either be running on the task goroutine or have
+// t.mu locked.
 func (t *Task) FSContext() *FSContext {
-	return t.fsContext
+	return t.fsContext.Load()
 }
 
 // FDTable returns t's FDTable. FDMTable does not take an additional reference
diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go
@@ -192,9 +192,9 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) {
 
 	var fsContext *FSContext
 	if args.Flags&linux.CLONE_FS == 0 || args.Flags&linux.CLONE_NEWNS != 0 {
-		fsContext = t.fsContext.Fork()
+		fsContext = t.FSContext().Fork()
 	} else {
-		fsContext = t.fsContext
+		fsContext = t.FSContext()
 		if !fsContext.share() {
 			// Linux fails clone with EAGAIN if there is a concurrent execve, see kernel/fork.c:copy_fs().
 			return 0, nil, linuxerr.EAGAIN
@@ -518,7 +518,7 @@ func (t *Task) Setns(fd *vfs.FileDescription, flags int32) error {
 			!t.Credentials().HasCapability(linux.CAP_SYS_ADMIN) {
 			return linuxerr.EPERM
 		}
-		oldFSContext := t.fsContext
+		oldFSContext := t.FSContext()
 		// The current task has to be an exclusive owner of its fs context.
 		if oldFSContext.ReadRefs() != 1 {
 			return linuxerr.EINVAL
@@ -535,7 +535,7 @@ func (t *Task) Setns(fd *vfs.FileDescription, flags int32) error {
 		ns.IncRef()
 		t.mu.Lock()
 		t.mountNamespace = ns
-		t.fsContext = fsContext
+		t.fsContext.Store(fsContext)
 		t.mu.Unlock()
 		oldNS.DecRef(t)
 		oldFSContext.DecRef(t)
@@ -671,7 +671,6 @@ func (t *Task) Unshare(flags int32) error {
 	defer cu.Clean()
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	// Can't defer unlock: DecRefs must occur without holding t.mu.
 	if flags&linux.CLONE_NEWUTS != 0 {
 		if !haveCapSysAdmin {
 			return linuxerr.EPERM
@@ -701,16 +700,21 @@ func (t *Task) Unshare(flags int32) error {
 		cu.Add(func() { oldFDTable.DecRef(t) })
 	}
 	if flags&linux.CLONE_FS != 0 || flags&linux.CLONE_NEWNS != 0 {
-		oldFSContext := t.fsContext
-		t.fsContext = oldFSContext.Fork()
-		cu.Add(func() { oldFSContext.DecRef(t) })
+		oldFSContext := t.FSContext()
+		// unshareFromTask() lowers the old fs context's ref count, but its for us to
+		// destroy it if there are no other references.
+		if oldFSContext.unshareFromTask(t, oldFSContext.Fork()) {
+			// destroy() requires t.mu to not be held, hence the deferral.
+			cu.Add(func() { oldFSContext.destroy(t) })
+		}
 	}
 	if flags&linux.CLONE_NEWNS != 0 {
 		if !haveCapSysAdmin {
 			return linuxerr.EPERM
 		}
 		oldMountNS := t.mountNamespace
-		mntns, err := t.k.vfs.CloneMountNamespace(t, creds.UserNamespace, oldMountNS, &t.fsContext.root, &t.fsContext.cwd, t.k)
+		fsContext := t.FSContext()
+		mntns, err := t.k.vfs.CloneMountNamespace(t, creds.UserNamespace, oldMountNS, &fsContext.root, &fsContext.cwd, t.k)
 		if err != nil {
 			return err
 		}
diff --git a/pkg/sentry/kernel/task_context.go b/pkg/sentry/kernel/task_context.go
@@ -96,7 +96,7 @@ func (t *Task) contextValue(key any, isTaskGoroutine bool) any {
 			t.mu.Lock()
 			defer t.mu.Unlock()
 		}
-		return t.fsContext.RootDirectory()
+		return t.FSContext().RootDirectory()
 	case vfs.CtxMountNamespace:
 		if !isTaskGoroutine {
 			t.mu.Lock()
diff --git a/pkg/sentry/kernel/task_exec.go b/pkg/sentry/kernel/task_exec.go
@@ -144,15 +144,16 @@ func (r *runExecveAfterExecveCredsLock) execute(t *Task) taskRunState {
 		return (*runInterrupt)(nil)
 	}
 
-	root := t.FSContext().RootDirectory()
+	fsContext := t.FSContext()
+	root := fsContext.RootDirectory()
 	defer root.DecRef(t)
-	wd := t.FSContext().WorkingDirectory()
+	wd := fsContext.WorkingDirectory()
 	defer wd.DecRef(t)
 
 	// Cannot gain privileges if the ptracer's capabilities prevent it.
 	stopPrivGain := t.shouldStopPrivGainDueToPtracerLocked()
 	// Cannot gain privileges if the FSContext is shared outside of our thread group.
-	if t.fsContext.checkAndPreventSharingOutsideTG(t.tg) {
+	if fsContext.checkAndPreventSharingOutsideTG(t.tg) {
 		stopPrivGain = true
 	}
 
@@ -612,8 +613,8 @@ func (t *Task) shouldStopPrivGainDueToPtracerLocked() bool {
 
 // Releases the mutex that serializes an execve(2) with a PTRACE_ATTACH and allows t.fsContext to
 // be shared again via clone(2). The caller must have called execveCredsMutexStartLock() and
-// fsContext.CheckAndPreventSharingOutsideTG() earlier.
+// fsContext.checkAndPreventSharingOutsideTG() earlier.
 func (t *Task) releaseExecveCredsLocks() {
 	t.execveCredsMutexUnlock()
-	t.fsContext.allowSharing()
+	t.FSContext().allowSharing()
 }
diff --git a/pkg/sentry/kernel/task_exit.go b/pkg/sentry/kernel/task_exit.go
@@ -298,7 +298,7 @@ func (*runExitMain) execute(t *Task) taskRunState {
 	// Releasing the MM unblocks a blocked CLONE_VFORK parent.
 	t.unstopVforkParent()
 
-	t.fsContext.DecRef(t)
+	t.FSContext().DecRef(t)
 	t.fdTable.DecRef(t)
 
 	// Detach task from all cgroups. This must happen before potentially the
diff --git a/pkg/sentry/kernel/task_start.go b/pkg/sentry/kernel/task_start.go
@@ -159,7 +159,6 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 		signalMask:      atomicbitops.FromUint64(uint64(cfg.SignalMask)),
 		signalStack:     linux.SignalStack{Flags: linux.SS_DISABLE},
 		image:           *image,
-		fsContext:       cfg.FSContext,
 		fdTable:         cfg.FDTable,
 		k:               cfg.Kernel,
 		ptraceTracees:   make(map[*Task]struct{}),
@@ -183,6 +182,7 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 	}
 	t.netns = cfg.NetworkNamespace
 	t.creds.Store(cfg.Credentials)
+	t.fsContext.Store(cfg.FSContext)
 	t.endStopCond.L = &t.tg.signalHandlers.mu
 	// We don't construct t.blockingTimer until Task.run(); see that function
 	// for justification.
diff --git a/pkg/sentry/socket/netlink/netfilter/protocol.go b/pkg/sentry/socket/netlink/netfilter/protocol.go
@@ -495,12 +495,12 @@ func (p *Protocol) addChain(attrs map[uint16]nlmsg.BytesView, tab *nftables.Tabl
 // chainParseHook parses the hook attributes and returns a complete
 // BaseChainInfo.
 func (p *Protocol) chainParseHook(chain *nftables.Chain, family stack.AddressFamily, hdata nlmsg.AttrsView) (*nftables.BaseChainInfo, *syserr.AnnotatedError) {
-	hookAttrs, ok := hdata.Parse()
-	var hookInfo nftables.HookInfo
+	hookAttrs, ok := nftables.NfParse(hdata)
 	if !ok {
 		return nil, syserr.NewAnnotatedError(syserr.ErrInvalidArgument, fmt.Sprintf("Nftables: Failed to parse hook attributes"))
 	}
 
+	var hookInfo nftables.HookInfo
 	if chain != nil {
 		// TODO: b/434243967 - Support updating existing chains.
 		return nil, syserr.NewAnnotatedError(syserr.ErrNotSupported, fmt.Sprintf("Nftables: Updating hook attributes are not supported for existing chains"))
@@ -935,7 +935,7 @@ func parseNestedExprs(nestedAttrBytes nlmsg.AttrsView) ([]nftables.ExprInfo, *sy
 		}
 		numExprs++
 
-		exprAttrs, ok := nlmsg.AttrsView(value).Parse()
+		exprAttrs, ok := nftables.NfParse(nlmsg.AttrsView(value))
 		if !ok {
 			return nil, syserr.NewAnnotatedError(syserr.ErrInvalidArgument, "Nftables: Failed to parse attributes for expression")
 		}
@@ -1139,7 +1139,7 @@ func (p *Protocol) ProcessMessage(ctx context.Context, s *netlink.Socket, msg *n
 		return syserr.ErrInvalidArgument
 	}
 
-	attrs, ok := atr.Parse()
+	attrs, ok := nftables.NfParse(atr)
 	if !ok {
 		log.Debugf("Failed to parse message attributes")
 		return syserr.ErrInvalidArgument
@@ -1206,7 +1206,7 @@ func (p *Protocol) receiveBatchMessage(ctx context.Context, s *netlink.Socket, m
 		return syserr.NewAnnotatedError(syserr.ErrInvalidArgument, "Nftables: Failed to get message data")
 	}
 
-	attrs, ok := atr.Parse()
+	attrs, ok := nftables.NfParse(atr)
 	if !ok {
 		return syserr.NewAnnotatedError(syserr.ErrInvalidArgument, "Failed to parse message attributes for batch message")
 	}
@@ -1322,7 +1322,7 @@ func (p *Protocol) processBatchMessage(ctx context.Context, s *netlink.Socket, b
 			continue
 		}
 
-		attrs, ok := atr.Parse()
+		attrs, ok := nftables.NfParse(atr)
 		if !ok {
 			netlink.DumpErrorMessage(hdr, ms, syserr.ErrInvalidArgument)
 			continue
diff --git a/pkg/tcpip/nftables/nft_immediate.go b/pkg/tcpip/nftables/nft_immediate.go
@@ -43,7 +43,7 @@ func newImmediate(dreg uint8, data registerData) (*immediate, *syserr.AnnotatedE
 // InitImmediate initializes the immediate operation from the expression info.
 func initImmediate(tab *Table, exprInfo ExprInfo) (*immediate, *syserr.AnnotatedError) {
 	// We now have attributes specific to immediate expressions.
-	immDataAttrs, ok := exprInfo.ExprData.Parse()
+	immDataAttrs, ok := NfParse(exprInfo.ExprData)
 	if !ok {
 		return nil, syserr.NewAnnotatedError(syserr.ErrInvalidArgument, "Nftables: Failed to parse immediate expression data")
 	}
diff --git a/pkg/tcpip/nftables/nftables_types.go b/pkg/tcpip/nftables/nftables_types.go
@@ -956,7 +956,7 @@ func AFtoNetlinkAF(af uint8) (stack.AddressFamily, *syserr.Error) {
 
 // nftDataInit creates a new registerData struct from the passed in data bytes.
 func nftDataInit(tab *Table, regType uint32, dataBytes nlmsg.AttrsView) (registerData, *syserr.AnnotatedError) {
-	dataAttrs, ok := dataBytes.Parse()
+	dataAttrs, ok := NfParse(dataBytes)
 	if !ok {
 		return nil, syserr.NewAnnotatedError(syserr.ErrInvalidArgument, fmt.Sprintf("Nftables: Failed to parse data bytes for nested expression data"))
 	}
@@ -1049,7 +1049,7 @@ func nftValidateRegister(reg uint32, regType uint32, data registerData) (uint8,
 // validateVerdictData validates the verdict data bytes and returns the data as a verdict.
 func validateVerdictData(tab *Table, bytes nlmsg.AttrsView) (stack.NFVerdict, *syserr.AnnotatedError) {
 	v := stack.NFVerdict{}
-	verdictAttrs, ok := bytes.Parse()
+	verdictAttrs, ok := NfParse(bytes)
 	if !ok {
 		return v, syserr.NewAnnotatedError(syserr.ErrInvalidArgument, "Nftables: Failed to parse verdict data")
 	}
@@ -1116,6 +1116,25 @@ func HasAttr(attrName uint16, attrs map[uint16]nlmsg.BytesView) bool {
 	return ok
 }
 
+// NfParse parses the data bytes, clearing the nested attribute bit if present.
+// For nested attributes, Linux supports these attributes having the bit
+// set or unset. It is cleared here for consistency.
+func NfParse(data nlmsg.AttrsView) (map[uint16]nlmsg.BytesView, bool) {
+	attrs, ok := data.Parse()
+	if !ok {
+		return nil, ok
+	}
+
+	newAttrs := make(map[uint16]nlmsg.BytesView)
+	// TODO - b/421437663: If any validation has to be done on nested attributes,
+	// it should be done here.
+	for attr, attrData := range attrs {
+		newAttrs[attr & ^linux.NLA_F_NESTED] = attrData
+	}
+
+	return newAttrs, ok
+}
+
 // deepCopyRule returns a deep copy of the Rule struct.
 func deepCopyRule(rule *Rule, chainCopy *Chain) *Rule {
 	return &Rule{

Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,7 @@ func (t *Task) contextValue(key any, isTaskGoroutine bool) any {`
`96`	`96`	`t.mu.Lock()`
`97`	`97`	`defer t.mu.Unlock()`
`98`	`98`	`}`
`99`		`- return t.fsContext.RootDirectory()`
	`99`	`+ return t.FSContext().RootDirectory()`
`100`	`100`	`case vfs.CtxMountNamespace:`
`101`	`101`	`if !isTaskGoroutine {`
`102`	`102`	`t.mu.Lock()`