From 964f0c190932e5dc53b05ec69ccda6e8d33a73b6 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Fri, 2 Jun 2023 14:24:44 +0200 Subject: fusefrontend: sharedstorage: use byte-range lock on file header creation Multiple hosts creating the same file at the same time could have overwritten each other's file header, leading to data corruption. Fix the race by placing a byte-range lock on the file when creating the file header. --- internal/fusefrontend/file.go | 16 ++++++++++++++++ internal/fusefrontend/file_lock.go | 29 ++++++++++++++++++++++++++++ internal/syscallcompat/sys_darwin.go | 4 ++++ internal/syscallcompat/sys_linux.go | 1 + tests/cluster/cluster_test.go | 37 ++++++++++++++++++++++++++++++++++++ 5 files changed, 87 insertions(+) create mode 100644 internal/fusefrontend/file_lock.go diff --git a/internal/fusefrontend/file.go b/internal/fusefrontend/file.go index 8d0ba01..9bd05e6 100644 --- a/internal/fusefrontend/file.go +++ b/internal/fusefrontend/file.go @@ -14,6 +14,8 @@ import ( "sync" "syscall" + "golang.org/x/sys/unix" + "github.com/hanwen/go-fuse/v2/fs" "github.com/hanwen/go-fuse/v2/fuse" @@ -91,6 +93,13 @@ func (f *File) readFileID() ([]byte, error) { // and not only the header. A header-only file will be considered empty. // This makes File ID poisoning more difficult. readLen := contentenc.HeaderLen + 1 + if f.rootNode.args.SharedStorage { + // With -sharedstorage, we consider a header-only file as valid, because + // another gocryptfs process may have either: + // 1) just created the header, and not written further data yet. + // 2) truncated the file down to just the header. + readLen = contentenc.HeaderLen + } buf := make([]byte, readLen) n, err := f.fd.ReadAt(buf, 0) if err != nil { @@ -267,18 +276,25 @@ func (f *File) doWrite(data []byte, off int64) (uint32, syscall.Errno) { // // If the file ID is not cached, read it from disk if f.fileTableEntry.ID == nil { + if err := f.LockSharedStorage(unix.F_WRLCK, 0, contentenc.HeaderLen); err != nil { + return 0, fs.ToErrno(err) + } var err error fileID, err := f.readFileID() // Write a new file header if the file is empty if err == io.EOF { fileID, err = f.createHeader() fileWasEmpty = true + // Having the unlock three times is ugly. But every other way I tried is even uglier. + f.LockSharedStorage(unix.F_UNLCK, 0, contentenc.HeaderLen) } else if err != nil { // Other errors mean readFileID() found a corrupt header tlog.Warn.Printf("doWrite %d: corrupt header: %v", f.qIno.Ino, err) + f.LockSharedStorage(unix.F_UNLCK, 0, contentenc.HeaderLen) return 0, syscall.EIO } if err != nil { + f.LockSharedStorage(unix.F_UNLCK, 0, contentenc.HeaderLen) return 0, fs.ToErrno(err) } f.fileTableEntry.ID = fileID diff --git a/internal/fusefrontend/file_lock.go b/internal/fusefrontend/file_lock.go new file mode 100644 index 0000000..6f92cfe --- /dev/null +++ b/internal/fusefrontend/file_lock.go @@ -0,0 +1,29 @@ +package fusefrontend + +import ( + "golang.org/x/sys/unix" + + "github.com/rfjakob/gocryptfs/v2/internal/syscallcompat" +) + +// SharedStorageLock conveniently wraps F_OFD_SETLKW +// See https://man7.org/linux/man-pages/man2/fcntl.2.html -> "Open file description locks (non-POSIX)" +// +// lkType is one of: +// * unix.F_RDLCK (shared read lock) +// * unix.F_WRLCK (exclusive write lock) +// * unix.F_UNLCK (unlock) +// +// This function is a no-op if args.SharedStorage == false. +func (f *File) LockSharedStorage(lkType int16, lkStart int64, lkLen int64) error { + if !f.rootNode.args.SharedStorage { + return nil + } + lk := unix.Flock_t{ + Type: lkType, + Whence: unix.SEEK_SET, + Start: lkStart, + Len: lkLen, + } + return unix.FcntlFlock(uintptr(f.intFd()), syscallcompat.F_OFD_SETLKW, &lk) +} diff --git a/internal/syscallcompat/sys_darwin.go b/internal/syscallcompat/sys_darwin.go index 06f09f0..b13b6a5 100644 --- a/internal/syscallcompat/sys_darwin.go +++ b/internal/syscallcompat/sys_darwin.go @@ -27,6 +27,10 @@ const ( RENAME_EXCHANGE = 2 RENAME_WHITEOUT = 4 + // F_OFD_SETLKW only exists on Linux. On Darwin, fall back to F_SETLKW as a + // flawed replacement. + F_OFD_SETLKW = unix.F_SETLKW + // KAUTH_UID_NONE and KAUTH_GID_NONE are special values to // revert permissions to the process credentials. KAUTH_UID_NONE = ^uint32(0) - 100 diff --git a/internal/syscallcompat/sys_linux.go b/internal/syscallcompat/sys_linux.go index a64b27e..3714b75 100644 --- a/internal/syscallcompat/sys_linux.go +++ b/internal/syscallcompat/sys_linux.go @@ -32,6 +32,7 @@ const ( RENAME_NOREPLACE = unix.RENAME_NOREPLACE RENAME_WHITEOUT = unix.RENAME_WHITEOUT RENAME_EXCHANGE = unix.RENAME_EXCHANGE + F_OFD_SETLKW = unix.F_OFD_SETLKW ) var preallocWarn sync.Once diff --git a/tests/cluster/cluster_test.go b/tests/cluster/cluster_test.go index 2e969ce..def03f1 100644 --- a/tests/cluster/cluster_test.go +++ b/tests/cluster/cluster_test.go @@ -10,8 +10,11 @@ import ( "math/rand" "os" "sync" + "syscall" "testing" + "golang.org/x/sys/unix" + "github.com/rfjakob/gocryptfs/v2/tests/test_helpers" ) @@ -109,3 +112,37 @@ func TestClusterConcurrentRW(t *testing.T) { go readThread(f2) wg.Wait() } + +// Check that byte-range locks work on an empty file +func TestFcntlFlock(t *testing.T) { + path := test_helpers.TmpDir + "/" + t.Name() + + fd1, err := syscall.Open(path, syscall.O_CREAT|syscall.O_WRONLY|syscall.O_EXCL, 0600) + if err != nil { + t.Fatal(err) + } + defer syscall.Close(fd1) + + // F_OFD_SETLK locks on the same fd always succeed, so we have to + // open a 2nd time. + fd2, err := syscall.Open(path, syscall.O_RDWR, 0) + if err != nil { + t.Fatal(err) + } + defer syscall.Close(fd2) + + lk := unix.Flock_t{ + Type: unix.F_WRLCK, + Whence: unix.SEEK_SET, + Start: 0, + Len: 0, + } + err = unix.FcntlFlock(uintptr(fd1), unix.F_OFD_SETLK, &lk) + if err != nil { + t.Fatal(err) + } + err = unix.FcntlFlock(uintptr(fd2), unix.F_OFD_SETLK, &lk) + if err == nil { + t.Fatal("double-lock succeeded but should have failed") + } +} -- cgit v1.2.3