Re: [PATCH v1 proxmox-backup 2/2] datastore: rephrase error messages for failed chunk insert

[Christian Ebner <c.ebner@proxmox.com>]

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 <r.obkircher@proxmox.com>
>>> ---
>>>    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.
> 
> 
>>
>>>                        )
>>>                    }
>>>    
>>