runsc/boot: add per-mount directfs opt-out

[shayonj]

The --directfs flag is global. When it is enabled, every gofer mount in
the sandbox includes the "directfs" option, which causes the sentry to
require a host FD for the mount root from the gofer's Mount RPC reply.
Custom gofer servers that back virtual or network-backed filesystems
(e.g. a blob-store-backed volume gofer) cannot donate such an FD and
currently have to disable --directfs globally to coexist with the stock
gofer, which loses the directfs performance benefit on every other
mount.

This PR adds a new "directfs" key to the
dev.gvisor.spec.mount..* mount annotation. When set to "off", the
sentry suppresses the directfs mount option for that mount only. When
unset or set to "default", the mount follows the global --directfs
setting.

This remains an opt-out rather than a full per-mount override. The
annotation does not enable directfs when --directfs is disabled because
directfs also requires sandbox, sentry, and gofer setup that is keyed
off the global --directfs flag.

Plumbing reuses the existing MountHint mechanism in runsc/boot. The
rootfs path passes no mount hint to goferMountData and is therefore
unaffected. For stock fsgofer mixed mode, the gofer keeps LisaFS seccomp
filters when a fsgofer-handled mount opts out, because that mount can
still issue LisaFS RPCs.

Tests cover annotation parsing, invalid directfs values, goferMountData
behavior, and the gofer seccomp filter behavior for mixed DirectFS and
LisaFS mounts.

Fixes #13098.

[ayushr2]

Thanks for the PR!

The code review on GitHub seems to be broken so can't make review comments. I had 2 comments:

• Can we use true/false instead of on/off? Since the --directfs flag accepts true/false as well, it will be consistent.
• The meaning of this mount annotation is a bit confusing. If --directfs=false globally, but a user sets directfsDisabled=false explicitly, we will still not selectively enable directfs for that mount. Instead what we should do is have a mount option like directfsOverride which has a zero value of unset. And users can explicitly set it to true/false. If set to non-default, we override directfs setting for that mount.

[shayonj]

Thanks for the quick look.

Can we use true/false instead of on/off? Since the --directfs flag accepts true/false as well, it will be consistent.

Ah! makes sense. I was looking at few other knobs and saw on/off, but i had boolean figures originally. Will update.

The meaning of this mount annotation is a bit confusing. If --directfs=false globally, but a user sets directfsDisabled=false explicitly,....

I was debating this for a while in my head (:D), because there are a few different cases here.

• --directfs=true with no mount annotation should keep using DirectFS.
  ---directfs=true with directfs=false should disable DirectFS for that mount only.
• --directfs=true with directfs=true is just an explicit form of the default.
• --directfs=false with no mount annotation should keep DirectFS off.
• --directfs=false with directfs=false is also just an explicit form of the default.
• --directfs=false with directfs=true is the tricky case, because today --directfs controls more than the gofer mount option. It also gates the sandbox and gofer setup that DirectFS needs IIUC.

I had landed on an opt-out shape because my immediate use case only needed the --directfs=true with directfs=false case, and that kept the setup behavior tied to the global --directfs flag. But I agree with where you are going (IIUC that is). The annotation reads more naturally as an override of the global default, and then true and false are better values than on and off.

So perhaps it is better to make this a tri-state override internally, where unset means inherit --directfs, true enables DirectFS for that specific mount, and false disables DirectFS for that mount. The mount-data path can use the per-mount effective value, while the sandbox and gofer setup paths can use a separate “DirectFS is needed by any mount” value for the global setup that is currently keyed off conf.DirectFS (i will have to take a second look at this, but this is sort of where my mind is at)

I can rework the PR in that direction.

[EtiennePerot]

Not a great name, for two reasons:

• DirectFSDisabled=false is a double-negative
• DirectFSDisabled=false sounds like it should enable DirectFS, but actually form reading this PR it looks like it means "follow the global setting".

My understanding is that it should not be possible to use the mount option to enable DirectFS on a mount when DirectFS isn't also globally enabled (presumably because seccomp filters would make that impossible anyway).

I suggest changing the name of this field to SuppressDirectFS (removes double-negative in the false case, and doesn't imply that DirectFS is enabled).

[ayushr2]

Ah good point about secomp filters. Well, I have some bad news. When --directfs=true, we enable extra syscalls in the sentry BUT we also reduce the gofer seccomp filters assuming that no mount will make lisafs RPCs. When --directfs=false, we enable these extra syscalls in the gofer. This set of seccomp filters is defined here:

gvisor/runsc/fsgofer/filter/config.go

Lines 239 to 262 in 7504d50

	var lisafsFilters = seccomp.MakeSyscallRules(map[uintptr]seccomp.SyscallRule{
	unix.SYS_FALLOCATE: seccomp.PerArg{
	seccomp.AnyValue{},
	seccomp.EqualTo(0),
	},
	unix.SYS_FGETXATTR: seccomp.MatchAll{},
	unix.SYS_FSTATFS: seccomp.MatchAll{},
	unix.SYS_GETDENTS64: seccomp.MatchAll{},
	unix.SYS_LINKAT: seccomp.PerArg{
	seccomp.NonNegativeFD{},
	seccomp.AnyValue{},
	seccomp.NonNegativeFD{},
	seccomp.AnyValue{},
	seccomp.EqualTo(0),
	},
	unix.SYS_MKDIRAT: seccomp.MatchAll{},
	unix.SYS_MKNODAT: seccomp.MatchAll{},
	unix.SYS_READLINKAT: seccomp.MatchAll{},
	unix.SYS_RENAMEAT2: seccomp.MatchAll{},
	unix.SYS_SYMLINKAT: seccomp.MatchAll{},
	unix.SYS_FTRUNCATE: seccomp.MatchAll{},
	unix.SYS_UNLINKAT: seccomp.MatchAll{},
	unix.SYS_UTIMENSAT: seccomp.MatchAll{},
	})

See here when they are conditionally enabled:

gvisor/runsc/fsgofer/filter/filter.go

Lines 67 to 70 in 7504d50

	// When DirectFS is not enabled, filters for LisaFS are installed.
	if !opt.DirectFS {
	s.Merge(lisafsFilters)
	}

[ayushr2]

I don't think it will hurt your use-case since you are using a custom gofer with different seccomp profile. But it can cause a sandbox crash if a user configures a mount to use lisafs explicitly and the gofer tries to make one of those syscalls.

The gofer process is created per container. So you can check mount hints in that container's spec to see if lisafs explicitly is enabled by a mount handled by the fsgofer.

[shayonj]

Great catch, thanks. I added the SuppressDirectFS docstring context and kept this as an opt-out rather than a full per-mount override.

I also updated the stock gofer seccomp setup for the mixed case you pointed out. The gofer now checks the current container’s mount hints and mount configs, and keeps the lisafs syscall filters when a fsgofer-handled mount opts out of DirectFS. That should avoid the --directfs=true plus directfs=off case running with a gofer seccomp profile that assumes no lisafs RPCs.

[EtiennePerot]

Welp, posted the above before seeing this reply :)

What's clear from the above is that the current naming is no good.

[shayonj]

Thanks, both of you. I think there are two slightly different APIs here agreed. Just to list them out (so you can help correct my mental model too) -

One is a per-mount opt-out, where --directfs remains the only setting that enables the DirectFS setup paths, and the mount annotation can only suppress the directfs mount option for one mount. This is where my head was at original (albeit with some naming convention fixes that Etienne mentioned).

The other is a full per-mount override, where unset inherits --directfs, false disables DirectFS for the mount, and true enables DirectFS for the mount even if --directfs=false globally. I agree that this is a clean model in isolation, but looking at it now and implementing the --directfs=false plus mount directfs=true case correctly is more than a mount-data change. Today --directfs also gates sandbox capabilities, namespace and chroot setup, sentry seccomp filters, umask handling, and stock gofer FD donation.

So, yeah, given that, it feels like it is better to keep this PR as an opt-out rather than a full override. I will make that explicit by renaming the internal field to SuppressDirectFS (feedback from @EtiennePerot) and changing the documented values to something like default and off, where default means follow the global --directfs setting and off means suppress DirectFS for this mount. And then i think that should avoid the confusion around DirectFSDisabled=false and avoid implying that the annotation can enable DirectFS when global setup has not been enabled (?)

[shayonj]

Addressed and went with SuppressDirectFS. Happy to keep iterating, but felt like a good middle ground

[ayushr2]

The other is a full per-mount override, where unset inherits --directfs, false disables DirectFS for the mount, and true enables DirectFS for the mount even if --directfs=false globally. I agree that this is a clean model in isolation, but looking at it now and implementing the --directfs=false plus mount directfs=true case correctly is more than a mount-data change. Today --directfs also gates sandbox capabilities, namespace and chroot setup, sentry seccomp filters, umask handling, and stock gofer FD donation.

This is useful context. Can you add this in SuppressDirectFS docstring.