diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index f84352a73..3382af540 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -15,6 +15,7 @@ package p9 import ( + "errors" "fmt" "io" "sync/atomic" @@ -126,15 +127,58 @@ func (c *clientFile) MultiGetAttr(names []string) ([]FullStat, error) { return nil, unix.EBADF } - if !versionSupportsTmultiGetAttr(c.client.version) { - return DefaultMultiGetAttr(c, names) + if versionSupportsTmultiGetAttr(c.client.version) { + rmultigetattr := Rmultigetattr{} + if err := c.client.sendRecv(&Tmultigetattr{FID: c.fid, Names: names}, &rmultigetattr); err != nil { + return nil, err + } + return rmultigetattr.Stats, nil } - rmultigetattr := Rmultigetattr{} - if err := c.client.sendRecv(&Tmultigetattr{FID: c.fid, Names: names}, &rmultigetattr); err != nil { - return nil, err + stats := make([]FullStat, 0, len(names)) + var start File = c + parent := start + closeParent := func() { + if parent != start { + _ = parent.Close() + } } - return rmultigetattr.Stats, nil + defer closeParent() + mask := AttrMaskAll() + for i, name := range names { + if len(name) == 0 && i == 0 { + qid, valid, attr, err := parent.GetAttr(mask) + if err != nil { + return nil, err + } + stats = append(stats, FullStat{ + QID: qid, + Valid: valid, + Attr: attr, + }) + continue + } + qids, child, valid, attr, err := parent.WalkGetAttr([]string{name}) + if err != nil { + if errors.Is(err, unix.ENOENT) { + return stats, nil + } + return nil, err + } + closeParent() + parent = child + stats = append(stats, FullStat{ + QID: qids[0], + Valid: valid, + Attr: attr, + }) + if attr.Mode.FileType() != ModeDirectory { + // Doesn't need to continue if entry is not a dir. Including symlinks + // that cannot be followed. + break + } + } + return stats, nil } // StatFS implements File.StatFS. diff --git a/pkg/p9/file.go b/pkg/p9/file.go index ce649f69e..6ebcd0cbe 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -15,8 +15,6 @@ package p9 import ( - "errors" - "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/fd" ) @@ -30,6 +28,10 @@ type AttacherOptions struct { // AllocateOnDeleted is set to true if it's safe to call File.Allocate for // deleted files. AllocateOnDeleted bool + + // MultiGetAttrSupported is set to true if it's safe to call + // File.MultiGetAttr with read concurrency guarantee only on start directory. + MultiGetAttrSupported bool } // NoServerOptions partially implements Attacher with empty AttacherOptions. @@ -360,50 +362,3 @@ func (*DisallowServerCalls) Renamed(File, string) { func (*DisallowServerCalls) ServerOptions() AttacherOptions { panic("ServerOptions should not be called on the client") } - -// DefaultMultiGetAttr implements File.MultiGetAttr() on top of File. -func DefaultMultiGetAttr(start File, names []string) ([]FullStat, error) { - stats := make([]FullStat, 0, len(names)) - parent := start - closeParent := func() { - if parent != start { - _ = parent.Close() - } - } - defer closeParent() - mask := AttrMaskAll() - for i, name := range names { - if len(name) == 0 && i == 0 { - qid, valid, attr, err := parent.GetAttr(mask) - if err != nil { - return nil, err - } - stats = append(stats, FullStat{ - QID: qid, - Valid: valid, - Attr: attr, - }) - continue - } - qids, child, valid, attr, err := parent.WalkGetAttr([]string{name}) - if err != nil { - if errors.Is(err, unix.ENOENT) { - return stats, nil - } - return nil, err - } - closeParent() - parent = child - stats = append(stats, FullStat{ - QID: qids[0], - Valid: valid, - Attr: attr, - }) - if attr.Mode.FileType() != ModeDirectory { - // Doesn't need to continue if entry is not a dir. Including symlinks - // that cannot be followed. - break - } - } - return stats, nil -} diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 842b00855..4d331a950 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -15,6 +15,7 @@ package p9 import ( + errors2 "errors" "fmt" "io" "os" @@ -1537,12 +1538,79 @@ func (t *Tmultigetattr) handle(cs *connState) message { } defer ref.DecRef() - var stats []FullStat - if err := ref.safelyRead(func() (err error) { - stats, err = ref.file.MultiGetAttr(t.Names) - return err - }); err != nil { - return newErr(err) + if cs.server.options.MultiGetAttrSupported { + var stats []FullStat + if err := ref.safelyRead(func() (err error) { + stats, err = ref.file.MultiGetAttr(t.Names) + return err + }); err != nil { + return newErr(err) + } + return &Rmultigetattr{Stats: stats} } + + stats := make([]FullStat, 0, len(t.Names)) + mask := AttrMaskAll() + start := ref.file + startNode := ref.pathNode + parent := start + parentNode := startNode + closeParent := func() { + if parent != start { + _ = parent.Close() + } + } + defer closeParent() + + cs.server.renameMu.RLock() + defer cs.server.renameMu.RUnlock() + + for i, name := range t.Names { + if len(name) == 0 && i == 0 { + startNode.opMu.RLock() + qid, valid, attr, err := start.GetAttr(mask) + startNode.opMu.RUnlock() + if err != nil { + return newErr(err) + } + stats = append(stats, FullStat{ + QID: qid, + Valid: valid, + Attr: attr, + }) + continue + } + + parentNode.opMu.RLock() + if atomic.LoadUint32(&parentNode.deleted) != 0 { + parentNode.opMu.RUnlock() + break + } + qids, child, valid, attr, err := parent.WalkGetAttr([]string{name}) + if err != nil { + parentNode.opMu.RUnlock() + if errors2.Is(err, unix.ENOENT) { + break + } + return newErr(err) + } + stats = append(stats, FullStat{ + QID: qids[0], + Valid: valid, + Attr: attr, + }) + // Update with next generation. + closeParent() + parent = child + childNode := parentNode.pathNodeFor(name) + parentNode.opMu.RUnlock() + parentNode = childNode + if attr.Mode.FileType() != ModeDirectory { + // Doesn't need to continue if entry is not a dir. Including symlinks + // that cannot be followed. + break + } + } + return &Rmultigetattr{Stats: stats} } diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index eb86332cb..a317e0c7a 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -150,8 +150,9 @@ func (a *attachPoint) Attach() (p9.File, error) { // a file that was created in the same path as the delete file. func (a *attachPoint) ServerOptions() p9.AttacherOptions { return p9.AttacherOptions{ - SetAttrOnDeleted: true, - AllocateOnDeleted: true, + SetAttrOnDeleted: true, + AllocateOnDeleted: true, + MultiGetAttrSupported: true, } } @@ -869,7 +870,7 @@ func (*localFile) Rename(p9.File, string) error { panic("rename called directly") } -// RenameAt implements p9.File.RenameAt. +// RenameAt implements p9.File. func (l *localFile) RenameAt(oldName string, directory p9.File, newName string) error { if err := l.checkROMount(); err != nil { return err @@ -1133,7 +1134,7 @@ func (l *localFile) Flush() error { return nil } -// Bind implements p9.File.Bind. +// Bind implements p9.File. func (l *localFile) Bind(sockType uint32, sockName string, uid p9.UID, gid p9.GID) (p9.File, p9.QID, p9.AttrMask, p9.Attr, error) { if !l.attachPoint.conf.HostUDS { // Bind on host UDS is not allowed. As per mknod(2), which is invoked as @@ -1252,7 +1253,7 @@ func (l *localFile) Connect(flags p9.ConnectFlags) (*fd.FD, error) { return fd.New(f), nil } -// Close implements p9.File.Close. +// Close implements p9.File. func (l *localFile) Close() error { l.mode = invalidMode err := l.file.Close() @@ -1313,6 +1314,7 @@ func (l *localFile) checkROMount() error { return nil } +// MultiGetAttr implements p9.File. func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) { stats := make([]p9.FullStat, 0, len(names)) @@ -1329,6 +1331,12 @@ func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) { names = names[1:] } + // Note that while performing the walk below, we do not have read + // concurrency guarantee for any descendants. So files can be created/deleted + // while the walk is being performed. However, this should be fine from a + // security perspective as we are using host FDs to walk and checking that + // each opened path component is a directory. We also set O_NOFOLLOW to + // ensure no symlinks are accidentally followed during walk. parent := l.file.FD() closeParent := func() { if parent != l.file.FD() { @@ -1377,7 +1385,7 @@ type socketLocalFile struct { sock int } -// Close implements p9.File.Close. +// Close implements p9.File. func (l *socketLocalFile) Close() error { err := l.localFile.Close() err2 := unix.Close(l.sock)