Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MetadataPath field to Mount struct #327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add MetadataPath field to Mount struct #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion filesystem/filesystem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ var SortDescriptorsByLastMtime = false
// "/", meaning that the entire filesystem is mounted, but
// it can differ for bind mounts.
// ReadOnly - True if this is a read-only mount
// MetadataPath - Absolute path to metadata information, if
// different from mountpoint.
//
// In order to use a Mount to store fscrypt metadata, some directories must be
// setup first. Specifically, the directories created look like:
Expand Down Expand Up @@ -176,6 +178,7 @@ type Mount struct {
DeviceNumber DeviceNumber
Subtree string
ReadOnly bool
MetadataPath string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// PathSorter allows mounts to be sorted by Path.
Expand Down Expand Up @@ -211,7 +214,12 @@ func (m *Mount) String() string {

// BaseDir returns the path to the base fscrypt directory for this filesystem.
func (m *Mount) BaseDir() string {
rawBaseDir := filepath.Join(m.Path, baseDirName)
var rawBaseDir string
if m.MetadataPath != "" {
rawBaseDir = m.MetadataPath
} else {
rawBaseDir = filepath.Join(m.Path, baseDirName)
}
// We allow the base directory to be a symlink, but some callers need
// the real path, so dereference the symlink here if needed. Since the
// directory the symlink points to may not exist yet, we have to read
Expand Down
39 changes: 29 additions & 10 deletions filesystem/mountpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my other comment, I think /.fscrypt should be the main mount in this case.

metadataPath := ""
// Index this filesystem's mounts by path. Note: paths are unique here,
// since non-last mounts were already excluded earlier.
//
Expand Down Expand Up @@ -240,6 +241,14 @@ func findMainMount(filesystemMounts []*Mount) *Mount {
uncontainedSubtrees := make(map[string]bool)
addUncontainedSubtreesRecursive(uncontainedSubtrees, mntNode, allUncontainedSubtrees)
if len(uncontainedSubtrees) != len(allUncontainedSubtrees) {
if mnt.Subtree == "/"+baseDirName && !allSubtrees["/"] {
metadataPath = mnt.Path
} else if len(lookuppath) > 0 &&
(mainMount == nil || mainMount.ReadOnly ||
(strings.HasPrefix(lookuppath, mnt.Path) &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

(len(lookuppath) == len(mnt.Path) || lookuppath[len(mnt.Path)] == '/'))) {
mainMount = mnt
}
continue
}
// If there's more than one eligible mount, they should have the
Expand All @@ -250,15 +259,25 @@ func findMainMount(filesystemMounts []*Mount) *Mount {
return nil
}
// Prefer a read-write mount to a read-only one.
if mainMount == nil || mainMount.ReadOnly {
if filepath.Base(mnt.Path) != baseDirName &&
(mainMount == nil || mainMount.ReadOnly ||
(len(lookuppath) > 0 && strings.HasPrefix(lookuppath, mnt.Path) &&
(len(lookuppath) == len(mnt.Path) || lookuppath[len(mnt.Path)] == '/'))) {
mainMount = mnt
}

if filepath.Base(mnt.Path) == baseDirName {
metadataPath = mnt.Path
}
}
if mainMount != nil {
mainMount.MetadataPath = metadataPath
}
return mainMount
}

// This is separate from loadMountInfo() only for unit testing.
func readMountInfo(r io.Reader) error {
func readMountInfo(r io.Reader, path string) error {
mountsByPath := make(map[string]*Mount)
mountsByDevice = make(map[DeviceNumber]*Mount)

Expand Down Expand Up @@ -292,21 +311,21 @@ func readMountInfo(r io.Reader) error {
append(allMountsByDevice[mnt.DeviceNumber], mnt)
}
for deviceNumber, filesystemMounts := range allMountsByDevice {
mountsByDevice[deviceNumber] = findMainMount(filesystemMounts)
mountsByDevice[deviceNumber] = findMainMount(filesystemMounts, path)
}
return nil
}

// loadMountInfo populates the Mount mappings by parsing /proc/self/mountinfo.
// It returns an error if the Mount mappings cannot be populated.
func loadMountInfo() error {
func loadMountInfo(path string) error {
if !mountsInitialized {
file, err := os.Open("/proc/self/mountinfo")
if err != nil {
return err
}
defer file.Close()
if err := readMountInfo(file); err != nil {
if err := readMountInfo(file, path); err != nil {
return err
}
mountsInitialized = true
Expand All @@ -324,7 +343,7 @@ func filesystemLacksMainMountError(deviceNumber DeviceNumber) error {
func AllFilesystems() ([]*Mount, error) {
mountMutex.Lock()
defer mountMutex.Unlock()
if err := loadMountInfo(); err != nil {
if err := loadMountInfo(""); err != nil {
return nil, err
}

Expand All @@ -345,7 +364,7 @@ func UpdateMountInfo() error {
mountMutex.Lock()
defer mountMutex.Unlock()
mountsInitialized = false
return loadMountInfo()
return loadMountInfo("")
}

// FindMount returns the main Mount object for the filesystem which contains the
Expand All @@ -355,7 +374,7 @@ func UpdateMountInfo() error {
func FindMount(path string) (*Mount, error) {
mountMutex.Lock()
defer mountMutex.Unlock()
if err := loadMountInfo(); err != nil {
if err := loadMountInfo(path); err != nil {
return nil, err
}
deviceNumber, err := getNumberOfContainingDevice(path)
Expand Down Expand Up @@ -431,7 +450,7 @@ func getMountFromLink(link string) (*Mount, error) {
// Lookup mountpoints for device in global store
mountMutex.Lock()
defer mountMutex.Unlock()
if err := loadMountInfo(); err != nil {
if err := loadMountInfo(searchPath); err != nil {
return nil, err
}
mnt, ok := mountsByDevice[deviceNumber]
Expand Down
87 changes: 86 additions & 1 deletion filesystem/mountpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ func endLoadMountInfoTest() {
}

func loadMountInfoFromString(str string) {
readMountInfo(strings.NewReader(str))
readMountInfo(strings.NewReader(str), "")
}

func loadMountInfoFromStringWithPath(str string, path string) {
readMountInfo(strings.NewReader(str), path)
}

func mountForDevice(deviceNumberStr string) *Mount {
Expand Down Expand Up @@ -373,6 +377,87 @@ func TestLoadAmbiguousMounts(t *testing.T) {
}
}

// Test when the .fscrypt directory is mounted directly
func TestLoadMetadataDir(t *testing.T) {
tempDir, err := ioutil.TempDir("", "fscrypt")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(tempDir)
tempDir, err = filepath.Abs(tempDir)
if err != nil {
t.Fatal(err)
}
if err := os.Mkdir(tempDir+"/.fscrypt", 0700); err != nil {
t.Fatal(err)
}
if err := os.MkdirAll(tempDir+"/home", 0700); err != nil {
t.Fatal(err)
}
if err := os.Mkdir(tempDir+"/home2", 0700); err != nil {
t.Fatal(err)
}
mountinfo := fmt.Sprintf(`
222 15 259:3 /.fscrypt %s rw shared:1 - ext4 /dev/root rw
222 15 259:3 /home %s rw shared:1 - ext4 /dev/root rw
222 15 259:3 /foo %s rw shared:1 - ext4 /dev/root rw
`, tempDir+"/.fscrypt", tempDir+"/home", tempDir+"/home2")
beginLoadMountInfoTest()
defer endLoadMountInfoTest()
loadMountInfoFromStringWithPath(mountinfo, "/home/userA")
mnt := mountForDevice("259:3")
if mnt == nil {
t.Fatal("Failed to load mount")
}
if mnt.Path != tempDir+"/home" {
t.Error("Wrong path")
}
if mnt.MetadataPath != tempDir+"/.fscrypt" {
t.Error("Wrong metadata path")
}
}

// Test when the .fscrypt directory is mounted directly but with a subtree
// equal to '/'. In this case only the mount path can help.
func TestLoadMetadataDirNoSubtree(t *testing.T) {
tempDir, err := ioutil.TempDir("", "fscrypt")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(tempDir)
tempDir, err = filepath.Abs(tempDir)
if err != nil {
t.Fatal(err)
}
if err := os.Mkdir(tempDir+"/.fscrypt", 0700); err != nil {
t.Fatal(err)
}
if err := os.MkdirAll(tempDir+"/home", 0700); err != nil {
t.Fatal(err)
}
if err := os.Mkdir(tempDir+"/home2", 0700); err != nil {
t.Fatal(err)
}
mountinfo := fmt.Sprintf(`
222 15 259:3 / %s rw shared:1 - ext4 /dev/root rw
222 15 259:3 / %s rw shared:1 - ext4 /dev/root rw
222 15 259:3 / %s rw shared:1 - ext4 /dev/root rw
`, tempDir+"/.fscrypt", tempDir+"/home", tempDir+"/home2")
beginLoadMountInfoTest()
defer endLoadMountInfoTest()
loadMountInfoFromStringWithPath(mountinfo, "/home/userA")
mnt := mountForDevice("259:3")
if mnt == nil {
t.Fatal("Failed to load mount")
}
if mnt.Path != tempDir+"/home" {
t.Error("Wrong path")
}
if mnt.MetadataPath != tempDir+"/.fscrypt" {
t.Error("Wrong metadata path")
}
}

// Test making a filesystem link (i.e. "UUID=...") and following it, and test
// that leading and trailing whitespace in the link is ignored.
func TestGetMountFromLink(t *testing.T) {
Expand Down