From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id E1C8E1FF0E7 for ; Wed, 12 Aug 2026 11:52:19 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 767CB21567; Wed, 12 Aug 2026 11:52:19 +0200 (CEST) Message-ID: <90d3d067-1db9-4b7d-b8d3-0b42da6686df@proxmox.com> Date: Wed, 12 Aug 2026 11:52:14 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 proxmox-backup 2/2] datastore: rephrase error messages for failed chunk insert To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260807095803.3051-1-r.obkircher@proxmox.com> <20260807095803.3051-3-r.obkircher@proxmox.com> Content-Language: en-US, de-AT From: Robert Obkircher In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786528320186 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.716 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: Q3DI2VTE6KPYJBLMQPFCR6NACFQFWA3O X-Message-ID-Hash: Q3DI2VTE6KPYJBLMQPFCR6NACFQFWA3O X-MailFrom: r.obkircher@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 07.08.26 14:43, Christian Ebner wrote: > On 8/7/26 11:58 AM, Robert Obkircher wrote: >> The "not allowed" phrasing sounds like an access-right problem, when >> the actual problem is likely file corruption. >> >> Link: https://forum.proxmox.com/threads/184140 >> Signed-off-by: Robert Obkircher >> --- >>   pbs-datastore/src/chunk_store.rs | 4 ++-- >>   1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/pbs-datastore/src/chunk_store.rs >> b/pbs-datastore/src/chunk_store.rs >> index 3f637730c..d9d0c9a26 100644 >> --- a/pbs-datastore/src/chunk_store.rs >> +++ b/pbs-datastore/src/chunk_store.rs >> @@ -718,7 +718,7 @@ impl ChunkStore { >>                   // includes data derived from the encryption key >>                   if magic == UNCOMPRESSED_BLOB_MAGIC_1_0 || magic >> == COMPRESSED_BLOB_MAGIC_1_0 { >>                       bail!( >> -                        "Overwriting unencrypted chunk >> '{digest_str}' on store '{name}' with encrypted chunk with same >> digest not allowed!" >> +                        "Cannot overwrite existing unencrypted >> chunk '{digest_str}' on store '{name}' with encrypted chunk." >>                       ); >>                   } >>   @@ -726,7 +726,7 @@ impl ChunkStore { >>                   // their sizes are different, one of them *must* >> be invalid >>                   if magic == ENCRYPTED_BLOB_MAGIC_1_0 && >> !chunk.is_compressed() { >>                       bail!( >> -                        "Overwriting existing (encrypted) chunk >> '{digest_str}' on store '{name}' is not allowed!" >> +                        "Found existing encrypted chunk >> '{digest_str}' of different size on store '{name}'. Consider >> running verification and garbage-collection because it may be >> corrupted.", > > question: pre-existing but since one of the chunks must be the > corrupt one here, and the server checks the CRC sum on upload, the > new chunk is most likely to be the fine one. Might be worth to > decode the full on-disk chunk then and double check its CRC? An only > bail if both are fine, which is very unlikely?  It is not necessarily the case that one chunk must be corrupt here. It could also be an attack from a malicious client that learned about an existing digest and is trying to override it. So, especially if both CRCs were valid, we must keep the older one. I do agree that decoding to find out more details makes sense, because this should be extremely rare in practice. But this is also the critical section of the most heavily contended lock, so removing the magic read entirely might be worth it. Fabian seemed convinced that this would be safe. > > anyways, I suggest to modify the error message to be a bit more > concise: > > "Unexpected encrypted chunk {} with different size already present > on store {}. Run verification to detect possibly corrupt chunks."  I think verification alone wouldn't be sufficient for unreferenced chunks. The user on the forum post mentioned being unable to verify the backup as it has been deleted via the gui. > >>                       ) >>                   } >>   >