Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] fix #5439: allow to reuse existing datastore

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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 <g.goller@proxmox.com>
> ---
> 
> 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<P: Into<PathBuf>>(
> +    pub fn open<P: Into<PathBuf>>(

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<T: AsRef<Path>>(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<T: AsRef<Path>>(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<String, Error> {
>      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