* [pbs-devel] [PATCH backup] fix #5946: disks: wipe: ensure GPT header backup is wiped @ 2024-12-04 10:33 Filip Schauer 2025-02-06 15:01 ` Wolfgang Bumiller 0 siblings, 1 reply; 3+ messages in thread From: Filip Schauer @ 2024-12-04 10:33 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. Signed-off-by: Filip Schauer <f.schauer@proxmox.com> --- src/tools/disks/mod.rs | 47 +++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs index 61aceccd..26e14603 100644 --- a/src/tools/disks/mod.rs +++ b/src/tools/disks/mod.rs @@ -1176,29 +1176,38 @@ 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}"); + let dd_zero = |seek: Option<&str>, bs: &str, count: &str| -> Result<(), Error> { + let mut dd_command = std::process::Command::new("dd"); + let mut of_path = OsString::from("of="); + of_path.push(disk_path); + dd_command + .arg("if=/dev/zero") + .arg(of_path) + .arg(format!("bs={bs}")) + .arg("conv=fdatasync") + .arg(format!("count={count}")); + + if let Some(seek) = seek { + dd_command.arg(format!("seek={seek}")); + } + + let dd_output = proxmox_sys::command::run_command(dd_command, None)?; + info!("dd output: {dd_output}"); + + Ok(()) + }; + + let size = disk.size()?; + let count = (size / 1024 / 1024).min(200).to_string(); + dd_zero(None, "1M", &count)?; if is_partition { // set the partition type to 0x83 'Linux filesystem' change_parttype(disk, "8300")?; + } else { + // Wipe the end of the disk to remove a potential GPT header backup + let seek = (size / 512 - 8).to_string(); + dd_zero(Some(&seek), "512", "8")?; } Ok(()) -- 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] 3+ messages in thread
* Re: [pbs-devel] [PATCH backup] fix #5946: disks: wipe: ensure GPT header backup is wiped 2024-12-04 10:33 [pbs-devel] [PATCH backup] fix #5946: disks: wipe: ensure GPT header backup is wiped Filip Schauer @ 2025-02-06 15:01 ` Wolfgang Bumiller 2025-02-11 16:29 ` Filip Schauer 0 siblings, 1 reply; 3+ messages in thread From: Wolfgang Bumiller @ 2025-02-06 15:01 UTC (permalink / raw) To: Filip Schauer; +Cc: pbs-devel On Wed, Dec 04, 2024 at 11:33:31AM +0100, Filip Schauer wrote: > 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. > > Signed-off-by: Filip Schauer <f.schauer@proxmox.com> > --- > src/tools/disks/mod.rs | 47 +++++++++++++++++++++++++----------------- > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs > index 61aceccd..26e14603 100644 > --- a/src/tools/disks/mod.rs > +++ b/src/tools/disks/mod.rs > @@ -1176,29 +1176,38 @@ 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}"); > + let dd_zero = |seek: Option<&str>, bs: &str, count: &str| -> Result<(), Error> { > + let mut dd_command = std::process::Command::new("dd"); > + let mut of_path = OsString::from("of="); > + of_path.push(disk_path); > + dd_command > + .arg("if=/dev/zero") > + .arg(of_path) > + .arg(format!("bs={bs}")) > + .arg("conv=fdatasync") > + .arg(format!("count={count}")); > + > + if let Some(seek) = seek { > + dd_command.arg(format!("seek={seek}")); > + } > + > + let dd_output = proxmox_sys::command::run_command(dd_command, None)?; > + info!("dd output: {dd_output}"); > + > + Ok(()) > + }; > + > + let size = disk.size()?; > + let count = (size / 1024 / 1024).min(200).to_string(); > + dd_zero(None, "1M", &count)?; > > if is_partition { > // set the partition type to 0x83 'Linux filesystem' > change_parttype(disk, "8300")?; > + } else { > + // Wipe the end of the disk to remove a potential GPT header backup > + let seek = (size / 512 - 8).to_string(); > + dd_zero(Some(&seek), "512", "8")?; > } > > Ok(()) > -- > 2.39.5 I don't know why we call out to dd for simple writes. I'd suggest just using `write_all_at` like so: ---8<--- diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs index 571446db..e22691b7 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; @@ -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_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_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(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 > 4096 { + 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, _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pbs-devel] [PATCH backup] fix #5946: disks: wipe: ensure GPT header backup is wiped 2025-02-06 15:01 ` Wolfgang Bumiller @ 2025-02-11 16:29 ` Filip Schauer 0 siblings, 0 replies; 3+ messages in thread From: Filip Schauer @ 2025-02-11 16:29 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pbs-devel Superseded by: https://lore.proxmox.com/pbs-devel/20250211162639.141541-1-f.schauer@proxmox.com/ On 06/02/2025 16:01, Wolfgang Bumiller wrote: > On Wed, Dec 04, 2024 at 11:33:31AM +0100, Filip Schauer wrote: >> 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. >> >> Signed-off-by: Filip Schauer <f.schauer@proxmox.com> >> --- >> src/tools/disks/mod.rs | 47 +++++++++++++++++++++++++----------------- >> 1 file changed, 28 insertions(+), 19 deletions(-) >> >> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs >> index 61aceccd..26e14603 100644 >> --- a/src/tools/disks/mod.rs >> +++ b/src/tools/disks/mod.rs >> @@ -1176,29 +1176,38 @@ 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}"); >> + let dd_zero = |seek: Option<&str>, bs: &str, count: &str| -> Result<(), Error> { >> + let mut dd_command = std::process::Command::new("dd"); >> + let mut of_path = OsString::from("of="); >> + of_path.push(disk_path); >> + dd_command >> + .arg("if=/dev/zero") >> + .arg(of_path) >> + .arg(format!("bs={bs}")) >> + .arg("conv=fdatasync") >> + .arg(format!("count={count}")); >> + >> + if let Some(seek) = seek { >> + dd_command.arg(format!("seek={seek}")); >> + } >> + >> + let dd_output = proxmox_sys::command::run_command(dd_command, None)?; >> + info!("dd output: {dd_output}"); >> + >> + Ok(()) >> + }; >> + >> + let size = disk.size()?; >> + let count = (size / 1024 / 1024).min(200).to_string(); >> + dd_zero(None, "1M", &count)?; >> >> if is_partition { >> // set the partition type to 0x83 'Linux filesystem' >> change_parttype(disk, "8300")?; >> + } else { >> + // Wipe the end of the disk to remove a potential GPT header backup >> + let seek = (size / 512 - 8).to_string(); >> + dd_zero(Some(&seek), "512", "8")?; >> } >> >> Ok(()) >> -- >> 2.39.5 > I don't know why we call out to dd for simple writes. > I'd suggest just using `write_all_at` like so: > > ---8<--- > diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs > index 571446db..e22691b7 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; > > @@ -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_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_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(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 > 4096 { > + 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, _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-02-11 16:29 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-12-04 10:33 [pbs-devel] [PATCH backup] fix #5946: disks: wipe: ensure GPT header backup is wiped Filip Schauer 2025-02-06 15:01 ` Wolfgang Bumiller 2025-02-11 16:29 ` Filip Schauer
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