[pve-devel] [RFC container] mountpoints: create parent dirs with correct owner

Tom Weber pve at junkyard.4t2.com
Wed Jul 24 14:10:46 CEST 2019


it seems like you're working in an area of code that could also be
relevant for my (small) problem from:
https://pve.proxmox.com/pipermail/pve-devel/2017-September/028814.html
https://pve.proxmox.com/pipermail/pve-devel/2017-October/029004.html

or maybe this could also be a problem if your mps are owned by user not
root in an unprivileged container.

i didn't really dig into your patch, just a hint.

Regards,
  Tom


Am Mittwoch, den 24.07.2019, 13:37 +0200 schrieb Fabian Grünbichler:
> otherwise unprivileged containers might end up with directories that
> they cannot modify since they are owned by the user root in the host
> namespace, instead of root inside the container.
> 
> note: the problematic behaviour is only exhibited when an
> intermediate
> directory needs to be created, e.g. a mountpoint /test/mp gets
> mounted,
> and /test does not yet exist.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> Notes:
>     requires fchownat support in PVE::Tools - see other patch and
> bump
>     build-depends + depends accordingly after applying!
>     
>     I am not sure whether this is 100% correct w.r.t. error edge
> cases, since we
>     potentially die after mkdirat without calling fchownat. it is for
> sure better
>     than the status quo though ;)
>     
>     thank you Dietmar for noticing the buggy behaviour!
> 
>  src/PVE/LXC.pm            | 27 ++++++++++++++++-----------
>  src/PVE/VZDump/LXC.pm     |  7 +++++--
>  src/lxc-pve-prestart-hook |  4 +++-
>  3 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 2252685..0a1d95a 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1177,13 +1177,15 @@ sub mount_all {
>      my $volid_list = PVE::LXC::Config->get_vm_volumes($conf);
>      PVE::Storage::activate_volumes($storage_cfg, $volid_list);
>  
> +    my (undef, $rootuid, $rootgid) = parse_id_maps($conf);
> +
>      eval {
>  	PVE::LXC::Config->foreach_mountpoint($conf, sub {
>  	    my ($ms, $mountpoint) = @_;
>  
>  	    $mountpoint->{ro} = 0 if $ignore_ro;
>  
> -	    mountpoint_mount($mountpoint, $rootdir, $storage_cfg);
> +	    mountpoint_mount($mountpoint, $rootdir, $storage_cfg,
> undef, $rootuid, $rootgid);
>          });
>      };
>      if (my $err = $@) {
> @@ -1258,8 +1260,8 @@ sub run_with_loopdev {
>  #   * the 2nd deepest directory (parent of the above)
>  #   * directory name of the last directory
>  # So that the path $2/$3 should lead to $1 afterwards.
> -sub walk_tree_nofollow($$$) {
> -    my ($start, $subdir, $mkdir) = @_;
> +sub walk_tree_nofollow($$$;$$) {
> +    my ($start, $subdir, $mkdir, $rootuid, $rootgid) = @_;
>  
>      # splitdir() returns '' for empty components including the
> leading /
>      my @comps = grep { length($_)>0 } File::Spec->splitdir($subdir);
> @@ -1288,6 +1290,9 @@ sub walk_tree_nofollow($$$) {
>  
>  	    $next = PVE::Tools::openat(fileno($fd), $component,
> O_NOFOLLOW | O_DIRECTORY);
>  	    die "failed to create path: $dir: $!\n" if !$next;
> +
> +	    PVE::Tools::fchownat(fileno($next), '', $rootuid, $rootgid,
> PVE::Tools::AT_EMPTY_PATH)
> +		if defined($rootuid) && defined($rootgid);
>  	}
>  
>  	close $second if defined($last_component);
> @@ -1381,17 +1386,17 @@ sub bindmount {
>  # from $rootdir and $mount and walk the path from $rootdir to the
> final
>  # directory to check for symlinks.
>  sub __mount_prepare_rootdir {
> -    my ($rootdir, $mount) = @_;
> +    my ($rootdir, $mount, $rootuid, $rootgid) = @_;
>      $rootdir =~ s!/+!/!g;
>      $rootdir =~ s!/+$!!;
>      my $mount_path = "$rootdir/$mount";
> -    my ($mpfd, $parentfd, $last_dir) = walk_tree_nofollow($rootdir,
> $mount, 1);
> +    my ($mpfd, $parentfd, $last_dir) = walk_tree_nofollow($rootdir,
> $mount, 1, $rootuid, $rootgid);
>      return ($rootdir, $mount_path, $mpfd, $parentfd, $last_dir);
>  }
>  
>  # use $rootdir = undef to just return the corresponding mount path
>  sub mountpoint_mount {
> -    my ($mountpoint, $rootdir, $storage_cfg, $snapname) = @_;
> +    my ($mountpoint, $rootdir, $storage_cfg, $snapname, $rootuid,
> $rootgid) = @_;
>  
>      my $volid = $mountpoint->{volume};
>      my $mount = $mountpoint->{mp};
> @@ -1408,7 +1413,7 @@ sub mountpoint_mount {
>      
>      if (defined($rootdir)) {
>  	($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
> -	    __mount_prepare_rootdir($rootdir, $mount);
> +	    __mount_prepare_rootdir($rootdir, $mount, $rootuid,
> $rootgid);
>      }
>      
>      my ($storage, $volname) = PVE::Storage::parse_volume_id($volid,
> 1);
> @@ -2007,7 +2012,7 @@ sub run_unshared {
>  }
>  
>  my $copy_volume = sub {
> -    my ($src_volid, $src, $dst_volid, $dest, $storage_cfg,
> $snapname, $bwlimit) = @_;
> +    my ($src_volid, $src, $dst_volid, $dest, $storage_cfg,
> $snapname, $bwlimit, $rootuid,  $rootgid) = @_;
>  
>      my $src_mp = { volume => $src_volid, mp => '/', ro => 1 };
>      $src_mp->{type} = PVE::LXC::Config-
> >classify_mountpoint($src_volid);
> @@ -2019,10 +2024,10 @@ my $copy_volume = sub {
>      eval {
>  	# mount and copy
>  	mkdir $src;
> -	mountpoint_mount($src_mp, $src, $storage_cfg, $snapname);
> +	mountpoint_mount($src_mp, $src, $storage_cfg, $snapname,
> $rootuid, $rootgid);
>  	push @mounted, $src;
>  	mkdir $dest;
> -	mountpoint_mount($dst_mp, $dest, $storage_cfg);
> +	mountpoint_mount($dst_mp, $dest, $storage_cfg, undef, $rootuid,
> $rootgid);
>  	push @mounted, $dest;
>  
>  	$bwlimit //= 0;
> @@ -2078,7 +2083,7 @@ sub copy_volume {
>  	}
>  
>  	run_unshared(sub {
> -	    $copy_volume->($mp->{volume}, $src, $new_volid, $dest,
> $storage_cfg, $snapname, $bwlimit);
> +	    $copy_volume->($mp->{volume}, $src, $new_volid, $dest,
> $storage_cfg, $snapname, $bwlimit, $rootuid, $rootgid);
>  	});
>      };
>      if (my $err = $@) {
> diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
> index ae793dc..befb370 100644
> --- a/src/PVE/VZDump/LXC.pm
> +++ b/src/PVE/VZDump/LXC.pm
> @@ -114,6 +114,9 @@ sub prepare {
>  
>      my ($id_map, $rootuid, $rootgid) =
> PVE::LXC::parse_id_maps($conf);
>      $task->{userns_cmd} = PVE::LXC::userns_command($id_map);
> +    $task->{rootuid} = $rootuid;
> +    $task->{rootgid} = $rootgid;
> +
>  
>      my $volids = $task->{volids} = [];
>      PVE::LXC::Config->foreach_mountpoint($conf, sub {
> @@ -219,7 +222,7 @@ sub snapshot {
>      PVE::Storage::activate_volumes($storage_cfg, $volids, 'vzdump');
>      foreach my $disk (@$disks) {
>  	$disk->{dir} = "${rootdir}$disk->{mp}";
> -	PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg,
> 'vzdump');
> +	PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg,
> 'vzdump', $task->{rootuid}, $task->{rootgid});
>      }
>  
>      $task->{snapdir} = $rootdir;
> @@ -306,7 +309,7 @@ sub archive {
>  	my $storage_cfg = $self->{storecfg};
>  	foreach my $disk (@$disks) {
>  	    $disk->{dir} = "${rootdir}$disk->{mp}";
> -	    PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg);
> +	    PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg,
> undef, $task->{rootuid}, $task->{rootgid});
>  	    # add every enabled mountpoint (since we use --one-file-
> system)
>  	    # mp already starts with a / so we only need to add the dot
>  	    push @sources, ".$disk->{mp}";
> diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
> index 3ff8d79..18b60cf 100755
> --- a/src/lxc-pve-prestart-hook
> +++ b/src/lxc-pve-prestart-hook
> @@ -85,11 +85,13 @@ __PACKAGE__->register_method ({
>  	unlink $devlist_file;
>  	my $devices = [];
>  
> +	my (undef, $rootuid, $rootgid) =
> PVE::LXC::parse_id_maps($conf);
> +
>  	my $setup_mountpoint = sub {
>  	    my ($ms, $mountpoint) = @_;
>  
>  	    #return if $ms eq 'rootfs';
> -	    my (undef, undef, $dev) =
> PVE::LXC::mountpoint_mount($mountpoint, $rootdir, $storage_cfg);
> +	    my (undef, undef, $dev) =
> PVE::LXC::mountpoint_mount($mountpoint, $rootdir, $storage_cfg,
> undef, $rootuid, $rootgid);
>  	    push @$devices, $dev if $dev && $mountpoint->{quota};
>  	};
>  




More information about the pve-devel mailing list