git: 2c26d77d989a - main - Remove /boot/efi from mtree, missed in 0b7472b3d8d2.

[Nathan Whitehorn]

On 3/3/21 5:25 PM, Warner Losh wrote:
>
>
> On Wed, Mar 3, 2021 at 10:21 AM Nathan Whitehorn 
> <nwhitehorn at freebsd.org <mailto:nwhitehorn at freebsd.org>> wrote:
>
>
>
>     On 3/3/21 11:53 AM, Warner Losh wrote:
>     >
>
>     [clipping non-technical pre-history]
>
>
> Thanks. re-reading it now, I think I was more grumpy than warranted. 
> And for that I apologize. Thanks for omitting it from the rest of the 
> thread.

No worries, this happens, especially with the pandemic. I know I've 
definitely been more prickly this year than normal...

>     >
>     >     The installer *does* mount the partition in advance, so checking
>     >     whether
>     >     there is a mounted file system is a perfectly reasonable test to
>     >     do. We
>     >     could also check fstab. I would like to understand what is
>     actually
>     >     wrong here first, though. Especially after this misfire --
>     which is
>     >     problematic for reasons that are still not clear to me, since
>     >     there are
>     >     a number of standard directories in hier(7) not in mtree --
>     I want to
>     >     make sure we actually do have consensus about what is
>     changing and
>     >     why.
>     >
>     >
>     > At the top level, we default to having directories in mtree unless
>     > there's a good reason not to. We disagree as to whether the
>     installer
>     > should take the presence or absence of the directory as a strong
>     > enough reason to do something. I don't think that's a good reason.
>     >
>     > But leaving that aside, let's say we wanted to reuse the install
>     boot
>     > part of the installer to update boot blocks as part of
>     installworld.
>     > If we can talk through that example w/o it in mtree, then we can
>     leave
>     > it out. The last time I worked through this, though, I thought I'd
>     > talked myself into needing it.
>
>     > Looking at bootconfig, we could use machdep.bootmethod to
>     determine if
>     > we need to update the ESP. If we didn't use that, then the ESP
>     > shouldn't be touched. This is, at the moment, x86 centric, but
>     could
>     > trivially be added to architectures (I'm happy to add it). This
>     would
>     > prevent the 'false positive' that's possible in cases where we've
>     > installed UEFI then downgraded to BIOS because of some problem
>     (though
>     > purely in the context of the installer, I guess this isn't an
>     issue).
>     > Even with your approach, we'd bogusly update an ESP (though one
>     could
>     > argue you might want that). We could also change the code so that
>     > 'unsupported' architectures just didn't update. This is why I think
>     > it's a bit fragile to rely only on the directory being present. It
>     > should have something mounted there. If you wanted to mkfs_dos +
>     mkdir
>     > efi at the top level, you could check for that directory if you
>     were
>     > looking for a flag, though that would still update on a BIOS
>     boot the
>     > ESP, and prevent false positives if run as part of an update.
>
>     I think we would *want* to update an ESP that is mounted but not
>     currently being used. If I set up a dual BIOS/EFI-boot system for
>     some
>     reason and happened to install an update while booted from BIOS, I
>     would
>     be deeply astonished if my configured-by-the-installer EFI bootloader
>     did not also get updated.
>
>
> Yea, it's unclear to me what POLA here is, to be honest. Some of that 
> is driven by a deep desire not to accidentally update USB drives that 
> have a bootable image on them as well, so that may overly color my 
> thinking.

Agreed on all counts here.

>     (As an aside, I would also much rather the installer use an update
>     utility to set up the ESP than have the update utility use the
>     installer.)
>
>
> Agreed. We can work towards that after the release. It would be better 
> if we could accumulate the scripts from a number of different places, 
> find a good way to make them callable from those places more easily 
> and start to move that tribal knowledge back into the base system 
> where it belongs, imho. Baptiste raised an important point years ago 
> that we also need to think about doing that with a way to 'plug in' 
> $NEWEST_CLOUD's packages, containers, layout such that they could 
> provide the details and then the automation would just work with them 
> too: image building, release customization, boot block update, etc.
>
>     So here's a proposal, now that everyone is in the CC list:
>     - We add /boot/efi back to mtree, even though I find it kind of
>     weird to
>     have it there I think we're too close to the release to have a
>     conclusion on this.
>     - We have the installer check for either the ESP directory being an
>     active mountpoint or being in the in-progress fstab, whichever is
>     easiest to implement (they are equivalent for the installer).
>
>
> I'm OK with both of these points. If others are opposed to the first 
> one, I'm willing to see how people react to it in the upgrade path 
> before changing it again. We should get closure on Ed's proposed 
> change here. I think it's good and should go in right after your 
> changes. I'd start on your changes, and give people until the morning 
> to pipe up with any objections.
Here's a patch to do this:
https://reviews.freebsd.org/D29068

It takes several hours to do the full test of building world, building 
release ISOs, and running them through qemu, so it will be a while yet 
before I feel comfortable committing. But it's a two-line diff and the 
pieces worked independently, so the chances it works are pretty high. 
Comments appreciated.

>     If that seems OK, I'll post another review for the change.
>
>     > A long-term project I've had has been to try to update the boot
>     blocks
>     > as part of installworld or maybe as part of installboot. We have
>     > really poor reuse as a project in this area. Every little
>     > orchestration thing wrote its own thing, and all of them have
>     done it
>     > badly. I was hoping to be able to reuse this code, or modify the
>     > installer to use whatever we come up with there. As part of that, I
>     > had talked myself into thinking we always needed /boot/efi, but I'm
>     > having trouble reconstructing why that is now though I know it
>     had to
>     > do with installed systems and bootstrapping issues... I know I was
>     > worried about questions about 'why isn't /boot/efi on the system by
>     > default so I can mount it' for people that have upgraded, but I
>     recall
>     > there was more to it than that. With it in mtree, an installworld
>     > (even w/o an ESP update) would create it and people could mount
>     it w/o
>     > having to mkdir which they might make as $SOMETHING_ELSE. So I
>     guess
>     > that's a bit of a weak reason to absolutely require it in mtree.
>
>     Thanks a lot for the explanation. I'm agreed entirely about the
>     problem
>     and the difficulty -- hopefully this set of changes helps at least.
>
>
> It does. It starts to get people to use the same mount point for the 
> ESP and we can then constrain the problem a bit and where we can't 
> constrain it we can parameterize it.
>
>     As for mtree, I was imagining this as something like /home, which
>     is a
>     standard part of the system but isn't part of mtree since it
>     depends on
>     local-system policy. It's also different from /home in that we
>     *do* want
>     it to be a standard place for updates, of course. I think there's
>     really
>     not a ton of precedent either way: we don't have any other mount
>     points
>     in there for file systems that may or may not exist depending on
>     circumstances, as far as I can tell. My worry with having it in
>     mtree is
>     that having it exist but potentially be a directory rather than an
>     actual ESP requires that update tools be a little smarter and errors
>     will be a little less obvious, since updates that don't pay enough
>     attention will be a bit more likely to splat files there assuming
>     there
>     is an ESP even if makes no sense. It's a weak consideration either
>     way,
>     I think.
>
>
> Yea. After a few hours of reflection, I've found that I could go 
> either way and am having trouble understanding why I was so dead set 
> this morning on a particular way. Chalk it up to me being a little 
> extra grumpy at surprise changes.
>
> This one seems less like local policy than /home, but there's still a 
> local aspect: Do I mount by default, and where. I think we should 
> always, though, have a fstab entry as we'll need to update it from 
> time to time. Even Windows has a nominal drive that it uses to mount 
> the ESP, even if it isn't mounted by default. That's used to update it 
> when scripts and such need to do that (or if you're the victim of an 
> upgrade script that did too much that now needs to be undone). I think 
> we should be similar in that regard. This would also let us take the 
> automation of updates to the next level if we can rely on some basic 
> things.

That makes sense to me. There's also still the issue of non-EFI systems, 
that differ only by install-time configuration from non-EFI systems. One 
of my worries of having /boot/efi always exist is that a non-EFI system 
may try to "update" the EFI by poking around in the empty /boot/efi and 
think it has updated/installed something useful but has in reality done 
nothing. But it's a tricky situation all around.
-Nathan

>
> Warner
>
>
>     >
>     > Warner
>
>