summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorFabricio Voznika <fvoznika@google.com>2021-11-02 12:26:18 -0700
committergVisor bot <gvisor-bot@google.com>2021-11-02 12:29:06 -0700
commit1e1d6b2be37873c5e62461834df973f41565c662 (patch)
tree8fc32650747443694f77956da5f744f62df20b18
parent42a08f036f7be69b6c6d1b971911cd1aea611ece (diff)
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
-rw-r--r--pkg/p9/file.go35
-rw-r--r--pkg/p9/handlers.go6
-rw-r--r--pkg/p9/p9test/BUILD2
-rw-r--r--pkg/p9/p9test/p9test.go1
-rw-r--r--pkg/p9/server.go9
-rw-r--r--runsc/fsgofer/fsgofer.go11
-rw-r--r--test/syscalls/BUILD4
-rw-r--r--test/syscalls/linux/BUILD15
-rw-r--r--test/syscalls/linux/deleted.cc116
-rw-r--r--test/util/fs_util.cc8
-rw-r--r--test/util/fs_util.h1
11 files changed, 200 insertions, 8 deletions
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 <errno.h>
+#include <fcntl.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <string>
+
+#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<FileDescriptor> createdDeleted() {
+ auto path = NewTempAbsPath();
+ PosixErrorOr<FileDescriptor> 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);