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.gruenbich...@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};
> };
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
