Improve uid shifting via chown.

Sysbox performs uid-shifting using chown for host dirs that are mounted into
certain special dirs in the container (e.g., /var/lib/docker, /var/lib/kubelet,
etc.)

Prior to this change, the uid shifting via chown worked well when the host dir
was initially owned by root:root.  In this case, when the container started,
Sysbox would shift uids to match the container's root user (e.g.,
165536:165536); later when the container stopped, Sysbox would revert the uids
back to the original one (i.e, root:root).

However, we've seen situations where the uid revert may not occur because the
host dir gets detached from the host. In this case, a subsequent creation of a
container where said host dir is again mounted into a special dir in the
container, would result in Sysbox not performing or mishandling the uid
shifting.

This commit fixes this by ensuring that Sysbox always performs the uid
shifting regardless of the original uid associated with the host dir
being mounted into the container.

Signed-off-by: Cesar Talledo <ctalledo@nestybox.com>

idShiftUtils/idShiftUtils.go

@@ -33,13 +33,6 @@ import (
 	mapset "github.com/deckarep/golang-set"
 )
 
-type OffsetType int
-
-const (
-	OffsetAdd OffsetType = iota
-	OffsetSub
-)
-
 type aclType int
 
 const (
@@ -48,7 +41,7 @@ const (
 )
 
 // Shifts the ACL type user and group IDs by the given offset
-func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offsetDir OffsetType) error {
+func shiftAclType(aclT aclType, path string, uidOffset, gidOffset int32) error {
 	var facl aclLib.ACL
 	var err error
 
@@ -77,13 +70,8 @@ func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offset
 				continue
 			}
 
-			if offsetDir == OffsetAdd {
-				uid = uid + uint64(uidOffset)
-			} else {
-				uid = uid - uint64(uidOffset)
-			}
-
-			e.Qualifier = strconv.FormatUint(uid, 10)
+			targetUid := uint64(int32(uid) + uidOffset)
+			e.Qualifier = strconv.FormatUint(targetUid, 10)
 			aclShifted = true
 		}
 
@@ -95,13 +83,8 @@ func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offset
 				continue
 			}
 
-			if offsetDir == OffsetAdd {
-				gid = gid + uint64(gidOffset)
-			} else {
-				gid = gid - uint64(gidOffset)
-			}
-
-			e.Qualifier = strconv.FormatUint(gid, 10)
+			targetGid := uint64(int32(gid) + gidOffset)
+			e.Qualifier = strconv.FormatUint(targetGid, 10)
 			aclShifted = true
 		}
 
@@ -129,17 +112,17 @@ func shiftAclType(aclT aclType, path string, uidOffset, gidOffset uint32, offset
 }
 
 // Shifts the ACL user and group IDs by the given offset, both for access and default ACLs
-func shiftAclIds(path string, isDir bool, uidOffset, gidOffset uint32, offsetDir OffsetType) error {
+func shiftAclIds(path string, isDir bool, uidOffset, gidOffset int32) error {
 
 	// Access list
-	err := shiftAclType(aclTypeAccess, path, uidOffset, gidOffset, offsetDir)
+	err := shiftAclType(aclTypeAccess, path, uidOffset, gidOffset)
 	if err != nil {
 		return err
 	}
 
 	// Default list (for directories only)
 	if isDir {
-		err = shiftAclType(aclTypeDefault, path, uidOffset, gidOffset, offsetDir)
+		err = shiftAclType(aclTypeDefault, path, uidOffset, gidOffset)
 		if err != nil {
 			return err
 		}
@@ -150,13 +133,12 @@ func shiftAclIds(path string, isDir bool, uidOffset, gidOffset uint32, offsetDir
 
 // "Shifts" ownership of user and group IDs on the given directory and files and directories
 // below it by the given offset, using chown.
-func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset uint32, offsetDir OffsetType) error {
+func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset int32) error {
 
 	hardLinks := []uint64{}
 
 	err := godirwalk.Walk(baseDir, &godirwalk.Options{
 		Callback: func(path string, de *godirwalk.Dirent) error {
-			var targetUid, targetGid uint32
 
 			// When doing the chown, we don't follow symlinks as we want to change
 			// the ownership of the symlinks themselves. We will chown the
@@ -184,13 +166,8 @@ func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset uint32, offsetDir Of
 				hardLinks = append(hardLinks, st.Ino)
 			}
 
-			if offsetDir == OffsetAdd {
-				targetUid = st.Uid + uidOffset
-				targetGid = st.Gid + gidOffset
-			} else {
-				targetUid = st.Uid - uidOffset
-				targetGid = st.Gid - gidOffset
-			}
+			targetUid := int32(st.Uid) + uidOffset
+			targetGid := int32(st.Gid) + gidOffset
 
 			logrus.Debugf("chown %s from %d:%d to %d:%d", path, st.Uid, st.Gid, targetUid, targetGid)
 
@@ -203,7 +180,7 @@ func ShiftIdsWithChown(baseDir string, uidOffset, gidOffset uint32, offsetDir Of
 			// the Linux access control list (ACL) for the file
 
 			if fi.Mode()&os.ModeSymlink == 0 {
-				if err := shiftAclIds(path, fi.IsDir(), uidOffset, gidOffset, offsetDir); err != nil {
+				if err := shiftAclIds(path, fi.IsDir(), uidOffset, gidOffset); err != nil {
 					return fmt.Errorf("failed to shift ACL for %s: %s", path, err)
 				}
 			}

mgr.go

@@ -476,15 +476,13 @@ func (mgr *SysboxMgr) unregister(id string) error {
 	// revert mount prep actions
 	for _, revInfo := range info.mntPrepRev {
 		if revInfo.uidShifted {
-			logrus.Infof("reverting uid-shift on %s for %s", revInfo.path, formatter.ContainerID{id})
 
-			// revInfo.targetUid is guaranteed to be higher than revInfo.origUid
-			// (we checked in prepMounts())
+			uidOffset := int32(revInfo.origUid) - int32(revInfo.targetUid)
+			gidOffset := int32(revInfo.origGid) - int32(revInfo.targetGid)
 
-			uidOffset := revInfo.targetUid - revInfo.origUid
-			gidOffset := revInfo.targetGid - revInfo.origGid
+			logrus.Infof("reverting uid-shift on %s for %s (%d -> %d)", revInfo.path, formatter.ContainerID{id}, revInfo.targetUid, revInfo.origUid)
 
-			if err = idShiftUtils.ShiftIdsWithChown(revInfo.path, uidOffset, gidOffset, idShiftUtils.OffsetSub); err != nil {
+			if err = idShiftUtils.ShiftIdsWithChown(revInfo.path, uidOffset, gidOffset); err != nil {
 				logrus.Warnf("failed to revert uid-shift of mount source at %s: %s", revInfo.path, err)
 			}
 
@@ -837,15 +835,15 @@ func (mgr *SysboxMgr) prepMounts(id string, uid, gid uint32, prepList []ipcLib.M
 		}
 
 		if needUidShift {
-			logrus.Infof("shifting uids at %s for %s", src, formatter.ContainerID{id})
 
-			// uid is guaranteed to be higher than origUid (we checked in
-			// mntSrcUidShiftNeeded())
+			// Offset may be positive or negative
+			uidOffset := int32(uid) - int32(origUid)
+			gidOffset := int32(gid) - int32(origGid)
 
-			uidOffset := uid - origUid
-			gidOffset := gid - origGid
+			logrus.Infof("shifting uids at %s for %s (%d -> %d)", src, formatter.ContainerID{id}, origUid, uid)
 
-			if err = idShiftUtils.ShiftIdsWithChown(src, uidOffset, gidOffset, idShiftUtils.OffsetAdd); err != nil {
+			// TODO: for kernel >= 5.12, we need to do this using the new ID-mapped mount feature (faster, more reliable).
+			if err = idShiftUtils.ShiftIdsWithChown(src, uidOffset, gidOffset); err != nil {
 				return fmt.Errorf("failed to shift uids via chown for mount source at %s: %s", src, err)
 			}
 

utils.go

@@ -723,28 +723,13 @@ func mntSrcUidShiftNeeded(mntSrc string, uid, gid uint32) (bool, uint32, uint32,
 	mntSrcUid = st.Uid
 	mntSrcGid = st.Gid
 
-	// If the host uid assigned to the container is higher than the uid
-	// of the dir being mounted into the container, then we perform uid
+	// If the host uid assigned to the container's root user differs from the
+	// uid of the dir being mounted into the container, then we perform uid
 	// shifting. Same for gid.
-	if uid > mntSrcUid && gid > mntSrcGid {
+	if uid != mntSrcUid && gid != mntSrcGid {
 		return true, mntSrcUid, mntSrcGid, nil
 	}
 
-	// If the host uid assigned to the container is lower than the uid of the dir
-	// being mounted, we skip the uid shift. This is in fact an anomalous case
-	// because in general the host dir being mounted into the container should
-	// have host range uids (e.g., 0->65535) and these are usually lower than the
-	// uids assigned to the container (which come from the subuid ranges in
-	// /etc/subuid).
-	if uid < mntSrcUid || gid < mntSrcGid {
-
-		logrus.Infof("skipping uid shift on %s because its uid:gid (%d:%d) is "+
-			"> the container's assigned uid:gid (%d:%d)",
-			mntSrc, mntSrcUid, mntSrcGid, uid, gid)
-
-		return false, mntSrcUid, mntSrcGid, nil
-	}
-
 	// If the mount dir has same ownership as the container, check the subdirs
 	// before we make a determination on whether ownership shifting will be
 	// required.
@@ -755,21 +740,16 @@ func mntSrcUidShiftNeeded(mntSrc string, uid, gid uint32) (bool, uint32, uint32,
 		return false, 0, 0, err
 	}
 
-	needChown := true
+	numNeedChown := 0
 	for _, fi := range dirFis {
 		st, _ := fi.Sys().(*syscall.Stat_t)
-
-		if uid <= st.Uid && gid <= st.Gid {
-
-			logrus.Infof("skipping uid shift on %s because subdir %s has uid:gid (%d:%d) which is "+
-				">= the container's assigned uid:gid (%d:%d)",
-				mntSrc, fi.Name(), st.Uid, st.Gid, uid, gid)
-
-			needChown = false
-			break
+		if uid != st.Uid || gid != st.Gid {
+			numNeedChown += 1
 		}
 	}
 
+	needChown := (numNeedChown == len(dirFis))
+
 	return needChown, mntSrcUid, mntSrcGid, nil
 }