vfs: skip write tracking for special files on open

[shayonj]

Linux skips the mount-level write check when opening a character device, block device, FIFO, or socket, so write-opening /dev/null on a read-only bind mount succeeds. gVisor was running that check unconditionally and returning EROFS, which breaks buildah's mount-bind-then-RO-remount hardening pattern when used under runsc.

This adds an IsSpecialFile option on FileDescription, paired across the open and close paths so the writer counter stays balanced, and set it on every open path that backs a special-file inode. The socket and host-imported FDs live on internal disconnected mounts that are never remounted RO, so flagging them is a no-op today and exists for parity with the kernel's predicate.

Fixes: #13148

[shayonj]

👋🏾 @nixprime - curious if you had any feedback 🙏🏾

[ayushr2]

Thanks for the fix!

[ayushr2]

Need a Googler to pull this in.

[milantracy]

there are some test failures at https://buildkite.com/gvisor/pipeline/builds/44757

do you want to take a look?

[shayonj]

thank you! just pushed the fix

[milantracy]

@shayonj

I made a tiny patch to your PR that fixes some test failure i see internally
i will merge the change at #13996

you can see the diff as follows,

• pkg/sentry/fsimpl/overlay/filesystem.go

@@ -896,7 +896,13 @@ func (d *dentry) ensureOpenableLocked(ct
                return nil
        }
 
-       if !ats.MayWrite() || linux.FileMode(d.mode.Load()).IsSpecialFile() {
+       if !ats.MayWrite() {
+               return nil
+       }
+       if !d.upperVD.Ok() && !d.canBeCopiedUp() {
+               return linuxerr.EPERM
+       }
+       if linux.FileMode(d.mode.Load()).IsSpecialFile() {
                return nil
        }

• test/syscalls/linux/mount.cc

 TEST(MountTest, ReadOnlyBindMountAllowsWriteOpenOfCharDevice) {
   SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));

-  auto const bindDir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
-  auto const mnt = ASSERT_NO_ERRNO_AND_VALUE(
-      Mount("/dev", bindDir.path(), "", MS_BIND, "", 0));
-  ASSERT_THAT(mount("/dev", bindDir.path().c_str(), nullptr,
-                    MS_BIND | MS_REMOUNT | MS_RDONLY, nullptr),
-              SyscallSucceeds());
-
-  std::string const nullPath = JoinPath(bindDir.path(), "null");
+  auto const dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  std::string const nullPath = JoinPath(dir.path(), "null");
+  FileDescriptor create_fd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(nullPath, O_CREAT | O_RDWR, 0666));
+  create_fd.reset();
+
+  auto const mnt = ASSERT_NO_ERRNO_AND_VALUE(
+      Mount("/dev/null", nullPath, "", MS_BIND, "", 0));
+  ASSERT_THAT(mount("/dev/null", nullPath.c_str(), nullptr,
+                    MS_BIND | MS_REMOUNT | MS_RDONLY, nullptr),
+              SyscallSucceeds());

   FileDescriptor wfd = ASSERT_NO_ERRNO_AND_VALUE(Open(nullPath, O_WRONLY));
   char msg[] = "hello";
   EXPECT_THAT(write(wfd.get(), msg, sizeof(msg)),
               SyscallSucceedsWithValue(sizeof(msg)));

[shayonj]

Thanks for catching this. I missed that my special file shortcut bypassed the existing check for lower layer files that aren't be copied up. For future - do you know why it didn't fail in the CI on this branch?

[milantracy]

the CI on buildkite doens't have any test coverage for the edge cases.

I created #13998, PRs are welcome if you are interesetd