* [pbs-devel] [PATCH backup v2 0/2] fix #5946: disks: wipe: ensure GPT header backup is wiped @ 2025-02-11 16:26 Filip Schauer 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 1/2] " Filip Schauer 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB Filip Schauer 0 siblings, 2 replies; 8+ messages in thread From: Filip Schauer @ 2025-02-11 16:26 UTC (permalink / raw) To: pbs-devel When wiping a block device with a GUID partition table, the header backup might get left behind at the end of the disk. This commit also wipes the last 4096 bytes of the disk, making sure that a GPT header backup is erased, even from disks with 4k sector sizes. Also lower the number of bytes that is zeroed out at the start of the disk from 200 MiB to 1 MiB. Changed since v1: * Use `std::os::unix::fs::FileExt::write_all_at` instead of calling `dd` * only zero out the first 1 MiB Filip Schauer (2): fix #5946: disks: wipe: ensure GPT header backup is wiped disks: wipe: only zero out the first 1 MiB src/tools/disks/mod.rs | 49 +++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 22 deletions(-) -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH backup v2 1/2] fix #5946: disks: wipe: ensure GPT header backup is wiped 2025-02-11 16:26 [pbs-devel] [PATCH backup v2 0/2] fix #5946: disks: wipe: ensure GPT header backup is wiped Filip Schauer @ 2025-02-11 16:26 ` Filip Schauer 2025-02-11 18:39 ` Thomas Lamprecht 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB Filip Schauer 1 sibling, 1 reply; 8+ messages in thread From: Filip Schauer @ 2025-02-11 16:26 UTC (permalink / raw) To: pbs-devel; +Cc: Wolfgang Bumiller When wiping a block device with a GUID partition table, the header backup might get left behind at the end of the disk. This commit also wipes the last 4096 bytes of the disk, making sure that a GPT header backup is erased, even from disks with 4k sector sizes. Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com> Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- src/tools/disks/mod.rs | 49 +++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs index 571446db..ad9df0b5 100644 --- a/src/tools/disks/mod.rs +++ b/src/tools/disks/mod.rs @@ -4,11 +4,11 @@ use std::collections::{HashMap, HashSet}; use std::ffi::{OsStr, OsString}; use std::io; use std::os::unix::ffi::{OsStrExt, OsStringExt}; -use std::os::unix::fs::MetadataExt; +use std::os::unix::fs::{FileExt, MetadataExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; use std::sync::{Arc, LazyLock}; -use anyhow::{bail, format_err, Error}; +use anyhow::{bail, format_err, Context as _, Error}; use libc::dev_t; use once_cell::sync::OnceCell; @@ -1145,7 +1145,7 @@ pub fn inititialize_gpt_disk(disk: &Disk, uuid: Option<&str>) -> Result<(), Erro Ok(()) } -/// Wipes all labels and the first 200 MiB of a disk/partition (or the whole if it is smaller). +/// Wipes all labels, the first 200 MiB and the last 4096 bytes of a disk/partition. /// If called with a partition, also sets the partition type to 0x83 'Linux filesystem'. pub fn wipe_blockdev(disk: &Disk) -> Result<(), Error> { let disk_path = match disk.device_path() { @@ -1176,25 +1176,7 @@ pub fn wipe_blockdev(disk: &Disk) -> Result<(), Error> { let wipefs_output = proxmox_sys::command::run_command(wipefs_command, None)?; info!("wipefs output: {wipefs_output}"); - let size = disk.size().map(|size| size / 1024 / 1024)?; - let count = size.min(200); - - let mut dd_command = std::process::Command::new("dd"); - let mut of_path = OsString::from("of="); - of_path.push(disk_path); - let mut count_str = OsString::from("count="); - count_str.push(count.to_string()); - let args = [ - "if=/dev/zero".into(), - of_path, - "bs=1M".into(), - "conv=fdatasync".into(), - count_str, - ]; - dd_command.args(args); - - let dd_output = proxmox_sys::command::run_command(dd_command, None)?; - info!("dd output: {dd_output}"); + zero_disk_start_and_end(disk)?; if is_partition { // set the partition type to 0x83 'Linux filesystem' @@ -1204,6 +1186,29 @@ pub fn wipe_blockdev(disk: &Disk) -> Result<(), Error> { Ok(()) } +pub fn zero_disk_start_and_end(disk: &Disk) -> Result<(), Error> { + let disk_path = match disk.device_path() { + Some(path) => path, + None => bail!("disk {:?} has no node in /dev", disk.syspath()), + }; + + let disk_size = disk.size()?; + let file = std::fs::OpenOptions::new() + .write(true) + .custom_flags(libc::O_CLOEXEC | libc::O_DSYNC) + .open(disk_path) + .with_context(|| "failed to open device {disk_path:?} for writing")?; + let write_size = disk_size.min(200 * 1024 * 1024); + let zeroes = proxmox_io::boxed::zeroed(write_size as usize); + file.write_all_at(&zeroes, 0) + .with_context(|| "failed to wipe start of device {disk_path:?}")?; + if disk_size > write_size { + file.write_all_at(&zeroes[0..4096], disk_size - 4096) + .with_context(|| "failed to wipe end of device {disk_path:?}")?; + } + Ok(()) +} + pub fn change_parttype(part_disk: &Disk, part_type: &str) -> Result<(), Error> { let part_path = match part_disk.device_path() { Some(path) => path, -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH backup v2 1/2] fix #5946: disks: wipe: ensure GPT header backup is wiped 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 1/2] " Filip Schauer @ 2025-02-11 18:39 ` Thomas Lamprecht 2025-02-12 11:01 ` Filip Schauer 0 siblings, 1 reply; 8+ messages in thread From: Thomas Lamprecht @ 2025-02-11 18:39 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Filip Schauer Cc: Wolfgang Bumiller Am 11.02.25 um 17:26 schrieb Filip Schauer: > When wiping a block device with a GUID partition table, the header > backup might get left behind at the end of the disk. This commit also > wipes the last 4096 bytes of the disk, making sure that a GPT header > backup is erased, even from disks with 4k sector sizes. change-wise I'd have preferred switching over from dd to writing ourself in a first patch and then adding the code for clearing the GPT backup header in a second patch, that's much easier to process and review compared to mixing those two things. The resulting code itself looks OK to me though. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH backup v2 1/2] fix #5946: disks: wipe: ensure GPT header backup is wiped 2025-02-11 18:39 ` Thomas Lamprecht @ 2025-02-12 11:01 ` Filip Schauer 0 siblings, 0 replies; 8+ messages in thread From: Filip Schauer @ 2025-02-12 11:01 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox Backup Server development discussion Cc: Wolfgang Bumiller Superseded by: https://lore.proxmox.com/pbs-devel/20250212105855.43853-1-f.schauer@proxmox.com/ On 11/02/2025 19:39, Thomas Lamprecht wrote: > change-wise I'd have preferred switching over from dd to writing ourself > in a first patch and then adding the code for clearing the GPT backup header > in a second patch, that's much easier to process and review compared to > mixing those two things. Done in v3 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB 2025-02-11 16:26 [pbs-devel] [PATCH backup v2 0/2] fix #5946: disks: wipe: ensure GPT header backup is wiped Filip Schauer 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 1/2] " Filip Schauer @ 2025-02-11 16:26 ` Filip Schauer 2025-02-11 18:42 ` Thomas Lamprecht 1 sibling, 1 reply; 8+ messages in thread From: Filip Schauer @ 2025-02-11 16:26 UTC (permalink / raw) To: pbs-devel Reduce the number of MiB zeroed out at the start of the disk from 200 to 1. This should still be sufficient to remove any remaining signatures missed by `wipefs`, preventing potential interference with later use of the disk. Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- src/tools/disks/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs index ad9df0b5..deee60a6 100644 --- a/src/tools/disks/mod.rs +++ b/src/tools/disks/mod.rs @@ -1145,7 +1145,7 @@ pub fn inititialize_gpt_disk(disk: &Disk, uuid: Option<&str>) -> Result<(), Erro Ok(()) } -/// Wipes all labels, the first 200 MiB and the last 4096 bytes of a disk/partition. +/// Wipes all labels, the first 1 MiB and the last 4096 bytes of a disk/partition. /// If called with a partition, also sets the partition type to 0x83 'Linux filesystem'. pub fn wipe_blockdev(disk: &Disk) -> Result<(), Error> { let disk_path = match disk.device_path() { @@ -1198,7 +1198,7 @@ pub fn zero_disk_start_and_end(disk: &Disk) -> Result<(), Error> { .custom_flags(libc::O_CLOEXEC | libc::O_DSYNC) .open(disk_path) .with_context(|| "failed to open device {disk_path:?} for writing")?; - let write_size = disk_size.min(200 * 1024 * 1024); + let write_size = disk_size.min(1024 * 1024); let zeroes = proxmox_io::boxed::zeroed(write_size as usize); file.write_all_at(&zeroes, 0) .with_context(|| "failed to wipe start of device {disk_path:?}")?; -- 2.39.5 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB Filip Schauer @ 2025-02-11 18:42 ` Thomas Lamprecht 2025-02-12 10:24 ` Filip Schauer 0 siblings, 1 reply; 8+ messages in thread From: Thomas Lamprecht @ 2025-02-11 18:42 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Filip Schauer Am 11.02.25 um 17:26 schrieb Filip Schauer: > Reduce the number of MiB zeroed out at the start of the disk from 200 to > 1. This should still be sufficient to remove any remaining signatures > missed by `wipefs`, preventing potential interference with later use of > the disk. > 200 MiB is a bit to specific to have no reason behind it, did you check up on that? Maybe something can be found in PVE's git/patch history. I have some faint remembrance that we had to increase the area we zero out for ceph OSDs and/or for the installer. Or do you have any references for why this should still be sufficient? _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB 2025-02-11 18:42 ` Thomas Lamprecht @ 2025-02-12 10:24 ` Filip Schauer 2025-02-12 13:44 ` Thomas Lamprecht 0 siblings, 1 reply; 8+ messages in thread From: Filip Schauer @ 2025-02-12 10:24 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox Backup Server development discussion On 11/02/2025 19:42, Thomas Lamprecht wrote: > I have some faint remembrance that we had to increase the area we zero > out for ceph OSDs and/or for the installer. You are right. We zero out the first 200 MiB to prevent a problem with OSD creation. [0] [1] Although I am unable to reproduce this on Ceph Quincy, making sure the disk is properly wiped does not hurt. So we can drop this patch. [0] https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=1b3caf4f2153bff3c0db7726275a571fbd1f07e0 [1] https://tracker.ceph.com/issues/22354 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB 2025-02-12 10:24 ` Filip Schauer @ 2025-02-12 13:44 ` Thomas Lamprecht 0 siblings, 0 replies; 8+ messages in thread From: Thomas Lamprecht @ 2025-02-12 13:44 UTC (permalink / raw) To: Filip Schauer, Proxmox Backup Server development discussion Am 12.02.25 um 11:24 schrieb Filip Schauer: > On 11/02/2025 19:42, Thomas Lamprecht wrote: >> I have some faint remembrance that we had to increase the area we zero >> out for ceph OSDs and/or for the installer. > > You are right. We zero out the first 200 MiB to prevent a problem with > OSD creation. [0] [1] Although I am unable to reproduce this on Ceph > Quincy, making sure the disk is properly wiped does not hurt. So we can > drop this patch. Thanks for checking, seems like Ceph nowadays writes out labels on quite a few more places [0] depending on the block dev size; albeit only once in the range of 0 to 200 MB. I checked and this is relatively new though (commit from January 2024 [1]) and only included in Squid 19.2.1 FWICT. [0]: https://github.com/ceph/ceph/blob/main/src/os/bluestore/BlueStore.cc#L138-L146 [1]: https://github.com/ceph/ceph/commit/4b8197b9931e1597b5aa3b02660a9b77ea4aa124 FWIW, I'm not completely opposed in doing less writes, if the block device is backed by some thin provisioned storage it might be also thankful if it has less data to explicitly track (albeit zeros are often special cased and as such relatively cheap most of the time). If we ever reuse this code for PVE it would need to become opt-in though so that we can enable it for the cases like wiping in preparation of creating a Ceph OSD. But I saw you send a v3 already with this left out, which is IMO fine too for now. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-02-12 13:44 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-02-11 16:26 [pbs-devel] [PATCH backup v2 0/2] fix #5946: disks: wipe: ensure GPT header backup is wiped Filip Schauer 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 1/2] " Filip Schauer 2025-02-11 18:39 ` Thomas Lamprecht 2025-02-12 11:01 ` Filip Schauer 2025-02-11 16:26 ` [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB Filip Schauer 2025-02-11 18:42 ` Thomas Lamprecht 2025-02-12 10:24 ` Filip Schauer 2025-02-12 13:44 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inboxService provided by Proxmox Server Solutions GmbH | Privacy | Legal