* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.Service provided by Proxmox Server Solutions GmbH | Privacy | Legal