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
