[pve-devel] [PATCH v2 storage 3/5] btrfs: add 'btrfs' import/export format

Wolfgang Bumiller w.bumiller at proxmox.com
Wed Jun 23 14:30:04 CEST 2021


On Wed, Jun 23, 2021 at 02:14:48PM +0200, Fabian Grünbichler wrote:
> On June 22, 2021 2:18 pm, Wolfgang Bumiller wrote:
> > Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>

(...)

> > +sub volume_export {
> > +    my (
> > +	$class,
> > +	$scfg,
> > +	$storeid,
> > +	$fh,
> > +	$volname,
> > +	$format,
> > +	$snapshot,
> > +	$base_snapshot,
> > +	$with_snapshots,
> > +    ) = @_;
> > +
> > +    if ($format ne 'btrfs') {
> > +	return PVE::Storage::Plugin::volume_export(@_);
> > +    }
> > +
> > +    die "format 'btrfs' only works on snapshots\n"
> > +	if !defined $snapshot;
> > +
> > +    die "'btrfs' format in incremental mode requires snapshots to be listed explicitly\n"
> > +	if defined($base_snapshot) && $with_snapshots && ref($with_snapshots) ne 'ARRAY';
> > +
> > +    my $volume_format = ($class->parse_volname($volname))[6];
> > +
> > +    die "btrfs-sending volumes of type $volume_format ('$volname') is not supported\n"
> > +	if $volume_format ne 'raw' && $volume_format ne 'subvol';
> 
> the three checks here are the same as in volume_export, maybe they could 
> go into a single export-helper?

we get nicer errors this way, though (and more easily placed in the code
when a user runs into them, since we don't use Carp ;-) )

(...)

> > +
> > +	# Now go through the remaining snapshots (if any)
> > +	foreach my $snap (@snapshots) {
> > +	    $class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snap", 'ro', 'false']);
> > +	    PVE::Tools::renameat2(
> > +		-1,
> > +		"$tmppath/$diskname\@$snap",
> > +		-1,
> > +		"$destination\@$snap",
> > +		&PVE::Tools::RENAME_NOREPLACE,
> > +	    ) or die "failed to move received snapshot '$tmppath/$diskname\@$snap'"
> > +		. " into place at '$destination\@$snap' - $!\n";
> > +	    eval { $class->btrfs_cmd(['property', 'set', "$destination\@$snap", 'ro', 'true']) };
> > +	    warn "failed to make $destination\@$snap read-only - $!\n" if $@;
> > +	}
> > +    };
> > +    my $err = $@;
> > +
> > +    eval {
> > +	# Cleanup all the received snapshots we did not move into place, so we can remove the temp
> > +	# directory.
> > +	if ($dh) {
> > +	    $dh->rewind;
> > +	    while (defined(my $entry = $dh->read)) {
> > +		next if $entry eq '.' || $entry eq '..';
> > +		eval { $class->btrfs_cmd(['subvolume', 'delete', '--', "$tmppath/$entry"]) };
> > +		warn $@ if $@;
> > +	    }
> > +	    $dh->close; undef $dh;
> > +	}
> > +	if (!rmdir($tmppath)) {
> > +	    warn "failed to remove temporary directory '$tmppath' - $!\n"
> > +	}
> > +    };
> > +    warn $@ if $@;
> > +    if ($err) {
> > +	# clean up if the directory ended up being empty after an error
> > +	rmdir($tmppath);
> > +	die $err;
> > +    }
> 
> I am a bit wary about this (also w.r.t. future btrfs features) - a 
> privileged container can do quite a lot of stuff with btrfs
> 
> - create snapshots from within container
> - set default subvolume
> - set properties
> - ... (the above is what I found in a few minutes without much BTRFS 
>   experience)
> 
> unprivileged containers can do the following
> - create new subvols
> - create new snapshots
> - set ro property
> 
> (same caveat of not looking too much at what else is possible, lots of 
> stuff does not work)
> 
> how do we guarantee that the send stream is not affected in a dangerous 
> fashion by an attacker inside the container?

Shouldn't be dangerous sine it's not recursive anyway.

However, we could just by default disable the corresponding ioctls via
seccomp for unprivileged containers...





More information about the pve-devel mailing list