Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snap-confine: add code to ensure that / or /snap is mounted "shared" #3136
Conversation
Some checks were not successful
mvo5
referenced this pull request
Apr 4, 2017
Closed
data: ship "snap.mount" service that ensures /snap is MS_SHARED #2971
zyga
requested changes
Apr 4, 2017
There's one thing that I'd like to see fixed and a few smaller nits. I will look at spread tests later.
| + struct sc_mountinfo_entry *entry = sc_first_mountinfo_entry(sm); | ||
| + while (entry != NULL) { | ||
| + const char *mount_dir = entry->mount_dir; | ||
| + if (strcmp(mount_dir, dir) == 0) { |
| +{ | ||
| + if (!is_mounted_with_shared_option("/") | ||
| + && !is_mounted_with_shared_option("/snap")) { | ||
| + sc_do_mount("/snap", "/snap", "none", MS_BIND | MS_REC, 0); |
| @@ -33,4 +33,6 @@ | ||
| **/ | ||
| void sc_populate_mount_ns(const char *snap_name); | ||
| +/** Ensure that /snap is mounted with the SHARED option */ | ||
| +void sc_ensure_shared_snap_mount(); |
zyga
Apr 4, 2017
Contributor
Can you please expand this to document how this works. It does the check on / and /snap and this is not mentioned.
| @@ -112,6 +112,10 @@ int main(int argc, char **argv) | ||
| #endif // ifdef HAVE_SECCOMP | ||
| if (geteuid() == 0) { | ||
| + // ensure that "/" or "/snap" is mounted with the |
zyga
Apr 4, 2017
Contributor
This is incorrect, you must do this while we hold a lock. Ideally the global lock in /run/ns/.lock; You want to add this to sc_initialize_ns_groups which already holds this lock and does similar treatment of /run/snapd/ns
mvo5
Apr 5, 2017
Collaborator
Hm, sc_initialize_ns_groups() feels a bit strange (conceptually). Also it requires that discard_ns links with mount-support which in turn requires libcap for discard-mount-support it seems. Nothing that cannot be fixed still feels a bit awkward. I wonder what is the worst that can happen if we don't lock - it seems the worst would be a double bind mount?
zyga
Apr 6, 2017
Contributor
If you don't lock it may get nasty. I'd rather move the locking elsewhere and allow us to do some host setup while holding a lock than to do this incorrectly.
zyga
Apr 6, 2017
Contributor
With the locking helper in #3149 we should be able to achieve this trivially now.
mvo5
added some commits
Apr 5, 2017
All checks have failed
Some checks were not successful
All checks have passed
|
We've recently been getting more and more requests about running snaps inside LXD containers and without this in place we really can't recommend any do so at this point (as refreshes don't work), so it'd be great if this could make it to the next snapd release. |
mvo5
added this to the 2.25 milestone
Apr 21, 2017
pedronis
changed the title from
snap-confine: add code o ensure that / or /snap is mounted "shared"
to
snap-confine: add code to ensure that / or /snap is mounted "shared"
Apr 24, 2017
mvo5
added some commits
Apr 24, 2017
All checks have failed
mvo5 commentedApr 4, 2017
Some systems (mostly non-systemd but also containers like lxd) do have a filesystem root that is not marked "rshared". However we need /snap to be rshared. So if "/" is not marked rshared we need to ensure that at least /snap is mounted rshared. This is done via
mount -o bind,shared /snap /snapcall in snap-confine (if it does not find a shared "/").If this lands we can probably kill the 14.04
snap.mount.servicefile.