From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id CE1451FF2C6 for ; Wed, 10 Jul 2024 15:29:21 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DD38EA8DD; Wed, 10 Jul 2024 15:29:00 +0200 (CEST) Date: Wed, 10 Jul 2024 15:28:21 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240612132300.352392-1-g.goller@proxmox.com> In-Reply-To: <20240612132300.352392-1-g.goller@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1720616817.d7d27nxn2b.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.050 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] fix #5439: allow to reuse existing datastore X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On June 12, 2024 3:22 pm, Gabriel Goller wrote: > Disallow creating datastores in non-empty directories. Allow adding > existing datastores via a 'reuse-datastore' checkmark. This only checks > if all the necessary directories (.chunks + subdirectories and .lock) > are existing and have the correct permissions, then opens the > datastore. > > Signed-off-by: Gabriel Goller > --- > > v2, thanks @Fabian: > - also check on frontend for root > - forbid datastore creation if dir not empty > - add reuse-datastore option > - verify chunkstore directories permissions and owners > > pbs-datastore/src/chunk_store.rs | 49 ++++++++++++++++++++++++++++- > src/api2/config/datastore.rs | 54 +++++++++++++++++++++++++------- > src/api2/node/disks/directory.rs | 1 + > src/api2/node/disks/zfs.rs | 1 + > 4 files changed, 93 insertions(+), 12 deletions(-) > > diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs > index 9f6289c9..077bc316 100644 > --- a/pbs-datastore/src/chunk_store.rs > +++ b/pbs-datastore/src/chunk_store.rs > @@ -164,7 +164,7 @@ impl ChunkStore { > /// Note that this must be used with care, as it's dangerous to create two instances on the > /// same base path, as closing the underlying ProcessLocker drops all locks from this process > /// on the lockfile (even if separate FDs) given this ^ > - pub(crate) fn open>( > + pub fn open>( wouldn't it make more sense to split out the path checks and call them both in open and in verify_chunkstore below > name: &str, > base: P, > sync_level: DatastoreFSyncLevel, > @@ -563,6 +563,53 @@ impl ChunkStore { > // unwrap: only `None` in unit tests > ProcessLocker::try_exclusive_lock(self.locker.clone().unwrap()) > } > + > + /// Checks permissions and owner of passed path. > + fn check_permissions>(path: T, file_mode: u32) -> Result<(), Error> { > + match nix::sys::stat::stat(path.as_ref()) { > + Ok(stat) => { > + if stat.st_uid != u32::from(pbs_config::backup_user()?.uid) > + || stat.st_gid != u32::from(pbs_config::backup_group()?.gid) > + || stat.st_mode & 0o700 != file_mode > + { > + bail!( > + "unable to open existing chunk store path {:?} - permissions or owner not correct", > + path.as_ref(), > + ); > + } > + } > + Err(err) => { > + bail!( > + "unable to open existing chunk store path {:?} - {err}", > + path.as_ref(), > + ); > + } > + } > + Ok(()) > + } > + > + /// Verify vital files in datastore. Checks the owner and permissions of: the chunkstore, it's > + /// subdirectories and the lock file. > + pub fn verify_chunkstore>(path: T) -> Result<(), Error> { > + // Check datastore root path perm/owner > + ChunkStore::check_permissions(path.as_ref(), 0o700)?; > + > + let chunk_dir = Self::chunk_dir(path.as_ref()); > + // Check datastore .chunks path perm/owner > + ChunkStore::check_permissions(&chunk_dir, 0o700)?; > + > + // Check all .chunks subdirectories > + for i in 0..64 * 1024 { > + let mut l1path = chunk_dir.clone(); > + l1path.push(format!("{:04x}", i)); > + ChunkStore::check_permissions(&l1path, 0o700)?; > + } > + > + // Check .lock file > + let lockfile_path = Self::lockfile_path(path.as_ref()); > + ChunkStore::check_permissions(lockfile_path, 0o600)?; > + Ok(()) > + } > } > > #[test] > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index 6b742acb..f1e80b2d 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -1,7 +1,7 @@ > use std::path::PathBuf; > > use ::serde::{Deserialize, Serialize}; > -use anyhow::Error; > +use anyhow::{bail, Error}; > use hex::FromHex; > use serde_json::Value; > > @@ -70,23 +70,48 @@ pub(crate) fn do_create_datastore( > _lock: BackupLockGuard, > mut config: SectionConfigData, > datastore: DataStoreConfig, > + reuse_datastore: bool, > worker: Option<&dyn WorkerTaskContext>, > ) -> Result<(), Error> { > let path: PathBuf = datastore.path.clone().into(); > > + if path.parent().is_none() { > + bail!("cannot create datastore in root path"); > + } > + > + let datastore_empty = std::fs::read_dir(path.clone())?.all(|dir| { doesn't this read_dir fail if `path` doesn't exist yet? prior to this change, creating a datastore would also create the datastore dir itself (and any parents!). > + dir.map_or(false, |file| { > + file.file_name().to_str().map_or(false, |name| { > + (name.starts_with('.') > + && !(name == ".chunks" || name == ".lock" || name == ".gc-status")) > + || name.starts_with("lost+found") I think the last check there could be == as well? the indentation here is kinda whacky.. maybe it would be enough to consider `.chunks` to mark a dir as non-empty? > + }) > + }) >+ }); should we do this always? or restructure the ifs below and just do it for the non-reusing create path? > + > let tuning: DatastoreTuning = serde_json::from_value( > DatastoreTuning::API_SCHEMA > .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?, > )?; > - let backup_user = pbs_config::backup_user()?; > - let _store = ChunkStore::create( > - &datastore.name, > - path, > - backup_user.uid, > - backup_user.gid, > - worker, > - tuning.sync_level.unwrap_or_default(), > - )?; > + > + if !datastore_empty && !reuse_datastore { > + bail!("Directory is not empty!"); > + } else if !datastore_empty && reuse_datastore { > + ChunkStore::verify_chunkstore(&path)?; > + > + let _store = > + ChunkStore::open(&datastore.name, path, tuning.sync_level.unwrap_or_default())?; (continued from first comment above) and leave out this call here? > + } else { > + let backup_user = pbs_config::backup_user()?; > + let _store = ChunkStore::create( > + &datastore.name, > + path, > + backup_user.uid, > + backup_user.gid, > + worker, > + tuning.sync_level.unwrap_or_default(), > + )?; > + } > > config.set_data(&datastore.name, "datastore", &datastore)?; > > @@ -103,6 +128,12 @@ pub(crate) fn do_create_datastore( > type: DataStoreConfig, > flatten: true, > }, > + "reuse-datastore": { > + type: Boolean, > + optional: true, > + default: false, > + description: "Re-use existing datastore directory." > + } > }, > }, > access: { > @@ -112,6 +143,7 @@ pub(crate) fn do_create_datastore( > /// Create new datastore config. > pub fn create_datastore( > config: DataStoreConfig, > + reuse_datastore: bool, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result { > let lock = pbs_config::datastore::lock_config()?; > @@ -156,7 +188,7 @@ pub fn create_datastore( > auth_id.to_string(), > to_stdout, > move |worker| { > - do_create_datastore(lock, section_config, config, Some(&worker))?; > + do_create_datastore(lock, section_config, config, reuse_datastore, Some(&worker))?; > > if let Some(prune_job_config) = prune_job_config { > do_create_prune_job(prune_job_config, Some(&worker)) > diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs > index 9f1112a9..25cfe784 100644 > --- a/src/api2/node/disks/directory.rs > +++ b/src/api2/node/disks/directory.rs > @@ -217,6 +217,7 @@ pub fn create_datastore_disk( > lock, > config, > datastore, > + false, > Some(&worker), > )?; > } > diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs > index 673dc1bf..ad2129fc 100644 > --- a/src/api2/node/disks/zfs.rs > +++ b/src/api2/node/disks/zfs.rs > @@ -323,6 +323,7 @@ pub fn create_zpool( > lock, > config, > datastore, > + false, > Some(&worker), > )?; > } > -- > 2.43.0 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel