Add MetadataPath field to Mount struct #327
Conversation
The new MetadataPath field in Mount struct is the absolute path to fscrypt metadata directory (.fscrypt), if different from where the file system is mounted (as stored in Path). This is useful when the .fscrypt directory is mounted directly instead of being visible via a whole-filesystem mount.
| @@ -176,6 +178,7 @@ type Mount struct { | |||
| DeviceNumber DeviceNumber | |||
| Subtree string | |||
| ReadOnly bool | |||
| MetadataPath string | |||
There was a problem hiding this comment.
I think it would be simpler to not have this field, and instead make BaseDir() append baseDirName only if Subtree is not equal to "/" + baseDirName.
There was a problem hiding this comment.
The need for this MetadataPath information associated with a Mount struct appeared during my testing. Without it, the chosen main mount would be the one that has a Subtree equal to "/" + baseDirName (if any). Problem is the Path associated with this Mount is used not only to locate fscrypt's metadata directory, but also to issue ioctl commands to lock or unlock for instance. So we could end up in a situation where a call to fscrypt unlock /home leads to an ioctl(FS_IOC_ADD_ENCRYPTION_KEY) on /.fscrypt. This is a problem in some circumstances; for instance I found myself not able to write to a file in a directory that I had previously unlocked.
This is why this patch implements the idea you suggested earlier, but with an additional field to the Mount struct to keep separated the notion of metadata directory from the notion of mount path. If MetadataPath is not empty, BaseDir() uses it to return the location of fscrypt's metadata directory, independently of the main mount path. If MetadataPath is empty, then BaseDir() proceeds with the main mount Path and baseDirName (the usual case when .fscrypt is not mounted directly).
There was a problem hiding this comment.
the chosen main mount would be the one that has a Subtree equal to "/" + baseDirName
Isn't that the desired behavior?
So we could end up in a situation where a call to fscrypt unlock /home leads to an ioctl(FS_IOC_ADD_ENCRYPTION_KEY) on /.fscrypt
That will work fine, since the ioctl operates on the filesystem, not on any particular file. You have to pick some file or directory on the filesystem to execute it on. Usually the root directory is chosen, but it could be any file or directory on the filesystem and the effect will be the same.
| @@ -191,7 +191,8 @@ func addUncontainedSubtreesRecursive(dst map[string]bool, | |||
| // Then, we choose one of these trees which contains (exactly or via path | |||
| // prefix) *all* mnt.Subtree. We then return the root of this tree. In both | |||
| // the above examples, this algorithm returns the first Mount. | |||
| func findMainMount(filesystemMounts []*Mount) *Mount { | |||
| func findMainMount(filesystemMounts []*Mount, lookuppath string) *Mount { | |||
There was a problem hiding this comment.
I don't understand the purpose of the lookuppath parameter. The path that the user is ultimately interested in should not affect the algorithm to determine the main mount of the filesystem.
There was a problem hiding this comment.
The lookuppath parameter is added to make sure that the main mount being chosen is the one whose Path is a parent of the directory given as the command's input parameter.
For example, consider the following mounts:
Filesystem Subtree Mountpoint
---------- ------- ----------
/dev/sda /home /home
/dev/sda /home2 /home2
/dev/sda /.fscrypt /.fscrypt
Here we want to make sure that if the command is fscrypt unlock /home2/jdoe, then the main mount chosen is the one corresponding to /home2, and not /.fscrypt or /home.
There was a problem hiding this comment.
As per my other comment, I think /.fscrypt should be the main mount in this case.
| metadataPath = mnt.Path | ||
| } else if len(lookuppath) > 0 && | ||
| (mainMount == nil || mainMount.ReadOnly || | ||
| (strings.HasPrefix(lookuppath, mnt.Path) && |
There was a problem hiding this comment.
It's tough to review this because the logic of this function is already pretty complex, in order to handle non-root subtrees. How about separating out looking for a / or /.fscrypt subtree into a separate function? So something like:
func findMainMount(filesystemMounts []*Mount) *Mount {
mnt := findRootMainMount(filesystemMounts)
if mnt != nil {
return mnt
}
return findNonRootMainMount(filesystemMounts)
}
findRootMainMount() would be new, while findNonRootMainMount() would be the existing findMainMount().
The new MetadataPath field in Mount struct is the absolute path to
fscrypt metadata directory (.fscrypt), if different from where the
file system is mounted (as stored in Path).
This is useful when the .fscrypt directory is mounted directly instead
of being visible via a whole-filesystem mount.