From f0b8875509257069b8700319c8dd764111df30c7 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 5 Apr 2023 14:14:04 -0700 Subject: [PATCH] Add intent to vfs.ResolvingPath in gofer client. In some cases, like walkParentDirLocked(), we do not want to resolve the entire path held in rp immediately. However, currently the gofer client still tries to make an RPC to walk the entire path. This can be wasteful as callers of walkParentDirLocked() are creation operations and the last path component does not exist. So the gofer has to attempt to walk a non-existing file. Add a non-exported `resolvingPath` type to gofer client which just wraps *vfs.ResolvingPath. It can additionally hold such information. This change also updates lisafsDentry to not attempt to walk the last path component for such an rp. PiperOrigin-RevId: 522151184 --- pkg/sentry/fsimpl/gofer/dentry_impl.go | 2 +- pkg/sentry/fsimpl/gofer/filesystem.go | 26 +++++++++-------- pkg/sentry/fsimpl/gofer/gofer.go | 37 ++++++++++++++++++++++++ pkg/sentry/fsimpl/gofer/lisafs_dentry.go | 6 ++-- pkg/sentry/fsimpl/gofer/revalidate.go | 30 ++++--------------- pkg/sentry/vfs/resolving_path.go | 8 +++-- 6 files changed, 67 insertions(+), 42 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/dentry_impl.go b/pkg/sentry/fsimpl/gofer/dentry_impl.go index 8cc74dd74..851bc0f43 100644 --- a/pkg/sentry/fsimpl/gofer/dentry_impl.go +++ b/pkg/sentry/fsimpl/gofer/dentry_impl.go @@ -248,7 +248,7 @@ func (d *dentry) getRemoteChild(ctx context.Context, name string) (*dentry, erro // Postcondition: The returned dentry is already cached appropriately. // // +checklocksread:d.opMu -func (d *dentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { +func (d *dentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp resolvingPath, ds **[]*dentry) (*dentry, error) { switch dt := d.impl.(type) { case *lisafsDentry: return dt.getRemoteChildAndWalkPathLocked(ctx, rp, ds) diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index fc9dec077..04c2c779d 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -177,7 +177,7 @@ func (fs *filesystem) renameMuUnlockAndCheckCaching(ctx context.Context, ds **[] // part of rp must have been revalidated. // // +checklocksread:d.opMu -func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, bool, error) { +func (fs *filesystem) stepLocked(ctx context.Context, rp resolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, bool, error) { if !d.isDir() { return nil, false, linuxerr.ENOTDIR } @@ -293,7 +293,7 @@ func (fs *filesystem) getRemoteChildLocked(ctx context.Context, parent *dentry, // may prefetch the entire path represented by rp. // // +checklocksread:parent.opMu -func (fs *filesystem) getChildAndWalkPathLocked(ctx context.Context, parent *dentry, rp *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { +func (fs *filesystem) getChildAndWalkPathLocked(ctx context.Context, parent *dentry, rp resolvingPath, ds **[]*dentry) (*dentry, error) { if child, err := parent.getCachedChildLocked(rp.Component()); child != nil || err != nil { return child, err } @@ -344,11 +344,12 @@ func (d *dentry) getCachedChildLocked(name string) (*dentry, error) { // - !rp.Done(). // - If !d.cachedMetadataAuthoritative(), then d's cached metadata must be up // to date. -func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, error) { - if err := fs.revalidateParentDir(ctx, rp, d, ds); err != nil { +func (fs *filesystem) walkParentDirLocked(ctx context.Context, vfsRP *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, error) { + rp := resolvingPathParent(vfsRP) + if err := fs.revalidatePath(ctx, rp, d, ds); err != nil { return nil, err } - for !rp.Final() { + for !rp.done() { d.opMu.RLock() next, followedSymlink, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds) d.opMu.RUnlock() @@ -357,7 +358,7 @@ func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.Resolving } d = next if followedSymlink { - if err := fs.revalidateParentDir(ctx, rp, d, ds); err != nil { + if err := fs.revalidatePath(ctx, rp, d, ds); err != nil { return nil, err } } @@ -371,12 +372,13 @@ func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.Resolving // resolveLocked resolves rp to an existing file. // // Preconditions: fs.renameMu must be locked. -func (fs *filesystem) resolveLocked(ctx context.Context, rp *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { +func (fs *filesystem) resolveLocked(ctx context.Context, vfsRP *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { + rp := resolvingPathFull(vfsRP) d := rp.Start().Impl().(*dentry) if err := fs.revalidatePath(ctx, rp, d, ds); err != nil { return nil, err } - for !rp.Done() { + for !rp.done() { d.opMu.RLock() next, followedSymlink, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds) d.opMu.RUnlock() @@ -599,7 +601,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b return linuxerr.ENOENT } } else { - child, _, err = fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds) + child, _, err = fs.stepLocked(ctx, resolvingPathFull(rp), parent, false /* mayFollowSymlinks */, &ds) if err != nil { return err } @@ -872,7 +874,7 @@ func (fs *filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v // to creating a synthetic one, i.e. one that is kept entirely in memory. // Check that we're not overriding an existing file with a synthetic one. - _, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, ds) // +checklocksforce: parent.opMu taken by doCreateAt. + _, _, err := fs.stepLocked(ctx, resolvingPathFull(rp), parent, false /* mayFollowSymlinks */, ds) // +checklocksforce: parent.opMu taken by doCreateAt. switch { case err == nil: // Step succeeded, another file exists. @@ -972,7 +974,7 @@ afterTrailingSymlink: // serializing OpenAt calls in the same directory in the common case // that the file exists. parent.opMu.RLock() - child, followedSymlink, err := fs.stepLocked(ctx, rp, parent, true /* mayFollowSymlinks */, &ds) + child, followedSymlink, err := fs.stepLocked(ctx, resolvingPathFull(rp), parent, true /* mayFollowSymlinks */, &ds) parent.opMu.RUnlock() if followedSymlink { if mustCreate { @@ -1012,7 +1014,7 @@ afterTrailingSymlink: // Step to the file again. Since we still hold opMu for // writing, there can't be a race here. - child, _, err = fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds) + child, _, err = fs.stepLocked(ctx, resolvingPathFull(rp), parent, false /* mayFollowSymlinks */, &ds) parent.opMu.Unlock() } if err != nil { diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index ea807e86c..61dcc73e3 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -2198,3 +2198,40 @@ func (fd *fileDescription) LockPOSIX(ctx context.Context, uid fslock.UniqueID, o func (fd *fileDescription) UnlockPOSIX(ctx context.Context, uid fslock.UniqueID, r fslock.LockRange) error { return fd.Locks().UnlockPOSIX(ctx, uid, r) } + +// resolvingPath is just a wrapper around *vfs.ResolvingPath. It additionally +// holds some information around the intent behind resolving the path. +type resolvingPath struct { + *vfs.ResolvingPath + + // excludeLast indicates whether the intent is to resolve until the last path + // component. If true, the last path component should remain unresolved. + excludeLast bool +} + +func resolvingPathFull(rp *vfs.ResolvingPath) resolvingPath { + return resolvingPath{ResolvingPath: rp, excludeLast: false} +} + +func resolvingPathParent(rp *vfs.ResolvingPath) resolvingPath { + return resolvingPath{ResolvingPath: rp, excludeLast: true} +} + +func (rp *resolvingPath) done() bool { + if rp.excludeLast { + return rp.Final() + } + return rp.Done() +} + +func (rp *resolvingPath) copy() resolvingPath { + return resolvingPath{ + ResolvingPath: rp.ResolvingPath.Copy(), + excludeLast: rp.excludeLast, + } +} + +// Precondition: !rp.done() && rp.Component() is not "." or "..". +func (rp *resolvingPath) getComponents(emit func(string) bool) { + rp.GetComponents(rp.excludeLast, emit) +} diff --git a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go index 578f429b2..0b4c2f831 100644 --- a/pkg/sentry/fsimpl/gofer/lisafs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/lisafs_dentry.go @@ -286,14 +286,14 @@ func (d *lisafsDentry) getRemoteChild(ctx context.Context, name string) (*dentry // - fs.renameMu must be locked. // - d.opMu must be locked. // - d.isDir(). -// - !rp.Done() && rp.Component() is not "." or "..". +// - !rp.done() && rp.Component() is not "." or "..". // // Postcondition: The returned dentry is already cached appropriately. -func (d *lisafsDentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp *vfs.ResolvingPath, ds **[]*dentry) (*dentry, error) { +func (d *lisafsDentry) getRemoteChildAndWalkPathLocked(ctx context.Context, rp resolvingPath, ds **[]*dentry) (*dentry, error) { // Collect as many path components as possible to walk. var namesArr [16]string // arbitrarily sized array to help avoid slice allocation. names := namesArr[:0] - rp.GetComponents(func(name string) bool { + rp.getComponents(func(name string) bool { if name == "." { return true } diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index 7f0c42a48..2448a3fa2 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -44,7 +44,7 @@ func (errRevalidationStepDone) Error() string { // // Preconditions: // - fs.renameMu must be locked. -func (fs *filesystem) revalidatePath(ctx context.Context, rpOrig *vfs.ResolvingPath, start *dentry, ds **[]*dentry) error { +func (fs *filesystem) revalidatePath(ctx context.Context, rpOrig resolvingPath, start *dentry, ds **[]*dentry) error { // Revalidation is done even if start is synthetic in case the path is // something like: ../non_synthetic_file. if fs.opts.interop != InteropModeShared { @@ -52,26 +52,8 @@ func (fs *filesystem) revalidatePath(ctx context.Context, rpOrig *vfs.ResolvingP } // Copy resolving path to walk the path for revalidation. - rp := rpOrig.Copy() - err := fs.revalidate(ctx, rp, start, rp.Done, ds) - rp.Release(ctx) - return err -} - -// revalidateParentDir does the same as revalidatePath, but stops at the parent. -// -// Preconditions: -// - fs.renameMu must be locked. -func (fs *filesystem) revalidateParentDir(ctx context.Context, rpOrig *vfs.ResolvingPath, start *dentry, ds **[]*dentry) error { - // Revalidation is done even if start is synthetic in case the path is - // something like: ../non_synthetic_file and parent is non synthetic. - if fs.opts.interop != InteropModeShared { - return nil - } - - // Copy resolving path to walk the path for revalidation. - rp := rpOrig.Copy() - err := fs.revalidate(ctx, rp, start, rp.Final, ds) + rp := rpOrig.copy() + err := fs.revalidate(ctx, rp, start, ds) rp.Release(ctx) return err } @@ -111,12 +93,12 @@ func (fs *filesystem) revalidateOne(ctx context.Context, vfsObj *vfs.VirtualFile // Preconditions: // - fs.renameMu must be locked. // - InteropModeShared is in effect. -func (fs *filesystem) revalidate(ctx context.Context, rp *vfs.ResolvingPath, start *dentry, done func() bool, ds **[]*dentry) error { +func (fs *filesystem) revalidate(ctx context.Context, rp resolvingPath, start *dentry, ds **[]*dentry) error { state := makeRevalidateState(start, true /* refreshStart */) defer state.release() done: - for cur := start; !done(); { + for cur := start; !rp.done(); { var err error cur, err = fs.revalidateStep(ctx, rp, cur, state) if err != nil { @@ -161,7 +143,7 @@ done: // - fs.renameMu must be locked. // - !rp.Done(). // - InteropModeShared is in effect (assumes no negative dentries). -func (fs *filesystem) revalidateStep(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, state *revalidateState) (*dentry, error) { +func (fs *filesystem) revalidateStep(ctx context.Context, rp resolvingPath, d *dentry, state *revalidateState) (*dentry, error) { switch name := rp.Component(); name { case ".": // Do nothing. diff --git a/pkg/sentry/vfs/resolving_path.go b/pkg/sentry/vfs/resolving_path.go index ad059a6f7..e1f9f9150 100644 --- a/pkg/sentry/vfs/resolving_path.go +++ b/pkg/sentry/vfs/resolving_path.go @@ -255,12 +255,16 @@ func (rp *ResolvingPath) Advance() { } // GetComponents emits all the remaining path components in rp. It does *not* -// update rp state. It halts if emit() returns false. -func (rp *ResolvingPath) GetComponents(emit func(string) bool) { +// update rp state. It halts if emit() returns false. If excludeLast is true, +// then the last path component is not emitted. +func (rp *ResolvingPath) GetComponents(excludeLast bool, emit func(string) bool) { // Copy rp state. cur := rp.pit curPart := rp.curPart for cur.Ok() { + if excludeLast && curPart == 0 && !cur.NextOk() { + break + } if !emit(cur.String()) { break }