* [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
* [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 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 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 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
* 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 inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal