Disk: Allow volume security.shifted and directory shift to be enabled when raw.idmap is set on a VM

doc/metadata.txt

@@ -226,12 +226,12 @@ See the Linux Kernel [shared subtree](https://www.kernel.org/doc/Documentation/f
 ```
 
 ```{config:option} shift device-disk-device-conf
-:condition: "container"
 :defaultdesc: "`false`"
 :required: "no"
 :shortdesc: "Whether to set up a UID/GID shifting overlay"
 :type: "bool"
-If enabled, this option sets up a shifting overlay to translate the source UID/GID to match the container instance.
+For containers, if enabled, this option sets up a shifting overlay to translate the source UID/GID to match the instance.
+For virtual machines, the source UID/GID is passed through unchanged, even if the instance `raw.idmap` is set.
 ```
 
 ```{config:option} size device-disk-device-conf

lxd/device/device_utils_disk.go

@@ -241,8 +241,59 @@ func diskAddRootUserNSEntry(idmaps []idmap.IdmapEntry, hostRootID int64) []idmap
 	return idmaps
 }
 
+// diskVMVirtiofsdResolveIDMaps returns explicit idmaps if provided, or the current namespace mappings otherwise.
+func diskVMVirtiofsdResolveIDMaps(idmaps []idmap.IdmapEntry, currentIdmapSetFunc func() (*idmap.IdmapSet, error)) ([]idmap.IdmapEntry, error) {
+	if len(idmaps) > 0 {
+		return idmaps, nil
+	}
+
+	if currentIdmapSetFunc == nil {
+		return nil, errors.New("Current idmap set function is nil")
+	}
+
+	currentIdmapSet, err := currentIdmapSetFunc()
+	if err != nil {
+		return nil, fmt.Errorf("Failed getting current idmap set: %w", err)
+	}
+
+	if currentIdmapSet == nil || len(currentIdmapSet.Idmap) == 0 {
+		return nil, errors.New("Current idmap set cannot be empty")
+	}
+
+	// The current idmap set maps IDs in the current namespace (Nsid) to IDs in its parent (Hostid).
+	// virtiofsd runs in a child namespace of the current one, so build an identity map over each
+	// current Nsid range (Hostid = Nsid) to pass the host's ID range through unchanged. Reusing the
+	// parent Hostid values directly would be incorrect when LXD itself is nested.
+	effectiveIDMaps := make([]idmap.IdmapEntry, 0, len(currentIdmapSet.Idmap))
+	hasUIDMap := false
+	hasGIDMap := false
+	for _, idmapEntry := range currentIdmapSet.Idmap {
+		effectiveIDMaps = append(effectiveIDMaps, idmap.IdmapEntry{
+			Hostid:   idmapEntry.Nsid,
+			Isuid:    idmapEntry.Isuid,
+			Isgid:    idmapEntry.Isgid,
+			Nsid:     idmapEntry.Nsid,
+			Maprange: idmapEntry.Maprange,
+		})
+
+		if idmapEntry.Isuid {
+			hasUIDMap = true
+		}
+
+		if idmapEntry.Isgid {
+			hasGIDMap = true
+		}
+	}
+
+	if !hasUIDMap || !hasGIDMap {
+		return nil, errors.New("Current idmap set must contain both UID and GID mappings")
+	}
+
+	return effectiveIDMaps, nil
+}
+
 // DiskVMVirtiofsdStart starts a new virtiofsd process with a socket present at the supplied path.
-// If the idmaps slice is supplied then the proxy process is run inside a user namespace using the supplied maps.
+// If the idmaps slice is empty, the current namespace mappings are used.
 // Returns UnsupportedError error if the host system or instance does not support virtiofsd, returns normal error
 // type if process cannot be started for other reasons.
 // Returns a revert function on success.
@@ -334,10 +385,23 @@ func DiskVMVirtiofsdStart(inst instance.Instance, socketPath string, pidPath str
 		return nil, err
 	}
 
-	if len(idmaps) > 0 {
-		proc.SetUserns(&idmap.IdmapSet{Idmap: idmaps})
+	// This is required because virtiofsd is split into two long-running processes.
+	// The child calls `pivot_root(2)`, which sandboxes both processes inside `sharePath`.
+	// However, it only pivots the working directory of the parent process when it starts as `/`.
+	// Normally this would only prevent unmounting LXD's working directory, which is OK.
+	// But when we run virtiofsd from a non-initial user namespace, all existing mounts are
+	// brought into the sandbox as a single unit (see `mount_namespaces(7)`). These remain
+	// alive even after unmounting them on the host (MNT_LOCKED), which can prevent LXD from
+	// deactivating instance volumes.
+	proc.Dir = "/"
+
+	effectiveIDMaps, err := diskVMVirtiofsdResolveIDMaps(idmaps, idmap.CurrentIdmapSet)
+	if err != nil {
+		return nil, err
 	}
 
+	proc.SetUserns(&idmap.IdmapSet{Idmap: effectiveIDMaps}, true)
+
 	err = proc.StartWithFiles(context.Background(), []*os.File{unixFile})
 	if err != nil {
 		return nil, fmt.Errorf("Failed starting virtiofsd: %w", err)

lxd/device/device_utils_disk_test.go

@@ -1,13 +1,127 @@
 package device
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 
 	"github.com/canonical/lxd/lxd/idmap"
 )
 
+func TestDiskVMVirtiofsdResolveIDMaps(t *testing.T) {
+	t.Run("Use supplied idmaps", func(t *testing.T) {
+		expected := []idmap.IdmapEntry{
+			{
+				Hostid:   1000,
+				Isuid:    true,
+				Nsid:     0,
+				Maprange: 1,
+			},
+			{
+				Hostid:   1000,
+				Isgid:    true,
+				Nsid:     0,
+				Maprange: 1,
+			},
+		}
+
+		currentIDMapSetFunc := func() (*idmap.IdmapSet, error) {
+			return nil, errors.New("should not be called")
+		}
+
+		actual, err := diskVMVirtiofsdResolveIDMaps(expected, currentIDMapSetFunc)
+		assert.NoError(t, err)
+		assert.Equal(t, expected, actual)
+	})
+
+	t.Run("Fallback to current idmap set", func(t *testing.T) {
+		// The current idmap set maps current-namespace IDs (Nsid) to parent IDs (Hostid), here a
+		// non-identity mapping (0 -> 100000). The resolved maps must be an identity map over the
+		// current Nsid range (Hostid = Nsid) so nested LXD can start virtiofsd correctly.
+		currentIDMapSet := &idmap.IdmapSet{Idmap: []idmap.IdmapEntry{
+			{
+				Hostid:   100000,
+				Isuid:    true,
+				Nsid:     0,
+				Maprange: 65536,
+			},
+			{
+				Hostid:   100000,
+				Isgid:    true,
+				Nsid:     0,
+				Maprange: 65536,
+			},
+		}}
+
+		expected := []idmap.IdmapEntry{
+			{
+				Hostid:   0,
+				Isuid:    true,
+				Nsid:     0,
+				Maprange: 65536,
+			},
+			{
+				Hostid:   0,
+				Isgid:    true,
+				Nsid:     0,
+				Maprange: 65536,
+			},
+		}
+
+		currentIDMapSetFunc := func() (*idmap.IdmapSet, error) {
+			return currentIDMapSet, nil
+		}
+
+		actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc)
+		assert.NoError(t, err)
+		assert.Equal(t, expected, actual)
+	})
+
+	t.Run("Fail if getting current idmap set", func(t *testing.T) {
+		currentIDMapSetFunc := func() (*idmap.IdmapSet, error) {
+			return nil, errors.New("boom")
+		}
+
+		actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc)
+		assert.Nil(t, actual)
+		assert.EqualError(t, err, "Failed getting current idmap set: boom")
+	})
+
+	t.Run("Fail if current idmap set empty", func(t *testing.T) {
+		currentIDMapSetFunc := func() (*idmap.IdmapSet, error) {
+			return &idmap.IdmapSet{}, nil
+		}
+
+		actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc)
+		assert.Nil(t, actual)
+		assert.EqualError(t, err, "Current idmap set cannot be empty")
+	})
+
+	t.Run("Fail if current idmap set has no gid map", func(t *testing.T) {
+		currentIDMapSetFunc := func() (*idmap.IdmapSet, error) {
+			return &idmap.IdmapSet{Idmap: []idmap.IdmapEntry{
+				{
+					Hostid:   100000,
+					Isuid:    true,
+					Nsid:     0,
+					Maprange: 65536,
+				},
+			}}, nil
+		}
+
+		actual, err := diskVMVirtiofsdResolveIDMaps(nil, currentIDMapSetFunc)
+		assert.Nil(t, actual)
+		assert.EqualError(t, err, "Current idmap set must contain both UID and GID mappings")
+	})
+
+	t.Run("Fail if current idmap set function is nil", func(t *testing.T) {
+		actual, err := diskVMVirtiofsdResolveIDMaps(nil, nil)
+		assert.Nil(t, actual)
+		assert.EqualError(t, err, "Current idmap set function is nil")
+	})
+}
+
 func TestDiskAddRootUserNSEntry(t *testing.T) {
 	// Check adds a combined uid/gid root entry to an empty list.
 	var idmaps []idmap.IdmapEntry

lxd/device/disk.go

@@ -239,12 +239,12 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
 		//  shortdesc: Whether to recursively mount the source path
 		"recursive": validate.Optional(validate.IsBool),
 		// lxdmeta:generate(entities=device-disk; group=device-conf; key=shift)
-		// If enabled, this option sets up a shifting overlay to translate the source UID/GID to match the container instance.
+		// For containers, if enabled, this option sets up a shifting overlay to translate the source UID/GID to match the instance.
+		// For virtual machines, the source UID/GID is passed through unchanged, even if the instance `raw.idmap` is set.
 		// ---
 		//  type: bool
 		//  defaultdesc: `false`
 		//  required: no
-		//  condition: container
 		//  shortdesc: Whether to set up a UID/GID shifting overlay
 		"shift": validate.Optional(validate.IsBool),
 		// lxdmeta:generate(entities=device-disk; group=device-conf; key=source)
@@ -1173,6 +1173,16 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) {
 					mount.FSType = "iso9660"
 				}
 
+				if shared.IsTrue(dbVolume.Config["security.shifted"]) {
+					// To be consistent with containers, we use the OwnerShift
+					// flag here even though it means something different for
+					// VMs. Containers use ID-mapped mounts because it makes
+					// UIDs and GIDs look the same on the host and in the
+					// VM. For VMs, the same effect is achieved by using an
+					// identity mapping for virtiofsd's nested user namespace.
+					mount.OwnerShift = deviceConfig.MountOwnerShiftDynamic
+				}
+
 				revertFunc, mountedPath, _, err := d.mountPoolVolume()
 				if err != nil {
 					return nil, diskSourceNotFoundError{msg: "Failed mounting volume", err: err}
@@ -1233,6 +1243,10 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) {
 					return nil, errors.New(`Missing mount "path" setting`)
 				}
 
+				if shared.IsTrue(d.config["shift"]) {
+					mount.OwnerShift = deviceConfig.MountOwnerShiftDynamic
+				}
+
 				// Mount the source in the instance devices directory.
 				// This will ensure that if the exported directory configured as readonly that this
 				// takes effect event if using virtio-fs (which doesn't support read only mode) by
@@ -1250,9 +1264,20 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) {
 				mount.TargetPath = d.config["path"]
 				mount.FSType = "virtiofs"
 
-				rawIDMaps, err := idmap.ParseRawIdmap(d.inst.ExpandedConfig()["raw.idmap"])
-				if err != nil {
-					return nil, fmt.Errorf(`Failed parsing instance "raw.idmap": %w`, err)
+				// When security.shifted=true, the volume's files are owned by real users on the
+				// host (e.g. UID 0 not 1000000). For containers, the mount needs to be shifted to
+				// counteract the effect of entering a user namespace. But VMs don't use user
+				// namespaces, so we actually don't want to shift the virtiofsd process.
+				//
+				// Also, we should ignore raw.idmap for consistency with containers. If I create a
+				// file as user 1000 inside the container, the file on disk is owned by UID 1000.
+				// We don't care that container user is actually 1001000 in the root namespace.
+				var rawIDMaps []idmap.IdmapEntry
+				if mount.OwnerShift != deviceConfig.MountOwnerShiftDynamic {
+					rawIDMaps, err = idmap.ParseRawIdmap(d.inst.ExpandedConfig()["raw.idmap"])
+					if err != nil {
+						return nil, fmt.Errorf(`Failed parsing instance "raw.idmap": %w`, err)
+					}
 				}
 
 				// If we are using restricted parent source path mode, or if a non-empty set of

lxd/metadata/configuration.json

@@ -254,9 +254,8 @@
 					},
 					{
 						"shift": {
-							"condition": "container",
 							"defaultdesc": "`false`",
-							"longdesc": "If enabled, this option sets up a shifting overlay to translate the source UID/GID to match the container instance.",
+							"longdesc": "For containers, if enabled, this option sets up a shifting overlay to translate the source UID/GID to match the instance.\nFor virtual machines, the source UID/GID is passed through unchanged, even if the instance `raw.idmap` is set.",
 							"required": "no",
 							"shortdesc": "Whether to set up a UID/GID shifting overlay",
 							"type": "bool"

lxd/subprocess/proc.go

@@ -38,6 +38,7 @@ type Process struct {
 	UID       uint32 `yaml:"uid"`
 	GID       uint32 `yaml:"gid"`
 	SetGroups bool   `yaml:"set_groups"`
+	Dir       string `yaml:"dir"`
 	StartTime int64  `yaml:"start_time"`
 
 	SysProcAttr *syscall.SysProcAttr
@@ -129,6 +130,7 @@ func (p *Process) start(ctx context.Context, fds []*os.File) error {
 	cmd.Stdout = p.stdout
 	cmd.Stderr = p.stderr
 	cmd.Stdin = p.stdin
+	cmd.Dir = p.Dir
 	cmd.SysProcAttr = p.SysProcAttr
 	if cmd.SysProcAttr == nil {
 		cmd.SysProcAttr = &syscall.SysProcAttr{}

lxd/subprocess/proc_linux.go

@@ -9,14 +9,16 @@ import (
 )
 
 // SetUserns allows running inside of a user namespace.
-func (p *Process) SetUserns(userns *idmap.IdmapSet) {
+// If enableSetgroups is true, the process is allowed to use the setgroups syscall inside the user namespace.
+func (p *Process) SetUserns(userns *idmap.IdmapSet, enableSetgroups bool) {
 	p.SysProcAttr = &syscall.SysProcAttr{
 		Cloneflags: syscall.CLONE_NEWUSER,
 		Credential: &syscall.Credential{
 			Uid: uint32(0),
 			Gid: uint32(0),
 		},
-		UidMappings: userns.ToUidMappings(),
-		GidMappings: userns.ToGidMappings(),
+		UidMappings:                userns.ToUidMappings(),
+		GidMappings:                userns.ToGidMappings(),
+		GidMappingsEnableSetgroups: enableSetgroups,
 	}
 }

lxd/subprocess/proc_others.go

@@ -7,6 +7,7 @@ import (
 )
 
 // SetUserns allows running inside of a user namespace.
-func (p *Process) SetUserns(userns *idmap.IdmapSet) {
+// If enableSetgroups is true, the process is allowed to use the setgroups syscall inside the user namespace.
+func (p *Process) SetUserns(userns *idmap.IdmapSet, enableSetgroups bool) {
 	return
 }

test/suites/vm.sh

@@ -179,6 +179,40 @@ test_vm_pcie_bus() {
   lxc config device remove v1 v1block
   lxc storage volume delete "${pool}" v1block
 
+  sub_test "Check security.shifted volumes and shift=true directory shares are not remapped by virtiofsd in VMs"
+  # VMs do not use user namespaces, so files on a security.shifted volume or a shift=true
+  # directory share keep their real on-disk ownership inside the VM (matching containers).
+  # virtiofsd must ignore raw.idmap for both, otherwise the files below would appear owned by
+  # nobody instead of 123:456. Both disk types are attached to a single boot to exercise them
+  # together.
+  lxc config set v1 raw.idmap="both 1000000 0"
+
+  lxc storage volume create "${pool}" v1shift --type=filesystem size=1MiB security.shifted=true
+  lxc config device add v1 v1shift disk source=v1shift pool="${pool}" path=/mnt
+
+  mkdir -p "${TEST_DIR}/vm-shift-source"
+  touch "${TEST_DIR}/vm-shift-source/shifted-file"
+  chown 123:456 "${TEST_DIR}/vm-shift-source/shifted-file"
+  lxc config device add v1 v1shiftdir disk source="${TEST_DIR}/vm-shift-source" path=/mnt-dir shift=true
+
+  lxc start v1
+  waitInstanceReady v1
+
+  lxc exec v1 -- findmnt /mnt -t virtiofs
+  lxc exec v1 -- findmnt /mnt-dir -t virtiofs
+  volPath="${LXD_DIR}/storage-pools/${pool}/custom/default_v1shift"
+  touch "${volPath}/shifted-file"
+  chown 123:456 "${volPath}/shifted-file"
+  [ "$(lxc exec v1 -- stat /mnt/shifted-file -c '%u:%g')" = "123:456" ]
+  [ "$(lxc exec v1 -- stat /mnt-dir/shifted-file -c '%u:%g')" = "123:456" ]
+
+  lxc stop -f v1
+  lxc config device remove v1 v1shift
+  lxc config device remove v1 v1shiftdir
+  lxc storage volume delete "${pool}" v1shift
+  lxc config unset v1 raw.idmap
+  rm -rf "${TEST_DIR}/vm-shift-source"
+
   lxc storage volume create "${pool}" v1dir --type=filesystem size=1MiB
   lxc start v1
   lxc config device add v1 mydir disk source=v1dir pool="${pool}" path=/mnt