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));
>
