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 DAEB9AC75 for ; Wed, 6 Apr 2022 09:53:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C84DA26F38 for ; Wed, 6 Apr 2022 09:53: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 EB79026F2D for ; Wed, 6 Apr 2022 09:53:05 +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 BB18942374 for ; Wed, 6 Apr 2022 09:53:05 +0200 (CEST) Message-ID: Date: Wed, 6 Apr 2022 09:52:53 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20220405124040.2996487-1-a.lauterer@proxmox.com> <1649229686.hkn6936l0d.astroid@nora.none> From: Aaron Lauterer In-Reply-To: <1649229686.hkn6936l0d.astroid@nora.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.346 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 NICE_REPLY_A -0.631 Looks like a legit reply (A) 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:53:37 -0000 On 4/6/22 09:36, Fabian Grünbichler wrote: > On April 5, 2022 2:40 pm, Aaron Lauterer wrote: [...] >> >> + # 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 cluster 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=3969 and >> + # https://bugzilla.proxmox.com/show_bug.cgi?id=3970 >> + # TODO: remove these checks once #3969 is fixed and we can clearly tell to >> + # which cluster an image belongs to >> + my $storecfg = PVE::Storage::config(); >> + foreach my $store (keys %{$storecfg->{ids}}) { > > I think this needs to go somewhere else - probably into a new private > helper that gets called in alloc_image, clone_image and rename_image (at > least those are the ones that currently call find_free_diskname). > > basically all existing volids are as they are (they should be fine, else > the user would probably already have noticed data loss/corruption), but > anything that takes a new slot should be blocked before causing mayhem. good point > >> + next if $store eq $storeid; >> + >> + my $checked_scfg = $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 = sub { return join('/', sort( PVE::Tools::split_list(' ', shift))) }; > > this doesn't do what you think it does ;) split_list takes a single > argument (the string to be split). I think joining with ';' might be > more natural (it's basically a 'split->sort->join-as-string-list' then), > and semicolons don't make any sense inside a monhost anyway. thanks for catching it :) > >> + my $cmp_mons = sub { $normalize_mons->($_[0]) cmp $normalize_mons->($_[1]) }; >> + my $cmp = sub { $_[0] cmp $_[1] }; > > that might be a nice addition to safe_compare (no $cmp -> use `cmp`), > but alas. > >> + # internal and internal, or external and external with identical monitors >> + # => same cluster >> + next if PVE::Tools::safe_compare($scfg->{monhost}, $checked_scfg->{monhost}, $cmp_mons) == 0; >> + >> + # different namespaces => no clash possible >> + next if !PVE::Tools::safe_compare($scfg->{namespace}, $checked_scfg->{namespace}, $cmp) == 0; > > != 0 please! yep :-/ > >> + >> + die "Other storage found which would lead to ambiguous mappings: '$store'\n"; > > it might make sense to include both storages here? e.g.: > "Cannot create volume on '$storeid' - RBD blockdev paths shared with > storage '$store'\n"; > > or even a reference to the bug that explains it all? could post a > comment with workarounds as well then (although I do hope that not many > people will run into this, and most of those are hopefully false > positives of the check and not actually problematic setups). hmm, a full on link to the bug in the error message? I tried to search via a few search engines for something like "proxmox bug #3969" and the results were not leading to bugzilla.proxmox.com. I don't think just adding the bug number will be that useful for most people.