Handle bind mounts correctly, and other mount-related improvements #154
Conversation
Will do, thanks for splitting this out into such nice commits! This is a fairly large PR, but it's mostly test code, so it should be easier to review than #148. |
There was a problem hiding this comment.
This looks mostly good, we mostly just need to figure out what bind mount cases we can safely support.
Also, right now GetMount() is the only function that uses mountsByPath after initialization, and that can be replaced with a wrapper around FindMount(), but I should fix that up in a future PR.
Make it clear that this refers to a type of filesystem such as "ext4", rather than to a specific filesystem instance.
fscrypt doesn't currently do anything with the mount options, so remove them from the Mount structure for now.
Make it clearer that this function loads data into global data structures, and doesn't return anything.
This is done in the latest version: |
filesystem/mountpoint.go
Outdated
| str[i+1] >= '0' && str[i+1] < '8' && | ||
| str[i+2] >= '0' && str[i+2] < '8' && | ||
| str[i+3] >= '0' && str[i+3] < '8' { | ||
| b = ((str[i+1] - '0') << 6) | | ||
| ((str[i+2] - '0') << 3) | | ||
| (str[i+3] - '0') |
There was a problem hiding this comment.
I agree with not having strconv handle all of the escaping, but it probably should handle the octal encoding. Something like:
func unescapeString(str string) string {
escapedStr := make([]byte, len(str))
j := 0
for i := 0; i < len(str); i++ {
b := str[i]
if b == '\\' && i+4 <= len(str) {
if parsed, err := strconv.ParseInt(str[i+1:i+4], 8, 8); err == nil {
b = parsed
i += 3
}
}
escapedStr[j] = b
j++
}
return string(escapedStr[:j])
}or if we use strings.Builder
func unescapeString(str string) string {
var sb strings.Builder
for i := 0; i < len(str); i++ {
b := str[i]
if b == '\\' && i+4 <= len(str) {
if parsed, err := strconv.ParseInt(str[i+1:i+4], 8, 8); err == nil {
b = parsed
i += 3
}
}
sb.WriteByte(b)
}
return sb.String()
}There was a problem hiding this comment.
Done with the strings.Builder version, but I kept i + 3 < len(str) rather than i + 4 <= len(str). I find the first version easier to understand since it checks whether 3 more bytes are available beyond the current one.
Also, parsed needed to be cast to uint8.
Change loadMountInfo() to load the mounts directly from /proc/self/mountinfo, rather than use the mntent.h C library calls. This is needed for correct handling of bind mounts and of "/dev/root", since /proc/self/mountinfo has extra fields which show the mounted subtree and the filesystem's device number. /proc/mounts lacks these fields, and the C library calls can't provide them. To start, this patch just switches to using /proc/self/mountinfo, without doing anything with the extra fields yet. As a bonus, this eliminates all C code in mountpoint.go.
The kernel always shows mountpoints as absolute paths without symlinks, so there's no need to canonicalize them in userspace.
Add a utility type and functions for handling device numbers.
A root filesystem mounted via the kernel command line always has a
source of "/dev/root", which isn't a real device node. This makes
fscrypt think this filesystem doesn't have a source device, which breaks
creating login passphrase-protected directories on other filesystems:
fscrypt encrypt: filesystem /: no device for mount "/": system error: cannot create filesystem link
This also makes 'fscrypt status' show a blank source device:
MOUNTPOINT DEVICE FILESYSTEM ENCRYPTION FSCRYPT
/ ext4 supported Yes
To fix this case, update loadMountInfo() to map the device number to the
device name via sysfs rather than use the mount source field.
The previous patch fixed making linked protectors to /dev/root, by setting Mount.Device to the real device node rather than /dev/root. That's good, but it also hints that the linked protector handling is unnecessarily fragile, as it relies on the device node name matching exactly. The Linux kernel allows the same device to have multiple device nodes, and path comparisons are slow and error-prone in general. Change it to compare the device number instead.
Currently, fscrypt treats bind mounts as separate filesystems. This is broken because fscrypt will look for a directory's encryption policy in different places depending on which mount it's accessed through. This forces users to create an fscrypt metadata directory at every bind mount, and to copy fscrypt metadata around between mounts. Fix this by storing fscrypt metadata only at the root of the filesystem. To accomplish this: - Make mountsByDevice store only a single Mount per filesystem, rather than multiple. For this Mount, choose a mount of the full filesystem if available, preferably a read-write mount. If the filesystem has only bind mounts, store a nil entry in mountsByDevice so we can show a proper error message later. - Change FindMount() and GetMount() to look up the Mount by device number rather than by path, so that they don't return different Mounts depending on which path is used. - Change AllFilesystems() to not return bind mounts. - Due to the above changes, the mountsByPath map is no longer needed outside of loadMountInfo(). So make it a local variable there. Resolves #59
Add a version of loadMountInfo() that takes an io.Reader parameter to allow injecting a custom mountinfo file, then add some unit tests.
|
Merging, Travis is passing, but GitHub just isn't updating: |
Store fscrypt metadata in only one place per filesystem, so that bind mounts don't get their own metadata directories (which was ambiguous, as the same file may be accessible via multiple mounts).
Also correctly set the source device for root filesystems mounted via the kernel command line, and fix creating linked protectors to such filesystems.
See the commit messages for details.