cmd/snap-confine: add detection of stale mount namespace

[zyga]

This patch adds detection of stale mount namespaces. This is 1/3 of
the required code, as we also need the code that checks if it is safe to
discard a stale namespace as well as code that actually performs the
discard (all somewhat tricky due to apparmor issues).

The detection logic is simple. We know which base snap we would like to
use and we measure it's major:minor device numbers. We then jump into an
existing namespace and measure the major:minor numbers of the device
backing the root filesystem. If they are different then the mount
namespace is stale.

The next branches will bring in detection of stale and unused
namespace, thanks to soon-landing freezer cgroup code.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

[codecov-io]

Codecov Report

Merging #3999 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3999      +/-   ##
==========================================
- Coverage    75.8%   75.78%   -0.02%     
==========================================
  Files         433      433              
  Lines       37174    37293     +119     
==========================================
+ Hits        28180    28264      +84     
- Misses       7022     7055      +33     
- Partials     1972     1974       +2

Impacted Files	Coverage Δ
overlord/hookstate/ctlcmd/helpers.go	50.94% <0%> (-13.77%)	⬇️
interfaces/kmod/backend.go	74.07% <0%> (-2.85%)	⬇️
interfaces/mount/backend.go	58.33% <0%> (-2.54%)	⬇️
interfaces/udev/backend.go	75.36% <0%> (-2.25%)	⬇️
interfaces/dbus/backend.go	60% <0%> (-1.65%)	⬇️
interfaces/seccomp/backend.go	75% <0%> (-1.6%)	⬇️
overlord/ifacestate/helpers.go	60.26% <0%> (-1.23%)	⬇️
interfaces/systemd/backend.go	53.84% <0%> (-1.22%)	⬇️
daemon/api.go	71.62% <0%> (ø)	⬆️
interfaces/ifacetest/backend.go	0% <0%> (ø)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc06d51...5aaa8db. Read the comment docs.

[jdstrand]

As requested, this is only a preliminary review on the approach and it is anot a full review (though I did make a couple small suggestions). I think the approach makes sense: squashfs are looback files that show up as maj:min 7:NN. Therefore, get the maj:min of the current base snap and see if it is mounted in the mount namespace and if it isn't, it is stale. You are comparing both the actual path in /snap/base/rev and the maj:min, which avoids accidentally matching against hostfs.

+1 on the general approach.

[jdstrand]

Can you initialize this to be: char base_snap_rev[PATH_MAX] = { 0, };. You may as well do the same with fname.

[zyga]

Done

[jdstrand]

readlink() can truncate. The components SNAP_MOUNT_DIR, base_snap_name and revision should keep it from doing so, but you can also simply check if 'base_snap_rev[sizeof base_snap_rev - 1] != '\0' and die(). This way you don't have to worry about assumptions changing down the line.

[zyga]

Done

[mvo5]

This looks like it might become a helper function?

[zyga]

Yes, I was thinking the same thing. I just wanted to show the initial code to get quick evaluation before I started messing with it more closely.

[zyga]

Done

[mvo5]

Thanks for working on this! Nice progress, one question about die()ing though.

[mvo5]

Hm, if try mode does not work with this function and we die() below on not-found, isn't that dangerous? I.e. should we just keep going if we cannot find the base snaps dev_t?

[zyga]

It will not die but it will find the block device for the (typically) home partition. What I mean by this is that we want to remember which directory of that block device was used so that when you do this, it still detects the staleness:

cd ~/foo/base
snap try
cd ~/bar/base
snap try

[mvo5]

Thanks for explaining this.

[niemeyer]

A few trivial remarks, but LGTM overall.

Glad to see this moving forward. Thanks!

[niemeyer]

If we need to reuse the info, let's hand it in from the outside instead of parsing it on every leaf function.

[zyga]

We're not parsing mountinfo in any vicinity to this code but I'll do a review of when we do parse it and if we could save some calls. I'll do this in a separate PR though.

[niemeyer]

Why the syntax initializing these to a zero first entry here? We don't do that in the change above. What's the overall convention for snap-confine, and what's the rationale behind it?

[zyga]

This is a new thing that @jdstrand has requested that we do. I'll do a pass to ensure all the buffers are zero-initialized so that we have consistency.

[niemeyer]

Where is this array being initialized to all zeros? Are we using some compiler flag to do that?

[zyga]

This is what the char base_snap_rev[PATH_MAX] = {0, } does. If there is any explicit initialisation then all other elements are implicitly initialised to zero.

[niemeyer]

The fact readlink truncated says absolutely nothing to a user or even to a developer. That's how we end up with those very awkward messages showing up in logs and in the terminal. Let's please be careful to have nice user-oriented error messages at all times.

[zyga]

Sure, I'll correct this and anything else that may be wrong

[niemeyer]

Also a very vague message, and curious wording too.

Note that "namespace" will provide zero hints of what this is really about even to developers that understand in depth the ideas we're dealing with here.

Instead this could be something like:

if (should_discard_ns) {
    debug("discarding obsolete base filesystem namespace")
}

[zyga]

Ah, sorry, this is just a temporary measure before the rest of this feature is present. I'll update this too.

[mvo5]

Gustavo asked in the other PR that we use { 0 } as the convention. Maybe update it here as well for consistency? Or we can do a followup, I would like to merge this one actually :)

[zyga]

Updated