From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 7FF091FF15E
for <inbox@lore.proxmox.com>; Tue, 25 Mar 2025 11:01:08 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 6670358B9;
Tue, 25 Mar 2025 11:01:03 +0100 (CET)
Message-ID: <5ace7ae9-232b-42d3-b48b-9c4b44bdbe45@proxmox.com>
Date: Tue, 25 Mar 2025 11:00:30 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Shannon Sterz <s.sterz@proxmox.com>
References: <20250324125157.165976-1-s.sterz@proxmox.com>
<20250324125157.165976-5-s.sterz@proxmox.com>
Content-Language: en-US, de-DE
From: Christian Ebner <c.ebner@proxmox.com>
In-Reply-To: <20250324125157.165976-5-s.sterz@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.031 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 v8 4/4] fix: api: avoid race
condition in set_backup_owner
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
On 3/24/25 13:51, Shannon Sterz wrote:
> when two clients change the owner of a backup store, a race condition
> arose. add locking to avoid this.
>
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
> src/api2/admin/datastore.rs | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 483e595c..3e532636 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -7,7 +7,7 @@ use std::os::unix::ffi::OsStrExt;
> use std::path::{Path, PathBuf};
> use std::sync::Arc;
>
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, format_err, Context, Error};
> use futures::*;
> use hyper::http::request::Parts;
> use hyper::{header, Body, Response, StatusCode};
> @@ -2347,10 +2347,9 @@ pub async fn set_backup_owner(
> let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
>
> let backup_group = datastore.backup_group(ns, backup_group);
> + let owner = backup_group.get_owner()?;
this needs to read the content from the owner file
>
> if owner_check_required {
> - let owner = backup_group.get_owner()?;
> -
> let allowed = match (owner.is_token(), new_owner.is_token()) {
> (true, true) => {
> // API token to API token, owned by same user
> @@ -2397,6 +2396,14 @@ pub async fn set_backup_owner(
> );
> }
>
> + let _guard = backup_group
> + .lock()
> + .with_context(|| format!("while setting the owner of group '{backup_group:?}'"))?;
> +
> + if owner != backup_group.get_owner()? {
same here.
> + bail!("{owner} does not own this group anymore");
> + }
> +
> backup_group.set_owner(&new_owner, true)?;
>
> Ok(())
reading from the filesystem is more expensive than the ownership checks
if required, so IMO it is better to lock the whole operation instead of
reading the owner file twice. Or is this done to avoid locking over and
over by unauthorized users?
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel