From: Stefan Reiter <s.reiter@proxmox.com>
To: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>,
"Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH proxmox-backup-qemu] fix #2866: invalidate bitmap on crypt_mode change
Date: Thu, 23 Jul 2020 12:43:07 +0200 [thread overview]
Message-ID: <2141b2d4-df90-1235-7755-83c1440911c2@proxmox.com> (raw)
In-Reply-To: <1595499980.xeb6wkgs4y.astroid@nora.none>
On 7/23/20 12:34 PM, Fabian Grünbichler wrote:
> On July 23, 2020 12:07 pm, Stefan Reiter wrote:
>> idea looks ok, comments inline
>>
>> On 7/23/20 11:21 AM, Fabian Grünbichler wrote:
>>> signed and plain backups share chunks, so bitmap reusal is okay for
>>> those combinations. switching from encrypted to not encrypted or
>>> vice-versa could have pretty fatal consequences - either referencing
>>> plain-text chunks in 'encrypted' backups, or referencing encrypted
>>> chunks in 'unencrypted' backups without still having the corresponding
>>> keys..
>>>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>>
>>> Notes:
>>> requires recent proxmox-backup with public lookup_file_info
>>>
>>> src/backup.rs | 3 ++-
>>> src/commands.rs | 35 +++++++++++++++++++++++++++++++++--
>>> 2 files changed, 35 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/backup.rs b/src/backup.rs
>>> index 717e099..b8108ef 100644
>>> --- a/src/backup.rs
>>> +++ b/src/backup.rs
>>> @@ -202,7 +202,8 @@ impl BackupTask {
>>> device_name: String,
>>> size: u64,
>>> ) -> bool {
>>> - check_last_incremental_csum(self.last_manifest(), device_name, size)
>>> + check_last_incremental_csum(self.last_manifest(), &device_name, size)
>>> + && check_last_encryption_mode(self.last_manifest(), &device_name, self.crypt_mode)
>>> }
>>>
>>> pub async fn register_image(
>>> diff --git a/src/commands.rs b/src/commands.rs
>>> index 6f26324..8d8f2a7 100644
>>> --- a/src/commands.rs
>>> +++ b/src/commands.rs
>>> @@ -80,7 +80,7 @@ pub(crate) async fn add_config(
>>>
>>> pub(crate) fn check_last_incremental_csum(
>>> manifest: Option<Arc<BackupManifest>>,
>>> - device_name: String,
>>> + device_name: &str,
>>> device_size: u64,
>>> ) -> bool {
>>>
>>> @@ -91,12 +91,43 @@ pub(crate) fn check_last_incremental_csum(
>>>
>>> let archive_name = format!("{}.img.fidx", device_name);
>>>
>>> - match PREVIOUS_CSUMS.lock().unwrap().get(&device_name) {
>>> + match PREVIOUS_CSUMS.lock().unwrap().get(device_name) {
>>> Some(csum) => manifest.verify_file(&archive_name, &csum, device_size).is_ok(),
>>> None => false,
>>> }
>>> }
>>>
>>> +pub(crate) fn check_last_encryption_mode(
>>> + manifest: Option<Arc<BackupManifest>>,
>>> + device_name: &str,
>>> + crypt_mode: CryptMode,
>>> +) -> bool {
>>> +
>>> + let manifest = match manifest {
>>> + Some(ref manifest) => manifest,
>>> + None => return false,
>>> + };
>>
>> this...
>>
>>> +
>>> + let archive_name = format!("{}.img.fidx", device_name);
>>
>> ...and this could probably be moved to check_incremental to avoid
>> duplication.
>
> probably device to archive name could also be refactored into a helper?
> with this patch we have three identical format! calls..
>
would make sense, or at least encode the .img.fidx in a constant somewhere
>>
>>> + match manifest.lookup_file_info(&archive_name) {
>>> + Ok(file) => {
>>> + eprintln!("device {} last mode: {:?} current mode {:?}", device_name, file.crypt_mode, crypt_mode);
>>
>> left over debug print or intentional? this would be hidden atm, as we
>> don't track QEMU output anywhere.
>
> both :-P I figured with all the issues we had with encrypted backups,
> telling users to start in the foreground and watch the output might be
> helpful. but I'm fine with dropping it.
>
I suppose this would be a good point to ping this patch of mine:
https://lists.proxmox.com/pipermail/pve-devel/2020-June/044143.html
Though in case we want to actually use it this way, maybe even a bit
more logging would be good?
>>
>>> + match file.crypt_mode {
>>> + CryptMode::Encrypt => match crypt_mode {
>>> + CryptMode::Encrypt => true,
>>> + _ => false,
>>> + },
>>> + CryptMode::SignOnly | CryptMode::None => match crypt_mode {
>>
>> you can use the _ match here too, same as in the inner match call.
>
> intentional, if we add a new CryptMode in proxmox-backup this forces us
> to match it here unless I misunderstood how match on enums works in
> Rust.
>
makes sense, though should probably be mentioned somewhere so no one
"optimizes" it away in the future.
>>
>>> + CryptMode::Encrypt => false,
>>> + _ => true,
>>> + },
>>> + }
>>> + },
>>> + _ => false,
>>> + }
>>> +}
>>> +
>>> +
>>> pub(crate) async fn register_image(
>>> client: Arc<BackupWriter>,
>>> crypt_config: Option<Arc<CryptConfig>>,
>>>
>>
next prev parent reply other threads:[~2020-07-23 10:43 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-23 9:21 Fabian Grünbichler
2020-07-23 10:07 ` Stefan Reiter
2020-07-23 10:34 ` Fabian Grünbichler
2020-07-23 10:43 ` Stefan Reiter [this message]
2020-07-23 11:09 ` Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2141b2d4-df90-1235-7755-83c1440911c2@proxmox.com \
--to=s.reiter@proxmox.com \
--cc=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox