[pbs-devel] [PATCH backup v2 0/2] fix #5946: disks: wipe: ensure GPT header backup is wiped

[pbs-devel] [PATCH backup v2 0/2] fix #5946: disks: wipe: ensure GPT header backup is wiped

[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.

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

[pbs-devel] [PATCH backup v2 1/2] fix #5946: disks: wipe: ensure GPT header backup is wiped

[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.

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

Re: [pbs-devel] [PATCH backup v2 1/2] fix #5946: disks: wipe: ensure GPT header backup is wiped

[Thomas Lamprecht]

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

Re: [pbs-devel] [PATCH backup v2 1/2] fix #5946: disks: wipe: ensure GPT header backup is wiped

[Filip Schauer]

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

[pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB

[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.

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

Re: [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB

[Thomas Lamprecht]

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

Re: [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB

[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.

[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

Re: [pbs-devel] [PATCH backup v2 2/2] disks: wipe: only zero out the first 1 MiB

[Thomas Lamprecht]

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