How does BRL hand off booting to hijacked os?

grumbly · 07-27-2019, 02:49 PM

A penny for your thoughts? This code diff works for me and resolves the issue perfectly, but I'm unsure what else it might break.
All I'm doing is checking that the system ESP mount point is empty and reporting that condition as "not yet mounted."
I'm building this on the main ClearLinux clr-boot-manager repo since that is the most upstream, I believe, so other OSes using clr-boot-manager would benefit as well.

ParadigmComplex · 07-27-2019, 03:45 PM

This doesn't differentiate between the EFI item not being mounted and the EFI item being empty. I don't know enough about EFI to guess is it being empty is ever a valid state to expect. It may be difficult to make the case to the maintainers to upstream this.

Instead, I'd be inclined instead to try and figure out whether it's a mount of whatever EFI thing is going on. It can then be sold to upstream based on correctness. Again, I don't have EFI experience, but from general Linux experience I'd poke around in:

/dev. For example, maybe the mounted item has a specific file name pattern we can expect. Maybe there's a symlink in /dev/disk/ to the device with useful information in the link name.
The corresponding line /proc/self/mounts or /proc/self/mountinfo when it is mounted

Or, honestly, look up the relevant standards. Figure out the term for the EFI thing being mounted, figure out how Linux/devpts/udev/etc handle it, etc.

grumbly · 07-27-2019, 06:23 PM

Yep, I can't disagree with that. Okay, back to the learning board!

Alright, I did eventually figure out how to do it the right way.
In summary, I used data structures that were already present to inspect the device, and I added steps to unmount if the mounted device is incorrect.
updated code here.

What do you think?

Oh, I'm pretty sure I fixed a bug too, where the user already has the EFI mounted somewhere else - they weren't captured the right directory.

ParadigmComplex · 07-27-2019, 06:53 PM

Unmounting Bedrock's preexisting mount first might make it work, but it will confuse Bedrock which expected that mount to be there. `brl status` will probably report the stratum as broken. Bedrock's expected workflow is that you mount over its mount point.

This article on shared subtree bind mounts may help illuminate what is going on from Bedrock's point of view: https://lwn.net/Articles/159077/

Note in that article how after `mount --make-shared /mnt` is run, any new mount point created over the existing mount on /mnt is propagated to /subtree/alice/mnt and /subtree/bob/mnt. If you mount a CD onto /mnt in Bedrock, exactly the same thing happens to make it accessible to processes from all strata.

Bedrock uses this same subsystem with /boot. Bedrock is designed with the expectation that, if someone needs to mount something onto /boot, it goes over Bedrock's shared subtree bind mount which then propagates it so all strata see the new mount. If something unmounts that mount, the underlying shared subtree bind mount propagates the unmount operation and it gets unmounted from from all strata.

In my experience this works fine so long as the software does not check for a preexisting mount. The only software I know which does is `mount -a` and, as you and I recently found out, clr-boot-manager.

The existing clr-boot-manager operation, as I understand it right now, in hacky pseudocode:

Code:

if (!already_has_mount("/boot")) {
	mount_efi("/boot")
}
do_stuff()
umount("/boot")

What I have in mind is:

Code:

if (!mount_is_efi("/boot")) {
	mount_efi("/boot")
}
do_stuff()
umount("/boot")

However, I haven't looked at clr-boot-manager's code yet and am running just off of my possibly faulty understanding from your comments. You might have a different understanding than I do. If I'm missing something do feel free to bring it up.

Also, very nice job finding an existing bug. That should help smooth over hesitation from their end on upstreaming this change.

grumbly · 07-28-2019, 10:45 AM

It's not that any of it is wrong, but any solution here is going to be overlooking some set of extenuating circumstances without a lot of new code happening. [edit] actually maybe we can do most of it!

Before I write more code, I think this might be a way I would translate what you wrote in your second code block:

Code:

if (!is_mounted(bootdir) || !is_mount_vfat(bootdir)) {
    mount(correct_device,bootdir)
}

That will do the right thing if:
* /boot is not yet mounted in any system (non-bedrock use case)
* /boot is "mounted" by bedrock, which makes it an ext4 mount in Solus
But of course will break if the user has mounted something stupid there, which is definitely the case of the current clr-boot-manager code.

Maybe the slightly better thing to do is also look if the required efi dir exists:
[edit] okay, I rewrote this a couple times. This is a mix of proposed code and what's already there.

Code:

final_mount_dir = NULL
// if user mounted the wrong ESP partition, unmount it (there can be many ESP partitions)
if ( is_mounted(bootdir) && is_mount_vfat(bootdir) && exists_efi_dir(bootdir) && !is_device_mounted(correct_device,bootdir) ) {
    umount(bootdir)
}
current_mountpoint = get_current_mountpoint_for_device(correct_device) // gets first vfat entry in /proc/self/mounts, ext4 or others are BRL
if (current_mountpoint != NULL) {
    // already mounted somewhere, so use that.
    final_mount_dir = current_mountpoint
} else {
    create_dir_if_not_exist(bootdir)
    mount(correct_device,bootdir)
    final_mount_dir = bootdir
}
// proceed with final_mount_dir...


Descriptions:
is_mounted()  // simply checks if /boot is mounted anywhere in /proc/self/mounts
              // (standard clr-boot-manager code)
is_mount_vfat() // checks for vfat in fstype of all matching mountpoint entries
                // of /proc/self/mounts (in bedrock there can be many)
exists_efi_dir() // see if there's a /boot/EFI directory
                 // (can annoyingly be lowercase efi)
get_current_mountpoint_for_device() // finds first entry in /proc/self/mounts 
                                    // mounting correct_device that is also vfat
                                    // (I added vfat check because all my BRL
                                    // entries in there for /boot are ext4)
is_device_mounted(device,dir) // check in /proc/self/mounts if the device and dir match up

I didn't realize we can simply mount over existing files, like if someone puts stuff in their /boot dir before using it as a mountpoint, mount won't care. That's nice.

ParadigmComplex · 07-28-2019, 11:35 AM

Checking if the mount point is vfat could easily false-positive. FAT32 is a perfectly valid filesystem for Linux use. Checking for <mount>/EFI is also kind of finicky, as I expect there may be a state where the EFI partition is empty that would need to be handled. If we exhaust everything else we can return to this, but I suspect there's a better strategy we could pursue.

Lets look at this from the other direction. For clr-boot-manager to mount the bootdir, it needs to know what information to pass to mount() to do so. How does it figure out what this information is? Once you know that, you can go the other way and check if a given mount point has those attributes. Is clr-boot-manager mounting something like /dev/efi corresponding to some weird EFI specific motherboard device, or is it mounting a more traditional /dev/sdXY device? If it's a more traditional /dev/sdXY device, how does it know which X or Y? A system could have multiple drives with multiple partitions on each; it would have to determine which. Or maybe it's not passing a device at all and doing some other dark magic with the data field or something.

Quote:

I didn't realize we can simply mount over existing files, like if someone puts stuff in their /boot dir before using it as a mountpoint, mount won't care. That's nice.

It's not something that's commonly leveraged, and so makes sense you might be missing the possibility. Bedrock R&D required I become familiar with some of the more obscure parts of the virtual filesystem. It's a fun thing to play with. For example, try to guess what happens if you mount over a directory that you've cd'd into:

Code:

$ mkfs.fat -F32 /dev/sdb1 # make new/empty partition
$ mkfs.fat -F32 /dev/sdc1 # make another new/empty partition
$ mkdir /example
$ mount /dev/sdb1 /example
$ cd /example
$ touch /example/bottom-file
$ mount /dev/sdc1 /example # mount over current working directory
$ touch /example/top-file
$ ls # does this list bottom-file or top-file or both or neither?

grumbly · 07-28-2019, 12:00 PM

Interesting! I thought maybe mounting over current working directory would make things like `cd ..` not work, but it's fine.

So I'm not sure I understand what you're saying.
There is already code clr-boot-manager is using to get the correct_device, and it does this by looking at /sys/firmware/efi/efivars/LoaderDevicePartUUID* for whichever file matches that pattern, then it stores the contents of that file, which is another UUID, then uses that UUID to get a device handle from /dev/disk/by-partuuid/<UUID>
That already happens and fills in the variable I've been using `correct_device`, so we know exactly what device indeed should be mounted at /boot.

However, I just now learned that mount for a particular mountpoint works like a stack, so we don't have to unmount anything ever in preparation. That's so nice! I would instead just do like what your block of code suggests, and check if the right device is mounted, and if not then mount it. I think that is rather clean and it would work very well. Thoughts?

ParadigmComplex · 07-28-2019, 12:10 PM

Provided we're understanding each other correctly, I am in favor of the plan you've described. Check if the device we know clr-boot-manager wants mounted to be mounted at bootdir and, if is not mounted, mount it over what is already there. We know clr-boot-manager unmounts when it is done, so whatever was there before will become accessible again once it's done.

grumbly · 07-28-2019, 12:19 PM

Great! I'll do that next chance I have to hack at it. Like many things, the solution ends up being so much simpler once everything is understood

Thanks for guiding me along while I learn about all this stuff.

ParadigmComplex · 07-28-2019, 12:31 PM

Happy to help, thanks for taking the initiative to do the actual grunt work here to get it done