fusefrontend: sharedstorage: use byte-range lock on file header creation

Multiple host writing to the same empty file at the same time
could have overwritten each other's newly created file header,
leading to data corruption.

Fix the race by placing a byte-range lock on the file when
creating the file header.

6 files changed, 175 insertions, 0 deletions

diff --git a/internal/fusefrontend/file.go b/internal/fusefrontend/file.go
index 64c6ca0..40dce6d 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"
 
@@ -90,6 +92,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 {
@@ -266,18 +275,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 52d4e0f..17e6a9e 100644
--- a/internal/syscallcompat/sys_darwin.go
+++ b/internal/syscallcompat/sys_darwin.go
@@ -33,6 +33,10 @@ const (
 	// On Darwin we use O_SYMLINK which allows opening a symlink itself.
 	// On Linux, we only have O_NOFOLLOW.
 	OpenatFlagNofollowSymlink = unix.O_SYMLINK
+
+	// F_OFD_SETLKW only exists on Linux. On Darwin, fall back to F_SETLKW as a
+	// flawed replacement.
+	F_OFD_SETLKW = unix.F_SETLKW
 )
 
 // Unfortunately fsetattrlist does not have a syscall wrapper yet.
diff --git a/internal/syscallcompat/sys_linux.go b/internal/syscallcompat/sys_linux.go
index a850ba1..51a2c09 100644
--- a/internal/syscallcompat/sys_linux.go
+++ b/internal/syscallcompat/sys_linux.go
@@ -35,6 +35,10 @@ const (
 
 	// Only defined on Linux
 	ENODATA = unix.ENODATA
+
+	// F_OFD_SETLKW only exists on Linux. On Darwin, fall back to F_SETLKW as a
+	// flawed replacement.
+	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 af93bc4..f9d0903 100644
--- a/tests/cluster/cluster_test.go
+++ b/tests/cluster/cluster_test.go
@@ -7,9 +7,12 @@ package cluster_test
 
 import (
 	"bytes"
+	"errors"
+	"io"
 	"math/rand"
 	"os"
 	"sync"
+	"syscall"
 	"testing"
 
 	"github.com/rfjakob/gocryptfs/v2/tests/test_helpers"
@@ -109,3 +112,75 @@ func TestClusterConcurrentRW(t *testing.T) {
 	go readThread(f2)
 	wg.Wait()
 }
+
+// Multiple hosts creating the same file at the same time could
+// overwrite each other's file header, leading to data
+// corruption. Passing "-sharedstorage" should prevent this.
+func TestConcurrentCreate(t *testing.T) {
+	cDir := test_helpers.InitFS(t)
+	mnt1 := cDir + ".mnt1"
+	mnt2 := cDir + ".mnt2"
+	test_helpers.MountOrFatal(t, cDir, mnt1, "-extpass=echo test", "-wpanic=0", "-sharedstorage")
+	defer test_helpers.UnmountPanic(mnt1)
+	test_helpers.MountOrFatal(t, cDir, mnt2, "-extpass=echo test", "-wpanic=0", "-sharedstorage")
+	defer test_helpers.UnmountPanic(mnt2)
+
+	var wg sync.WaitGroup
+	const loops = 10000
+
+	createOrOpen := func(path string) (f *os.File, err error) {
+		// Use the high-level os.Create/OpenFile instead of syscall.Open because we
+		// *want* Go's EINTR retry logic. glibc open(2) has similar logic.
+		f, err = os.OpenFile(path, os.O_CREATE|os.O_RDWR|os.O_EXCL, 0600)
+		if err == nil {
+			return
+		}
+		if !errors.Is(err, os.ErrExist) {
+			t.Logf("POSIX compliance issue: exclusive create failed with unexpected error: err=%v", errors.Unwrap(err))
+		}
+		f, err = os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0600)
+		if err == nil {
+			return
+		}
+		t.Logf("POSIX compliance issue: non-exlusive create failed with err=%v", errors.Unwrap(err))
+		return
+	}
+
+	workerThread := func(path string) {
+		defer wg.Done()
+		buf := make([]byte, 10)
+		for i := 0; i < loops; i++ {
+			if t.Failed() {
+				return
+			}
+			f, err := createOrOpen(path)
+			if err != nil {
+				// retry
+				continue
+			}
+			defer f.Close()
+			_, err = f.WriteAt(buf, 0)
+			if err != nil {
+				t.Errorf("iteration %d: Pwrite: %v", i, err)
+				return
+			}
+			buf2 := make([]byte, len(buf)+1)
+			n, err := f.ReadAt(buf2, 0)
+			if err != nil && err != io.EOF {
+				t.Errorf("iteration %d: ReadAt: %v", i, err)
+				return
+			}
+			buf2 = buf2[:n]
+			if !bytes.Equal(buf, buf2) {
+				t.Errorf("iteration %d: corrupt data received: %x", i, buf2)
+				return
+			}
+			syscall.Unlink(path)
+		}
+	}
+
+	wg.Add(2)
+	go workerThread(mnt1 + "/foo")
+	go workerThread(mnt2 + "/foo")
+	wg.Wait()
+}
diff --git a/tests/cluster/poc_test.go b/tests/cluster/poc_test.go
new file mode 100644
index 0000000..4d7182a
--- /dev/null
+++ b/tests/cluster/poc_test.go
@@ -0,0 +1,47 @@
+package cluster
+
+// poc_test.go contains proof of concept tests for the byte-range locking logic.
+// This goes directly to an underlying filesystem without going through gocryptfs.
+
+import (
+	"syscall"
+	"testing"
+
+	"golang.org/x/sys/unix"
+
+	"github.com/rfjakob/gocryptfs/v2/tests/test_helpers"
+)
+
+// Check that byte-range locks work on an empty file
+func TestPoCFcntlFlock(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")
+	}
+}