snap-bootstrap: add scan-disk subcommand #12006

valentindavid · 2022-07-28T15:43:05Z

This command is expected to be called from udev. This creates
symlinks /dev/ubuntu/ubuntu-{seed,boot,data,save}.

Then we can bind snap-bootstrap initramfs-mounts to
dev-ubuntu-ubuntu-seed.device.

valentindavid · 2022-07-28T15:43:18Z

This works with canonical/core-initrd#107

mardy

Would it be possible to get the same information just by running blkid instead of using the library?

xnox · 2022-08-09T14:56:24Z

cmd/snap-bootstrap/cmd_scan_disk.go

+		} else if part.Name == "ubuntu-data-enc" {
+			has_data = true
+			data_uuid = part.UUID
+		} else if part.Name == "ubuntu-data" {


if someone appends unencrypted ubuntu-data to a hard-drive, then it will be mounted instead of ubuntu-data-enc? surely we should try to find -enc paritions, and not search for non-encrypted ones afterwords?

Also I thought udev database already has all of this information inside it's database, which maybe can be queried directly in the udev rule alone without this helper binary? Apart from the LoaderDevicePartUUID bit, which i thought there should be a udev symlink for.

if someone appends unencrypted ubuntu-data to a hard-drive, then it will be mounted instead of ubuntu-data-enc? surely we should try to find -enc paritions, and not search for non-encrypted ones afterwords?

Maybe I mixed up, but I think the filesystem labels have "-enc" suffix. But the partition name does not. See for example:

/dev/vda4: UUID="2eeba654-032f-4384-8a46-075be2b415b5" LABEL="ubuntu-save-enc" TYPE="crypto_LUKS" PARTLABEL="ubuntu-save" PARTUUID="a154643d-4398-714a-9704-71335e5ea856"

PARTLABEL is not the same as LABEL

Inside ubuntu-save-enc there is ubuntu-save. Outside of ubuntu-save-enc there can be a malicious second partition with an FS ubuntu-save. Does above code handle this fine? As far as I can tell, after scanning and finding ubuntu-data-enc and using it; it will then find unencrypted ubuntu-data and override it.

Somehow this feels odd that after the first one is detected, it can be detected again, and override previously found result.

Also I thought udev database already has all of this information inside it's database, which maybe can be queried directly in the udev rule alone without this helper binary? Apart from the LoaderDevicePartUUID bit, which i thought there should be a udev symlink for.

Good question. I have tried before to use more rules and less code. But there were issues. And I do not remember. I will try again to document why this is not possible.

I am guessing it was the fallback for non-uefi boot that was a bit tricky.

gpt-auto-generator can create a symlink. But it has to be a partition with a specific uuid on the same disk as the booting partition. We do not use that uuid. And we should not use it. It is not made for that.

Inside ubuntu-save-enc there is ubuntu-save. Outside of ubuntu-save-enc there can be a malicious second partition with an FS ubuntu-save.

I am not sure I understood this.

This binary only looks at partition names. Not filesystem labels. If there are other partitions with conflicting filesystem labels, we will not look at them, because we only look at the partition names.

Today, someone can remove an encrypted ubuntu-save and replace it by a non encrypted one. snap-bootstrap will then fallback to it. What would be the difference?

Also I thought udev database already has all of this information inside it's database, which maybe can be queried directly in the udev rule alone without this helper binary? Apart from the LoaderDevicePartUUID bit, which i thought there should be a udev symlink for.

Good question. I have tried before to use more rules and less code. But there were issues. And I do not remember. I will try again to document why this is not possible.

Now I remember. It is possible to not call it on ENV{DEVTYPE}=="partition". Though we need to make sure that IMPORT{builtin}="blkid [...]" has been called yet, so post 60-persistent-storage.rules. Then what we have to do is compare ID_PART_ENTRY_UUID with UBUNTU_SEED (or the 3 others).

However, for ENV{DEVTYPE}=="disk" we do need to scan the partitions and find the booting one. And this we cannot do with just udev rules. We have LoaderDevicePartUUID which points to the boot partition. But we are processing the disk at this point.

gpt-auto-generator can create a symlink.

By the way, I was wrong about it. It is in an udev rule that the symlink is created. gpt-auto-generator only makes sysroot.mount and -.mount to use that symlink.

Now I remember. It is possible to not call it on ENV{DEVTYPE}=="partition". Though we need to make sure that IMPORT{builtin}="blkid [...]" has been called yet, so post 60-persistent-storage.rules. Then what we have to do is compare ID_PART_ENTRY_UUID with UBUNTU_SEED (or the 3 others).

what's the conclusion here? it seemed related to what Alfonso is also asking below. Anyway as Alfonso said this needs a bit of a description of what is the logic/flow and why we went that way in the code itself

Looking back at this thread, I realize that the -enc suffix is only for the luks2 label. Not the partition label. So maybe I can use only udev rules for the partitions.

valentindavid · 2022-08-11T14:28:17Z

Would it be possible to get the same information just by running blkid instead of using the library?

We cannot list partitions from blkid.

xnox · 2022-08-15T11:52:24Z

Given SEED is ESP; and we bind the initramfs-mounts unit to the SEED; i am wondering if we can instead turn on / patch the systemd's gpt-auto-generator which can create esp mount in /efi and use that instead?

Looking more at this, I actually don't quite like the behaviour of gpt-auto-generator. Because the good bits of it are not what we need here.

Ideally we would only read efi_loader_get_device_part_uuid (aka LoaderDevicePartUUID) and generate .device unit for it, and bind initramfs-mounts service unit to it. Before any drives appear.

Thus without waiting udev to exec scan-disk to process them all either.

valentindavid · 2022-08-15T15:27:58Z

Ideally we would only read efi_loader_get_device_part_uuid (aka LoaderDevicePartUUID) and generate .device unit for it, and bind initramfs-mounts service unit to it. Before any drives appear.

Thus without waiting udev to exec scan-disk to process them all either.

So almost what I have done, but a symlink on the whole disk rather than the partitions?

pedronis · 2023-02-23T19:01:42Z

@valentindavid can the description or some comment in the code include the actual udev rules that this will be used from? it's a bit unclear just from the code how things go from the code in scan-disk to have /dev/ubuntu/disk symlinked?

What's the plan for testing the cmd_scan_disk code?

valentindavid · 2023-02-23T23:47:59Z

@valentindavid can the description or some comment in the code include the actual udev rules that this will be used from? it's a bit unclear just from the code how things go from the code in scan-disk to have /dev/ubuntu/disk symlinked?

Sure. We could even add the rules to snapd and install them into core-initrd.

What's the plan for testing the cmd_scan_disk code?

I think I will need to mock libblkid somehow to test it.

Loop devices need root to be setup, and I do not think there is as this point any nice user land block devices. I think only character devices can be emulated with CUSE. Not block.

valentindavid · 2023-03-03T14:52:28Z

What's the plan for testing the cmd_scan_disk code?

I have added tests.

I still have to add tests for the changes in cmd_initramfs_mounts.go (when /dev/ubuntu/disk exists). But that should be straight forward.

pedronis · 2025-04-11T18:02:26Z

I suppose this needs to be marked blocked then, also if we need to run that many go commands are we sure it’s going to be faster than master?

…

On Fri, 11 Apr 2025 at 19:52, alfonsosanchezbeato ***@***.***> wrote: Something that I have been thinking is if we might have race conditions as it is possible that the partition nodes appear later that the base device (the kernel will spend a small amount of time reading the partition table between both events). Should we create the disk symlink after all partitions that we expect appear? With something like: 1. scan is run for the block devices. It detects the boot device and reads the expected partitions. It exports booleans for UBUNTU_DISK but also others, one for each expected partition. 2. Another command is run for the partitions in UBUNTU_DISK, for the expected partitions (could be just seed, or seed+boot+data, or seed+boot+data+save). This commands creates the link for the partition it runs for, then checks if the rest of the links for the expected partitions are also created (looking at the env it will know which are the expcted symlinks). If that is the case, it creates the disk symlink. With this, all devices for partitions that snap-bootstrap wants to mount must exist when the initramfs-mounts subcommand is run. — Reply to this email directly, view it on GitHub <#12006 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOHDLAA5XHLQKJ7FR7MX32Y76MHAVCNFSM6AAAAABRD2SSCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJXGY2DIMBVGM> . You are receiving this because your review was requested.Message ID: ***@***.***> *alfonsosanchezbeato* left a comment (canonical/snapd#12006) <#12006 (comment)> Something that I have been thinking is if we might have race conditions as it is possible that the partition nodes appear later that the base device (the kernel will spend a small amount of time reading the partition table between both events). Should we create the disk symlink after all partitions that we expect appear? With something like: 1. scan is run for the block devices. It detects the boot device and reads the expected partitions. It exports booleans for UBUNTU_DISK but also others, one for each expected partition. 2. Another command is run for the partitions in UBUNTU_DISK, for the expected partitions (could be just seed, or seed+boot+data, or seed+boot+data+save). This commands creates the link for the partition it runs for, then checks if the rest of the links for the expected partitions are also created (looking at the env it will know which are the expcted symlinks). If that is the case, it creates the disk symlink. With this, all devices for partitions that snap-bootstrap wants to mount must exist when the initramfs-mounts subcommand is run. — Reply to this email directly, view it on GitHub <#12006 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHOHDLAA5XHLQKJ7FR7MX32Y76MHAVCNFSM6AAAAABRD2SSCOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJXGY2DIMBVGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

alfonsosanchezbeato · 2025-04-17T17:17:08Z

af69054 has a possible solution to the described race (#15359). It makes the scan command export an additional variable with the number of expected partitions and a specific symlink is created with all those partitions have appeared.

alfonsosanchezbeato · 2025-04-17T21:13:54Z

af69054 has a possible solution to the described race (#15359). It makes the scan command export an additional variable with the number of expected partitions and a specific symlink is created with all those partitions have appeared.

I've changed a bit the approach and use now UUIDs, see 2328620

valentindavid · 2025-04-22T13:12:31Z

There should not be any uevent emitted until devtmpfs has been populated, that means once /dev/disks/ubuntu/disk can only appear after the device nodes have appeared in /dev. See https://github.com/torvalds/linux/blob/a33b5a08cbbdd7aadff95f40cbb45ab86841679e/block/genhd.c#L463
We do not follow yet other symlinks in this PR because we would need to add dependency to the unit to them. But this is a bit complicated. It might depend on splitting snap-boostrap execution into several units.

alfonsosanchezbeato · 2025-04-22T13:57:54Z

There is should not any uevent emitted until devtmpfs has been populated, that means once /dev/disks/ubuntu/disk can only appear after the device nodes have appeared in /dev. See https://github.com/torvalds/linux/blob/a33b5a08cbbdd7aadff95f40cbb45ab86841679e/block/genhd.c#L463

We do not follow yet other symlinks in this PR because we would need to add dependency to the unit to them. But this is a bit complicated and done before. It might depend on splitting snap-boostrap execution into several units.

Thanks for checking! Re: waiting on symlinks I think it should be possible if the scan commands provides information to udev on what symlink/partition we want to wait for depending on the mode. And create a symlink on which mount command can wait that could point to different partitions.

valentindavid · 2025-04-23T11:50:48Z

Re: waiting on symlinks I think it should be possible if the scan commands provides information to udev on what symlink/partition we want to wait for depending on the mode. And create a symlink on which mount command can wait that could point to different partitions.

The problem here is that we need to add dependencies in generators. And that would require daemon-reload. And start commands as well. The best way is to split snap-bootstrap so we can add correct dependencies to every part. This branch works fine as is as long as we do not expect to read the symlinks yet...

But they will be useful as soon as we move things out. For instance we could move mount of boot and seed as automount units.

andrewphelpsj

Thank you!

andrewphelpsj · 2025-04-29T20:45:17Z

cmd/snap-bootstrap/cmd_initramfs_mounts.go

 // determine what partition the booted kernel came from. If which disk the
 // kernel came from cannot be determined, then it will fallback to mounting via
 // the specified disk label.
 func mountNonDataPartitionMatchingKernelDisk(dir, fallbacklabel string, opts *systemdMountOptions) error {


Doc comment here I think could use an update to explain this new flow.

andrewphelpsj · 2025-04-29T20:51:29Z

cmd/snap-bootstrap/cmd_scan_disk.go

+	}
+
+	partitions, err := probe.GetPartitions()
+	if partitions == nil {


I think a normal check on err != nil would be more consistent here.

andrewphelpsj · 2025-04-29T20:52:45Z

cmd/snap-bootstrap/cmd_scan_disk.go

+	 * this is not an error. There are plenty of block devices
+	 * that are not the boot device.
+	 */
+	return nil;


Probably need to run gofmt on these files.

andrewphelpsj · 2025-04-29T21:08:10Z

cmd/snap-bootstrap/blkid/blkid.go

+	cnode := C.CString(node)
+	defer C.free(unsafe.Pointer(cnode))
+	C.blkid_new_probe_from_filename(cnode)
+	probe, err := C.blkid_new_probe_from_filename(cnode)


From my understanding, err will contain some representation of errno, from C. Would it be safer to check both err and probe?

Do we know for sure err != nil when probe == nil?

probe is always nil on error yes.

https://cdn.kernel.org/pub/linux/utils/util-linux/v2.41/libblkid-docs/libblkid-Low-level-probing.html#blkid-new-probe-from-filename

I see. Although that is a bit different, I think. I wanted to make sure that errno is always set by these functions on failure, but that isn't mentioned in the docs. I took a look at the code, and I think that errno would be set in all failure paths, but it is hard to tell.

I have changed it to use err.

If we use err, then we're saying that errno will always be set by the C function on failure. Is that definitely true?

andrewphelpsj · 2025-04-29T21:09:00Z

cmd/snap-bootstrap/blkid/blkid.go

+func newProbeFromFilenameImpl(node string) (AbstractBlkidProbe, error) {
+	cnode := C.CString(node)
+	defer C.free(unsafe.Pointer(cnode))
+	C.blkid_new_probe_from_filename(cnode)


Errant/extra call here?

Yep... that was a leak.

valentindavid · 2025-04-30T15:11:43Z

I have resplit and rebased all the commits for merge.

pedronis

thanks, couple minor things

pedronis · 2025-05-02T14:43:12Z

cmd/snap-bootstrap/cmd_scan_disk.go

+			 * is not an error. There are lots of block
+			 * devices.
+			 */
+			return nil


tests don't get here

pedronis · 2025-05-02T14:43:26Z

cmd/snap-bootstrap/cmd_scan_disk.go

+		return scanDiskNodeFallback(output, node)
+	}
+
+	// TODO: split the rest of this function in 2, fallback and non fallback.


this was taken care afaict

This will be needed for snap-boostrap scan-disk

This command is expected to be called from udev. This creates symlinks in `/dev/disk/ubuntu/`. Then we can bind `snap-bootstrap initramfs-mounts` to `dev-disk-ubuntu-disk.device`.

…can-disk

valentindavid · 2025-05-07T13:51:55Z

Could I get a forced merge?

Failures:

Preparing:
* openstack:centos-9-64

This PR is not related to centos.

* google-nested:ubuntu-20.04-64:tests/nested/manual/core20-new-snapd-does-not-break-old-initrd

This looks like nvidia component related. Not related to this.

* google:ubuntu-22.04-64:tests/main/nvidia-files:550

Download error from package repositories

Executing:

* google-nested:ubuntu-24.04-64:tests/nested/manual/remodel-min-size

Serial port suggests it boot correctly. Only ssh fails to connect. That is after mounting disks.

Restoring:

* openstack:centos-9-64

This PR is not related to centos.

Also unit-tests-cross-distros, unrelated and already fixed in master.

valentindavid mentioned this pull request Jul 28, 2022

Wait for /dev/ubuntu/disk device to start snap-initramfs-mounts.service canonical/core-initrd#107

Draft

mardy reviewed Aug 2, 2022

View reviewed changes

xnox reviewed Aug 9, 2022

View reviewed changes

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Aug 22, 2022

pedronis changed the title ~~snap-bootstrap: Add scan-disk subcommand~~ snap-bootstrap: add scan-disk subcommand Aug 23, 2022

pedronis self-requested a review September 19, 2022 13:19

valentindavid force-pushed the valentindavid/udev-scan-disk branch 2 times, most recently from 4e7a21e to 999e91c Compare December 13, 2022 10:42

valentindavid mentioned this pull request Dec 13, 2022

cmd_scan_disk: Add kernel arg to disallow subsystems for boot disk #12432

Draft

valentindavid force-pushed the valentindavid/udev-scan-disk branch 2 times, most recently from 49c9437 to 4060872 Compare February 14, 2023 12:09

valentindavid mentioned this pull request Feb 15, 2023

snap-bootstrap: split in smaller commands #12570

Draft

valentindavid force-pushed the valentindavid/udev-scan-disk branch 3 times, most recently from ebb8a9b to 7b6adbd Compare February 22, 2023 10:06

valentindavid force-pushed the valentindavid/udev-scan-disk branch from 7b6adbd to 71cc643 Compare March 3, 2023 14:51

valentindavid force-pushed the valentindavid/udev-scan-disk branch 4 times, most recently from 2f663fa to 05f7751 Compare March 9, 2023 12:37

valentindavid mentioned this pull request Mar 10, 2023

Bisect golangci-lint crash #12636

Closed

valentindavid force-pushed the valentindavid/udev-scan-disk branch from 42a76f8 to 8b70e59 Compare March 10, 2023 10:21

valentindavid marked this pull request as ready for review March 23, 2023 15:19

pedronis added the ⛔ Blocked label Apr 11, 2025

valentindavid requested a review from andrewphelpsj April 25, 2025 13:41

andrewphelpsj approved these changes Apr 29, 2025

View reviewed changes

andrewphelpsj reviewed Apr 29, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/udev-scan-disk branch from 513059a to afe58c2 Compare April 30, 2025 15:11

alfonsosanchezbeato removed the ⛔ Blocked label May 2, 2025

alfonsosanchezbeato requested a review from pedronis May 2, 2025 14:18

pedronis approved these changes May 2, 2025

View reviewed changes

valentindavid force-pushed the valentindavid/udev-scan-disk branch 2 times, most recently from 8bf84c5 to f01a2ca Compare May 5, 2025 13:12

valentindavid added 8 commits May 6, 2025 17:22

many: add build and runtime dependencies to libblkid

96650dc

This will be needed for snap-boostrap scan-disk

cmd/snap-bootstrap/blkid: add new blkid wrapper

5ee235d

snap-bootstrap: Add scan-disk subcommand

6ddbf5d

This command is expected to be called from udev. This creates symlinks in `/dev/disk/ubuntu/`. Then we can bind `snap-bootstrap initramfs-mounts` to `dev-disk-ubuntu-disk.device`.

core-initrd: add udev rules to call snap-bootstrap scan-disk

0864e59

core-initrd/.../snap-initramfs-mounts.service: wait for disk device

7b44331

osutil/disks: add a function too find partition by uuid on a disk

9c20173

cmd/snap-bootstrap: use /dev/disk/ubuntu/disk if found

a87aba4

tests/nested/manual/core22-basic: test symlinks created by udev and s…

247246b

…can-disk

valentindavid force-pushed the valentindavid/udev-scan-disk branch from f01a2ca to 247246b Compare May 6, 2025 15:22

alfonsosanchezbeato merged commit 54cfe92 into canonical:master May 7, 2025
75 of 83 checks passed

alfonsosanchezbeato mentioned this pull request Oct 21, 2025

osutil/disks: remove calls to udevadm trigger/settle when finding #16182

Open

snap-bootstrap: add scan-disk subcommand #12006

snap-bootstrap: add scan-disk subcommand #12006

Uh oh!

Conversation

valentindavid commented Jul 28, 2022 • edited by mvo5 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentindavid commented Jul 28, 2022

Uh oh!

mardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentindavid commented Aug 11, 2022

Uh oh!

xnox commented Aug 15, 2022

Uh oh!

valentindavid commented Aug 15, 2022

Uh oh!

pedronis commented Feb 23, 2023

Uh oh!

valentindavid commented Feb 23, 2023

Uh oh!

valentindavid commented Mar 3, 2023

Uh oh!

pedronis commented Apr 11, 2025 via email

Uh oh!

alfonsosanchezbeato commented Apr 17, 2025

Uh oh!

alfonsosanchezbeato commented Apr 17, 2025

Uh oh!

valentindavid commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alfonsosanchezbeato commented Apr 22, 2025

Uh oh!

valentindavid commented Apr 23, 2025

Uh oh!

andrewphelpsj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentindavid commented Jul 28, 2022 •

edited by mvo5

Loading

valentindavid commented Apr 22, 2025 •

edited

Loading