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

tmpfiles: remove old ICE and X11 sockets at boot #6979

Merged
merged 2 commits into from
Oct 5, 2017

Conversation

fcrozat
Copy link
Contributor

@fcrozat fcrozat commented Oct 3, 2017

When not using tmpfs based /tmp, leftover sockets
might prevent X startup. Ensure directory is clean at boot time.

When not using tmpfs based /tmp, leftover sockets
might prevent X startup. Ensure directory is clean at boot time.
@poettering
Copy link
Member

poettering commented Oct 4, 2017

hmm, wouldn't it be better to change the three top-level lines from "d" to "D" instead? (i.e. create + empty instead of just create?)

@poettering poettering added tmpfiles reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Oct 4, 2017
@poettering
Copy link
Member

(also, this really should move to some X11 package sooner or later)

@fcrozat
Copy link
Contributor Author

fcrozat commented Oct 5, 2017

Updated with using D! this is much cleaner this way

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Oct 5, 2017
@poettering
Copy link
Member

we usually merged only "perfect" PRs, i.e. where each individual commit in the PR is a logical step, and not a historical one. Hence, next time, please squash patches like yours. Since this specific one is a very simple case we can do that when merging, but for the next time please keep this in mind

@mbiebl
Copy link
Contributor

mbiebl commented Oct 5, 2017

(also, this really should move to some X11 package sooner or later)

Agreed. We had this discussion in Debian a while ago to move those rules over to a X11 specific package. Our first idea was x11-common (https://packages.debian.org/sid/x11-common). Unfortunately this is Debian specific and we'd really like this to go upstream. Any ideas this should be added?

@poettering poettering merged commit 4a1f92c into systemd:master Oct 5, 2017
@poettering
Copy link
Member

@mbiebl i figure that's for the X11 folks to figure out, I don't know X11 well enough to make an informed suggestion which package should take that best.

@poettering
Copy link
Member

@whot any chance you know where best to file a bug regarding this? i.e. which X11 package might want to take this tmpfiles.d snippet over?

(we are talking about this file: https://github.com/systemd/systemd/blob/master/tmpfiles.d/x11.conf that should be dropped in /usr/lib/tmpfiles.d/ and currently is shipped with systemd but probably shouldn't be)

@whot
Copy link
Contributor

whot commented Oct 9, 2017

I'd say the X server itself, that creates the X11-unix socket and the rest are misc things effectively related to the server as well. I've got this on my todo list now, but no promises as to when this will happen, sorry

@guns
Copy link

guns commented Oct 13, 2017

I upgraded to 235 and noticed that /tmp/.X11-unix/X0 on the host was being deleted by systemd-nspawn when launched with --bind=/tmp/.X11-unix.

Does this commit account for bind mounts into the host?

@fcrozat
Copy link
Contributor Author

fcrozat commented Oct 13, 2017

According to manpage:
If the exclamation mark is used, this line is only safe of execute during boot, and can break a running system. Lines without the exclamation mark are presumed to be safe to execute at any time,
e.g. on package upgrades. systemd-tmpfiles will execute line with an exclamation mark only if option --boot is given.

@guns
Copy link

guns commented Oct 13, 2017

Well, I can see the logic behind this commit, but there is a practical problem. systemd-nspawn … --bind=/tmp/.X11-unix is a widely adopted way to communicate with the X server on the host, so this commit is breaking existing setups.

Should I open a new issue?

@guns
Copy link

guns commented Oct 13, 2017

On second thought, I think proactively clearing old X sockets on boot to account for a non-default case in which the user does not use tmpfs on /tmp is going a bit too far.

The .X11-unix dirs and friends are created on boot as a security measure and have no other side effects aside from creating a few possibly unused directories.

Deleting files within these directories, however, is strictly a matter of convenience and breaks a simple and common method for booting a machine that communicates with the host X server.

I think this PR should be reconsidered. After all, this really is an issue that X itself should handle.

@shibumi
Copy link
Contributor

shibumi commented Oct 14, 2017

Well the files are not removed they are just isolated from the rest of the filesystem. But I fully agree with you. I used that --bind=/tmp/.X11-unix feature quite often too. It would be nice if this could be enabled again. It's a nice feature to spawn applications on the desktop out of a container.

@poettering
Copy link
Member

Well. Thats a general problem of making host things available to the container in a writable way. Quite frankly the right way to fix this is to use --bind-ro= instead of --bind= so that the container payload cannot modify what you pass in. That should fix your issue robustly and safely.

@guns
Copy link

guns commented Oct 15, 2017

Quite frankly the right way to fix this is to use --bind-ro= instead of --bind=

Ah, I was under the impression that connect() on a unix socket would fail if it was accessed through a readonly bind mount, but I was wrong. Thank you for clearing that up.

@shibumi
Copy link
Contributor

shibumi commented Oct 16, 2017

@poettering Sorry to say this but bind-ro doesn't solve anything in that case.. see: #7093

SergioAtSUSE pushed a commit to SergioAtSUSE/systemd_systemd that referenced this pull request Jun 7, 2018
tmpfiles: remove old ICE and X11 sockets at boot

When not using tmpfs based /tmp, leftover sockets
might prevent X startup. Ensure directory is clean at boot time.

(cherry picked from commit 4a1f92c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed squash-on-merge tmpfiles
Development

Successfully merging this pull request may close these issues.

6 participants