From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 8933C75123 for ; Wed, 23 Jun 2021 14:30:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7EE84AD5D for ; Wed, 23 Jun 2021 14:30:07 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 39541AD50 for ; Wed, 23 Jun 2021 14:30:06 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 06F0146771 for ; Wed, 23 Jun 2021 14:30:06 +0200 (CEST) Date: Wed, 23 Jun 2021 14:30:04 +0200 From: Wolfgang Bumiller To: Fabian =?utf-8?Q?Gr=C3=BCnbichler?= Cc: Proxmox VE development discussion Message-ID: <20210623123004.oje4prbpx3spph25@olga.proxmox.com> References: <20210622121828.84178-1-w.bumiller@proxmox.com> <20210622121828.84178-4-w.bumiller@proxmox.com> <1624448167.ijsofk7p3p.astroid@nora.none> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1624448167.ijsofk7p3p.astroid@nora.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.782 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v2 storage 3/5] btrfs: add 'btrfs' import/export format X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Jun 2021 12:30:07 -0000 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 (...) > > +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...