chunked: fix ETXTBSY race in FS_IOC_ENABLE_VERITY with ForkLock

[giuseppe]

Concurrent fork(2) from other goroutines can duplicate a writable file descriptor. When the parent closes its copy, the forked child still holds a reference, so the kernel does not run __fput and inode->i_writecount remains elevated. FS_IOC_ENABLE_VERITY then fails with ETXTBSY because deny_write_access() sees a positive write count.

Fix this by holding syscall.ForkLock.RLock() while a writable fd exists. Go's forkExec acquires the exclusive ForkLock.Lock(), so no fork(2) can proceed while we hold the read lock. The writable fd is closed and the lock released as early as possible: immediately after writing completes and a read-only fd has been obtained via /proc/self/fd.

Closes: podman-container-tools/podman#28813

[mtrmac]

Just a quick look for now …

Preventing all forks for the duration of a hundreds-of-megabytes file operations is a bit of a cost, but without an O_CLOFORK it really might be the best we can do.

[mtrmac]

I have filed #1035 for unrelated the BlobInfoCache test failure, and restarted the test.

[Luap99]

Preventing all forks for the duration of a hundreds-of-megabytes file operations is a bit of a cost, but without an O_CLOFORK it really might be the best we can do.

Likely not a big deal for local podman, however the podman service and cri-o who do other work in parallel could need a long time until they fork some other process then if it writes a really big file.

I did not know go even offered ForkLock, but yeah I also see no way around using that then,

[mtrmac]

EnableVerity is also called in drivers/overlay; don’t we need something similar there as well?

---

It would probably be worth it to only prevent forks if FS verity is enabled; the PR does that in some but not all places. (A downside is that we might not notice all of the bad effects in non-FS-verity scenarios … but that’s also the upside.)

---

Hypothesizing about alternative approaches: The “unexpected fork” scenario presumably implies a future exec, I don’t think one can meaningfully for a multi-threaded Go program and expect it to continue. Could we first write all files, and then (in the same order) enable FS verity on all of them? That should mean a meaningful amount of time passes between closing the first writable FD (and risking a fork) and FS_IOC_ENABLE_VERITY, hoping that the exec happens in the meantime … except that might still be too quick for very small layers. Ugh… add a sleep?!

I don’t think ^^^ works well enough, hopefully others have better ideas.

---

To be explicit, do we have evidence that fork is the cause, or is it, at this point, a plausible hypothesis? (I generally agree that it is plausible, although I’m unsure what in the Podman process would be forking during a chunked pull).

[Luap99]

(I generally agree that it is plausible, although I’m unsure what in the Podman process would be forking during a chunked pull).

The flake was only visible on podman-remote testing, aka podman system service does the pulling. bats tests run in parallel with other test cases. Other tests case runs a container == fork/exec various processes, conmon, netavark, etc... and podman system service handles all requests in parallel.

So at least I think the story here checks out to the extend that we do only observe that failing on podman-remote. A podman pull will be just fine.

[giuseppe]

you can use this reproducer to see fork is enough:

package main

import (
	"fmt"
	"os"
	"runtime"
	"sync/atomic"
	"syscall"
	"unsafe"

	"golang.org/x/sys/unix"
)

func enableVerity(fd int) error {
	arg := unix.FsverityEnableArg{
		Version:        1,
		Hash_algorithm: unix.FS_VERITY_HASH_ALG_SHA256,
		Block_size:     4096,
	}
	_, _, e1 := syscall.Syscall(unix.SYS_IOCTL, uintptr(fd),
		uintptr(unix.FS_IOC_ENABLE_VERITY), uintptr(unsafe.Pointer(&arg)))
	if e1 != 0 {
		return e1
	}
	return nil
}

func forkAndExit() {
	pid, _, e1 := syscall.RawSyscall(syscall.SYS_CLONE, uintptr(syscall.SIGCHLD), 0, 0)
	if e1 != 0 {
		return
	}
	if pid == 0 {
		// Child: exit immediately.  Must use raw syscall — Go runtime
		// is not fork-safe.
		syscall.RawSyscall(syscall.SYS_EXIT_GROUP, 0, 0, 0)
	}
	syscall.Wait4(int(pid), nil, 0, nil)
}

func main() {
	nfiles := 2000

	fmt.Printf("fsverity ETXTBSY reproducer (fork-only, no exec) — GOMAXPROCS=%d, files=%d\n\n",
		runtime.GOMAXPROCS(0), nfiles)

	dir := "reproducer_tmp"
	os.MkdirAll(dir, 0755)
	defer os.RemoveAll(dir)

	var stopFork atomic.Bool
	for i := 0; i < 4; i++ {
		go func() {
			for !stopFork.Load() {
				forkAndExit()
			}
		}()
	}

	etxtbsy := 0
	other := 0

	for i := 0; i < nfiles; i++ {
		name := fmt.Sprintf("%s/f_%d", dir, i)

		// Open writable, write content.
		fd, err := unix.Open(name, unix.O_CREAT|unix.O_EXCL|unix.O_WRONLY|unix.O_TRUNC, 0644)
		if err != nil {
			fmt.Printf("open: %v\n", err)
			continue
		}
		unix.Write(fd, []byte("test content for verity\n"))

		roFd, err := unix.Open(fmt.Sprintf("/proc/self/fd/%d", fd), unix.O_RDONLY|unix.O_CLOEXEC, 0)
		if err != nil {
			unix.Close(fd)
			fmt.Printf("reopen: %v\n", err)
			continue
		}

		unix.Close(fd)

		err = enableVerity(roFd)
		if err != nil {
			if err == unix.ETXTBSY {
				etxtbsy++
			} else {
				other++
				fmt.Printf("enable verity: %v\n", err)
			}
		}

		unix.Close(roFd)
		unix.Unlink(name)
	}

	stopFork.Store(true)

	fmt.Printf("Results: %d/%d ETXTBSY", etxtbsy, nfiles)
	if other > 0 {
		fmt.Printf(", %d other errors", other)
	}
	fmt.Println()

	if etxtbsy > 0 {
		fmt.Println("Race reproduced!")
	} else {
		fmt.Println("No race observed (try again, or increase nfiles).")
	}
}

I get:

 go run reproducer.go
fsverity ETXTBSY reproducer (fork-only, no exec) — GOMAXPROCS=22, files=2000

Results: 1083/2000 ETXTBSY
Race reproduced!

I'll take care of your other comments

[mtrmac]

Sure I believe that a fork can trigger the error, it was more “is that how Podman is triggering it” — but Paul has a good explanation for that.

[giuseppe]

comments addressed and re-vendored in: podman-container-tools/podman#29309

[giuseppe]

@mtrmac any other blocker?