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

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);