Re: [PATCH] switch_root: remove /init check
Linus Heckemann 於 2024年4月19日 星期五寫道: > We were having some difficulty switching out of our custom initramfs > into the final filesystem, with the error "message '/init' is not a > regular file". We were confused as to why it was looking for `/init` > -- we didn't have `/init`, neither in our initrd nor in the > destination filesystem -- we were using the rdinit= command line > parameter to provide an alternative path for the init in the initrd, > and the target init was determined by userspace. > > Thankfully, the error message was fairly clear and easy to find in the > source! > > We thus propose removing this check, since not all initrds have their > init at /init -- setting rdinit= on the kernel command line allows > using an alternative path. Thus, this check can prevent switching root > in a scenario where it would be perfectly appropriate. > --- > util-linux/switch_root.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/util-linux/switch_root.c b/util-linux/switch_root.c > index 14139736e..ef6669f5c 100644 > --- a/util-linux/switch_root.c > +++ b/util-linux/switch_root.c > @@ -238,9 +238,6 @@ int switch_root_main(int argc UNUSED_PARAM, char **argv) > // Additional sanity checks: we're about to rm -rf /, so be REALLY SURE > // we mean it. I could make this a CONFIG option, but I would get email > // from all the people who WILL destroy their filesystems. > - if (stat("/init", &st) != 0 || !S_ISREG(st.st_mode)) { > - bb_error_msg_and_die("'%s' is not a regular file", "/init"); > - } > statfs("/", &stfs); // this never fails > if ((unsigned)stfs.f_type != RAMFS_MAGIC > && (unsigned)stfs.f_type != TMPFS_MAGIC > -- > 2.42.0 Did you read the code comments on the lines above what you deleted? It's a sanity check before deleting everything on the "/" filesystem. Perhaps a better approach is to check the existence of what's specified in the "rdinit" parameter instead. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] switch_root: remove /init check
Kang-Che Sung writes: > Linus Heckemann 於 2024年4月19日 星期五寫道: >> We were having some difficulty switching out of our custom initramfs >> into the final filesystem, with the error "message '/init' is not a >> regular file". We were confused as to why it was looking for `/init` >> -- we didn't have `/init`, neither in our initrd nor in the >> destination filesystem -- we were using the rdinit= command line >> parameter to provide an alternative path for the init in the initrd, >> and the target init was determined by userspace. >> >> Thankfully, the error message was fairly clear and easy to find in the >> source! >> >> We thus propose removing this check, since not all initrds have their >> init at /init -- setting rdinit= on the kernel command line allows >> using an alternative path. Thus, this check can prevent switching root >> in a scenario where it would be perfectly appropriate. >> --- >> util-linux/switch_root.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/util-linux/switch_root.c b/util-linux/switch_root.c >> index 14139736e..ef6669f5c 100644 >> --- a/util-linux/switch_root.c >> +++ b/util-linux/switch_root.c >> @@ -238,9 +238,6 @@ int switch_root_main(int argc UNUSED_PARAM, char > **argv) >> // Additional sanity checks: we're about to rm -rf /, so be > REALLY SURE >> // we mean it. I could make this a CONFIG option, but I would get > email >> // from all the people who WILL destroy their filesystems. >> - if (stat("/init", &st) != 0 || !S_ISREG(st.st_mode)) { >> - bb_error_msg_and_die("'%s' is not a regular file", > "/init"); >> - } >> statfs("/", &stfs); // this never fails >> if ((unsigned)stfs.f_type != RAMFS_MAGIC >> && (unsigned)stfs.f_type != TMPFS_MAGIC >> -- >> 2.42.0 > > Did you read the code comments on the lines above what you deleted? It's a > sanity check before deleting everything on the "/" filesystem. I did! I don't really see the existence of /init being so critical for this check given that we check below that it's a ramfs or tmpfs, which seems to me to be enough that people won't be destroying filesystems they cared a great deal about. > Perhaps a better approach is to check the existence of what's specified in > the "rdinit" parameter instead. That would introduce an additional dependency on /proc being mounted and require additional parsing. I don't think the check is that necessary, again because we have the /-is-ramfs-or-tmpfs check. But if you do think we need it I can rewrite the patch to check for rdinit= on cmdline as well. Cheers Linus ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] switch_root: remove /init check
Linus Heckemann 於 2024年4月23日 星期二寫道: > > I don't really see the existence of /init being so critical for > this check given that we check below that it's a ramfs or tmpfs, which > seems to me to be enough that people won't be destroying filesystems > they cared a great deal about. > The ramfs/tmpfs check was good as it won't destroy any _permanent_ file the user could have, but it wouldn't hurt either when there is another sanity check. >> Perhaps a better approach is to check the existence of what's specified in >> the "rdinit" parameter instead. > > That would introduce an additional dependency on /proc being mounted and > require additional parsing. I don't think the check is that necessary, > again because we have the /-is-ramfs-or-tmpfs check. But if you do think > we need it I can rewrite the patch to check for rdinit= on cmdline as well. > /proc should be mounted by most init systems anyway. But we can skip the check when /proc doesn't exist, just in case. The logic would be roughly like this: If "/proc/cmdline" exists Read the "rdinit" parameter from "/proc/cmdline"; if it's unspecified, default to "/init" If the file in "rdinit" doesn't exist, stop. Else Skip the "rdinit" existence check and continue the switch_root process ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] switch_root: remove /init check
Kang-Che Sung writes: > Linus Heckemann 於 2024年4月23日 星期二寫道: >> >> I don't really see the existence of /init being so critical for >> this check given that we check below that it's a ramfs or tmpfs, which >> seems to me to be enough that people won't be destroying filesystems >> they cared a great deal about. >> > > The ramfs/tmpfs check was good as it won't destroy any _permanent_ file the > user could have, but it wouldn't hurt either when there is another sanity > check. > > >>> Perhaps a better approach is to check the existence of what's specified > in >>> the "rdinit" parameter instead. >> >> That would introduce an additional dependency on /proc being mounted and >> require additional parsing. I don't think the check is that necessary, >> again because we have the /-is-ramfs-or-tmpfs check. But if you do think >> we need it I can rewrite the patch to check for rdinit= on cmdline as > well. >> > > /proc should be mounted by most init systems anyway. But we can skip the > check when /proc doesn't exist, just in case. > > The logic would be roughly like this: > > If "/proc/cmdline" exists >Read the "rdinit" parameter from "/proc/cmdline"; if it's unspecified, > default to "/init" >If the file in "rdinit" doesn't exist, stop. > Else >Skip the "rdinit" existence check and continue the switch_root process I was having a look at this again, and I'm not sure reimplementing the kernel command line parsing logic[1][2] is really desirable. I still think removing the check completely would be best, but I'm fine with just dropping this and keeping the patch only on my end or working around it with `touch /init` if you're not convinced :) Linus [1]: https://lxr.linux.no/#linux+v6.7.1/kernel/params.c#L162 [2]: https://lxr.linux.no/#linux+v6.7.1/lib/cmdline.c#L227 ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH] switch_root: remove /init check
On 31/05/2024 00.08, Linus Heckemann wrote: > I was having a look at this again, and I'm not sure reimplementing the > kernel command line parsing logic[1][2] is really desirable. FWIW, I'm with Linus. It's also odd for busybox to have a check that the util-linux version of this doesn't. For the original patch: Acked-by: Rasmus Villemoes ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox