[pbs-devel] [PATCH proxmox-backup 1/2] use new atomic_open_or_create_file

Fabian Grünbichler f.gruenbichler at proxmox.com
Mon Jul 19 12:45:12 CEST 2021


this breaks tests and thus builds as unprivileged user:
locking -> creating temp file -> fchown -> EPERM!

it (or the next patch?) is missing one no-longer-needed import:

warning: unused import: `nix::sys::stat::Mode`
 --> src/config/node.rs:4:5
  |
4 | use nix::sys::stat::Mode;
  |     ^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

and one call-site:

diff --git a/src/api2/access/openid.rs b/src/api2/access/openid.rs
index 783864cc..efcc0dbb 100644
--- a/src/api2/access/openid.rs
+++ b/src/api2/access/openid.rs
@@ -9,7 +9,6 @@ use proxmox::api::router::{Router, SubdirMap};
 use proxmox::api::{api, Permission, RpcEnvironment};
 use proxmox::{list_subdirs_api_method};
 use proxmox::{identity, sortable};
-use proxmox::tools::fs::open_file_locked;
 
 use proxmox_openid::{OpenIdAuthenticator,  OpenIdConfig};
 
@@ -19,6 +18,7 @@ use pbs_tools::ticket::Ticket;
 
 use crate::server::ticket::ApiTicket;
 
+use crate::backup::open_backup_lockfile;
 use crate::config::domains::{OpenIdUserAttribute, OpenIdRealmConfig};
 use crate::config::cached_user_info::CachedUserInfo;
 
@@ -117,7 +117,7 @@ pub fn openid_login(
     if !user_info.is_active_user_id(&user_id) {
         if config.autocreate.unwrap_or(false) {
             use crate::config::user;
-            let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
+            let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
             let user = user::User {
                 userid: user_id.clone(),
                 comment: None,

On July 16, 2021 10:28 am, Dietmar Maurer wrote:
> Factor out open_backup_lockfile() method to acquire locks owned by
> user backup with permission 0660.
> ---
>  src/api2/access/acl.rs                  |   4 +-
>  src/api2/access/user.rs                 |  14 +--
>  src/api2/config/datastore.rs            |   2 +-
>  src/api2/config/remote.rs               |   8 +-
>  src/api2/config/sync.rs                 |   8 +-
>  src/api2/config/tape_backup_job.rs      |   9 +-
>  src/api2/config/tape_encryption_keys.rs |  15 +---
>  src/api2/config/verify.rs               |   9 +-
>  src/api2/node/disks/directory.rs        |   4 +-
>  src/api2/node/network.rs                |   9 +-
>  src/backup/datastore.rs                 |   9 +-
>  src/backup/mod.rs                       |  26 ++++++
>  src/config/acme/plugin.rs               |   6 +-
>  src/config/datastore.rs                 |   6 +-
>  src/config/domains.rs                   |   6 +-
>  src/config/drive.rs                     |   6 +-
>  src/config/media_pool.rs                |   6 +-
>  src/config/node.rs                      |   8 +-
>  src/config/tape_encryption_keys.rs      |   8 +-
>  src/config/tfa.rs                       |  11 ++-
>  src/config/token_shadow.rs              |   9 +-
>  src/server/jobstate.rs                  |  16 ++--
>  src/server/worker_task.rs               |  14 ++-
>  src/tape/drive/mod.rs                   |  46 +++++-----
>  src/tape/drive/virtual_tape.rs          |   3 +-
>  src/tape/inventory.rs                   |  49 ++---------
>  src/tape/media_pool.rs                  |   5 +-
>  src/tools/memcom.rs                     | 110 +++---------------------
>  28 files changed, 158 insertions(+), 268 deletions(-)
> 
> diff --git a/src/api2/access/acl.rs b/src/api2/access/acl.rs
> index a78aabcd..88a2667c 100644
> --- a/src/api2/access/acl.rs
> +++ b/src/api2/access/acl.rs
> @@ -3,12 +3,12 @@
>  use anyhow::{bail, Error};
>  
>  use proxmox::api::{api, Router, RpcEnvironment, Permission};
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::api2::types::*;
>  use crate::config::acl;
>  use crate::config::acl::{Role, PRIV_SYS_AUDIT, PRIV_PERMISSIONS_MODIFY};
>  use crate::config::cached_user_info::CachedUserInfo;
> +use crate::backup::open_backup_lockfile;
>  
>  fn extract_acl_node_data(
>      node: &acl::AclTreeNode,
> @@ -200,7 +200,7 @@ pub fn update_acl(
>          };
>      }
>  
> -    let _lock = open_file_locked(acl::ACL_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(acl::ACL_CFG_LOCKFILE, None, true)?;
>  
>      let (mut tree, expected_digest) = acl::config()?;
>  
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index e080d57a..23c999b4 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -8,13 +8,13 @@ use std::collections::HashMap;
>  use proxmox::api::{api, ApiMethod, Router, RpcEnvironment, Permission};
>  use proxmox::api::router::SubdirMap;
>  use proxmox::api::schema::{Schema, StringSchema};
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::api2::types::*;
>  use crate::config::user;
>  use crate::config::token_shadow;
>  use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_PERMISSIONS_MODIFY};
>  use crate::config::cached_user_info::CachedUserInfo;
> +use crate::backup::open_backup_lockfile;
>  
>  pub const PBS_PASSWORD_SCHEMA: Schema = StringSchema::new("User Password.")
>      .format(&PASSWORD_FORMAT)
> @@ -226,7 +226,7 @@ pub fn create_user(
>      rpcenv: &mut dyn RpcEnvironment
>  ) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
>  
>      let user: user::User = serde_json::from_value(param)?;
>  
> @@ -368,7 +368,7 @@ pub fn update_user(
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = user::config()?;
>  
> @@ -461,7 +461,7 @@ pub fn update_user(
>  pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error> {
>  
>      let _tfa_lock = crate::config::tfa::write_lock()?;
> -    let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = user::config()?;
>  
> @@ -597,7 +597,7 @@ pub fn generate_token(
>      digest: Option<String>,
>  ) -> Result<Value, Error> {
>  
> -    let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = user::config()?;
>  
> @@ -678,7 +678,7 @@ pub fn update_token(
>      digest: Option<String>,
>  ) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = user::config()?;
>  
> @@ -746,7 +746,7 @@ pub fn delete_token(
>      digest: Option<String>,
>  ) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(user::USER_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(user::USER_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = user::config()?;
>  
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 19b822d4..a55e3bdc 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -60,7 +60,7 @@ pub fn list_datastores(
>  }
>  
>  pub(crate) fn do_create_datastore(
> -    _lock: std::fs::File,
> +    _lock: BackupLockGuard,
>      mut config: SectionConfigData,
>      datastore: DataStoreConfig,
>      worker: Option<&dyn TaskState>,
> diff --git a/src/api2/config/remote.rs b/src/api2/config/remote.rs
> index 446a2604..33c99bff 100644
> --- a/src/api2/config/remote.rs
> +++ b/src/api2/config/remote.rs
> @@ -4,13 +4,13 @@ use ::serde::{Deserialize, Serialize};
>  
>  use proxmox::api::{api, ApiMethod, Router, RpcEnvironment, Permission};
>  use proxmox::http_err;
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::api2::types::*;
>  use crate::client::{HttpClient, HttpClientOptions};
>  use crate::config::cached_user_info::CachedUserInfo;
>  use crate::config::remote;
>  use crate::config::acl::{PRIV_REMOTE_AUDIT, PRIV_REMOTE_MODIFY};
> +use crate::backup::open_backup_lockfile;
>  
>  #[api(
>      input: {
> @@ -94,7 +94,7 @@ pub fn list_remotes(
>  /// Create new remote.
>  pub fn create_remote(password: String, param: Value) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(remote::REMOTE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(remote::REMOTE_CFG_LOCKFILE, None, true)?;
>  
>      let mut data = param;
>      data["password"] = Value::from(base64::encode(password.as_bytes()));
> @@ -216,7 +216,7 @@ pub fn update_remote(
>      digest: Option<String>,
>  ) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(remote::REMOTE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(remote::REMOTE_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = remote::config()?;
>  
> @@ -290,7 +290,7 @@ pub fn delete_remote(name: String, digest: Option<String>) -> Result<(), Error>
>          }
>      }
>  
> -    let _lock = open_file_locked(remote::REMOTE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(remote::REMOTE_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = remote::config()?;
>  
> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> index e784029a..bc7b9f24 100644
> --- a/src/api2/config/sync.rs
> +++ b/src/api2/config/sync.rs
> @@ -3,7 +3,6 @@ use serde_json::Value;
>  use ::serde::{Deserialize, Serialize};
>  
>  use proxmox::api::{api, Permission, Router, RpcEnvironment};
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::api2::types::*;
>  
> @@ -18,6 +17,7 @@ use crate::config::acl::{
>  
>  use crate::config::cached_user_info::CachedUserInfo;
>  use crate::config::sync::{self, SyncJobConfig};
> +use crate::backup::open_backup_lockfile;
>  
>  pub fn check_sync_job_read_access(
>      user_info: &CachedUserInfo,
> @@ -152,7 +152,7 @@ pub fn create_sync_job(
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>      let user_info = CachedUserInfo::new()?;
>  
> -    let _lock = open_file_locked(sync::SYNC_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(sync::SYNC_CFG_LOCKFILE, None, true)?;
>  
>      let sync_job: sync::SyncJobConfig = serde_json::from_value(param)?;
>      if !check_sync_job_modify_access(&user_info, &auth_id, &sync_job) {
> @@ -296,7 +296,7 @@ pub fn update_sync_job(
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>      let user_info = CachedUserInfo::new()?;
>  
> -    let _lock = open_file_locked(sync::SYNC_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(sync::SYNC_CFG_LOCKFILE, None, true)?;
>  
>      // pass/compare digest
>      let (mut config, expected_digest) = sync::config()?;
> @@ -379,7 +379,7 @@ pub fn delete_sync_job(
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>      let user_info = CachedUserInfo::new()?;
>  
> -    let _lock = open_file_locked(sync::SYNC_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(sync::SYNC_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = sync::config()?;
>  
> diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
> index 22afeb6d..02fa6f7d 100644
> --- a/src/api2/config/tape_backup_job.rs
> +++ b/src/api2/config/tape_backup_job.rs
> @@ -3,7 +3,6 @@ use serde_json::Value;
>  use ::serde::{Deserialize, Serialize};
>  
>  use proxmox::api::{api, Router, RpcEnvironment, Permission};
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::{
>      api2::types::{
> @@ -17,6 +16,7 @@ use crate::{
>          MEDIA_POOL_NAME_SCHEMA,
>          SYNC_SCHEDULE_SCHEMA,
>      },
> +    backup::open_backup_lockfile,
>      config::{
>          self,
>          cached_user_info::CachedUserInfo,
> @@ -89,8 +89,7 @@ pub fn create_tape_backup_job(
>      job: TapeBackupJobConfig,
>      _rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<(), Error> {
> -
> -    let _lock = open_file_locked(TAPE_JOB_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(TAPE_JOB_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, _digest) = config::tape_job::config()?;
>  
> @@ -233,7 +232,7 @@ pub fn update_tape_backup_job(
>      delete: Option<Vec<DeletableProperty>>,
>      digest: Option<String>,
>  ) -> Result<(), Error> {
> -    let _lock = open_file_locked(TAPE_JOB_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(TAPE_JOB_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = config::tape_job::config()?;
>  
> @@ -312,7 +311,7 @@ pub fn delete_tape_backup_job(
>      digest: Option<String>,
>      _rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<(), Error> {
> -    let _lock = open_file_locked(TAPE_JOB_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(TAPE_JOB_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = config::tape_job::config()?;
>  
> diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
> index e1b42fb8..6fbfaded 100644
> --- a/src/api2/config/tape_encryption_keys.rs
> +++ b/src/api2/config/tape_encryption_keys.rs
> @@ -9,7 +9,6 @@ use proxmox::{
>          RpcEnvironment,
>          Permission,
>      },
> -    tools::fs::open_file_locked,
>  };
>  
>  use pbs_datastore::{KeyInfo, Kdf};
> @@ -35,6 +34,7 @@ use crate::{
>          PASSWORD_HINT_SCHEMA,
>      },
>      backup::{
> +        open_backup_lockfile,
>          KeyConfig,
>          Fingerprint,
>      },
> @@ -122,11 +122,7 @@ pub fn change_passphrase(
>          bail!("Please specify a key derivation function (none is not allowed here).");
>      }
>  
> -    let _lock = open_file_locked(
> -        TAPE_KEYS_LOCKFILE,
> -        std::time::Duration::new(10, 0),
> -        true,
> -    )?;
> +    let _lock = open_backup_lockfile(TAPE_KEYS_LOCKFILE, None, true)?;
>  
>      let (mut config_map, expected_digest) = load_key_configs()?;
>  
> @@ -261,12 +257,7 @@ pub fn delete_key(
>      digest: Option<String>,
>      _rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<(), Error> {
> -
> -    let _lock = open_file_locked(
> -        TAPE_KEYS_LOCKFILE,
> -        std::time::Duration::new(10, 0),
> -        true,
> -    )?;
> +    let _lock = open_backup_lockfile(TAPE_KEYS_LOCKFILE, None, true)?;
>  
>      let (mut config_map, expected_digest) = load_key_configs()?;
>      let (mut key_map, _) = load_keys()?;
> diff --git a/src/api2/config/verify.rs b/src/api2/config/verify.rs
> index 477cda89..1a613327 100644
> --- a/src/api2/config/verify.rs
> +++ b/src/api2/config/verify.rs
> @@ -3,7 +3,6 @@ use serde_json::Value;
>  use ::serde::{Deserialize, Serialize};
>  
>  use proxmox::api::{api, Permission, Router, RpcEnvironment};
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::api2::types::*;
>  
> @@ -13,8 +12,8 @@ use crate::config::acl::{
>  };
>  
>  use crate::config::cached_user_info::CachedUserInfo;
> -
>  use crate::config::verify::{self, VerificationJobConfig};
> +use crate::backup::open_backup_lockfile;
>  
>  #[api(
>      input: {
> @@ -102,7 +101,7 @@ pub fn create_verification_job(
>  
>      user_info.check_privs(&auth_id, &["datastore", &verification_job.store], PRIV_DATASTORE_VERIFY, false)?;
>  
> -    let _lock = open_file_locked(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(verify::VERIFICATION_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, _digest) = verify::config()?;
>  
> @@ -230,7 +229,7 @@ pub fn update_verification_job(
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>      let user_info = CachedUserInfo::new()?;
>  
> -    let _lock = open_file_locked(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(verify::VERIFICATION_CFG_LOCKFILE, None, true)?;
>  
>      // pass/compare digest
>      let (mut config, expected_digest) = verify::config()?;
> @@ -315,7 +314,7 @@ pub fn delete_verification_job(
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>      let user_info = CachedUserInfo::new()?;
>  
> -    let _lock = open_file_locked(verify::VERIFICATION_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(verify::VERIFICATION_CFG_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = verify::config()?;
>  
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index ae8a3974..75e0ea02 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -5,7 +5,6 @@ use ::serde::{Deserialize, Serialize};
>  use proxmox::api::{api, Permission, RpcEnvironment, RpcEnvironmentType};
>  use proxmox::api::section_config::SectionConfigData;
>  use proxmox::api::router::Router;
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
>  use crate::tools::disks::{
> @@ -18,6 +17,7 @@ use crate::server::WorkerTask;
>  
>  use crate::api2::types::*;
>  use crate::config::datastore::{self, DataStoreConfig};
> +use crate::backup::open_backup_lockfile;
>  
>  #[api(
>      properties: {
> @@ -180,7 +180,7 @@ pub fn create_datastore_disk(
>              systemd::start_unit(&mount_unit_name)?;
>  
>              if add_datastore {
> -                let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +                let lock = open_backup_lockfile(datastore::DATASTORE_CFG_LOCKFILE, None, true)?;
>                  let datastore: DataStoreConfig =
>                      serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
>  
> diff --git a/src/api2/node/network.rs b/src/api2/node/network.rs
> index 0dc321b8..ecf112a4 100644
> --- a/src/api2/node/network.rs
> +++ b/src/api2/node/network.rs
> @@ -4,12 +4,12 @@ use ::serde::{Deserialize, Serialize};
>  
>  use proxmox::api::{api, ApiMethod, Router, RpcEnvironment, Permission};
>  use proxmox::api::schema::parse_property_string;
> -use proxmox::tools::fs::open_file_locked;
>  
>  use crate::config::network::{self, NetworkConfig};
>  use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
>  use crate::api2::types::*;
>  use crate::server::{WorkerTask};
> +use crate::backup::open_backup_lockfile;
>  
>  fn split_interface_list(list: &str) -> Result<Vec<String>, Error> {
>      let value = parse_property_string(&list, &NETWORK_INTERFACE_ARRAY_SCHEMA)?;
> @@ -238,7 +238,7 @@ pub fn create_interface(
>      let interface_type = crate::tools::required_string_param(&param, "type")?;
>      let interface_type: NetworkInterfaceType = serde_json::from_value(interface_type.into())?;
>  
> -    let _lock = open_file_locked(network::NETWORK_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(network::NETWORK_LOCKFILE, None, true)?;
>  
>      let (mut config, _digest) = network::config()?;
>  
> @@ -502,7 +502,7 @@ pub fn update_interface(
>      param: Value,
>  ) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(network::NETWORK_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(network::NETWORK_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = network::config()?;
>  
> @@ -642,8 +642,7 @@ pub fn update_interface(
>  )]
>  /// Remove network interface configuration.
>  pub fn delete_interface(iface: String, digest: Option<String>) -> Result<(), Error> {
> -
> -    let _lock = open_file_locked(network::NETWORK_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let _lock = open_backup_lockfile(network::NETWORK_LOCKFILE, None, true)?;
>  
>      let (mut config, expected_digest) = network::config()?;
>  
> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
> index d47c412b..5695adcc 100644
> --- a/src/backup/datastore.rs
> +++ b/src/backup/datastore.rs
> @@ -5,12 +5,11 @@ use std::sync::{Arc, Mutex};
>  use std::convert::TryFrom;
>  use std::str::FromStr;
>  use std::time::Duration;
> -use std::fs::File;
>  
>  use anyhow::{bail, format_err, Error};
>  use lazy_static::lazy_static;
>  
> -use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions, open_file_locked};
> +use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions};
>  
>  use pbs_api_types::upid::UPID;
>  use pbs_api_types::{Authid, GarbageCollectionStatus};
> @@ -32,6 +31,8 @@ use pbs_tools::fs::{lock_dir_noblock, DirLockGuard};
>  
>  use crate::config::datastore::{self, DataStoreConfig};
>  use crate::tools;
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
> +
>  
>  lazy_static! {
>      static ref DATASTORE_MAP: Mutex<HashMap<String, Arc<DataStore>>> = Mutex::new(HashMap::new());
> @@ -777,12 +778,12 @@ impl DataStore {
>      fn lock_manifest(
>          &self,
>          backup_dir: &BackupDir,
> -    ) -> Result<File, Error> {
> +    ) -> Result<BackupLockGuard, Error> {
>          let path = self.manifest_lock_path(backup_dir)?;
>  
>          // update_manifest should never take a long time, so if someone else has
>          // the lock we can simply block a bit and should get it soon
> -        open_file_locked(&path, Duration::from_secs(5), true)
> +        open_backup_lockfile(&path, Some(Duration::from_secs(5)), true)
>              .map_err(|err| {
>                  format_err!(
>                      "unable to acquire manifest lock {:?} - {}", &path, err
> diff --git a/src/backup/mod.rs b/src/backup/mod.rs
> index 4161b402..1db61cf5 100644
> --- a/src/backup/mod.rs
> +++ b/src/backup/mod.rs
> @@ -108,3 +108,29 @@ pub use catalog_shell::*;
>  
>  mod cached_chunk_reader;
>  pub use cached_chunk_reader::*;
> +
> +pub struct BackupLockGuard(std::fs::File);
> +
> +/// Open or create a lock file owned by user "backup" and lock it.
> +///
> +/// Owner/Group of the file is set to backup/backup.
> +/// File mode is 0660.
> +/// Default timeout is 10 seconds.
> +///
> +/// Note: This method needs to be called by user "root" or "backup".
> +pub fn open_backup_lockfile<P: AsRef<std::path::Path>>(
> +    path: P,
> +    timeout: Option<std::time::Duration>,
> +    exclusive: bool,
> +) -> Result<BackupLockGuard, Error> {
> +    let user = backup_user()?;
> +    let options = proxmox::tools::fs::CreateOptions::new()
> +        .perm(nix::sys::stat::Mode::from_bits_truncate(0o660))
> +        .owner(user.uid)
> +        .group(user.gid);
> +
> +    let timeout = timeout.unwrap_or(std::time::Duration::new(10, 0));
> +
> +    let file = proxmox::tools::fs::open_file_locked(&path, timeout, exclusive, options)?;
> +    Ok(BackupLockGuard(file))
> +}
> diff --git a/src/config/acme/plugin.rs b/src/config/acme/plugin.rs
> index 960aec3a..fde800e2 100644
> --- a/src/config/acme/plugin.rs
> +++ b/src/config/acme/plugin.rs
> @@ -12,6 +12,7 @@ use proxmox::api::{
>  use proxmox::tools::{fs::replace_file, fs::CreateOptions};
>  
>  use crate::api2::types::PROXMOX_SAFE_ID_FORMAT;
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
>  
>  pub const PLUGIN_ID_SCHEMA: Schema = StringSchema::new("ACME Challenge Plugin ID.")
>      .format(&PROXMOX_SAFE_ID_FORMAT)
> @@ -142,11 +143,10 @@ fn init() -> SectionConfig {
>  
>  const ACME_PLUGIN_CFG_FILENAME: &str = pbs_buildcfg::configdir!("/acme/plugins.cfg");
>  const ACME_PLUGIN_CFG_LOCKFILE: &str = pbs_buildcfg::configdir!("/acme/.plugins.lck");
> -const LOCK_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
>  
> -pub fn lock() -> Result<std::fs::File, Error> {
> +pub fn lock() -> Result<BackupLockGuard, Error> {
>      super::make_acme_dir()?;
> -    proxmox::tools::fs::open_file_locked(ACME_PLUGIN_CFG_LOCKFILE, LOCK_TIMEOUT, true)
> +    open_backup_lockfile(ACME_PLUGIN_CFG_LOCKFILE, None, true)
>  }
>  
>  pub fn config() -> Result<(PluginData, [u8; 32]), Error> {
> diff --git a/src/config/datastore.rs b/src/config/datastore.rs
> index d22fa316..9e37073d 100644
> --- a/src/config/datastore.rs
> +++ b/src/config/datastore.rs
> @@ -14,12 +14,12 @@ use proxmox::api::{
>  };
>  
>  use proxmox::tools::fs::{
> -    open_file_locked,
>      replace_file,
>      CreateOptions,
>  };
>  
>  use crate::api2::types::*;
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
>  
>  lazy_static! {
>      pub static ref CONFIG: SectionConfig = init();
> @@ -138,8 +138,8 @@ pub const DATASTORE_CFG_FILENAME: &str = "/etc/proxmox-backup/datastore.cfg";
>  pub const DATASTORE_CFG_LOCKFILE: &str = "/etc/proxmox-backup/.datastore.lck";
>  
>  /// Get exclusive lock
> -pub fn lock_config() -> Result<std::fs::File, Error> {
> -    open_file_locked(DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)
> +pub fn lock_config() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(DATASTORE_CFG_LOCKFILE, None, true)
>  }
>  
>  pub fn config() -> Result<(SectionConfigData, [u8;32]), Error> {
> diff --git a/src/config/domains.rs b/src/config/domains.rs
> index 775c02f3..9f513a44 100644
> --- a/src/config/domains.rs
> +++ b/src/config/domains.rs
> @@ -14,12 +14,12 @@ use proxmox::api::{
>  };
>  
>  use proxmox::tools::fs::{
> -    open_file_locked,
>      replace_file,
>      CreateOptions,
>  };
>  
>  use crate::api2::types::*;
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
>  
>  lazy_static! {
>      pub static ref CONFIG: SectionConfig = init();
> @@ -110,8 +110,8 @@ pub const DOMAINS_CFG_FILENAME: &str = "/etc/proxmox-backup/domains.cfg";
>  pub const DOMAINS_CFG_LOCKFILE: &str = "/etc/proxmox-backup/.domains.lck";
>  
>  /// Get exclusive lock
> -pub fn lock_config() -> Result<std::fs::File, Error> {
> -    open_file_locked(DOMAINS_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)
> +pub fn lock_config() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(DOMAINS_CFG_LOCKFILE, None, true)
>  }
>  
>  pub fn config() -> Result<(SectionConfigData, [u8;32]), Error> {
> diff --git a/src/config/drive.rs b/src/config/drive.rs
> index 57f6911f..9c20051f 100644
> --- a/src/config/drive.rs
> +++ b/src/config/drive.rs
> @@ -26,13 +26,13 @@ use proxmox::{
>          },
>      },
>      tools::fs::{
> -        open_file_locked,
>          replace_file,
>          CreateOptions,
>      },
>  };
>  
>  use crate::{
> +    backup::{open_backup_lockfile, BackupLockGuard},
>      api2::types::{
>          DRIVE_NAME_SCHEMA,
>          VirtualTapeDrive,
> @@ -79,8 +79,8 @@ pub const DRIVE_CFG_FILENAME: &str = "/etc/proxmox-backup/tape.cfg";
>  pub const DRIVE_CFG_LOCKFILE: &str = "/etc/proxmox-backup/.tape.lck";
>  
>  /// Get exclusive lock
> -pub fn lock() -> Result<std::fs::File, Error> {
> -    open_file_locked(DRIVE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)
> +pub fn lock() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(DRIVE_CFG_LOCKFILE, None, true)
>  }
>  
>  /// Read and parse the configuration file
> diff --git a/src/config/media_pool.rs b/src/config/media_pool.rs
> index 5ba1a6b2..e50992d8 100644
> --- a/src/config/media_pool.rs
> +++ b/src/config/media_pool.rs
> @@ -21,13 +21,13 @@ use proxmox::{
>          }
>      },
>      tools::fs::{
> -        open_file_locked,
>          replace_file,
>          CreateOptions,
>      },
>  };
>  
>  use crate::{
> +    backup::{open_backup_lockfile, BackupLockGuard},
>      api2::types::{
>          MEDIA_POOL_NAME_SCHEMA,
>          MediaPoolConfig,
> @@ -59,8 +59,8 @@ pub const MEDIA_POOL_CFG_LOCKFILE: &str = "/etc/proxmox-backup/.media-pool.lck";
>  
>  
>  /// Get exclusive lock
> -pub fn lock() -> Result<std::fs::File, Error> {
> -    open_file_locked(MEDIA_POOL_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)
> +pub fn lock() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(MEDIA_POOL_CFG_LOCKFILE, None, true)
>  }
>  
>  /// Read and parse the configuration file
> diff --git a/src/config/node.rs b/src/config/node.rs
> index 27e32cd1..dc3eeeb0 100644
> --- a/src/config/node.rs
> +++ b/src/config/node.rs
> @@ -1,6 +1,4 @@
>  use std::collections::HashSet;
> -use std::fs::File;
> -use std::time::Duration;
>  
>  use anyhow::{bail, Error};
>  use nix::sys::stat::Mode;
> @@ -14,6 +12,7 @@ use proxmox_http::ProxyConfig;
>  
>  use pbs_buildcfg::configdir;
>  
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
>  use crate::acme::AcmeClient;
>  use crate::api2::types::{
>      AcmeAccountName, AcmeDomain, ACME_DOMAIN_PROPERTY_SCHEMA, HTTP_PROXY_SCHEMA,
> @@ -21,10 +20,9 @@ use crate::api2::types::{
>  
>  const CONF_FILE: &str = configdir!("/node.cfg");
>  const LOCK_FILE: &str = configdir!("/.node.lck");
> -const LOCK_TIMEOUT: Duration = Duration::from_secs(10);
>  
> -pub fn lock() -> Result<File, Error> {
> -    proxmox::tools::fs::open_file_locked(LOCK_FILE, LOCK_TIMEOUT, true)
> +pub fn lock() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(LOCK_FILE, None, true)
>  }
>  
>  /// Read the Node Config.
> diff --git a/src/config/tape_encryption_keys.rs b/src/config/tape_encryption_keys.rs
> index e3aab1d8..5ee0ac1f 100644
> --- a/src/config/tape_encryption_keys.rs
> +++ b/src/config/tape_encryption_keys.rs
> @@ -18,12 +18,12 @@ use serde::{Deserialize, Serialize};
>  use proxmox::tools::fs::{
>      file_read_optional_string,
>      replace_file,
> -    open_file_locked,
>      CreateOptions,
>  };
>  
>  use crate::{
>      backup::{
> +        open_backup_lockfile,
>          Fingerprint,
>          KeyConfig,
>      },
> @@ -187,11 +187,7 @@ pub fn save_key_configs(map: HashMap<Fingerprint, KeyConfig>) -> Result<(), Erro
>  /// Get the lock, load both files, insert the new key, store files.
>  pub fn insert_key(key: [u8;32], key_config: KeyConfig, force: bool) -> Result<(), Error> {
>  
> -    let _lock = open_file_locked(
> -        TAPE_KEYS_LOCKFILE,
> -        std::time::Duration::new(10, 0),
> -        true,
> -    )?;
> +    let _lock = open_backup_lockfile(TAPE_KEYS_LOCKFILE, None, true)?;
>  
>      let (mut key_map, _) = load_keys()?;
>      let (mut config_map, _) = load_key_configs()?;
> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
> index 341307db..efba0c13 100644
> --- a/src/config/tfa.rs
> +++ b/src/config/tfa.rs
> @@ -4,7 +4,6 @@ use std::io::{self, Read, Seek, SeekFrom};
>  use std::os::unix::fs::OpenOptionsExt;
>  use std::os::unix::io::AsRawFd;
>  use std::path::PathBuf;
> -use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Error};
>  use nix::sys::stat::Mode;
> @@ -29,25 +28,25 @@ use proxmox::tools::AsHex;
>  use pbs_buildcfg::configdir;
>  
>  use crate::api2::types::Userid;
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
>  
>  /// Mapping of userid to TFA entry.
>  pub type TfaUsers = HashMap<Userid, TfaUserData>;
>  
>  const CONF_FILE: &str = configdir!("/tfa.json");
>  const LOCK_FILE: &str = configdir!("/tfa.json.lock");
> -const LOCK_TIMEOUT: Duration = Duration::from_secs(5);
>  
>  const CHALLENGE_DATA_PATH: &str = pbs_buildcfg::rundir!("/tfa/challenges");
>  
>  /// U2F registration challenges time out after 2 minutes.
>  const CHALLENGE_TIMEOUT: i64 = 2 * 60;
>  
> -pub fn read_lock() -> Result<File, Error> {
> -    proxmox::tools::fs::open_file_locked(LOCK_FILE, LOCK_TIMEOUT, false)
> +pub fn read_lock() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(LOCK_FILE, None, false)
>  }
>  
> -pub fn write_lock() -> Result<File, Error> {
> -    proxmox::tools::fs::open_file_locked(LOCK_FILE, LOCK_TIMEOUT, true)
> +pub fn write_lock() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(LOCK_FILE, None, true)
>  }
>  
>  /// Read the TFA entries.
> diff --git a/src/config/token_shadow.rs b/src/config/token_shadow.rs
> index 9f8bb2e0..a210ffb2 100644
> --- a/src/config/token_shadow.rs
> +++ b/src/config/token_shadow.rs
> @@ -1,18 +1,17 @@
>  use std::collections::HashMap;
> -use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Error};
>  use serde::{Serialize, Deserialize};
>  use serde_json::{from_value, Value};
>  
> -use proxmox::tools::fs::{open_file_locked, CreateOptions};
> +use proxmox::tools::fs::CreateOptions;
>  
>  use crate::api2::types::Authid;
>  use crate::auth;
> +use crate::backup::open_backup_lockfile;
>  
>  const LOCK_FILE: &str = pbs_buildcfg::configdir!("/token.shadow.lock");
>  const CONF_FILE: &str = pbs_buildcfg::configdir!("/token.shadow");
> -const LOCK_TIMEOUT: Duration = Duration::from_secs(5);
>  
>  #[derive(Serialize, Deserialize)]
>  #[serde(rename_all="kebab-case")]
> @@ -65,7 +64,7 @@ pub fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
>          bail!("not an API token ID");
>      }
>  
> -    let _guard = open_file_locked(LOCK_FILE, LOCK_TIMEOUT, true)?;
> +    let _guard = open_backup_lockfile(LOCK_FILE, None, true)?;
>  
>      let mut data = read_file()?;
>      let hashed_secret = auth::encrypt_pw(secret)?;
> @@ -81,7 +80,7 @@ pub fn delete_secret(tokenid: &Authid) -> Result<(), Error> {
>          bail!("not an API token ID");
>      }
>  
> -    let _guard = open_file_locked(LOCK_FILE, LOCK_TIMEOUT, true)?;
> +    let _guard = open_backup_lockfile(LOCK_FILE, None, true)?;
>  
>      let mut data = read_file()?;
>      data.remove(tokenid);
> diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
> index 96dd21aa..4b15fabc 100644
> --- a/src/server/jobstate.rs
> +++ b/src/server/jobstate.rs
> @@ -37,18 +37,17 @@
>  //! # }
>  //!
>  //! ```
> -use std::fs::File;
>  use std::path::{Path, PathBuf};
> -use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Error};
>  use proxmox::tools::fs::{
> -    create_path, file_read_optional_string, open_file_locked, replace_file, CreateOptions,
> +    create_path, file_read_optional_string, replace_file, CreateOptions,
>  };
>  use serde::{Deserialize, Serialize};
>  
>  use crate::{
> -   tools::systemd::time::{
> +    backup::{open_backup_lockfile, BackupLockGuard},
> +    tools::systemd::time::{
>          parse_calendar_event,
>          compute_next_event,
>      },
> @@ -83,7 +82,7 @@ pub struct Job {
>      jobname: String,
>      /// The State of the job
>      pub state: JobState,
> -    _lock: File,
> +    _lock: BackupLockGuard,
>  }
>  
>  const JOB_STATE_BASEDIR: &str = "/var/lib/proxmox-backup/jobstates";
> @@ -107,16 +106,13 @@ fn get_path(jobtype: &str, jobname: &str) -> PathBuf {
>      path
>  }
>  
> -fn get_lock<P>(path: P) -> Result<File, Error>
> +fn get_lock<P>(path: P) -> Result<BackupLockGuard, Error>
>  where
>      P: AsRef<Path>,
>  {
>      let mut path = path.as_ref().to_path_buf();
>      path.set_extension("lck");
> -    let lock = open_file_locked(&path, Duration::new(10, 0), true)?;
> -    let backup_user = crate::backup::backup_user()?;
> -    nix::unistd::chown(&path, Some(backup_user.uid), Some(backup_user.gid))?;
> -    Ok(lock)
> +    open_backup_lockfile(&path, None, true)
>  }
>  
>  /// Removes the statefile of a job, this is useful if we delete a job
> diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs
> index 13578446..8507842b 100644
> --- a/src/server/worker_task.rs
> +++ b/src/server/worker_task.rs
> @@ -14,7 +14,7 @@ use tokio::sync::oneshot;
>  
>  use proxmox::sys::linux::procfs;
>  use proxmox::try_block;
> -use proxmox::tools::fs::{create_path, open_file_locked, replace_file, CreateOptions};
> +use proxmox::tools::fs::{create_path, replace_file, CreateOptions};
>  
>  use super::{UPID, UPIDExt};
>  
> @@ -24,6 +24,7 @@ use crate::server;
>  use crate::tools::logrotate::{LogRotate, LogRotateFiles};
>  use crate::tools::{FileLogger, FileLogOptions};
>  use crate::api2::types::{Authid, TaskStateType};
> +use crate::backup::{open_backup_lockfile, BackupLockGuard};
>  
>  macro_rules! taskdir {
>      ($subdir:expr) => (concat!(pbs_buildcfg::PROXMOX_BACKUP_LOG_DIR_M!(), "/tasks", $subdir))
> @@ -313,13 +314,8 @@ pub struct TaskListInfo {
>      pub state: Option<TaskState>, // endtime, status
>  }
>  
> -fn lock_task_list_files(exclusive: bool) -> Result<std::fs::File, Error> {
> -    let backup_user = crate::backup::backup_user()?;
> -
> -    let lock = open_file_locked(PROXMOX_BACKUP_TASK_LOCK_FN, std::time::Duration::new(10, 0), exclusive)?;
> -    nix::unistd::chown(PROXMOX_BACKUP_TASK_LOCK_FN, Some(backup_user.uid), Some(backup_user.gid))?;
> -
> -    Ok(lock)
> +fn lock_task_list_files(exclusive: bool) -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(PROXMOX_BACKUP_TASK_LOCK_FN, None, exclusive)
>  }
>  
>  /// checks if the Task Archive is bigger that 'size_threshold' bytes, and
> @@ -481,7 +477,7 @@ pub struct TaskListInfoIterator {
>      list: VecDeque<TaskListInfo>,
>      end: bool,
>      archive: Option<LogRotateFiles>,
> -    lock: Option<File>,
> +    lock: Option<BackupLockGuard>,
>  }
>  
>  impl TaskListInfoIterator {
> diff --git a/src/tape/drive/mod.rs b/src/tape/drive/mod.rs
> index fb4b6f47..a6cbed84 100644
> --- a/src/tape/drive/mod.rs
> +++ b/src/tape/drive/mod.rs
> @@ -5,19 +5,21 @@ mod virtual_tape;
>  mod lto;
>  pub use lto::*;
>  
> -use std::os::unix::io::AsRawFd;
>  use std::path::PathBuf;
>  
>  use anyhow::{bail, format_err, Error};
>  use ::serde::{Deserialize};
>  use serde_json::Value;
> +use nix::fcntl::OFlag;
> +use nix::sys::stat::Mode;
>  
>  use proxmox::{
>      tools::{
>          Uuid,
>          io::ReadExt,
>          fs::{
> -            fchown,
> +            lock_file,
> +            atomic_open_or_create_file,
>              file_read_optional_string,
>              replace_file,
>              CreateOptions,
> @@ -604,20 +606,34 @@ fn tape_device_path(
>  
>  pub struct DeviceLockGuard(std::fs::File);
>  
> -// Acquires an exclusive lock on `device_path`
> -//
>  // Uses systemd escape_unit to compute a file name from `device_path`, the try
>  // to lock `/var/lock/<name>`.
> -fn lock_device_path(device_path: &str) -> Result<DeviceLockGuard, TapeLockError> {
> -
> +fn open_device_lock(device_path: &str) -> Result<std::fs::File, Error> {
>      let lock_name = crate::tools::systemd::escape_unit(device_path, true);
>  
>      let mut path = std::path::PathBuf::from(crate::tape::DRIVE_LOCK_DIR);
>      path.push(lock_name);
>  
> +    let user = crate::backup::backup_user()?;
> +    let options = CreateOptions::new()
> +        .perm(Mode::from_bits_truncate(0o660))
> +        .owner(user.uid)
> +        .group(user.gid);
> +
> +    atomic_open_or_create_file(
> +        path,
> +        OFlag::O_RDWR | OFlag::O_CLOEXEC | OFlag::O_APPEND,
> +        &[],
> +        options,
> +    )
> +}
> +
> +// Acquires an exclusive lock on `device_path`
> +//
> +fn lock_device_path(device_path: &str) -> Result<DeviceLockGuard, TapeLockError> {
> +    let mut file = open_device_lock(device_path)?; 
>      let timeout = std::time::Duration::new(10, 0);
> -    let mut file = std::fs::OpenOptions::new().create(true).append(true).open(path)?;
> -    if let Err(err) =  proxmox::tools::fs::lock_file(&mut file, true, Some(timeout)) {
> +    if let Err(err) = lock_file(&mut file, true, Some(timeout)) {
>          if err.kind() == std::io::ErrorKind::Interrupted {
>              return Err(TapeLockError::TimeOut);
>          } else {
> @@ -625,9 +641,6 @@ fn lock_device_path(device_path: &str) -> Result<DeviceLockGuard, TapeLockError>
>          }
>      }
>  
> -    let backup_user = crate::backup::backup_user()?;
> -    fchown(file.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
> -
>      Ok(DeviceLockGuard(file))
>  }
>  
> @@ -635,14 +648,10 @@ fn lock_device_path(device_path: &str) -> Result<DeviceLockGuard, TapeLockError>
>  // non-blocking, and returning if the file is locked or not
>  fn test_device_path_lock(device_path: &str) -> Result<bool, Error> {
>  
> -    let lock_name = crate::tools::systemd::escape_unit(device_path, true);
> -
> -    let mut path = std::path::PathBuf::from(crate::tape::DRIVE_LOCK_DIR);
> -    path.push(lock_name);
> +    let mut file = open_device_lock(device_path)?; 
>  
>      let timeout = std::time::Duration::new(0, 0);
> -    let mut file = std::fs::OpenOptions::new().create(true).append(true).open(path)?;
> -    match proxmox::tools::fs::lock_file(&mut file, true, Some(timeout)) {
> +    match lock_file(&mut file, true, Some(timeout)) {
>          // file was not locked, continue
>          Ok(()) => {},
>          // file was locked, return true
> @@ -650,8 +659,5 @@ fn test_device_path_lock(device_path: &str) -> Result<bool, Error> {
>          Err(err) => bail!("{}", err),
>      }
>  
> -    let backup_user = crate::backup::backup_user()?;
> -    fchown(file.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
> -
>      Ok(false)
>  }
> diff --git a/src/tape/drive/virtual_tape.rs b/src/tape/drive/virtual_tape.rs
> index 3c5f9502..f48afa79 100644
> --- a/src/tape/drive/virtual_tape.rs
> +++ b/src/tape/drive/virtual_tape.rs
> @@ -49,8 +49,9 @@ impl VirtualTapeDrive {
>              let mut lock_path = std::path::PathBuf::from(&self.path);
>              lock_path.push(".drive.lck");
>  
> +            let options = CreateOptions::new();
>              let timeout = std::time::Duration::new(10, 0);
> -            let lock = proxmox::tools::fs::open_file_locked(&lock_path, timeout, true)?;
> +            let lock = proxmox::tools::fs::open_file_locked(&lock_path, timeout, true, options)?;
>  
>              Ok(VirtualTapeHandle {
>                  _lock: lock,
> diff --git a/src/tape/inventory.rs b/src/tape/inventory.rs
> index 4bb6d4f8..e97f07f3 100644
> --- a/src/tape/inventory.rs
> +++ b/src/tape/inventory.rs
> @@ -24,8 +24,6 @@
>  
>  use std::collections::{HashMap, BTreeMap};
>  use std::path::{Path, PathBuf};
> -use std::os::unix::io::AsRawFd;
> -use std::fs::File;
>  use std::time::Duration;
>  
>  use anyhow::{bail, Error};
> @@ -35,9 +33,7 @@ use serde_json::json;
>  use proxmox::tools::{
>      Uuid,
>      fs::{
> -        open_file_locked,
>          replace_file,
> -        fchown,
>          file_get_json,
>          CreateOptions,
>      },
> @@ -51,6 +47,7 @@ use crate::{
>          MediaStatus,
>          MediaLocation,
>      },
> +    backup::{open_backup_lockfile, BackupLockGuard},
>      tape::{
>          TAPE_STATUS_DIR,
>          MediaSet,
> @@ -149,17 +146,8 @@ impl Inventory {
>      }
>  
>      /// Lock the database
> -    fn lock(&self) -> Result<std::fs::File, Error> {
> -        let file = open_file_locked(&self.lockfile_path, std::time::Duration::new(10, 0), true)?;
> -        if cfg!(test) {
> -            // We cannot use chown inside test environment (no permissions)
> -            return Ok(file);
> -        }
> -
> -        let backup_user = crate::backup::backup_user()?;
> -        fchown(file.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
> -
> -        Ok(file)
> +    fn lock(&self) -> Result<BackupLockGuard, Error> {
> +        open_backup_lockfile(&self.lockfile_path, None, true)
>      }
>  
>      fn load_media_db(path: &Path) -> Result<BTreeMap<Uuid, MediaStateEntry>, Error> {
> @@ -756,27 +744,16 @@ impl Inventory {
>  }
>  
>  /// Lock a media pool
> -pub fn lock_media_pool(base_path: &Path, name: &str) -> Result<File, Error> {
> +pub fn lock_media_pool(base_path: &Path, name: &str) -> Result<BackupLockGuard, Error> {
>      let mut path = base_path.to_owned();
>      path.push(format!(".pool-{}", name));
>      path.set_extension("lck");
>  
> -    let timeout = std::time::Duration::new(10, 0);
> -    let lock = proxmox::tools::fs::open_file_locked(&path, timeout, true)?;
> -
> -    if cfg!(test) {
> -        // We cannot use chown inside test environment (no permissions)
> -        return Ok(lock);
> -    }
> -
> -    let backup_user = crate::backup::backup_user()?;
> -    fchown(lock.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
> -
> -    Ok(lock)
> +    open_backup_lockfile(&path, None, true)
>  }
>  
>  /// Lock for media not assigned to any pool
> -pub fn lock_unassigned_media_pool(base_path: &Path)  -> Result<File, Error> {
> +pub fn lock_unassigned_media_pool(base_path: &Path)  -> Result<BackupLockGuard, Error> {
>      // lock artificial "__UNASSIGNED__" pool to avoid races
>      lock_media_pool(base_path, "__UNASSIGNED__")
>  }
> @@ -788,22 +765,12 @@ pub fn lock_media_set(
>      base_path: &Path,
>      media_set_uuid: &Uuid,
>      timeout: Option<Duration>,
> -) -> Result<File, Error> {
> +) -> Result<BackupLockGuard, Error> {
>      let mut path = base_path.to_owned();
>      path.push(format!(".media-set-{}", media_set_uuid));
>      path.set_extension("lck");
>  
> -    let timeout = timeout.unwrap_or(Duration::new(10, 0));
> -    let file = open_file_locked(&path, timeout, true)?;
> -    if cfg!(test) {
> -        // We cannot use chown inside test environment (no permissions)
> -        return Ok(file);
> -    }
> -
> -    let backup_user = crate::backup::backup_user()?;
> -    fchown(file.as_raw_fd(), Some(backup_user.uid), Some(backup_user.gid))?;
> -
> -    Ok(file)
> +    open_backup_lockfile(&path, timeout, true)
>  }
>  
>  // shell completion helper
> diff --git a/src/tape/media_pool.rs b/src/tape/media_pool.rs
> index dd471551..7bb7fbdb 100644
> --- a/src/tape/media_pool.rs
> +++ b/src/tape/media_pool.rs
> @@ -8,7 +8,6 @@
>  //!
>  
>  use std::path::{PathBuf, Path};
> -use std::fs::File;
>  
>  use anyhow::{bail, Error};
>  use ::serde::{Deserialize, Serialize};
> @@ -16,7 +15,7 @@ use ::serde::{Deserialize, Serialize};
>  use proxmox::tools::Uuid;
>  
>  use crate::{
> -    backup::Fingerprint,
> +    backup::{Fingerprint, BackupLockGuard},
>      api2::types::{
>          MediaStatus,
>          MediaLocation,
> @@ -61,7 +60,7 @@ pub struct MediaPool {
>      inventory: Inventory,
>  
>      current_media_set: MediaSet,
> -    current_media_set_lock: Option<File>,
> +    current_media_set_lock: Option<BackupLockGuard>,
>  }
>  
>  impl MediaPool {
> diff --git a/src/tools/memcom.rs b/src/tools/memcom.rs
> index 9921f5c9..87b73561 100644
> --- a/src/tools/memcom.rs
> +++ b/src/tools/memcom.rs
> @@ -1,21 +1,17 @@
>  //! Memory based communication channel between proxy & daemon for things such as cache
>  //! invalidation.
>  
> -use std::ffi::CString;
> -use std::io;
>  use std::os::unix::io::AsRawFd;
>  use std::sync::atomic::{AtomicUsize, Ordering};
>  use std::sync::Arc;
>  
> -use anyhow::{bail, format_err, Error};
> -use nix::errno::Errno;
> +use anyhow::Error;
>  use nix::fcntl::OFlag;
>  use nix::sys::mman::{MapFlags, ProtFlags};
>  use nix::sys::stat::Mode;
>  use once_cell::sync::OnceCell;
>  
> -use proxmox::sys::error::SysError;
> -use proxmox::tools::fd::Fd;
> +use proxmox::tools::fs::CreateOptions;
>  use proxmox::tools::mmap::Mmap;
>  
>  /// In-memory communication channel.
> @@ -32,6 +28,7 @@ struct Head {
>  static INSTANCE: OnceCell<Arc<Memcom>> = OnceCell::new();
>  
>  const MEMCOM_FILE_PATH: &str = pbs_buildcfg::rundir!("/proxmox-backup-memcom");
> +const EMPTY_PAGE: [u8; 4096] = [0u8; 4096];
>  
>  impl Memcom {
>      /// Open the memory based communication channel singleton.
> @@ -41,15 +38,20 @@ impl Memcom {
>  
>      // Actual work of `new`:
>      fn open() -> Result<Arc<Self>, Error> {
> -        let fd = match open_existing() {
> -            Ok(fd) => fd,
> -            Err(err) if err.not_found() => create_new()?,
> -            Err(err) => bail!("failed to open {} - {}", MEMCOM_FILE_PATH, err),
> -        };
> +        let user = crate::backup::backup_user()?;
> +        let options = CreateOptions::new()
> +            .perm(Mode::from_bits_truncate(0o660))
> +            .owner(user.uid)
> +            .group(user.gid);
> +
> +        let file = proxmox::tools::fs::atomic_open_or_create_file(
> +            MEMCOM_FILE_PATH,
> +            OFlag::O_RDWR | OFlag::O_CLOEXEC,
> +            &EMPTY_PAGE, options)?;
>  
>          let mmap = unsafe {
>              Mmap::<u8>::map_fd(
> -                fd.as_raw_fd(),
> +                file.as_raw_fd(),
>                  0,
>                  4096,
>                  ProtFlags::PROT_READ | ProtFlags::PROT_WRITE,
> @@ -77,87 +79,3 @@ impl Memcom {
>              .fetch_add(1, Ordering::AcqRel);
>      }
>  }
> -
> -/// The fast path opens an existing file.
> -fn open_existing() -> Result<Fd, nix::Error> {
> -    Fd::open(
> -        MEMCOM_FILE_PATH,
> -        OFlag::O_RDWR | OFlag::O_CLOEXEC,
> -        Mode::empty(),
> -    )
> -}
> -
> -/// Since we need to initialize the file, we also need a solid slow path where we create the file.
> -/// In order to make sure the next user's `open()` vs `mmap()` race against our `truncate()` call,
> -/// we create it in a temporary location and rotate it in place.
> -fn create_new() -> Result<Fd, Error> {
> -    // create a temporary file:
> -    let temp_file_name = format!("{}.{}", MEMCOM_FILE_PATH, unsafe { libc::getpid() });
> -    let fd = Fd::open(
> -        temp_file_name.as_str(),
> -        OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_RDWR | OFlag::O_CLOEXEC,
> -        Mode::from_bits_truncate(0o660),
> -    )
> -    .map_err(|err| {
> -        format_err!(
> -            "failed to create new in-memory communication file at {} - {}",
> -            temp_file_name,
> -            err
> -        )
> -    })?;
> -
> -    // let it be a page in size, it'll be initialized to zero by the kernel
> -    nix::unistd::ftruncate(fd.as_raw_fd(), 4096)
> -        .map_err(|err| format_err!("failed to set size of {} - {}", temp_file_name, err))?;
> -
> -    // if this is the pbs-daemon (running as root) rather than the proxy (running as backup user),
> -    // make sure the backup user can access the file:
> -    if let Ok(backup_user) = crate::backup::backup_user() {
> -        match nix::unistd::fchown(fd.as_raw_fd(), None, Some(backup_user.gid)) {
> -            Ok(()) => (),
> -            Err(err) if err.is_errno(Errno::EPERM) => {
> -                // we're not the daemon (root), so the file is already owned by the backup user
> -            }
> -            Err(err) => bail!(
> -                "failed to set group to 'backup' for {} - {}",
> -                temp_file_name,
> -                err
> -            ),
> -        }
> -    }
> -
> -    // rotate the file into place, but use `RENAME_NOREPLACE`, so in case 2 processes race against
> -    // the initialization, the first one wins!
> -    // TODO: nicer `renameat2()` wrapper in `proxmox::sys`?
> -    let c_file_name = CString::new(temp_file_name.as_bytes()).unwrap();
> -    let new_path = CString::new(MEMCOM_FILE_PATH).unwrap();
> -    let rc = unsafe {
> -        libc::renameat2(
> -            -1,
> -            c_file_name.as_ptr(),
> -            -1,
> -            new_path.as_ptr(),
> -            libc::RENAME_NOREPLACE,
> -        )
> -    };
> -    if rc == 0 {
> -        return Ok(fd);
> -    }
> -    let err = io::Error::last_os_error();
> -
> -    // if another process has already raced ahead and created the file, let's just open theirs
> -    // instead:
> -    if err.kind() == io::ErrorKind::AlreadyExists {
> -        // someone beat us to it:
> -        drop(fd);
> -        return open_existing().map_err(Error::from);
> -    }
> -
> -    // for any other errors, just bail out
> -    bail!(
> -        "failed to move file at {} into place at {} - {}",
> -        temp_file_name,
> -        MEMCOM_FILE_PATH,
> -        err
> -    );
> -}
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 





More information about the pbs-devel mailing list