[PATCH proxmox{,-backup} v4 0/5] fix #6990: server: drop verify state on push & pull job

[PATCH proxmox{,-backup} v4 0/5] fix #6990: server: drop verify state on push & pull job

[Jakob Klocker]

When syncing a snapshot to another datastore, the source's verify_state
is currently carried over to the target. This reports the target
snapshot as verified even though its stored copy was never checked
there. Since verify jobs skip snapshots that already carry a
verify_state, the target's copy can't be verified again.

Chunks are checksummed in memory while being transferred, but a verify
serves a different purpose: confirming the write to the (external)
target actually succeeded.

This series stops the target from inheriting the source's
verify_state, so the target is verified independently. On pull, a 
verify_state the target obtained on its own is preserved; on push the
state is dropped unconditionally. It also moves the blocking calls in
the touched code paths off the runtime's worker threads onto the
blocking thread pool.

Tested (verify_state correctly stripped on target):
 * Pull, verified source, new content, clean sync
 * Pull, verified source, existing content, clean sync
 * Pull, corrupted target, resync-corrupt
 * Push, verified source (has no corrupt path)
 * Pull, independently-verified target, clean re-sync - state preserved

changes from v3 to v4 (thanks @Christian):
 * new patch for pbs-api-types: implement `Hash` and
   `Borrow<str>` for `BackupArchiveName`
 * use `HashSet<BackupArchiveName>` instead of `HashSet<String>` for 
   the set of files to keep
 * split the `cleanup_unreferenced_files` / `expected_files` refactor
   into its own preparatory patch
 * reword the `expected_files` doc comment, as the client log is
   optional and not necessarily present

changes from v2 to v3 (thanks @Christian):
 * offload remaining removes to `spawn_blocking()`
 * add `target_path` parameter to `write_to_path` and perform the
   rename inside the helper
 * add a helper to return expected files as hashset
 * refactor `cleanup_unreferenced_files` to take the hashset instead
   of the manifest

changes from v1 to v2 (thanks @Christian):
 * move the manifest write helper onto `BackupManifest`
 * offload fsync, rename and cleanup to `spawn_blocking()`

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990


proxmox:

Jakob Klocker (1):
  pbs-api-types: implement Hash and Borrow<str> for BackupArchiveName

 pbs-api-types/src/datastore.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


proxmox-backup:

Jakob Klocker (4):
  server: pull: run blocking file operations on the blocking pool
  pbs-datastore: pass expected files to cleanup_unreferenced_files
  fix #6990: server: drop verify state on non-decrypt pull job
  fix #6990: server: drop verify state on push job

 pbs-datastore/src/backup_info.rs | 24 +++++------
 pbs-datastore/src/manifest.rs    | 51 ++++++++++++++++++++++-
 src/server/pull.rs               | 69 +++++++++++++++++++-------------
 src/server/push.rs               |  7 ++++
 4 files changed, 110 insertions(+), 41 deletions(-)


Summary over all repositories:
  5 files changed, 124 insertions(+), 41 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH proxmox v4 1/5] pbs-api-types: implement Hash and Borrow<str> for BackupArchiveName

[Jakob Klocker]

Sets of archive names are currently built as HashSet<String>, losing
the validation and type information BackupArchiveName provides.
Implementing Hash allows using BackupArchiveName directly as a set or
map key, and Borrow<str> keeps lookups by plain &str possible, e.g.
when matching file names read from a snapshot directory.

Suggested-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 93ccaf07..39ff5e9a 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1,5 +1,7 @@
+use std::borrow::Borrow;
 use std::convert::{AsRef, TryFrom};
 use std::fmt;
+use std::hash::{Hash, Hasher};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::sync::LazyLock;
@@ -2139,6 +2141,18 @@ impl AsRef<str> for BackupArchiveName {
     }
 }
 
+impl Borrow<str> for BackupArchiveName {
+    fn borrow(&self) -> &str {
+        &self.name
+    }
+}
+
+impl Hash for BackupArchiveName {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.name.hash(state)
+    }
+}
+
 impl BackupArchiveName {
     pub fn from_path(path: impl AsRef<Path>) -> Result<Self, Error> {
         let path = path.as_ref();
-- 
2.47.3

[PATCH proxmox-backup v4 2/5] server: pull: run blocking file operations on the blocking pool

[Jakob Klocker]

The atomic renames and the unreferenced-file cleanup are blocking
operations that ran directly on the runtime's worker threads, stalling
other tasks scheduled there for their duration.

Use the tokio counterpart for the renames, the removes, and move the
cleanup onto the blocking thread pool.

Note that tokio::fs runs these on the blocking thread pool, which
carries some overhead over std::fs, but that is acceptable here as these
are one-off operations rather than tight loops.

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/server/pull.rs | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 501c05a1b..1e24b038b 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -455,7 +455,7 @@ async fn pull_single_archive<'a>(
         .with_context(|| archive_prefix.clone())?
         .is_none()
     {
-        let _ = std::fs::remove_file(&tmp_path);
+        let _ = tokio::fs::remove_file(&tmp_path).await;
         bail!("{archive_prefix}: archive missing on source");
     };
 
@@ -596,14 +596,14 @@ async fn pull_single_archive<'a>(
         }
     }
     let source_path = if crypt_config.is_some() {
-        if let Err(err) = std::fs::remove_file(&tmp_path) {
+        if let Err(err) = tokio::fs::remove_file(&tmp_path).await {
             bail!("{archive_prefix}: Failed to remove temp. file {tmp_path:?} failed - {err}");
         }
         tmp_dec_path
     } else {
         tmp_path
     };
-    if let Err(err) = std::fs::rename(&source_path, &path) {
+    if let Err(err) = tokio::fs::rename(&source_path, &path).await {
         bail!("{archive_prefix}: Atomic rename file {path:?} failed - {err}");
     }
 
@@ -662,7 +662,7 @@ async fn pull_snapshot<'a>(
         .await
         .with_context(|| prefix.clone())?
     else {
-        let _ = std::fs::remove_file(&tmp_manifest_name);
+        let _ = tokio::fs::remove_file(&tmp_manifest_name).await;
         log_sender
             .log(
                 Level::INFO,
@@ -713,7 +713,7 @@ async fn pull_snapshot<'a>(
         log_sender
             .log(Level::INFO, format!("{prefix}: no data changes"))
             .await?;
-        let _ = std::fs::remove_file(&tmp_manifest_name);
+        let _ = tokio::fs::remove_file(&tmp_manifest_name).await;
         Ok::<(), Error>(())
     };
 
@@ -752,7 +752,7 @@ async fn pull_snapshot<'a>(
         params.verified_only,
         params.encrypted_only,
     ) {
-        let _ = std::fs::remove_file(&tmp_manifest_name);
+        let _ = tokio::fs::remove_file(&tmp_manifest_name).await;
         log_sender
             .log(
                 Level::INFO,
@@ -893,7 +893,7 @@ async fn pull_snapshot<'a>(
         nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
     }
 
-    if let Err(err) = std::fs::rename(&tmp_manifest_name, &manifest_name) {
+    if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
         bail!("{prefix}: Atomic rename file {manifest_name:?} failed - {err}");
     }
     if let DatastoreBackend::S3(s3_client) = backend {
@@ -913,8 +913,9 @@ async fn pull_snapshot<'a>(
 
     fetch_log(crypt_config).await?;
 
-    snapshot
-        .cleanup_unreferenced_files(&manifest)
+    let snapshot = snapshot.clone();
+    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&manifest))
+        .await?
         .map_err(|err| format_err!("{prefix}: failed to cleanup unreferenced files - {err}"))?;
 
     Ok(Some(sync_stats))
-- 
2.47.3

[PATCH proxmox-backup v4 3/5] pbs-datastore: pass expected files to cleanup_unreferenced_files

[Jakob Klocker]

Instead of passing the whole manifest and building the set of wanted
files inside cleanup_unreferenced_files, let the caller pass the set
directly. Add a BackupManifest::expected_files helper returning the set
of all files which might be encountered in the snapshot described by
the manifest, i.e. the manifest entries plus the manifest and client
log blobs.

This decouples the cleanup from the manifest instance and is in
preparation for writing a modified manifest during pull, without
having to keep the manifest around until the cleanup step.

No functional change intended.

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 24 ++++++++++++------------
 pbs-datastore/src/manifest.rs    | 19 ++++++++++++++++++-
 src/server/pull.rs               |  3 ++-
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index be4ec8b3e..cce2b699f 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -1,3 +1,4 @@
+use std::collections::HashSet;
 use std::fmt;
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::os::unix::prelude::OsStrExt;
@@ -16,8 +17,7 @@ use proxmox_systemd::escape_unit;
 
 use pbs_api_types::{
     ArchiveType, Authid, BACKUP_DATE_REGEX, BackupArchiveName, BackupGroupDeleteStats,
-    BackupNamespace, BackupType, CLIENT_LOG_BLOB_NAME, GroupFilter, MANIFEST_BLOB_NAME,
-    VerifyState,
+    BackupNamespace, BackupType, GroupFilter, MANIFEST_BLOB_NAME, VerifyState,
 };
 use pbs_config::{BackupLockGuard, open_backup_lockfile};
 
@@ -1029,17 +1029,17 @@ impl BackupDir {
         Ok(())
     }
 
-    /// Cleans up the backup directory by removing any file not mentioned in the manifest.
-    pub fn cleanup_unreferenced_files(&self, manifest: &BackupManifest) -> Result<(), Error> {
+    /// Removes any file in the backup directory not present in `files_to_keep`.
+    ///
+    /// The set is supplied by the caller (see `BackupManifest::expected_files`)
+    /// rather than derived here, so passing an incomplete set will delete files
+    /// that should be retained.
+    pub fn cleanup_unreferenced_files(
+        &self,
+        files_to_keep: &HashSet<BackupArchiveName>,
+    ) -> Result<(), Error> {
         let full_path = self.full_path();
 
-        let mut wanted_files = std::collections::HashSet::new();
-        wanted_files.insert(MANIFEST_BLOB_NAME.to_string());
-        wanted_files.insert(CLIENT_LOG_BLOB_NAME.to_string());
-        manifest.files().iter().for_each(|item| {
-            wanted_files.insert(item.filename.to_string());
-        });
-
         for item in proxmox_sys::fs::read_subdir(libc::AT_FDCWD, &full_path)?.flatten() {
             if let Some(file_type) = item.file_type() {
                 if file_type != nix::dir::Type::File {
@@ -1051,7 +1051,7 @@ impl BackupDir {
                 continue;
             };
             if let Ok(name) = std::str::from_utf8(file_name) {
-                if wanted_files.contains(name) {
+                if files_to_keep.contains(name) {
                     continue;
                 }
             }
diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index 4d482c252..8c7a5f8b6 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -1,9 +1,14 @@
+use std::collections::HashSet;
+
 use anyhow::{Error, bail, format_err};
 
 use serde::{Deserialize, Serialize};
 use serde_json::{Value, json};
 
-use pbs_api_types::{BackupArchiveName, BackupType, CryptMode, Fingerprint, SnapshotVerifyState};
+use pbs_api_types::{
+    BackupArchiveName, BackupType, CLIENT_LOG_BLOB_NAME, CryptMode, Fingerprint,
+    MANIFEST_BLOB_NAME, SnapshotVerifyState,
+};
 use pbs_tools::crypt_config::CryptConfig;
 
 use super::DataBlob;
@@ -295,6 +300,18 @@ impl BackupManifest {
 
         Ok(Some(Deserialize::deserialize(value)?))
     }
+
+    /// Returns the set of all files which might be encountered in
+    /// the snapshot represented by given manifest.
+    pub fn expected_files(&self) -> HashSet<BackupArchiveName> {
+        let mut expected = HashSet::new();
+        expected.insert(MANIFEST_BLOB_NAME.clone());
+        expected.insert(CLIENT_LOG_BLOB_NAME.clone());
+        self.files().iter().for_each(|item| {
+            expected.insert(item.filename.clone());
+        });
+        expected
+    }
 }
 
 impl TryFrom<DataBlob> for BackupManifest {
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 1e24b038b..ef7f714b7 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -852,6 +852,7 @@ async fn pull_snapshot<'a>(
         sync_stats.add(stats);
     }
 
+    let files_to_keep = manifest.expected_files();
     if let Some(new_manifest) = new_manifest {
         let mut new_manifest = Arc::try_unwrap(new_manifest)
             .map_err(|_arc| {
@@ -914,7 +915,7 @@ async fn pull_snapshot<'a>(
     fetch_log(crypt_config).await?;
 
     let snapshot = snapshot.clone();
-    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&manifest))
+    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&files_to_keep))
         .await?
         .map_err(|err| format_err!("{prefix}: failed to cleanup unreferenced files - {err}"))?;
 
-- 
2.47.3

[PATCH proxmox-backup v4 4/5] fix #6990: server: drop verify state on non-decrypt pull job

[Jakob Klocker]

On a non-decrypt pull the source manifest is written to the target
as-is, so the target inherits the source's verify_state flag instead of
being verified independently on its own storage. Because a snapshot
carrying a verify_state is skipped by verify jobs, the target's copy can
never be checked.

The decrypt path already drops verify_state; do the same on the
non-decrypt path when the snapshot is newly pulled or re-synced due to
corruption. Also factor the manifest blob encoding and writing into a
helper, shared by both paths.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 pbs-datastore/src/manifest.rs | 34 +++++++++++++++++++++--
 src/server/pull.rs            | 51 ++++++++++++++++++++++-------------
 2 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index 8c7a5f8b6..ea8642f6e 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -1,7 +1,9 @@
 use std::collections::HashSet;
+use std::io::Write;
+use std::os::fd::AsRawFd;
+use std::path::Path;
 
-use anyhow::{Error, bail, format_err};
-
+use anyhow::{Context, Error, bail, format_err};
 use serde::{Deserialize, Serialize};
 use serde_json::{Value, json};
 
@@ -301,6 +303,34 @@ impl BackupManifest {
         Ok(Some(Deserialize::deserialize(value)?))
     }
 
+    /// Encode the manifest and write it atomically to `target_path`.
+    ///
+    /// The encoded blob is first written to `tmp_path` and fsync'd, then
+    /// atomically renamed onto `target_path`. Writing via a temp file and
+    /// rename ensures concurrent readers never observe a partially written,
+    /// corrupt manifest.
+    ///
+    /// Returns the raw blob data.
+    pub fn write_to_path(&self, tmp_path: &Path, target_path: &Path) -> Result<Vec<u8>, Error> {
+        let raw_data = self.to_data_blob(None)?.raw_data().to_vec();
+
+        let mut file = std::fs::OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .open(tmp_path)
+            .with_context(|| format!("failed to open manifest {tmp_path:?}"))?;
+
+        file.write_all(&raw_data)?;
+        file.flush()?;
+        nix::unistd::fsync(file.as_raw_fd())?;
+
+        std::fs::rename(tmp_path, target_path)
+            .with_context(|| format!("atomic rename manifest {target_path:?} failed"))?;
+
+        Ok(raw_data)
+    }
+
     /// Returns the set of all files which might be encountered in
     /// the snapshot represented by given manifest.
     pub fn expected_files(&self) -> HashSet<BackupArchiveName> {
diff --git a/src/server/pull.rs b/src/server/pull.rs
index ef7f714b7..1665bee3c 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -3,7 +3,6 @@
 use std::collections::hash_map::Entry;
 use std::collections::{HashMap, HashSet};
 use std::io::Seek;
-use std::os::fd::AsRawFd;
 use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::{Duration, SystemTime};
@@ -16,8 +15,6 @@ use super::sync::{
 use crate::backup::{check_ns_modification_privs, check_ns_privs};
 use crate::server::sync::SharedGroupProgress;
 use anyhow::{Context, Error, bail, format_err};
-use tokio::fs::OpenOptions;
-use tokio::io::AsyncWriteExt;
 
 use pbs_api_types::{
     ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
@@ -744,7 +741,8 @@ async fn pull_snapshot<'a>(
     };
 
     let mut manifest_data = tmp_manifest_blob.raw_data().to_vec();
-    let manifest = BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
+    let mut manifest =
+        BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
 
     if ignore_not_verified_or_encrypted(
         &manifest,
@@ -852,7 +850,11 @@ async fn pull_snapshot<'a>(
         sync_stats.add(stats);
     }
 
+    let target_verify_state = existing_target_manifest
+        .as_ref()
+        .and_then(|m| m.unprotected.get("verify_state").cloned());
     let files_to_keep = manifest.expected_files();
+
     if let Some(new_manifest) = new_manifest {
         let mut new_manifest = Arc::try_unwrap(new_manifest)
             .map_err(|_arc| {
@@ -879,24 +881,35 @@ async fn pull_snapshot<'a>(
             new_manifest.set_sync_source_signature(expected.bytes())?;
         }
 
-        // keep signature
-        let manifest_blob = new_manifest.to_data_blob(None)?;
-        // update contents to be uploaded to backend
-        manifest_data = manifest_blob.raw_data().to_vec();
+        manifest_data = tokio::task::spawn_blocking(move || {
+            new_manifest.write_to_path(&tmp_manifest_name, &manifest_name)
+        })
+        .await??;
+    } else if manifest.unprotected.get("verify_state") != target_verify_state.as_ref() {
+        // the manifest is copied as-is on the non-decrypted path: never inherit the
+        // source's verify state, but keep one the target obtained on its own
+        let Some(unprotected) = manifest.unprotected.as_object_mut() else {
+            bail!("{prefix}: unexpected manifest without 'unprotected' section");
+        };
+        match &target_verify_state {
+            Some(state) => {
+                unprotected.insert("verify_state".to_string(), state.clone());
+            }
+            None => {
+                unprotected.remove("verify_state");
+            }
+        }
 
-        let mut tmp_manifest_file = OpenOptions::new()
-            .write(true)
-            .truncate(true) // clear pre-existing manifest content
-            .open(&tmp_manifest_name)
-            .await?;
-        tmp_manifest_file.write_all(&manifest_data).await?;
-        tmp_manifest_file.flush().await?;
-        nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
+        manifest_data = tokio::task::spawn_blocking(move || {
+            manifest.write_to_path(&tmp_manifest_name, &manifest_name)
+        })
+        .await??;
+    } else {
+        if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
+            bail!("{prefix}: Atomic rename file {manifest_name:?} failed - {err}");
+        }
     }
 
-    if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
-        bail!("{prefix}: Atomic rename file {manifest_name:?} failed - {err}");
-    }
     if let DatastoreBackend::S3(s3_client) = backend {
         let object_key = pbs_datastore::s3::object_key_from_path(
             &snapshot.relative_path(),
-- 
2.47.3

[PATCH proxmox-backup v4 5/5] fix #6990: server: drop verify state on push job

[Jakob Klocker]

When pushing a snapshot the target manifest is recreated from the source
manifest, copying its verify_state. As with pull, this makes the pushed
snapshot appear verified on the remote without its stored copy having
been checked there.

Strip verify_state after copying the unprotected section so the pushed
snapshot is verified independently on the target.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>
Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/server/push.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/server/push.rs b/src/server/push.rs
index 98f843a49..c1f8b4e13 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -1454,6 +1454,13 @@ pub(crate) async fn push_snapshot(
     } else {
         target_manifest.signature = source_manifest.signature.clone();
     };
+
+    if let Some(unprotected) = target_manifest.unprotected.as_object_mut() {
+        unprotected.remove("verify_state");
+    } else {
+        bail!("Encountered unexpected manifest without 'unprotected' section.");
+    }
+
     // FIXME: replace me with to_data_blob once there is an upload_blob
     let manifest_string =
         target_manifest.to_string(encrypt_using_key.map(|(_, config)| config).as_deref())?;
-- 
2.47.3