Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd: add non-stub implementation of snap-update-ns #2718
Conversation
Some checks were not successful
zyga
added this to the
2.23 milestone
Jan 26, 2017
jdstrand
reviewed
Jan 26, 2017
This is just a high-level look at the PR per @zyga's request and not a deep review. This appears to adhere to the spec except for the TODO part for the wiki (I summarized my understanding there).
While I think for a design perspective, this is fine to proceed, @tyhicks and I agree that https://bugs.launchpad.net/apparmor/+bug/1656121 needs to be worked out before the security team spends time on a deep review of this PR, as the outcome of that bug might impact the implementation and full review.
| +int main(int argc, char **argv) | ||
| +{ | ||
| + if (argc != 2) { | ||
| + printf("Usage: snap-alter-ns SNAP-NAME"); |
| + return 0; | ||
| + } | ||
| + // TODO: correct the wiki, we don't quit if something is not present as | ||
| + // this is a valid case as well (e.g. a profile gets removed). |
jdstrand
Jan 26, 2017
Contributor
The use cases are:
- neither exists: nothing to do
- current exists but desired does not: unmount everything and remove current (profile removal)
- current does not exist, but desired does: mount everything and create current (first run after profile add)
- both exist: mount/umount as necessary and update current (subsequent updates not including profile removal)
zyga
Jan 27, 2017
Contributor
- neither exist == both are empty => nothing to do (check, agreed)
- current exists but desired does not == treat desired as empty => will unmount everything in current (check, agreed)
- current does not exist but desired does == treat current as empty => will mount everything in desired (check, agreed with the exception that I plan to make snap-confine write current on successful startup)
- both exists: unmount those that are not in desired but are in current, mount those that are in desired but are not in current; (check, agreed)
I think we are in agreement here.
| + } | ||
| + } | ||
| + if (num_skipped > 0) { | ||
| + debug("Mount namespace of snap %s has not been fully altered.", |
| + struct mountinfo __attribute__ ((cleanup(cleanup_mountinfo))) * mi = | ||
| + NULL; | ||
| + const char *mnt_dir; | ||
| + mi = parse_mountinfo(NULL); |
| + mountinfo_entry_parent_id | ||
| + (parent_mi_entry)); | ||
| + } | ||
| + return false; |
jdstrand
Jan 26, 2017
Contributor
While there is a lot going on here to deal with debug info, it's good that you performed this "mounting over mountpoints" check.
zyga
Jan 27, 2017
Contributor
I'll trim down the debug info if required (though we don't show it unless you ask for it). I'd like to return a simple and understandable error message when we reject the over-mount.
|
I'll address the review feedbackc and focus on investigating the kernel issue. |
zyga commentedJan 25, 2017
This patch adds an almost full implementation of snap-update-ns.
This new executable includes a simple utility module for working with
mount entries (e.g. /etc/fstab) that is based on the C library for the
hard part (parsing).
The missing bits are:
The last missing bit requires some more explanation. The tool already
inspect existing mount points but due to the way bind mounts are exposed
in the kernel it is non-trivial to realize that a given place is a bind
mount (even if we have all the information in front of us).
One notable unfortunate fact is that snap-update-ns cannot take advantage
of valgrind for debugging:
--41370-- WARNING: unhandled amd64-linux syscall: 308
--41370-- You may be able to write your own handler.
--41370-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
--41370-- Nevertheless we consider this a bug. Please report
--41370-- it at http://valgrind.org/support/bug_reports.html.
cannot re-associate with mount namespace of snap unix-domain-client: Function not implemented
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com