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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id DF4ACAE56 for ; Wed, 6 Apr 2022 09:37:29 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CDC6A26B2C for ; Wed, 6 Apr 2022 09:36:59 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id C8C2026B20 for ; Wed, 6 Apr 2022 09:36:57 +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 97C0841FDB for ; Wed, 6 Apr 2022 09:36:57 +0200 (CEST) Date: Wed, 06 Apr 2022 09:36:35 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220405124040.2996487-1-a.lauterer@proxmox.com> In-Reply-To: <20220405124040.2996487-1-a.lauterer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1649229686.hkn6936l0d.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.176 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH storage] rbd: alloc image: fix #3970 avoid ambiguous rbd path 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, 06 Apr 2022 07:37:29 -0000 On April 5, 2022 2:40 pm, Aaron Lauterer wrote: > If two RBD storages use the same pool, but connect to different > clusters, we cannot say to which cluster the mapped RBD image belongs to > if krbd is used. To avoid potential data loss, we need to verify that no > other storage is configured that could have a volume mapped under the > same path before we allocate the image. >=20 > The ambiguous mapping is in > /dev/rbd/// where the namespace is optional. >=20 > Once we can tell the clusters apart in the mapping, we can remove these > checks again. >=20 > See bug #3969 for more information on the root cause. >=20 > Signed-off-by: Aaron Lauterer > --- > changes since RFC: >=20 > * moved check to pve-storage since containers and VMs both have issues > not just on a move or clone of the image, but also when creating a new > volume > * reworked the checks, instead of large if conditions, we use > PVE::Tools::safe_compare with comparison functions > * normalize monhost list to match correctly if the list is in different > order > * add storage name to error message that triggered the checks > * ignore disabled storages >=20 > PVE/Storage/RBDPlugin.pm | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) >=20 > diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm > index e287e28..a9dbf5e 100644 > --- a/PVE/Storage/RBDPlugin.pm > +++ b/PVE/Storage/RBDPlugin.pm > @@ -516,6 +516,40 @@ sub alloc_image { > die "illegal name '$name' - should be 'vm-$vmid-*'\n" > if $name && $name !~ m/^vm-$vmid-/; > =20 > + # check if another rbd storage with the same pool name but different > + # cluster exists. If so, allocating a new volume can potentially be > + # dangerous because the RBD mapping, exposes it in an ambiguous way = under > + # /dev/rbd///. Without any information to which clu= ster it > + # belongs, we cannot clearly determine which image we access and > + # potentially use the wrong one. See > + # https://bugzilla.proxmox.com/show_bug.cgi?id=3D3969 and > + # https://bugzilla.proxmox.com/show_bug.cgi?id=3D3970 > + # TODO: remove these checks once #3969 is fixed and we can clearly t= ell to > + # which cluster an image belongs to > + my $storecfg =3D PVE::Storage::config(); > + foreach my $store (keys %{$storecfg->{ids}}) { I think this needs to go somewhere else - probably into a new private=20 helper that gets called in alloc_image, clone_image and rename_image (at=20 least those are the ones that currently call find_free_diskname). basically all existing volids are as they are (they should be fine, else=20 the user would probably already have noticed data loss/corruption), but=20 anything that takes a new slot should be blocked before causing mayhem. > + next if $store eq $storeid; > + > + my $checked_scfg =3D $storecfg->{ids}->{$store}; > + > + next if $checked_scfg->{type} ne 'rbd'; > + next if $checked_scfg->{disable}; > + next if $scfg->{pool} ne $checked_scfg->{pool}; > + > + my $normalize_mons =3D sub { return join('/', sort( PVE::Tools::split_l= ist(' ', shift))) }; this doesn't do what you think it does ;) split_list takes a single=20 argument (the string to be split). I think joining with ';' might be=20 more natural (it's basically a 'split->sort->join-as-string-list' then),=20 and semicolons don't make any sense inside a monhost anyway. > + my $cmp_mons =3D sub { $normalize_mons->($_[0]) cmp $normalize_mons->($= _[1]) }; > + my $cmp =3D sub { $_[0] cmp $_[1] }; that might be a nice addition to safe_compare (no $cmp -> use `cmp`),=20 but alas. > + # internal and internal, or external and external with identical monito= rs > + # =3D> same cluster > + next if PVE::Tools::safe_compare($scfg->{monhost}, $checked_scfg->{monh= ost}, $cmp_mons) =3D=3D 0; > + > + # different namespaces =3D> no clash possible > + next if !PVE::Tools::safe_compare($scfg->{namespace}, $checked_scfg->{n= amespace}, $cmp) =3D=3D 0; !=3D 0 please! > + > + die "Other storage found which would lead to ambiguous mappings: '$stor= e'\n"; it might make sense to include both storages here? e.g.: "Cannot create volume on '$storeid' - RBD blockdev paths shared with=20 storage '$store'\n"; or even a reference to the bug that explains it all? could post a=20 comment with workarounds as well then (although I do hope that not many=20 people will run into this, and most of those are hopefully false=20 positives of the check and not actually problematic setups). > + } > + > $name =3D $class->find_free_diskname($storeid, $scfg, $vmid) if !$na= me; > =20 > my @options =3D ( > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20