On Sat, Sep 23, 2023 at 01:11:32PM +0200, Mark Kettenis wrote: > > Date: Thu, 21 Sep 2023 22:30:01 +0000 > > From: Klemens Nanni <k...@openbsd.org> > > > > In comparison to MI boot which only cares about /bsd.upgrade's x bit, > > powerpc64 rdboot just wants a regular file. > > > > Require and strip u+x before execution to prevent sysupgrade(8) loop. > > I'm new to powerpc64 and can't think of a reason to be different. > > > > Feedback? Objection? OK? > > So there is a problem with this approach. Calling chmod() will mean > the bootloader will change the filesystem. What happens if you're > booting with a root filesystem that was not cleanly unmounted? > Modifying a forcibly mounted filesystem may not be without risk.
Isn't that already the case with chmo() /etc/random.seed? Can you explain how that is not a problem in other non-kernel boot loaders using libsa's ufs2_open() instead of mount(2)? > This relates to a problem with the powerpc64 bootloader that we still > haven't fixed. Once your root filesystem gets "dirty", it will remain > dirty forever, even if the OS does a fsck. The solution I came up > with was to drop the MNT_FORCE flag and instead fall back on mounting > the filesystem read-only. That should make the chmod() safe, but it > will fail if you were booting with a dirty root filesystem of course. > > I never got an ok for this diff, but I still think it is a good idea, > and it would make your diff safer. I don't know much about file systems, but not making a dirty root worse seems sane to me; better be able to boot without re-sysupgrade detection and entropy file handling than requiring more fsck(8). Either way, octeon would need the same treat. > > > Index: arch/powerpc64/stand/rdboot/disk.c > =================================================================== > RCS file: /cvs/src/sys/arch/powerpc64/stand/rdboot/disk.c,v > retrieving revision 1.3 > diff -u -p -r1.3 disk.c > --- arch/powerpc64/stand/rdboot/disk.c 29 Aug 2020 11:46:54 -0000 > 1.3 > +++ arch/powerpc64/stand/rdboot/disk.c 23 Sep 2023 11:08:01 -0000 > @@ -195,11 +195,13 @@ disk_open(const char *path) > > memset(&ffs_args, 0, sizeof(ffs_args)); > ffs_args.fspec = devpath; > - if (mount(MOUNT_FFS, "/mnt", MNT_FORCE | MNT_NOATIME, > - &ffs_args) == -1) { > - fprintf(stderr, "failed to mount %s: %s\n", devpath, > - strerror(errno)); > - return NULL; > + if (mount(MOUNT_FFS, "/mnt", MNT_NOATIME, &ffs_args) == -1) { > + if (mount(MOUNT_FFS, "/mnt", MNT_RDONLY, &ffs_args) == -1) { > + fprintf(stderr, "failed to mount %s: %s\n", devpath, > + strerror(errno)); > + return NULL; > + } > + fprintf(stderr, "%s: mounted read-only\n", devpath); > } > if (chroot("/mnt") == -1) { > fprintf(stderr, "failed to chroot: %s\n", strerror(errno)); >