[PATCH] common: Do not chown ro mountpoint when creating idmapped mount

[PATCH] common: Do not chown ro mountpoint when creating idmapped mount

[Gabriel Niebler]

The function _idmapped_mount tries to change the ownership of the mountpoint
for which it aims to create an idmapped mount, to ensure that the mapped UID
and GID can actually create objects within it. Some tests set up a read-only
mount, however, which lets the chown call fail. This patch fixes the
function to check whether the mount is read-only and skip the chown, if so.

Signed-off-by: Gabriel Niebler <gniebler@suse.com>
---
 common/rc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 81ce1026..19ab8062 100644
--- a/common/rc
+++ b/common/rc
@@ -414,8 +414,11 @@ _idmapped_mount()
 
 	# We create an idmapped mount where {g,u}id 0 writes to disk as
 	# {g,u}id 10000000 and $(id -u fsgqa) + 10000000. We change ownership
-        # of $mnt so {g,u} id 0 can actually create objects in there.
-	chown 10000000:10000000 $mnt || return 1
+	# of $mnt, provided it's not read-only, so {g,u} id 0 can actually
+	# create objects in there.
+	if [[ "$mount_rec" != *"ro,"* && "$mount_rec" != *",ro"* ]]; then
+		chown 10000000:10000000 $mnt || return 1
+	fi
 	$here/src/vfs/mount-idmapped \
 		--map-mount b:10000000:0:100000000000 \
 		$mnt $tmp
-- 
2.39.1

[PATCH] common: Chown mount even if already idmapped to account for remounts

[Gabriel Niebler]

This is a logical consequence of introducing the chown check in _idmapped_mount,
since now a read-only mount can be made idmapped successfully. But if the mount
is then remounted rw the chown never happens, as _idmapped_mount sees that it's
already idmapped and bows out early.

This patch fixes that by simply moving the chown ahead of the idmapped check,
so it will be performed in any case, even on already idmapped mounts.

Signed-off-by: Gabriel Niebler <gniebler@suse.com>
---
 common/rc | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index 19ab8062..88da356e 100644
--- a/common/rc
+++ b/common/rc
@@ -408,10 +408,6 @@ _idmapped_mount()
 	local tmp=`mktemp -d`
 
 	local mount_rec=`findmnt -rncv -S $dev -o OPTIONS`
-	if [[ "$mount_rec" == *"idmapped"* ]]; then
-		return 0
-	fi
-
 	# We create an idmapped mount where {g,u}id 0 writes to disk as
 	# {g,u}id 10000000 and $(id -u fsgqa) + 10000000. We change ownership
 	# of $mnt, provided it's not read-only, so {g,u} id 0 can actually
@@ -419,6 +415,11 @@ _idmapped_mount()
 	if [[ "$mount_rec" != *"ro,"* && "$mount_rec" != *",ro"* ]]; then
 		chown 10000000:10000000 $mnt || return 1
 	fi
+	# But if the mount is already idmapped, then there's nothing more to do.
+	if [[ "$mount_rec" == *"idmapped"* ]]; then
+		return 0
+	fi
+
 	$here/src/vfs/mount-idmapped \
 		--map-mount b:10000000:0:100000000000 \
 		$mnt $tmp
-- 
2.39.1

Re: [PATCH] common: Do not chown ro mountpoint when creating idmapped mount

[Christian Brauner]

On Fri, Feb 03, 2023 at 03:35:45PM +0100, Gabriel Niebler wrote:
> The function _idmapped_mount tries to change the ownership of the mountpoint
> for which it aims to create an idmapped mount, to ensure that the mapped UID
> and GID can actually create objects within it. Some tests set up a read-only
> mount, however, which lets the chown call fail. This patch fixes the
> function to check whether the mount is read-only and skip the chown, if so.
> 
> Signed-off-by: Gabriel Niebler <gniebler@suse.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Thanks for fixing this!

Re: [PATCH] common: Chown mount even if already idmapped to account for remounts

[Christian Brauner]

On Fri, Feb 03, 2023 at 06:54:37PM +0100, Gabriel Niebler wrote:
> This is a logical consequence of introducing the chown check in _idmapped_mount,
> since now a read-only mount can be made idmapped successfully. But if the mount
> is then remounted rw the chown never happens, as _idmapped_mount sees that it's
> already idmapped and bows out early.
> 
> This patch fixes that by simply moving the chown ahead of the idmapped check,
> so it will be performed in any case, even on already idmapped mounts.
> 
> Signed-off-by: Gabriel Niebler <gniebler@suse.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Thanks!