lvutil: use wipefs not dd to clear existing signatures (fixes #624)

[ydirson]

This PR addresses #624.

With a ZFS fs in nvme0n1p3, originally:

[11:19 xcp-ng-bzkcpvhy sm]# xe sr-create name-label=ydi-local type=ext device-config:device=/dev/nvme0n1p3 
Error code: SR_BACKEND_FAILURE_77
Error parameters: , Logical Volume group creation failed, 

SMlog:

Jul  6 11:20:10 xcp-ng-bzkcpvhy SM: [6624] ['/sbin/vgcreate', '--metadatasize', '10M', 'XSLocalEXT-d98c6847-446e-a962-630c-a82346a73c5b', '/dev/nvme0n1p3']
Jul  6 11:20:11 xcp-ng-bzkcpvhy SM: [6624] FAILED in util.pread: (rc 5) stdout: '', stderr: 'WARNING: zfs_member signature detected on /dev/nvme0n1p3 at offset 505193836544. Wipe it? [y/n]: [n]
Jul  6 11:20:11 xcp-ng-bzkcpvhy SM: [6624]   Aborted wiping of zfs_member.
Jul  6 11:20:11 xcp-ng-bzkcpvhy SM: [6624]   1 existing signature left on the device.
Jul  6 11:20:11 xcp-ng-bzkcpvhy SM: [6624] '

With patch:

[11:20 xcp-ng-bzkcpvhy sm]# xe sr-create name-label=ydi-local type=ext device-config:device=/dev/nvme0n1p3 
5c72255b-e454-fe34-03c3-cc4f03b9b7c7

SMlog:

Jul  6 11:20:30 xcp-ng-bzkcpvhy SM: [6839] ['/sbin/vgcreate', '--metadatasize', '10M', 'XSLocalEXT-5c72255b-e454-fe34-03c3-cc4f03b9b7c7', '/dev/nvme0n1p3']
Jul  6 11:20:30 xcp-ng-bzkcpvhy SM: [6839]   pread SUCCESS

[edwintorok]

Have you tried what happens when you give an entire device (as opposed to a partition) as device-config?
The manpage says that in that case '--force' should be used:

  -f, --force
           Force erasure, even if the filesystem is mounted. This is required in order to erase a partition-table signature on a block device.

Although in general using 'force' is a bad idea: if the FS is mounted then failing is the right thing (but apparently having active partitions counts as in-use).
OTOH the previous 'dd' would just go and wipe the device anyway without checking whether it was mounted or not, so perhaps using force would be equivalent?

[edwintorok]

Perhaps wipefs could be used in force mode only to wipe partition table signatures, e.g. wipefs -a --force -t gpt,PMBR,dos, followed by a regular wipefs in non-force mode: wipefs -a. And have a 'device-config' parameter to make the 2nd one use '--force' too.
That way you're protected from typos a little bit (e.g. if you happen to give Dom0's partition that is mounted it will fail and not wipe it), although not completely (if you press enter too soon before typing the partition number it will get a whole device and forcefully all partitions).

[ydirson]

Maybe their documentation is outdated, there does not seem to be any requirement to force the wipe, at least with the wipefs version we ship:

[16:09 xcp-ng-bzkcpvhy sm]# file -s /dev/nvme0n2
/dev/nvme0n2: x86 boot sector; partition 1: ID=0xee, starthead 0, startsector 1, 209715199 sectors, extended partition table (last)\011, code offset 0x0
[16:09 xcp-ng-bzkcpvhy sm]# wipefs -a /dev/nvme0n2
/dev/nvme0n2: 8 bytes were erased at offset 0x00000200 (gpt): 45 46 49 20 50 41 52 54
/dev/nvme0n2: 8 bytes were erased at offset 0x18fffffe00 (gpt): 45 46 49 20 50 41 52 54
/dev/nvme0n2: 2 bytes were erased at offset 0x000001fe (PMBR): 55 aa
/dev/nvme0n2: calling ioclt to re-read partition table: Success
[16:09 xcp-ng-bzkcpvhy sm]# file -s /dev/nvme0n2
/dev/nvme0n2: data

Edit: works the same with an msdos disklabel

[ydirson]

Last updates makes logging the command systematic - wipefs is destructive so don't hide its execution

[MarkSymsCtx]

LGTM and as discussed in #624

[TimSmithCtx]

When we invoked dd it was done via util.CMD_DD which gave /bin/dd as an explicit path.

I think it would be good to follow the same procedure and create util.CMD_WIPEFS with an explicit path (I believe that would be /usr/sbin/wipefs) just to maintain the same level of rigour.

Other than that I don't see an issue.

[ydirson]

I'm curious about any upsides with this approach, other than consistency with existing code. As for downsides I see:

• hardcoding paths decreases portability
• using a variable defined somewhere else makes the code less readable

The init system is responsible to set a proper PATH in environment (and nowadays systemd ensures this even when launching the service from the CLI). If we don't trust the environment we're just doomed anyway: anything could be achieved with a LD_PRELOAD, including replacing the path passed to execve.

[TimSmithCtx]

Portability isn't really an issue for this code as it's part of an appliance use-case.

You're right that at present someone could use LD_PRELOAD to do anything they could do by subverting the PATH, but that may not always be the case. Should future hardening work move this call into a situation of elevated privilege, the dynamic linker would ignore any LD_PRELOAD with a path separator, rendering it useless without additional PATH subversion (which a hard-coded path would prevent).

I'm willing to let this go at present, but it's something we'll need to consider if we decide to harden this path in the future.

[stormi]

Ping to reviewers 👋

[stormi]

This pull request brings noticeable improvements when XenServer or XCP-ng are installed on disks with existing metadata (ZFS or such), at the cost of a rather simple code change. Plus, Yann answered all questions that were asked, 6 months ago.

Would it be possible to review it?