[PATCH v1 proxmox-backup 0/2] minor chunk insert improvements

[PATCH v1 proxmox-backup 0/2] minor chunk insert improvements

[Robert Obkircher]

Improve error messages and avoid custom unsafe code.

I forgot to send these patches from about 2 months ago. At the time
Fabian suggested that we could potentially remove the read entirely and
simply ignore these collisions.

Robert Obkircher (2):
  datastore: prefer standard library over custom unsafe code
  datastore: rephrase error messages for failed chunk insert

 pbs-datastore/src/chunk_store.rs | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.47.3

[PATCH v1 proxmox-backup 1/2] datastore: prefer standard library over custom unsafe code

[Robert Obkircher]

The allocation is unnecessary overhead, and the unsafe code in
read_exact_allocated looks a bit dangerous.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 6d2ffdbde..3f637730c 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -1,3 +1,4 @@
+use std::io::Read;
 use std::os::unix::fs::MetadataExt;
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
@@ -10,7 +11,6 @@ use tracing::{info, warn};
 
 use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
 use pbs_config::BackupLockGuard;
-use proxmox_io::ReadExt;
 use proxmox_s3_client::S3Client;
 use proxmox_sys::fs::{CreateOptions, create_dir, create_path, file_type_from_file_stat};
 use proxmox_sys::process_locker::{
@@ -708,8 +708,11 @@ impl ChunkStore {
                 }
             } else if chunk.is_encrypted() {
                 // incoming chunk is encrypted, possible attack or hash collision!
-                let mut existing_file = std::fs::File::open(&chunk_path)?;
-                let magic = existing_file.read_exact_allocated(8)?;
+
+                let mut magic = [0u8; 8];
+                std::fs::File::open(&chunk_path)
+                    .and_then(|mut f| f.read_exact(&mut magic))
+                    .map_err(|e| format_err!("Failed to read header of existing chunk '{digest_str}' on store '{name}: {e}"))?;
 
                 // going from unencrypted to encrypted can never be right, since the digest
                 // includes data derived from the encryption key
-- 
2.47.3

Re: [PATCH v1 proxmox-backup 1/2] datastore: prefer standard library over custom unsafe code

[Christian Ebner]

On 8/7/26 11:58 AM, Robert Obkircher wrote:
> The allocation is unnecessary overhead, and the unsafe code in
> read_exact_allocated looks a bit dangerous.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-datastore/src/chunk_store.rs | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index 6d2ffdbde..3f637730c 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -1,3 +1,4 @@
> +use std::io::Read;
>   use std::os::unix::fs::MetadataExt;
>   use std::os::unix::io::AsRawFd;
>   use std::path::{Path, PathBuf};
> @@ -10,7 +11,6 @@ use tracing::{info, warn};
>   
>   use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
>   use pbs_config::BackupLockGuard;
> -use proxmox_io::ReadExt;
>   use proxmox_s3_client::S3Client;
>   use proxmox_sys::fs::{CreateOptions, create_dir, create_path, file_type_from_file_stat};
>   use proxmox_sys::process_locker::{
> @@ -708,8 +708,11 @@ impl ChunkStore {
>                   }
>               } else if chunk.is_encrypted() {
>                   // incoming chunk is encrypted, possible attack or hash collision!
> -                let mut existing_file = std::fs::File::open(&chunk_path)?;
> -                let magic = existing_file.read_exact_allocated(8)?;
> +
> +                let mut magic = [0u8; 8];
> +                std::fs::File::open(&chunk_path)
> +                    .and_then(|mut f| f.read_exact(&mut magic))
> +                    .map_err(|e| format_err!("Failed to read header of existing chunk '{digest_str}' on store '{name}: {e}"))?;

nit: this does not read the chunk header 
(DataBlobHeader/EncryptedDataBlobHeader), just the magic number. So 
maybe better to also refer to it in the error message as such.

>   
>                   // going from unencrypted to encrypted can never be right, since the digest
>                   // includes data derived from the encryption key

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

[Robert Obkircher]

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.",
                     )
                 }
 
-- 
2.47.3

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

[Christian Ebner]

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?

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."

>                       )
>                   }
>   

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

[Robert Obkircher]

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.

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.


>
>>                       )
>>                   }
>>   
>

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

[Christian Ebner]

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.
> 
> 
>>
>>>                        )
>>>                    }
>>>    
>>