Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootmisc accidentally wiped non-/tmp settings #458

Open
kaniini opened this issue Oct 7, 2021 · 40 comments
Open

bootmisc accidentally wiped non-/tmp settings #458

kaniini opened this issue Oct 7, 2021 · 40 comments
Labels

Comments

@kaniini
Copy link
Contributor

kaniini commented Oct 7, 2021

In Alpine, we had an incident where a user's box got rm -rf'd due to the bootmisc script: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13070

It seems to me that we should just mount a tmpfs at /tmp, and not "wipe" anything there at all.

@vapier
Copy link
Member

vapier commented Oct 7, 2021

this goes against FHS recommendations, what OpenRC has been doing for decades, and what pretty much every other distro does. iirc, a good amount of the /tmp cleanup logic was inspired by logic Debian had.
https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch03s18.html

bootmisc already supports /tmp being mounted as tmpfs if the user wants that. add it to your /etc/fstab. OpenRC shouldn't be requiring either behavior.

bootmisc already supports not wiping /tmp if the user doesn't want it. /etc/conf.d/bootmisc has a specific wipe_tmp knob.

so i don't see us changing any defaults here. if you can see where there's a bug in the logic, we can fix it, but this is the first report i've heard, and this logic has been around for a long time.

@vapier vapier changed the title bootmisc service shouldn't "wipe" /tmp bootmisc accidentally wiped non-/tmp settings Oct 7, 2021
@vapier vapier added the bug label Oct 7, 2021
@kaniini
Copy link
Contributor Author

kaniini commented Oct 7, 2021

The wipe_tmp knob does indeed choose less destructive cleanup actions, but perhaps it would be best to be able to skip the cleanup actions entirely?

@richfelker
Copy link

richfelker commented Oct 7, 2021

For whatever reason (not clear; may have been a result of fs corruption, but the bootmisc script itself was not corrupted because I read it during recovery) the rm -rf ran in the wrong directory, or else there were some sort of bind mounts that allowed it to recurse out of true /tmp (although I can't see any way that would have happened; this was on reboot after a crash, and I did not have any bind mount scripts running at boot).

In any case, executing rm -rf as part of a boot script is extremely unsafe, especially without an absolute pathname. Something should be done so that it fails safe rather than destructively.

@williamh
Copy link
Contributor

williamh commented Oct 7, 2021

The function that cleans up temp directories is cleanup_tmp_dir().
I read through the find / rm commands in that function, and I'm coming up with a couple of ideas (just brainstorming).

  • A symlink inside /tmp was pointing outside /tmp and that symlink was followed, but I'm not sure how likely that is.
  • clean_tmp_dirs was set in /etc/conf.d/bootmisc. This overrides the directories you want us to clean.

@vapier @richfelker

@kaniini
Copy link
Contributor Author

kaniini commented Oct 7, 2021

I plan on spinning up a VM this evening and seeing if I can reproduce it.

@richfelker
Copy link

richfelker commented Oct 8, 2021

If it helps, the crash just prior to this was my attempt to run anbox. It failed to run and my system was left in some sort of weird state. I don't recall any specifics of it beyond that it seemed like rebooting would be a good idea... So it's possible it left something in /tmp that the script handled badly, though I can't think what. rm does not follow symlinks.

@vapier
Copy link
Member

vapier commented Oct 9, 2021

if you saw the message Wiping /tmp directory, then i think we can assume cwd was /tmp. the func clearly has a cd $dir || exit just before this point.

rm should not deref symlinks since we aren't putting / at the end of paths, nor should the calls walk into ../ -- if it did, pretty sure that would be biting everyone all the time.

if you had a ton of files in /tmp, we fallback to using find, but that also should not be dereferencing symlinks or walking ../ since we only pass it one argument -- ..

if you had mounts inside of /tmp, then rm and find would descend into them and delete whatever it found. no one has ever requested such support before, so i'm not really inclined to add it. while POSIX find has -xdev, there is no such standard rm option, and probing for GNU --one-file-system is a pita.

@richfelker
Copy link

richfelker commented Oct 9, 2021

Since this was during boot I cannot think of any way there could have been mounts (bind or otherwise) inside of tmp. There would not be under normal runtime circumstances either. This could be mitigated with -xdev and -exec rm {} + instead of rm -rf though (with some additional type logic and a second pass with rmdir) but I seriously doubt it was the cause in my case.

I think the most likely cause was actual filesystem corruption - perhaps / getting cross-linked into /tmp/something as a "directory hardlink" (which you can't make, but is representable in the on-disk filesystem structures). I don't really know any good way to mitigate something like that, but the very possibility makes me think it's a very very bad idea to ever have any rm -rf execute during system boot (which might be happening after an event that caused fs corruption). The risk/benefit ratio here just does not make any sense.

@vapier
Copy link
Member

vapier commented Oct 9, 2021

i understand your preference. you're free to disable the behavior, either through existing knobs or by having /tmp be a tmpfs. but the fact is that clearing of /tmp is, and has been for decades, the standard in the *NIX world, and has been strongly recommended by FHS for decades (such that they would have mandated it if they felt FHS's scope included administration in general). changing this default behavior would be a strong disservice to users, the vast majority of which want this.

so if we agree that the code as-is doesn't look like it would ascend into the / or otherwise start operating on paths not reachable via /tmp, then it sounds like there's nothing left to do or discuss here.

@richfelker
Copy link

richfelker commented Oct 9, 2021

In that case I'll just continue to follow up on the Alpine side about fixing this in a patch, and about this as yet another reason for moving off OpenRC.

@williamh
Copy link
Contributor

williamh commented Oct 9, 2021

Hey @richfelker I would like to know more about why Alpine is looking into moving off of OpenRC; I would like to see if we can work on ways to fix your issues.
I just now saw your comment above, and would like to know more about what other init systems are doing about /tmp.
Also, is -xdev an option to find (I'm not familiar with it)? Someone also mentioned -delete (I'm pretty sure this is an option to find). More info would be helpful.

@williamh
Copy link
Contributor

williamh commented Oct 9, 2021

@richfelker It looks like -xdev is an option for find, but it looks like -mount is the more portable option, so I would probably use that. @vapier Isn't it reasonable to assume that /tmp is on one file system?

@vapier
Copy link
Member

vapier commented Oct 9, 2021

In that case I'll just continue to follow up on the Alpine side about fixing this in a patch, and about this as yet another reason for moving off OpenRC.

you make it sound like OpenRC is unique or weird in its behavior. as i clearly mentioned above, it is not, so any reasonable system you move to would have similar default behavior. unless you write something from scratch, in which case you're free to do whatever non-standard leaky stuff you prefer. it's not like there's that many choices out there anymore since systemd became the defacto Linux standard.

-xdev is an option for find, but it looks like -mount is the more portable option

-xdev is in POSIX, -mount is not, so not sure why you think that's more portable to use.

but as i noted, that only helps with find, not with rm, and the rm code path is the default and most common.

@kaniini
Copy link
Contributor Author

kaniini commented Oct 10, 2021

Why can't we just remove the rm stuff and always use find, which can be defanged with -xdev?

@vapier
Copy link
Member

vapier commented Oct 10, 2021

if you read the code comments you'd see:

        # Faster than raw find
        if ! rm -rf -- [!ajlq\.]* 2>/dev/null ; then

i don't recall the exact scale needed before one starts to see a difference between the two, but i don't think it was "tons of files"

@kaniini
Copy link
Contributor Author

kaniini commented Oct 10, 2021

A while ago, you noted that the logic in OpenRC originates from Debian (presumably by way of Gentoo baselayout-1).

Looking at Debian, they never use rm -rf anywhere. In fact, they unconditionally use find -depth -xdev -delete.

Is this really something that needs performance optimization?

@vapier
Copy link
Member

vapier commented Oct 10, 2021

yes, openrc is simply baselayout-2 rebranded and relicensed to BSD

using rm was a measurable difference, and people want their system to boot fast. Debian didn't really focus on that since everything in their init setup was slow. it's easy to only use numbers from the latest top of the line systems and hardware stacks and not care about older or embedded systems.

I think systemd just uses tmpfs for /tmp, so they wouldn't need to clean (like we already do if you set your system up to use tmpfs). if Debian has added -xdev to their find, I'd be fine adding it to ours. an argument could be made in either direction, but feels mostly bike shedding.

@williamh
Copy link
Contributor

williamh commented Oct 10, 2021

@vapier I thought at the time that -mount was more portable because of this wording in the find man page:

-mount Don't descend directories on other filesystems. An
alternate name for -xdev, for compatibility with some
other versions of find.

But, I'm fine with -xdev since that is in posix.

@vapier
Copy link
Member

vapier commented Oct 10, 2021

and the reason we have the rm is because we had user reports that the /tmp cleaning step took a long time

I wonder if we can guide people towards the tmpfs method by default somehow

@williamh
Copy link
Contributor

williamh commented Oct 10, 2021

I can make bootmisc complain on Linux systems if any directory in cleanup_tmp_dirs is not a mounted tmpfs; it would be a modification of the loop at line 207.
What I would do is put a warning in for a release or so then in the future stop cleaning the directories for linux systems.
I don't know if I would have to keep the code in the *bsd world since I have no idea whether *BSD supports anything like a tmpfs.
Is this what we want to do?

@williamh
Copy link
Contributor

williamh commented Oct 10, 2021

@kaniini If we do this, would you want the warning to be on the 0.44.x branch?

@williamh
Copy link
Contributor

williamh commented Oct 10, 2021

@vapier Do you have any more specific info about why we need the rm -rf? I would like to understand what the problem there is, or if we can try by relying only on find again, I'm open to doing that.

@kaniini
Copy link
Contributor Author

kaniini commented Oct 10, 2021

Yes, we should have a deprecation warning if the behavior is planned to change. Backporting would be appreciated.

@williamh
Copy link
Contributor

williamh commented Oct 10, 2021

@vapier What do you think about attempting the following instead of the rm -fr logic? I'll open a pr with this change and a deprecation warning about non-tmpfs /tmp in a second.

find . -depth -xdev ! -type d -delete
find . -depth -xdev -type d -empty -delete

@vapier
Copy link
Member

vapier commented Oct 11, 2021

not using tmpfs is not, and should not be, a problem. we should not be warning people or making it seem like there is a problem with their system. there are plenty of legitimate reasons why one would not use or want a tmpfs mount at /tmp. we should recommend that people use one if they can, but that's it.

we don't install the fstab file, but we can update the Gentoo default in baselayout and the handbook to mention it. that should at least help with new installs. other distros are also free to adjust their own setups and defaults accordingly without openrc being involved in their decision tree.

/tmp can be full of cruft from the previous boot, and exist on slow media (like MMC or spinning media). running find in the first place is slower, and running it twice is worse. we shouldn't be penalizing everyone in order to support corrupted systems. this is the wrong trade off.

we don't even know if this was the reason for the reported breakage in the first place -- a corrupt or hard linked directory could make the rootfs appear under /tmp and not as a mount (I.e. xdev wouldn't help). a corrupt filesystem could lead to data loss even just by writing to it regardless of file removal. it's impossible to protect against data loss when the system is already screwed. this is the entire reason we run fsck early on, and we don't code defensively against every possible method of corruption in every filesystem related operation.

so I'll reiterate again: if you don't like clearing /tmp due to some philosophical reason, then we already offer multiple alternatives.

@williamh
Copy link
Contributor

williamh commented Oct 11, 2021

@vapier

and the reason we have the rm is because we had user reports that the /tmp cleaning step took a long time

Can you site these reports? how long ago were they? could this be some legasy thing that isn't an issue any longer?

I wonder if we can guide people towards the tmpfs method by default somehow

I see that guidance being two parts.

  • Yes, distros can change their default fstab to support /tmp as a default. I will make this change in baselayout on Gentoo soon.
    However, it does not affect live systems.
  • If we want people to change their live systems, we need to guide them toward that change too.
    The only way I see to make that happen is to put something in bootmisc like this that warns them about the change.

@vapier
Copy link
Member

vapier commented Oct 11, 2021

this code stretches back decades, and some devs have a history of not documenting things well when they make commits (this is a huge ongoing problem in Gentoo even today), so unfortunately i only have my recollection of events and the code comments left. but as i said, i don't think telling people to buy modern big hardware is the answer to getting a fast boot. "legacy" is just code for "not my current desktop". the power & resource constraints of desktops from 10 years isn't that far off from embedded systems of today, or of container/microservices.

if you want to put something in bootmisc, you can update the clean_tmp_dirs comment to clarify its relationship to a tmpfs mount and recommend people use tmpfs if possible. This way people upgrading will see the comment when doing etc-update. There def should not be a warning issued during boot about it.

something like:

-# List of /tmp directories we should clean up
+# List of /tmp-like directories we should clean up.  tmpfs mounts are ignored since they will be
+# empty at boot.  Use of tmpfs for /tmp itself is strongly recommended if possible.
 clean_tmp_dirs="/tmp"

@robbat2
Copy link
Contributor

robbat2 commented Oct 11, 2021

Hi, @vapier didn't write that original conversion, but you need to go deep into history to see where it changed, and why.

https://gitweb.gentoo.org/proj/baselayout.git/commit/init.d?h=fb86ee4424c8918a2e006ab24cf3517eecf61c02 is where it switched from find .. -print0 | xargs -0 rm .. to rm -rf. It would help if the commit message had been correct instead of being completely unrelated.

The crucial thing is that find's -delete option is NOT POSIX. See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/find.html

If we're willing to be NOT-POSIX, then absolutely, switching to find ... -delete is very much an improvement over rm -rf, for multiple reasons, including safety and performance.

Otherwise, we're back to the point of rm -rf vs the pipe. The rm -rf will always win there because it doesn't have any forking or exec overhead.

Wiping /tmp on boot by default historically sense for MOST distros (indeed, Gentoo copied the behavior from Debian & Redhat). That doesn't mean it still makes sense today in all circumstances.

  • If /tmp is NOT persistent between boots, then cleanup is not needed.

  • If /tmp IS persistent, then the cleanup is needed for ensuring that temporary files created by crashed programs are removed when their own cleanup code did not have a chance to run. This might include coredumps with sensitive material (and those programs SHOULD take better care overall in the modern day).

@richfelker
Copy link

richfelker commented Oct 11, 2021

If -delete not being POSIX is a problem can't you just use -find ... -exec rm {} + (or rmdir for the dir case)? The hacks to avoid -exec seem to be from the pre-+ era (;) where -exec was one exec per result rather than accumulating them onto a command line. For a long time GNU find failed to support + despite it being POSIX, but that was decades ago.

@robbat2
Copy link
Contributor

robbat2 commented Oct 11, 2021

I was thinking about the overall best course of action here, and I think it's likely to just go BACK to Debian's solution, which has evolved to be tmpfiles-based and inherited from systemd: D /tmp 1777 root root - and don't traverse mountpoints within /tmp.

  • Absolutely be safer about making sure it's ACTUALLY inside the correct directory: find "${dir}" ...
  • get rid of this horrible hack: [!ajlq\.]* (originally for keeping journals, quota, aquotas, lost+found).

Regarding -exec rm {} \+, it'll still be SOME more exec calls than rm, but safety is better, and find provides a LOT more control.

Maybe detection of -delete being supported in the local find, and using that if possible, with a fallback to -exec rm {} \+ (and rmdir for removing the empty dirs afterwards).

williamh added a commit to williamh/openrc that referenced this issue Oct 11, 2021
@williamh
Copy link
Contributor

williamh commented Oct 11, 2021

@robbat2 @richfelker @kaniini @vapier How does the pr look now? Is this ok?

@vapier
Copy link
Member

vapier commented Oct 12, 2021

our usage of tmpfiles.d today is pretty light. that said, i'm totally fine with making it a hard requirement and going all in on it. it'd allow us to deprecate the checkpath tool which was only written because we didn't have something like tmpfiles.d.

@williamh your PR is buggy & inconsistent. maybe focus on tmpfiles.d and avoid the debate.

@williamh
Copy link
Contributor

williamh commented Oct 12, 2021

@vapier If I can avoid tmpfiles.d I'd rather do that, systemd-tmpfiles isn't portable.

@robbat2
Copy link
Contributor

robbat2 commented Oct 12, 2021

@vapier can you please clarify your statement on how rm is safer than find if the filesystem has corruption? Esp. in the example case where /tmp/foobar ends up linked to / or /usr, so traversal visits unintentional places.

@vapier
Copy link
Member

vapier commented Oct 13, 2021

@vapier If I can avoid tmpfiles.d I'd rather do that, systemd-tmpfiles isn't portable.

i'm talking about the tmpfiles.d specification, not a specific implementation of the runtime.

systemd-tmpfiles itself should work on all Linux systems. it builds independently of systemd and can be installed in isolation. Gentoo has more than proved that fact.

opentmpfiles is a POSIX shell script that can run anywhere (ignoring the problems it had that we could rectify, but are choosing not to and instead telling people to use systemd-tmpfiles).

we could delete the vast majority of bootmisc by installing a single tmpfiles.d entry, and leave it to users to add more. then say OpenRC requires a tmpfiles.d implementation at runtime and we're done. if you're extremely concerned about providing a tmpfiles.d that runs under *BSD's, then you could always dust off opentmpfiles (and my partial conversion to a standalone C implementation).

continuing to bend over backwards to maintain our own ad-hoc checkpath tool instead of declarative & fast tmpfiles.d is a waste of our time, and results in less robust code that we ship (exhibit A: bootmisc).

can you please clarify your statement on how rm is safer than find if the filesystem has corruption

i didn't say it was safer, i said being defensive in the fact of filesystem corruption is a fools errand, and thus they're equiv at that point

Esp. in the example case where /tmp/foobar ends up linked to / or /usr, so traversal visits unintentional places.

i think you're misunderstanding things here. if /tmp/foobar is a hard linked directory to / or /usr, then find -xdev won't save you. find will happily descend into that subdirectory because that's all it is -- a subdirectory. xdev checks for mount points which hardlink directories are not.

yes, i'm aware that Linux tends to ban them, but that's at the level of where it won't allow them to be created via syscalls. in many filesystems, it is possible to create hardlinks in the FS itself via debugfs or raw FS manipulation, and since we're talking about FS corruption, that is possible. in fact, CrOS recently had a persistence root exploit that involved creating a directory hardlink in an EXT4 FS via debugfs.

@robbat2
Copy link
Contributor

robbat2 commented Oct 13, 2021

@vapier If you feel they are in fact equivalent, then you should have no fundamental objection to using find in place of rm.
The minimum viable change is what William is proposing, not "what's the best way we can do this".

I agree that using find/rm is not as much of an improvement as using some tmpfiles.d code, or tmpreaper, but it's small & lightweight.

If Gentoo is about choice, it SHOULD be possible to ship a bootmisc that uses find, as well as a "better" version that supports tmpfiles.d.

As for @kaniini's other request about being able to mostly disable the functionality:

  • what's the minimum set of files that need to be wiped to guarantee safe boots?
  • Is the historical list really still relevant?
  • If it's still relevant, can those entries can wind up in their own tmpfiles.d spec, and the rest of the wiping can be optional at that point?

That historical list:

  • esrv* - obsolete
  • kio* - likely safe, moved to /tmp/kde-$USER/
  • jpsock.* - never heard of this one
  • .fam* - obsolete by inotify?
  • .esd* - really obsolete unless you hate ALSA & PulseAudio
  • orbit-* - obsolete?
  • ssh-* - ssh agent? probably safe to delete on boot
  • ksocket-* - obsolete KDE?
  • .*-unix - maybe convert to .{ICE,X11}-unix, which need to exist for X still
  • .X*-lock - keep cleanup, can prevent X from starting
  • .Xauth* - probably ensure cleanup still, leaks X MIT auth cookies

And the other to clarify: what files in /tmp are really dangerous to remove?

  • {a,}quota{.user,.group} - if we're deleting things, why do we need to keep quota data?
  • lost+found - if we're cleaning up, why keep it at all?
  • journal - historical ext[34] journal file, likely needed to avoid corruption

@kaniini
Copy link
Contributor Author

kaniini commented Oct 13, 2021

Alpine would prefer to avoid tmpfiles.d, as that opens the pandora's box of importing systemd components, or writing our own implementation. We could write our own implementation, but that puts us in a position where we are permanently in a position of chasing systemd. We chose to do so with pkg-config, because pkg-config was not in a developmental state where we had to worry about chasing it.

@vapier
Copy link
Member

vapier commented Oct 13, 2021

If you feel they are in fact equivalent

please do not extrapolate what i've said in directions that i did not say or mean. i clearly stated that, in the context of a corrupt filesystem, these commands are effectively equivalent wrt safety. that has nothing to do with their speed or overhead costs in the non-corrupt case, which is to say, the 99.9999% of cases.

If Gentoo is about choice, it SHOULD be possible to ship a bootmisc that uses find, as well as a "better" version that supports tmpfiles.d.

you already have a choice: pick whatever tmpfilesd implementation you want, or write your own. much like you can pick your own POSIX compliant shell. or POSIX find implementation. or any other low level program. that's not the same as OpenRC providing multiple parallel implementations. it's a bit of a stretch to try and say "it's a matter of Gentoo choice" here.

As for @kaniini's other request about being able to mostly disable the functionality:

i already documented how to do that.
(1) use a tmpfs mount at /tmp and you disable just about everything (*).
or
(2a) set wipe_tmp=no to disable the large rm/find hammer.
(2b) set clean_tmp_dirs=" " to disable the reduced set of rm calls and there's pretty much nothing left (*).

(*) currently the minimal set you can attain is what i already highlighted -- ICE-unix & X11-unix initialization.

If it's still relevant, can those entries can wind up in their own tmpfiles.d spec, and the rest of the wiping can be optional at that point?

moving individual entries out of the historical list and to their relevant packages sounds fine to me. but that's still orthogonal to the default blanket wipe (which we should retain), and to the fact that it's already possible to disable these things.

williamh added a commit to williamh/openrc that referenced this issue Oct 13, 2021
@williamh
Copy link
Contributor

williamh commented Oct 13, 2021

It was pointed out to me that the -name switch was on the original find for a reason, so I put that back and added -xdev to all of the find calls. Also, if we make the directory, we don't need to wipe it since it will be empty.
This is also a less invasive change than the previous proposal.
@kaniini @robbat2 @vapier
Please look at the pr again and let me know what you think. Hopefully this one will be the final iteration. ;-)

@williamh
Copy link
Contributor

williamh commented Oct 13, 2021

@richfelker please comment if you can also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants