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 3094B1FF0DF for ; Fri, 28 Aug 2026 13:11:52 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id DC273214DA; Fri, 28 Aug 2026 13:11:51 +0200 (CEST) Message-ID: <675514ab-3958-471e-82cb-dbcec00f1c4b@proxmox.com> Date: Fri, 28 Aug 2026 13:11:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 3/3] fix #6990: server: drop verify state on non-decrypt pull job To: Jakob Klocker , pbs-devel@lists.proxmox.com References: <20260821111826.299588-1-j.klocker@proxmox.com> <20260821111826.299588-4-j.klocker@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260821111826.299588-4-j.klocker@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787915496141 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.693 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) KAM_SHORT 0.001 Use of a URL Shortener for very short URL 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: VWIBX72KK2YUFXAAVUMLI6E7N36FQK3V X-Message-ID-Hash: VWIBX72KK2YUFXAAVUMLI6E7N36FQK3V 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: Two smaller comments and considerations inline, rest looks good to me! On 8/21/26 1:18 PM, Jakob Klocker wrote: > 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 > Signed-off-by: Jakob Klocker > --- > pbs-datastore/src/manifest.rs | 25 ++++++++++++++++++- > src/server/pull.rs | 47 +++++++++++++++++++++++------------ > 2 files changed, 55 insertions(+), 17 deletions(-) > > diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs > index 11de1085b..c87797474 100644 > --- a/pbs-datastore/src/manifest.rs > +++ b/pbs-datastore/src/manifest.rs > @@ -1,5 +1,8 @@ > -use anyhow::{Error, bail, format_err}; > +use std::io::Write; > +use std::os::fd::AsRawFd; > +use std::path::Path; > > +use anyhow::{Context, Error, bail, format_err}; > use serde::{Deserialize, Serialize}; > use serde_json::{Value, json}; > > @@ -278,6 +281,26 @@ impl BackupManifest { > > Ok(Some(Deserialize::deserialize(value)?)) > } > + > + /// Encode the manifest and write the raw blob data to `path`, fsync'ing it. comment: I would like for this comment to also include a note/warning that this should only be used to write manifest files to temp file path (or maybe we even want to check/encode that)? If this will be used to write the mainfest directly, it would lead to concurrent readers to potentially read incomplete and therefore corrupt manifests. Another option and probably even preferable would be for the rename to be included in this helper as well and maybe pass both, tmp and target path. From the diff below that should be doable and give us guarantees that the manifest is persisted atomically. Also this whole content writing could be performed as async via tokio::fs::File, but taking the performance considerations into account [0] it probably is best kept sync for now. Major win would be that this could then use `io-uring` for some operations if enabled in tokio in the future without code changes, e.g. the OpenOptions used by File have config and feature flags for this [1]. [0] https://docs.rs/tokio/latest/tokio/fs/index.html [1] https://docs.rs/tokio/latest/src/tokio/fs/open_options.rs.html#122 > + /// > + /// Returns the raw blob data. > + pub fn write_to_path(&self, path: &Path) -> Result, Error> { This could take ownership of the manifest instead, so no further modifications are to be made afterwards and we can get rid of the Arc below, since it can be moved without issues into the spawn_blocking call > + 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(path) > + .with_context(|| format!("failed to open manifest {path:?}"))?; > + > + file.write_all(&raw_data)?; > + file.flush()?; > + nix::unistd::fsync(file.as_raw_fd())?; > + > + Ok(raw_data) > + } > } > > impl TryFrom for BackupManifest { > diff --git a/src/server/pull.rs b/src/server/pull.rs > index fd401ac85..acb7a98dc 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, > @@ -741,7 +738,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 = > + Arc::new(BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?); > > if ignore_not_verified_or_encrypted( > &manifest, > @@ -849,6 +847,10 @@ 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()); > + > if let Some(new_manifest) = new_manifest { > let mut new_manifest = Arc::try_unwrap(new_manifest) > .map_err(|_arc| { > @@ -875,19 +877,32 @@ 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(); > + let tmp_manifest_name = tmp_manifest_name.clone(); > + manifest_data = > + tokio::task::spawn_blocking(move || new_manifest.write_to_path(&tmp_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 manifest_mut = Arc::get_mut(&mut manifest) > + .ok_or_else(|| format_err!("{prefix}: manifest unexpectedly shared"))?; ... as commented above, by taking ownership in BackupManifest::write_to_path() there is no need for the Arc anymore, and this can be simplified. > + let Some(unprotected) = manifest_mut.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())?; > + let manifest = Arc::clone(&manifest); > + let tmp_manifest_name = tmp_manifest_name.clone(); > + manifest_data = > + tokio::task::spawn_blocking(move || manifest.write_to_path(&tmp_manifest_name)) > + .await??; > } > > if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await { ... same as mentioned above the rename could be part of write_to_path and only the remaining else branch handled via the rename. Would help to protect against misusing write_to_path().