From 63e2820f7bc5b15eacd406ac10b8e83b3bc87fa4 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Wed, 18 Jul 2018 11:48:56 -0700 Subject: Fix lock-ordering violation in Create by logging BaseName instead of FullName. Dirent.FullName takes the global renameMu, but can be called during Create, which itself takes dirent.mu and dirent.dirMu, which is a lock-order violation: Dirent.Create d.dirMu.Lock d.mu.Lock Inode.Create gofer.inodeOperations.Create gofer.NewFile Dirent.FullName d.renameMu.RLock We only use the FullName here for logging, and in this case we can get by with logging only the BaseName. A `BaseName` method was added to Dirent, which simply returns the name, taking d.parent.mu as required. In the Create pathway, we can't call d.BaseName() because taking d.parent.mu after d.mu violates the lock order. But we already know the base name of the file we just created, so that's OK. In the Open/GetFile pathway, we are free to call d.BaseName() because the other dirent locks are not held. PiperOrigin-RevId: 205112278 Change-Id: Ib45c734081aecc9b225249a65fa8093eb4995f10 --- pkg/sentry/fs/gofer/file.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'pkg/sentry/fs/gofer/file.go') diff --git a/pkg/sentry/fs/gofer/file.go b/pkg/sentry/fs/gofer/file.go index 69cee7026..039618808 100644 --- a/pkg/sentry/fs/gofer/file.go +++ b/pkg/sentry/fs/gofer/file.go @@ -57,7 +57,14 @@ type fileOperations struct { var _ fs.FileOperations = (*fileOperations)(nil) // NewFile returns a file. NewFile is not appropriate with host pipes and sockets. -func NewFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags, i *inodeOperations, handles *handles) *fs.File { +// +// The `name` argument is only used to log a warning if we are returning a +// writeable+executable file. (A metric counter is incremented in this case as +// well.) Note that we cannot call d.BaseName() directly in this function, +// because that would lead to a lock order violation, since this is called in +// d.Create which holds d.mu, while d.BaseName() takes d.parent.mu, and the two +// locks must be taken in the opposite order. +func NewFile(ctx context.Context, dirent *fs.Dirent, name string, flags fs.FileFlags, i *inodeOperations, handles *handles) *fs.File { // Remote file systems enforce readability/writability at an offset, // see fs/9p/vfs_inode.c:v9fs_vfs_atomic_open -> fs/open.c:finish_open. flags.Pread = true @@ -70,7 +77,6 @@ func NewFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags, i *inod } if flags.Write { if err := dirent.Inode.CheckPermission(ctx, fs.PermMask{Execute: true}); err == nil { - name, _ := dirent.FullName(fs.RootFromContext(ctx)) openedWX.Increment() log.Warningf("Opened a writable executable: %q", name) } -- cgit v1.2.3