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

Native encryption - datasets do not mount with newer version #6597

Closed
mklemm2 opened this issue Sep 3, 2017 · 6 comments
Closed

Native encryption - datasets do not mount with newer version #6597

mklemm2 opened this issue Sep 3, 2017 · 6 comments
Assignees

Comments

@mklemm2
Copy link

mklemm2 commented Sep 3, 2017

Hi,

I have created a pool with an encrypted dataset using tcaputi@4188aa3

I experienced some issues and updated to tcaputi@5aef9be

Now I can't mount the dataset anymore. Loading the encryption keys seems to work but when I try to actually mount the dataset, I get

filesystem 'storage' can not be mounted: Invalid exchange
cannot mount 'storage': Invalid argument

After going back to the older version, the dataset mounts without problems and I do have a backup ... but I would rather not move 12T data, again. So is there anything I can do?

@ghost
Copy link

ghost commented Sep 3, 2017

same here, 100% reproducible after zfs send | zfs recv of an encrypted filesystem. zpool scrub is clean, all other operations work fine, only mount fails.

@tcaputi
Copy link
Contributor

tcaputi commented Sep 4, 2017

TL;DR
I believe your first commit is from before the patch was merged into master, correct? Unfortunately, we made 1 last small change to the on-disk format right before the patch was merged into master. This change was made due to a bug that was found involving sending empty files. I could probably provide you with a quick fix that would allow you to mount your encrypted datasets again if this was your only copy, but seeing as you have backups and this will NOT be on-disk compatible with zfs going forward I would not advise you to do so. I apologize for the inconvenience of having to re-copy your data. Feel free to send me an email if you need any further help or if my assumption is incorrect.

More detail for developers:
Before the encryption code was merged into master we found a bug sending empty files. Essentially, the MAC on the block of dnodes was incorrect. After investigating a little bit we found that this was due to a quirk in receiving empty files where the DNODE_FLAG_USED_BYTES flag was not properly preserved. It seems that this has always been the case, but has never been noticed since it normally doesn't matter. However, the MAC was calculated with that flag set and those did not match on the receiving side.

There was already a mechanism in-place for masking out dn_flags that are not portable across sends / recvs, so we simply added this flag to the blacklist, which resolved the issue. Unfortunatley this was an on-disk format change but it was definitely the clean and "correct" solution and the code wasn't merged into master yet so we felt ok making it.

PS: The scrubs return clean because scrub does NOT check the MAC (which allows scrubs to work without loaded keys).

@numinit
Copy link
Contributor

numinit commented Nov 27, 2017

@tcaputi - a patch against latest master would be much appreciated, if you have the time. I looked for references to DNODE_FLAG_USED_BYTES, but couldn't find where it was being blacklisted in dnode_*.c.

@tcaputi
Copy link
Contributor

tcaputi commented Nov 27, 2017

@numinit I think this should do it (its actually a whitelist instead of a blacklist):

diff --git a/include/sys/dnode.h b/include/sys/dnode.h
index a2bef9d..ef927f9 100644
--- a/include/sys/dnode.h
+++ b/include/sys/dnode.h
@@ -150,7 +150,8 @@ enum dnode_dirtycontext {
 /* User/Group dnode accounting */
 #define	DNODE_FLAG_USEROBJUSED_ACCOUNTED	(1 << 3)
 
-#define	DNODE_CRYPT_PORTABLE_FLAGS_MASK		(DNODE_FLAG_SPILL_BLKPTR)
+#define	DNODE_CRYPT_PORTABLE_FLAGS_MASK		\
+	(DNODE_FLAG_USED_BYTES | DNODE_FLAG_SPILL_BLKPTR)
 
 /*
  * VARIABLE-LENGTH (LARGE) DNODES

Keep in mind, this should not be used as a long-term fix since this will not be supported down the line.

@numinit
Copy link
Contributor

numinit commented Nov 27, 2017

@tcaputi thanks, have a thin client I was testing ZFS encryption on that I'd like to get a couple files off of :-)

@tcaputi
Copy link
Contributor

tcaputi commented Nov 27, 2017

Let me know if it doesn't work... (I can't say I have any pools from then lying around).

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

No branches or pull requests

3 participants