From 1e1d6b2be37873c5e62461834df973f41565c662 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Tue, 2 Nov 2021 12:26:18 -0700 Subject: Allow SetAttr and Allocate for deleted files It's safe to call SetAttr and Allocate on fsgofer because the file path is not used to open the file, if needed. Fixes #3654 PiperOrigin-RevId: 407149393 --- pkg/p9/file.go | 35 +++++++++++-- pkg/p9/handlers.go | 6 +-- pkg/p9/p9test/BUILD | 2 +- pkg/p9/p9test/p9test.go | 1 + pkg/p9/server.go | 9 +++- runsc/fsgofer/fsgofer.go | 11 ++++ test/syscalls/BUILD | 4 ++ test/syscalls/linux/BUILD | 15 ++++++ test/syscalls/linux/deleted.cc | 116 +++++++++++++++++++++++++++++++++++++++++ test/util/fs_util.cc | 8 +++ test/util/fs_util.h | 1 + 11 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 test/syscalls/linux/deleted.cc diff --git a/pkg/p9/file.go b/pkg/p9/file.go index 8d6af2d6b..b4b556cb9 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -21,13 +21,37 @@ import ( "gvisor.dev/gvisor/pkg/fd" ) +// AttacherOptions contains Attacher configuration. +type AttacherOptions struct { + // SetAttrOnDeleted is set to true if it's safe to call File.SetAttr for + // deleted files. + SetAttrOnDeleted bool + + // AllocateOnDeleted is set to true if it's safe to call File.Allocate for + // deleted files. + AllocateOnDeleted bool +} + +// NoServerOptions partially implements Attacher with empty AttacherOptions. +type NoServerOptions struct{} + +// ServerOptions implements Attacher. +func (*NoServerOptions) ServerOptions() AttacherOptions { + return AttacherOptions{} +} + // Attacher is provided by the server. type Attacher interface { // Attach returns a new File. // - // The client-side attach will be translate to a series of walks from + // The client-side attach will be translated to a series of walks from // the file returned by this Attach call. Attach() (File, error) + + // ServerOptions returns configuration options for this attach point. + // + // This is never caller in the client-side. + ServerOptions() AttacherOptions } // File is a set of operations corresponding to a single node. @@ -301,7 +325,7 @@ type File interface { type DefaultWalkGetAttr struct{} // WalkGetAttr implements File.WalkGetAttr. -func (DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) { +func (*DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) { return nil, nil, AttrMask{}, Attr{}, unix.ENOSYS } @@ -309,7 +333,7 @@ func (DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, er type DisallowClientCalls struct{} // SetAttrClose implements File.SetAttrClose. -func (DisallowClientCalls) SetAttrClose(SetAttrMask, SetAttr) error { +func (*DisallowClientCalls) SetAttrClose(SetAttrMask, SetAttr) error { panic("SetAttrClose should not be called on the server") } @@ -321,6 +345,11 @@ func (*DisallowServerCalls) Renamed(File, string) { panic("Renamed should not be called on the client") } +// ServerOptions implements Attacher. +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)) diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 2657081e3..c85af5e9e 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -178,7 +178,7 @@ func (t *Tsetattrclunk) handle(cs *connState) message { // This might be technically incorrect, as it's possible that // there were multiple links and you can still change the // corresponding inode information. - if ref.isDeleted() { + if !cs.server.options.SetAttrOnDeleted && ref.isDeleted() { return unix.EINVAL } @@ -913,7 +913,7 @@ func (t *Tsetattr) handle(cs *connState) message { // This might be technically incorrect, as it's possible that // there were multiple links and you can still change the // corresponding inode information. - if ref.isDeleted() { + if !cs.server.options.SetAttrOnDeleted && ref.isDeleted() { return unix.EINVAL } @@ -946,7 +946,7 @@ func (t *Tallocate) handle(cs *connState) message { } // We don't allow allocate on files that have been deleted. - if ref.isDeleted() { + if !cs.server.options.AllocateOnDeleted && ref.isDeleted() { return unix.EINVAL } diff --git a/pkg/p9/p9test/BUILD b/pkg/p9/p9test/BUILD index 9c1ada0cb..f3eb8468b 100644 --- a/pkg/p9/p9test/BUILD +++ b/pkg/p9/p9test/BUILD @@ -12,7 +12,7 @@ MOCK_SRC_PACKAGE = "gvisor.dev/gvisor/pkg/p9" # mockgen_reflect is a source file that contains mock generation code that # imports the p9 package and generates a specification via reflection. The # usual generation path must be split into two distinct parts because the full -# source tree is not available to all build targets. Only declared depencies +# source tree is not available to all build targets. Only declared dependencies # are available (and even then, not the Go source files). genrule( name = "mockgen_reflect", diff --git a/pkg/p9/p9test/p9test.go b/pkg/p9/p9test/p9test.go index fd5ac3dbe..56939d100 100644 --- a/pkg/p9/p9test/p9test.go +++ b/pkg/p9/p9test/p9test.go @@ -307,6 +307,7 @@ func NewHarness(t *testing.T) (*Harness, *p9.Client) { } // Start the server, synchronized on exit. + h.Attacher.EXPECT().ServerOptions().Return(p9.AttacherOptions{}).Times(1) server := p9.NewServer(h.Attacher) h.wg.Add(1) go func() { diff --git a/pkg/p9/server.go b/pkg/p9/server.go index 6428ad745..e7d129f9d 100644 --- a/pkg/p9/server.go +++ b/pkg/p9/server.go @@ -34,6 +34,8 @@ type Server struct { // attacher provides the attach function. attacher Attacher + options AttacherOptions + // pathTree is the full set of paths opened on this server. // // These may be across different connections, but rename operations @@ -48,10 +50,15 @@ type Server struct { renameMu sync.RWMutex } -// NewServer returns a new server. +// NewServer returns a new server. attacher may be nil. func NewServer(attacher Attacher) *Server { + opts := AttacherOptions{} + if attacher != nil { + opts = attacher.ServerOptions() + } return &Server{ attacher: attacher, + options: opts, pathTree: newPathNode(), } } diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index 600b21189..3d610199c 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -140,6 +140,17 @@ func (a *attachPoint) Attach() (p9.File, error) { return lf, nil } +// ServerOptions implements p9.Attacher. It's safe to call SetAttr and Allocate +// on deleted files because fsgofer either uses an existing FD or opens a new +// one using the magic symlink in `/proc/[pid]/fd` and cannot mistakely open +// 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, + } +} + // makeQID returns a unique QID for the given stat buffer. func (a *attachPoint) makeQID(stat *unix.Stat_t) p9.QID { a.deviceMu.Lock() diff --git a/test/syscalls/BUILD b/test/syscalls/BUILD index f748d685a..7952fd969 100644 --- a/test/syscalls/BUILD +++ b/test/syscalls/BUILD @@ -1053,3 +1053,7 @@ syscall_test( syscall_test( test = "//test/syscalls/linux:verity_mount_test", ) + +syscall_test( + test = "//test/syscalls/linux:deleted_test", +) diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 6217ff4dc..020c4673a 100644 --- a/test/syscalls/linux/BUILD +++ b/test/syscalls/linux/BUILD @@ -4432,3 +4432,18 @@ cc_binary( "@com_google_absl//absl/container:flat_hash_set", ], ) + +cc_binary( + name = "deleted_test", + testonly = 1, + srcs = ["deleted.cc"], + linkstatic = 1, + deps = [ + "//test/util:file_descriptor", + "//test/util:fs_util", + gtest, + "//test/util:temp_path", + "//test/util:test_main", + "//test/util:test_util", + ], +) diff --git a/test/syscalls/linux/deleted.cc b/test/syscalls/linux/deleted.cc new file mode 100644 index 000000000..695ceafd3 --- /dev/null +++ b/test/syscalls/linux/deleted.cc @@ -0,0 +1,116 @@ +// Copyright 2021 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include + +#include + +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "test/util/file_descriptor.h" +#include "test/util/fs_util.h" +#include "test/util/temp_path.h" +#include "test/util/test_util.h" + +constexpr mode_t mode = 1; + +namespace gvisor { +namespace testing { + +namespace { + +PosixErrorOr createdDeleted() { + auto path = NewTempAbsPath(); + PosixErrorOr fd = Open(path, O_RDWR | O_CREAT, mode); + if (!fd.ok()) { + return fd.error(); + } + + auto err = Unlink(path); + if (!err.ok()) { + return err; + } + return fd; +} + +TEST(DeletedTest, Utime) { + auto fd = ASSERT_NO_ERRNO_AND_VALUE(createdDeleted()); + + const struct timespec times[2] = {{10, 0}, {20, 0}}; + EXPECT_THAT(futimens(fd.get(), times), SyscallSucceeds()); + + struct stat stat; + ASSERT_THAT(fstat(fd.get(), &stat), SyscallSucceeds()); + EXPECT_EQ(10, stat.st_atime); + EXPECT_EQ(20, stat.st_mtime); +} + +TEST(DeletedTest, Chmod) { + auto fd = ASSERT_NO_ERRNO_AND_VALUE(createdDeleted()); + + ASSERT_THAT(fchmod(fd.get(), mode + 1), SyscallSucceeds()); + + struct stat stat; + ASSERT_THAT(fstat(fd.get(), &stat), SyscallSucceeds()); + EXPECT_EQ(mode + 1, stat.st_mode & ~S_IFMT); +} + +TEST(DeletedTest, Truncate) { + auto fd = ASSERT_NO_ERRNO_AND_VALUE(createdDeleted()); + const std::string data = "foobar"; + ASSERT_THAT(write(fd.get(), data.c_str(), data.size()), SyscallSucceeds()); + + ASSERT_THAT(ftruncate(fd.get(), 0), SyscallSucceeds()); + + struct stat stat; + ASSERT_THAT(fstat(fd.get(), &stat), SyscallSucceeds()); + ASSERT_EQ(stat.st_size, 0); +} + +TEST(DeletedTest, Fallocate) { + auto fd = ASSERT_NO_ERRNO_AND_VALUE(createdDeleted()); + + ASSERT_THAT(fallocate(fd.get(), 0, 0, 123), SyscallSucceeds()); + + struct stat stat; + ASSERT_THAT(fstat(fd.get(), &stat), SyscallSucceeds()); + EXPECT_EQ(123, stat.st_size); +} + +// Tests that a file can be created with the same path as a deleted file that +// still have an open FD to it. +TEST(DeletedTest, Replace) { + auto path = NewTempAbsPath(); + auto fd = ASSERT_NO_ERRNO_AND_VALUE(Open(path, O_RDWR | O_CREAT, mode)); + ASSERT_NO_ERRNO(Unlink(path)); + + auto other = + ASSERT_NO_ERRNO_AND_VALUE(Open(path, O_RDWR | O_CREAT | O_EXCL, mode)); + + auto stat = ASSERT_NO_ERRNO_AND_VALUE(Fstat(fd.get())); + auto stat_other = ASSERT_NO_ERRNO_AND_VALUE(Fstat(other.get())); + ASSERT_NE(stat.st_ino, stat_other.st_ino); + + // Check that the path points to the new file. + stat = ASSERT_NO_ERRNO_AND_VALUE(Stat(path)); + ASSERT_EQ(stat.st_ino, stat_other.st_ino); +} + +} // namespace + +} // namespace testing +} // namespace gvisor diff --git a/test/util/fs_util.cc b/test/util/fs_util.cc index 253411858..1c24d9ffc 100644 --- a/test/util/fs_util.cc +++ b/test/util/fs_util.cc @@ -188,6 +188,14 @@ PosixError MknodAt(const FileDescriptor& dfd, absl::string_view path, int mode, return NoError(); } +PosixError Unlink(absl::string_view path) { + int res = unlink(std::string(path).c_str()); + if (res < 0) { + return PosixError(errno, absl::StrCat("unlink ", path)); + } + return NoError(); +} + PosixError UnlinkAt(const FileDescriptor& dfd, absl::string_view path, int flags) { int res = unlinkat(dfd.get(), std::string(path).c_str(), flags); diff --git a/test/util/fs_util.h b/test/util/fs_util.h index bb2d1d3c8..3ae0a725a 100644 --- a/test/util/fs_util.h +++ b/test/util/fs_util.h @@ -71,6 +71,7 @@ PosixError MknodAt(const FileDescriptor& dfd, absl::string_view path, int mode, dev_t dev); // Unlink the file. +PosixError Unlink(absl::string_view path); PosixError UnlinkAt(const FileDescriptor& dfd, absl::string_view path, int flags); -- cgit v1.2.3