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.

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 flag to only disable pathFsOpts.ClientInodes (but keep caches) #435

Closed
adrian-bl opened this issue Nov 4, 2019 · 4 comments
Closed
Labels

Comments

@adrian-bl
Copy link

gocryptfs currently doesn't play well with btrfs subvolumes due to pathfs's inode mapping:

Steps to reproduce

Assume the following setup:

/foo <-- btrfs filesystem and cipherdir
/foo/M6LB0sjNfK2HT2c0UbAaMw <--- subvolume (btrfs subvol create .....)
/foo/cULkSfhPk6LTWKsPPYO2Sw <--- another subvolume

M6LB0sjNfK2HT2c0UbAaMw and cULkSfhPk6LTWKsPPYO2Sw are the encrypted names of two directories in the root of the encrypted filesystem. Let's call them dir1 and dir2 in cleartext.

Mounting such a setup with gocryptfs /foo /bar will serve a filesystem at /bar with 2 subfolders, just as expected:

$ ls /bar
dir1 dir2

However: The issue with such a setup is that M6LB0sjNfK2HT2c0UbAaMw and cULkSfhPk6LTWKsPPYO2Sw are considered to be separated devices by BTRFS / VFS and therefore may use the same inodes (this is not exclusive to btrfs subvolumes: ZFS or a mount within the cipherdir would have the same eissue).

When this happens (inode collision in the 2 folders), gocryptfs logs
2019/11/04 21:24:22 Found linked inode, but Nlink == 1, ino=8191, fullPath="... and the client sees various fun errors (usually '..not a directory..').

Looking at the go-fuse code shows that the error comes from here:
https://github.com/hanwen/go-fuse/blob/master/fuse/pathfs/pathfs.go#L570
..after it detected that something is fishy with its clientInodeMap - which is not surprising as the code only tracks the inode but omits the device id - therefore making it easy to have 'bogus' data in the cache.

My current workaround is to use the --sharedstorage flag which disables pathFSOpts.ClientInodes - but also disables all caching, which isn't really needed in this situation.

It would therefore be great to have a flag/option to only disable pathFSOpts.ClientInodes while still keeping the caching functionality.

(Also: it may make sense to disable ClientInodes on BTRFS and ZFS by default? .. not sure :-) )

Oh: and thanks a lot for writing gocryptfs!

@rfjakob rfjakob added the bug label Nov 16, 2019
@rfjakob
Copy link
Owner

rfjakob commented Nov 16, 2019

This is actually a bug in gocryptfs. The device number in those subvolumes is different, right? gocryptfs should not get confused by inodes on different devices.

@adrian-bl
Copy link
Author

The device number in those subvolumes is different, right?

Yes: each btrfs or zfs subvolume has its own unique device id. (...something which gocryptfs should probably also expose in the fuse mount)

rfjakob added a commit that referenced this issue Nov 16, 2019
Generates unique inode numbers for files on different
devices.

#435
@rfjakob
Copy link
Owner

rfjakob commented Nov 16, 2019

Should be fixed now. Inodes from different devices are now mapped to the number range above 10000000000000000000 . If you have to opportunity to test again this would be very much appreciated!

@adrian-bl
Copy link
Author

Thanks a lot for looking into this!

I can confirm that the issue i was seeing is fixed in the current HEAD.

Simple verification test:

TEST contains 2 btrfs subvolumes, each subvolume has a few files:

with gocryptfs at HEAD:

# find TEST -exec stat {} \; | awk '/Device/ { print $4 } ' | sort | uniq -c |sort -n
      1 10000000000000000000
      1 10000000000000000001
      1 10000000000000000002
      1 10000000000000000012
      1 10000000000000000013
      1 10000000000000000014
      1 10000000000000000015
      1 10000000000000000016
      1 10000000000000000017
      1 10000000000000000018
      1 10000000000000000019
      1 10000000000000000020
      1 10000000000000000021
      1 10000000000000000022
      1 10000000000000000023
      1 10000000000000000024
      1 10000000000000000025
      1 10000000000000000026
      1 10000000000000000027
      1 10000000000000000028
      1 10000000000000000029
      1 10000000000000000030
      1 10000000000000000031
      1 10000000000000000032
      1 10000000000000000033
      1 10000000000000000034
      1 10000000000000000035
      1 10000000000000000036
      1 10000000000000000037
      1 10000000000000000038
      1 10000000000000000039
      1 10000000000000000040
      1 10000000000000000041
      1 10000000000000000042
      1 10000000000000000043
      1 10000000000000000044
      1 10000000000000000045
      1 262
      1 269

Version without bb6155a:

# find TEST -exec stat {} \; | awk '/Device/ { print $4 } ' | sort | uniq -c |sort -n
find: ‘TEST/b/xyz’: Not a directory   # <- ouch
      1 260
      1 261
      1 263
      1 264
      1 265
      1 266
      1 267
      1 268
      1 271
      1 272
      1 273
      2 256 # <- dup
      2 258 # <- dup
      2 262 # <- dup
      2 269 # <- dup
      2 270 # <- dup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants