diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 32a49dbd5..068ecd091 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -714,6 +714,12 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b if child != nil { toDecRef = vfsObj.CommitDeleteDentry(ctx, &child.vfsd) // +checklocksforce: see above. child.setDeleted() + // If an extra reference is held on child as described by the comment + // for dentry.refs, drop that reference now. We can't race with another + // fs.unlinkAt() or invalidation since parent.opMu has been locked for + // writing since before we obtained child, and we can't race with + // fs.RenameAt() since fs.renameMu has been locked since before we + // obtained child. if child.isSynthetic() { parent.syntheticChildren-- child.decRefNoCaching() @@ -1507,6 +1513,10 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa toDecRef = vfsObj.CommitRenameReplaceDentry(ctx, &renamed.vfsd, replacedVFSD) if replaced != nil { replaced.setDeleted() + // If an extra reference is held on replaced as described by the + // comment for dentry.refs, drop that reference now. We can't race with + // fs.unlinkAt() or invalidation since fs.renameMu has been locked for + // writing since before we obtained replaced. if replaced.isSynthetic() { newParent.syntheticChildren-- replaced.decRefNoCaching() diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index 4d720391c..9231d9c0f 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -198,29 +198,30 @@ func (fs *filesystem) revalidateStep(ctx context.Context, rp resolvingPath, d *d // Precondition: fs.renameMu must be locked. func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, ds **[]*dentry) { // Remove d from its parent. - func() { + removed := func() bool { parent := d.parent.Load() parent.opMu.RLock() defer parent.opMu.RUnlock() parent.childrenMu.Lock() defer parent.childrenMu.Unlock() - if d.isSynthetic() { - // Normally we don't mark invalidated dentries as deleted since - // they may still exist (but at a different path), and also for - // consistency with Linux. However, synthetic files are guaranteed - // to become unreachable if their dentries are invalidated, so - // treat their invalidation as deletion. - d.deleteSynthetic(parent, ds) - } - // Since the opMu was just reacquired above, re-check that the // parent's child with this name is still the same. Do not touch it if // it has been replaced with a different one. if child := parent.children[d.name]; child == d { // Invalidate dentry so it gets reloaded next time it's accessed. delete(parent.children, d.name) + if d.isSynthetic() { + // Normally we don't mark invalidated dentries as deleted since + // they may still exist (but at a different path), and also for + // consistency with Linux. However, synthetic files are + // guaranteed to become unreachable if their dentries are + // invalidated, so treat their invalidation as deletion. + d.deleteSynthetic(parent, ds) + } + return true } + return false }() // Invalidate d and its descendants. @@ -239,7 +240,14 @@ func (d *dentry) invalidate(ctx context.Context, vfsObj *vfs.VirtualFilesystem, rc.DecRef(ctx) } d.decRefNoCaching() - if d.isSynthetic() || d.endpoint != nil { + + // If an extra reference is held on d as described by the comment for + // dentry.refs, and d hasn't been racily removed by + // filesystem.unlinkAt() or another revalidation, drop that reference + // now. (The same would apply to racy replacement by + // filesystem.RenameAt(), but we can't race with rename since renameMu + // has been locked since entering filesystem.revalidatePath().) + if removed && (d.isSynthetic() || d.endpoint != nil) { d.decRefNoCaching() }