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 743761FF0E5 for ; Wed, 12 Aug 2026 12:03:39 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1117A21573; Wed, 12 Aug 2026 12:03:39 +0200 (CEST) Message-ID: Date: Wed, 12 Aug 2026 12:03:33 +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: Robert Obkircher , pbs-devel@lists.proxmox.com References: <20260807095803.3051-1-r.obkircher@proxmox.com> <20260807095803.3051-3-r.obkircher@proxmox.com> <90d3d067-1db9-4b7d-b8d3-0b42da6686df@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <90d3d067-1db9-4b7d-b8d3-0b42da6686df@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786528999166 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.867 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: ZJKY7NJ7XK5S3POZG7BQPDRW5DW6LSWM X-Message-ID-Hash: ZJKY7NJ7XK5S3POZG7BQPDRW5DW6LSWM X-MailFrom: c.ebner@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 8/12/26 11:52 AM, Robert Obkircher wrote: > > 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. Yes, that is why I suggested to bail if both CRC sums are fine! But I see the point in not holding the lock here for longer than required. > 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. Maybe rephrase to `Verify and garbage-collect the full datastore to cleanup possibly corrupt chunks.` then? > The user on the forum post mentioned being unable to verify the backup > as it has been deleted via the gui. > > >> >>>                       ) >>>                   } >>> >>