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

8727 Native data and metadata encryption for zfs #124

Closed
wants to merge 1 commit into from

Conversation

@lundman
Copy link

commented Jun 7, 2016

Original ZOL PR zfsonlinux/zfs#5769

@zettabot

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2016

Can one of the admins verify this patch?

@ahrens

This comment has been minimized.

Copy link
Member

commented Jun 7, 2016

@@ -39,7 +39,7 @@ MODSRCS_DIR = ../../../common/modules/zfs
GENUNIX_DIR = ../../../common/modules/genunix

CPPFLAGS += -I../../../../../lib/libzpool/common \
-I../../../../../uts/common/fs/zfs
-I../../../../../uts/common/fs/zfs -I../../../../../common/zfs

This comment has been minimized.

Copy link
@gwr

gwr Jun 7, 2016

These are a little easier to read (and manage later) if written one-per-line.

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch 3 times, most recently from 47c1067 to 5e1f9c1 Jun 8, 2016

@ahrens

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

@@ -204,6 +206,7 @@ static zfs_command_t command_table[] = {
{ "holds", zfs_do_holds, HELP_HOLDS },
{ "release", zfs_do_release, HELP_RELEASE },
{ "diff", zfs_do_diff, HELP_DIFF },
{ "key", zfs_do_crypto, HELP_CRYPTO },

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

Unless there's a good reason, these 3 strings should have the same "x":
"x", zfs_do_x, HELP_X

It looks like the indentation is wrong here, but maybe that's just github.

{ "aes-128-gcm", ZIO_CRYPT_AES_128_GCM },
{ "aes-192-gcm", ZIO_CRYPT_AES_192_GCM },
{ "aes-256-gcm", ZIO_CRYPT_AES_256_GCM },
{ NULL }

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

the alignment of the ZIO_CRYPT_... looks wrong here, but maybe its just github.

{ "none", ZFS_KEYSTATUS_NONE},
{ "unavailable", ZFS_KEYSTATUS_UNAVAILABLE},
{ "available", ZFS_KEYSTATUS_AVAILABLE},
{ NULL }

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

the alignment of the ZFS_KEYSTATUS... looks wrong here, but maybe its just github.

@@ -171,27 +171,32 @@ lzc_ioctl(zfs_ioc_t ioc, const char *name,
}

int
lzc_create(const char *fsname, enum lzc_dataset_type type, nvlist_t *props)
lzc_create(const char *fsname, enum lzc_dataset_type type, nvlist_t *props,
nvlist_t *hidden_args)

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

While I'm OK with changing the function signature, and we have done it before, I have also seen that it can become a big pain for consumers. I will be OK either way, but I'd encourage you to seriously consider adding another func that takes the additional argument. You could call it lzc_create2() -- icky but sustainable.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 8, 2016

Someone else commented about this in the mailing list and this is my response. Let me know if you agree or disagree:

I could do that, if that is what the community wants (it would take
only a few minutes). However, the comment at the top of the file says:
"Currently, the interface is Evolving (not Committed), but we intend
to commit to it once it is more complete and we determine that it
meets the needs of all consumers." It seems to me that either the we
are committed enough to the interface that we should remove that note
or we should make changes to it now while it is still "evolving".

lzc_clone(const char *fsname, const char *origin,
nvlist_t *props)
lzc_clone(const char *fsname, const char *origin, nvlist_t *props,
nvlist_t *hidden_args)

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

same here.

This comment has been minimized.

Copy link
@tcaputi
@gwr

This comment has been minimized.

Copy link

commented Jun 8, 2016

One high-level question about this work: Is any of it derived from the old OpenSolaris "zfs-crypto" project? If so, can we get some references to (and/or differences from) the previous code?

{ DMU_BSWAP_UINT64, TRUE, FALSE, "DSL deadlist map hdr" },
{ DMU_BSWAP_ZAP, TRUE, FALSE, "DSL dir clones" },
{ DMU_BSWAP_UINT64, TRUE, FALSE, "bpobj subobj" },
{ DMU_BSWAP_ZAP, TRUE, FALSE, "DSL Keychain" }

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

Unless there's a really good reason, we should use the DMU_OTN_* types rather than adding a new object type.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 8, 2016

I used DMU_OT_* because I wanted the DSL keychain to appear with a name in zdb. Of course, I can change this easily. May I ask what the reason is for not wanting to add more types?

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

Yeah, I know it isn't as nice for debugging :-(

The types here are statically allocated, so if someone else wants to add a new type, they would use the same enum value as you. Then it's impossible to integrate the two features into one codebase. For example, we have implemented several new features at Delphix that introduce new metadata objects. If we added new enum values here, we wouldn't be able to integrate both our features and your feature upstream.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 10, 2016

@ahrens

Quick question: I'm adding this change right now and everything works. Only problem is that zdb core dumps. The reason is that DSL Keychain is no longer a designated type. Instead it is now just a generic ZAP. As a result, zap_lookup() from dump_zap() panics because it should be using zap_lookup_uint64(). So my question is essentially what is the best way to fix this?

I can modify zap_dump_stats() to also return zap_phys_t->zap_flags in the zap stats. I could then look for this flag in dump_zap() . However, this gets messy because (technically) dump_zap() doesn't use the stats directly; it just calls dump_zap_stats(). So now dump_zap_stats() needs to return the flags by reference somehow (which is a messy) and all callers need to be changed to deal with this.

So should I do all of this work or leave DMU_OT_DSL_KEYCHAIN in the enum? Personally I still think being able to tell an object is a keychain in ZDB without looking for its parent is valuable as well, but that's just me.

EDIT:

To clarify what I meant above, in broader terms, with the DMU_OTN_* interface, we lose the ability to create behaviors for specific object types. Linux IOCTL definitions actually solve this exact problem by including a magic number bitmask However, they have 32 bits to work with and we have 8, so this probably won't help us much.

I see that this is a lot of text over a little zdb print function and I'm quickly leaving the scope of 'ZFS Encryption', so I can just make the change as I stated before, even if its not the cleanest code in the world. However, I think this topic will probably come up again sometime as people add new object types.

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 11, 2016

Member

How about adding an accessor function to get both kinds of flags:
int zap_get_flags(objset_t *os, uint64_t object, int *normflagsp, zap_flags_t *flagsp);

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 11, 2016

I did something a little different in the end. I added attr.za_binary_key boolean to the ZAP iterator API. Basically the field is set if the ZAP uses binary keys.

@@ -1138,7 +1138,7 @@ dmu_send_estimate_from_txg(dsl_dataset_t *ds, uint64_t from_txg,
* traverse the blocks of the snapshot with birth times after
* from_txg, summing their uncompressed size
*/
err = traverse_dataset(ds, from_txg, TRAVERSE_POST,
err = traverse_dataset(ds, from_txg, TRAVERSE_POST|TRAVERSE_NO_DECRYPT,

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

cstyle wants spaces around the | operator

dsl_keychain_destroy_sync(dsl_dir_phys(dd)->dd_keychain_obj,
tx);
(void) spa_keystore_unload_wkey_impl(dp->dp_spa,
dd->dd_object);

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

continuation lines should be indented by 4 spaces more than the previous line (looks like it's 1 tab more, here)

@@ -873,7 +873,7 @@ dsl_fs_ss_count_adjust(dsl_dir_t *dd, int64_t delta, const char *prop,

uint64_t
dsl_dir_create_sync(dsl_pool_t *dp, dsl_dir_t *pds, const char *name,
dmu_tx_t *tx)
dmu_tx_t *tx)

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

please revert, the style was correct before.

@@ -102,16 +103,18 @@ typedef enum dmu_object_byteswap {

#define DMU_OT_NEWTYPE 0x80
#define DMU_OT_METADATA 0x40
#define DMU_OT_BYTESWAP_MASK 0x3f
#define DMU_OT_ENCRYPTED 0x20
#define DMU_OT_BYTESWAP_MASK 0x1f

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

I don't see how this could work, given that DMU_OT_NUMTYPES is 54, which is less than DMU_OT_BYTESWAP_MASK (31). I would actually be OK in principle with adding a bit for ENCRYPTED, but it seems that there just isn't space.

Nevermind, DMU_OT_BYTESWAP_MASK is for DMU_BSWAP_NUMFUNCS, which is only 10.

@@ -81,7 +82,8 @@ typedef struct dsl_dir_phys {
uint64_t dd_flags;
uint64_t dd_used_breakdown[DD_USED_NUM];
uint64_t dd_clones; /* dsl_dir objects */
uint64_t dd_pad[13]; /* pad out to 256 bytes for good measure */
uint64_t dd_keychain_obj; /* DSL Keychain object number */

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

It's preferable to not add fields here, and instead add a DD_FIELD_* to the "zapified" dsl_dir. see dsl_dir_init_fs_ss_count for an example of how to do this.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 8, 2016

I can look into this

@@ -326,7 +327,8 @@ typedef enum bp_embedded_type {
typedef struct blkptr {
dva_t blk_dva[SPA_DVAS_PER_BP]; /* Data Virtual Addresses */
uint64_t blk_prop; /* size, compression, type, etc */
uint64_t blk_pad[2]; /* Extra space for the future */
uint32_t blk_iv[3]; /* IV for block level encryption */
uint32_t blk_pad; /* Extra space for the future */

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

I'd much prefer to preserve the pad for when we really need it, and instead use the 3rd DVA. The only downsides would be that you couldn't do copies=3 encryption=on, and perhaps some code complexity (which does make me sad).

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

Also, this needs to remain uint64_t's, so that it will be byteswapped correctly.

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

I'm told that it may be possible (and still secure) to use 2 of the checksum words for the IV. That would be even better than consuming the 3rd DVA.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 8, 2016

I'd much prefer to preserve the pad for when we really need it, and instead use the 3rd DVA.

@richardelling mentioned on the original github issue that the 3rd DVA was being used by Nexenta for some acceleration. (I did not confirm this, but it seemed like enough reason to avoid putting it there.)

I'm told that it may be possible (and still secure) to use 2 of the checksum words for the IV. That would be even better than consuming the 3rd DVA.

I'm already using 2 words of the checksum for the MAC. I could put the IV in the remaining 2 words, but then we really don't use any checksumming algorithm at all.

Also, this needs to remain uint64_t's, so that it will be byteswapped correctly.

OK. I can do this, although it will mean that half of the 2nd IV word is unused, which is a bit strange.

This comment has been minimized.

Copy link
@gwr

gwr Jun 9, 2016

Does it make any sense to use both a checksum and encryption at the same time?
I would think encryption would be stronger than any checksum (making the checksum pointless).

The idea comes from SMB3, where encryption replaces packet signing, instead of being in addition to packet signing.

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 9, 2016

Member

WBC uses bit #61

To be clear, that's the "X" bit which is used for encryption on Solaris, and is proposed to be used for encryption by this pull request. I'd say let's set that aside until we figure out where the IV will be stored, as that may help solve both problems.

This comment has been minimized.

Copy link
@gwr

gwr Jun 9, 2016

Just to clarify the comment I made referencing SMB3, here's a little more detail:

If the message should be encrypted, then we’ll skip the signing, because SMB 3 encryption ciphers (AES CCM and GCM) provide message integrity in addition to privacy.

My point was, as long as ZFS encryption uses a cipher that provides message integrity, then there should be no need to also have a data "checksum" for data that's encrypted.
Hope that helps.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 9, 2016

My point was, as long as ZFS encryption uses a cipher that provides message integrity, then there should be no need to also have a data "checksum" for data that's encrypted.

There is a need, actually. The message integrity given by the encryption algorithms we use is only useful if we have the key loaded. Without the regular checksum, ZFS can't detect any faults in a dataset without the keys loaded. This means that admin scrubbing, resilvering, etc all can't happen anymore.

This comment has been minimized.

Copy link
@gwr

gwr Jun 10, 2016

That's an interesting point. So yes, you essentially want encryption to be effectively encapsulated within the data structures ZFS has today (allowing scrubs, resilver, send/recv? etc).
Would an SA work for storing the IVs? I gather we can extend those as much as we like.

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 10, 2016

Unfortunately no. SA's only exist on a per-object basis and encryption is done on a per-block basis. Changing encryption to object-based is infeasible because an update of a single block in a large object would require re-encrypting the whole object. Using an SA to store per-block data won't work either once the object gets enoguh blocks since each block will still require a MAC.

@@ -395,6 +397,9 @@ _NOTE(CONSTCOND) } while (0)
#define BP_GET_LEVEL(bp) BF64_GET((bp)->blk_prop, 56, 5)
#define BP_SET_LEVEL(bp, x) BF64_SET((bp)->blk_prop, 56, 5, x)

#define BP_IS_ENCRYPTED(bp) BF64_GET((bp)->blk_prop, 61, 1)

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

You should modify the comment describing the X bit above.

This comment has been minimized.

Copy link
@tcaputi
/* physical representation of a wrapped key in the DSL Keychain */
typedef struct dsl_crypto_key_phys {
/* encryption algorithm (see zio_encrypt enum) */
uint64_t dk_crypt_alg;

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

You are storing this in a ZAP object as an array of uint8_t's. Therefore this struct must have be equivalent to an array of uint8_t's, otherwise it will not byteswap correctly.

It would be good to add a comment here explaining this.

This comment has been minimized.

Copy link
@tcaputi
@@ -2581,7 +2662,7 @@ zio_dva_unallocate(zio_t *zio, zio_gang_node_t *gn, blkptr_t *bp)
*/
int
zio_alloc_zil(spa_t *spa, uint64_t txg, blkptr_t *new_bp, blkptr_t *old_bp,
uint64_t size, boolean_t use_slog)
uint64_t size, boolean_t encrypt, boolean_t use_slog)

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

continuation line should be indented by 4 spaces

bzero(key->l2ck_key.ck_data,
BITS_TO_BYTES(key->l2ck_key.ck_length));
kmem_free(key->l2ck_key.ck_data,
BITS_TO_BYTES(key->l2ck_key.ck_length));

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

continuation lines indent 4 spaces

ZFS_PROP_ENCRYPTION,
ZFS_PROP_SALT,
ZFS_PROP_KEYSOURCE,
ZFS_PROP_KEYSTATUS,

This comment has been minimized.

Copy link
@ahrens

ahrens Jun 8, 2016

Member

I think that Linux likes to have things added to the end of the enum so that new userland doesn't confuse old kernel modules?

This comment has been minimized.

Copy link
@tcaputi

tcaputi Jun 8, 2016

these were added in the middle because master added more properties while I was still working on the patch. I will fix this now.

@lundman

This comment has been minimized.

Copy link
Author

commented Jun 8, 2016

All the indention issues are just my emacs setting, I will get to fixing them. (Don't suppose there is an official .emacs for illumos? :) )

@tcaputi

This comment has been minimized.

Copy link

commented Jun 8, 2016

For those of you who don't know me, I am the person maintaining the original version of this PR in ZFS on Linux. I would like to apologize upfront for any style errors you might have found. I made a grievous mistake with my editor early in development and did not notice it for a long time.

@gwr

One high-level question about this work: Is any of it derived from the old OpenSolaris "zfs-crypto" project? If so, can we get some references to (and/or differences from) the previous code?

It is to a small extent. When I first started working on this project I was using it as a reference. The first iteration of the changes in libzfs_crypto.c did look a lot like that project. However, as I started developing I realized that the old project was not going to support the features I wanted and I started redesigning everything from scratch. At this point, pretty much all of the code has been reworked several times (without using the old project), so I don't think a diff would be particularly useful.

@ahrens ahrens referenced this pull request Jun 8, 2016

Closed

ZFS Encryption #4329

@lundman

This comment has been minimized.

Copy link
Author

commented Jun 8, 2016

dmake: Warning: Command failed for target `packages.i386/system-test-zfstest.dep'
dmake: Warning: Target `install' not remade because of errors

No idea what that means.

Entries present in manifests but not proto area:
    dir group=bin mode=0755 owner=root path=opt/zfs-tests/tests/functional/cli_root/zfs_key
    file NOHASH group=bin mode=0555 owner=root path=opt/zfs-tests/tests/functional/cli_root/zfs_key/cleanup

looks like I missed zfs_key subdirectory in functional/cli_root/Makefile - Will be fixed next push.

@lundman

This comment has been minimized.

Copy link
Author

commented Jun 9, 2016

Ok, I have done the indentation fixes, where I could find them, and also ran all the source files through cstyle and corrected its complaints. Not sure it is worth pushing that yet, since github will then hide the comments? (and it has no logic changes).

I should point out I had to rewrite zdb.c logic pointing uint64_t to za_name as the compiler complained of possible mis-aligned 64bit access.

hopefully, I got it right

         zap_cursor_t zc;
         zap_attribute_t attr;
         dsl_crypto_key_phys_t dckp;
-        uint64_t *txgid;
+        uint64_t txgid;
         size_t keylen;

         dump_zap_stats(os, object);
@@ -13,16 +13,16 @@
         for (zap_cursor_init(&zc, os, object);
                 zap_cursor_retrieve(&zc, &attr) == 0; zap_cursor_advance(&zc)) {

-                txgid = ((uint64_t *)attr.za_name);
+                bcopy(attr.za_name, &txgid, sizeof (txgid));

-                VERIFY0(zap_lookup_uint64(os, object, txgid, 1, 1,
+                VERIFY0(zap_lookup_uint64(os, object, &txgid, 1, 1,
                         sizeof (dsl_crypto_key_phys_t), &dckp));

                 keylen = BYTES_TO_BITS(
                         zio_crypt_table[dckp.dk_crypt_alg].ci_keylen);

                 (void) printf("\t\ttxg %llu : wkeylen = %u\n",
-                        (u_longlong_t)*txgid, (uint_t)keylen);
+                        (u_longlong_t)txgid, (uint_t)keylen);
         }
         zap_cursor_fini(&zc);
 }

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch 4 times, most recently from 4278e74 to 8748ffb Oct 5, 2017

@GernotS

This comment has been minimized.

Copy link

commented Oct 5, 2017

sorry to report that send/receive (see Aug. 18th/19th) still crashes when testing with more than 10GB of data:

panic[cpu1]/thread=ffffff000faf4c40:
BAD TRAP: type=e (#pf Page fault) rp=ffffff000faf4870 addr=68 occurred in module "zfs" due to a NULL pointer dereference
sched:
#pf Page fault
Bad kernel fault at addr=0x68
pid=0, pc=0xfffffffff7d49176, sp=0xffffff000faf4960, eflags=0x10202
cr0: 8005003b<pg,wp,ne,et,ts,mp,pe> cr4: 6f8<xmme,fxsr,pge,mce,pae,pse,de>
cr2: 68
cr3: 1dc00000
cr8: c

    rdi:               68 rsi: ffffff03dde12120 rdx: ffffff000faf4c40
    rcx:                d  r8: ffffff03db3662a0  r9:                0
    rax:                0 rbx: ffffff03dde12120 rbp: ffffff000faf4970
    r10: fffffffffb858740 r11: fffffffffb800983 r12:                0
    r13: ffffffffc001af00 r14: ffffffffc001e890 r15: ffffff03dde12120
    fsb:                0 gsb: ffffff03c9b89080  ds:               4b
     es:               4b  fs:                0  gs:              1c3
    trp:                e err:                0 rip: fffffffff7d49176
     cs:               30 rfl:            10202 rsp: ffffff000faf4960
     ss:               38

ffffff000faf4750 unix:real_mode_stop_cpu_stage2_end+b21c ()
ffffff000faf4860 unix:trap+e08 ()
ffffff000faf4870 unix:_cmntrap+e6 ()
ffffff000faf4970 zfs:arc_buf_remove+16 ()
ffffff000faf49b0 zfs:arc_buf_destroy_impl+bd ()
ffffff000faf4a10 zfs:arc_evict_hdr+b3 ()
ffffff000faf4aa0 zfs:arc_evict_state_impl+154 ()
ffffff000faf4b40 zfs:arc_evict_state+12e ()
ffffff000faf4b70 zfs:arc_adjust_impl+44 ()
ffffff000faf4ba0 zfs:arc_adjust+d9 ()
ffffff000faf4c20 zfs:arc_reclaim_thread+ae ()
ffffff000faf4c30 unix:thread_start+8 ()

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch 2 times, most recently from ad60b8f to bc464bb Oct 6, 2017

@arekinath

This comment has been minimized.

Copy link

commented Oct 11, 2017

I think you missed this from ZOL in the latest update:

diff --git a/usr/src/uts/common/fs/zfs/bptree.c b/usr/src/uts/common/fs/zfs/bptree.c
index c74d072..25c08f6 100644
--- a/usr/src/uts/common/fs/zfs/bptree.c
+++ b/usr/src/uts/common/fs/zfs/bptree.c
@@ -211,7 +211,8 @@ bptree_iterate(objset_t *os, uint64_t obj, boolean_t free, bptree_itor_t func,
        err = 0;
        for (i = ba.ba_phys->bt_begin; i < ba.ba_phys->bt_end; i++) {
                bptree_entry_phys_t bte;
-               int flags = TRAVERSE_PREFETCH_METADATA | TRAVERSE_POST;
+               int flags = TRAVERSE_PREFETCH_METADATA | TRAVERSE_POST |
+                   TRAVERSE_NO_DECRYPT;
 
                err = dmu_read(os, obj, i * sizeof (bte), sizeof (bte),
                    &bte, DMU_READ_NO_PREFETCH);

(without it, after the recent change to turn async destroy back on for encrypted datasets, you get a panic)

Matching code in ZOL at https://github.com/zfsonlinux/zfs/blob/master/module/zfs/bptree.c#L215

@lundman

This comment has been minimized.

Copy link
Author

commented Oct 11, 2017

Ah well spotted, thanks!

@arekinath

This comment has been minimized.

Copy link

commented Oct 11, 2017

Re: the dump issues, doing this:

diff --git a/usr/src/uts/common/fs/zfs/zvol.c b/usr/src/uts/common/fs/zfs/zvol.c
index c6a6338..080b2cd 100644
--- a/usr/src/uts/common/fs/zfs/zvol.c
+++ b/usr/src/uts/common/fs/zfs/zvol.c
@@ -2157,6 +2157,9 @@ zvol_dumpify(zvol_state_t *zv)
 	if (zv->zv_flags & ZVOL_RDONLY)
 		return (SET_ERROR(EROFS));

+	if (os->os_encrypted)
+		return (SET_ERROR(ENOTSUP));
+
 	if (zap_lookup(zv->zv_objset, ZVOL_ZAP_OBJ, ZVOL_DUMPSIZE,
 	    8, 1, &dumpsize) != 0 || dumpsize != zv->zv_volsize) {
 		boolean_t resize = (dumpsize > 0);

Seems to be good enough to guard against dumpifying encrypted zvols, preventing the panic. Attempting to use dumpadm on them after adding those two lines exits like this:

# zfs get encryption zones
NAME   PROPERTY    VALUE        SOURCE
zones  encryption  aes-256-ccm  -
# zfs create -V 8G zones/dump2
# dumpadm -d /dev/zvol/dsk/zones/dump2
dumpadm: dumps not supported on /dev/zvol/dsk/zones/dump2
@lundman

This comment has been minimized.

Copy link
Author

commented Oct 12, 2017

I prefer print myself, but there were no uses of print in existing zfs-tester test files, whereas there was precedent in using tr -d in zfs_create_014_pos.

@ikozhukhov

This comment has been minimized.

Copy link

commented Oct 12, 2017

you can use 'print' instead of 'echo' - it is applicable for portability.
zfs-tests was developed on illumos based platform and for more universal/portable way we can update it.
for example: with DilOS i have GNU coreutils in primary place instead of illumos tools and i have the same issues what can found others non-illumos based platforms.
i did some tricks to use illumos tools from /system/bin/ location for ZFS tests, but right now my env with zfs tests is broken by latest big update by delphix - where they wants to use easy way with PATH.
for my opinion - ZFS tests should be more universal for others platforms and we have to use universal tools - not depend on illumos versions of tools.
more universal will be:
to use GNU coreutils - more platforms have it, but only illumos based platforms have it at /usr/gnu/
to use GNU find - all platforms are using it
GNU SED, GREP/EGREP, DD, etc.
it is why it was more important to me to have environment variables file where we can redefine tools locations if needed like:
export ECHO=/usr/gnu/bin/echo
and use ${ECHO} where we need it, instead of to use PATH with links to tools.

@lundman

This comment has been minimized.

Copy link
Author

commented Oct 12, 2017

I agree with everything you say, I already have had to change a quite a few things to the $ENV version on osx. It certainly would be nice if such a standard was agreed upon

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch from d6a584c to be6dbca Oct 12, 2017

@lundman

This comment has been minimized.

Copy link
Author

commented Oct 12, 2017

Looks like we managed to pass all tests finally. I plan to squash everything into one single commit unless there is some other number of commits people would prefer..?

@ikozhukhov

This comment has been minimized.

Copy link

commented Oct 13, 2017

i think single commit will be fine.

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch 4 times, most recently from b4a2aaf to 6ce01a1 Oct 13, 2017

@lundman lundman changed the title Native data and metadata encryption for zfs 8727 Native data and metadata encryption for zfs Oct 21, 2017

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch 2 times, most recently from 43e9d1a to 9bb6c76 Oct 21, 2017

8727 Native data and metadata encryption for zfs
    This change incorporates three major pieces:

    The first change is a keystore that manages wrapping
    and encryption keys for encrypted datasets. These
    commands mostly involve manipulating the new
    DSL Crypto Key ZAP Objects that live in the MOS. Each
    encrypted dataset has its own DSL Crypto Key that is
    protected with a user's key. This level of indirection
    allows users to change their keys without re-encrypting
    their entire datasets. The change implements the new
    subcommands "zfs load-key", "zfs unload-key" and
    "zfs change-key" which allow the user to manage their
    encryption keys and settings. In addition, several new
    flags and properties have been added to allow dataset
    creation and to make mounting and unmounting more
    convenient.

    The second piece of this patch provides the ability to
    encrypt, decyrpt, and authenticate protected datasets.
    Each object set maintains a Merkel tree of Message
    Authentication Codes that protect the lower layers,
    similarly to how checksums are maintained. This part
    impacts the zio layer, which handles the actual
    encryption and generation of MACs, as well as the ARC
    and DMU, which need to be able to handle encrypted
    buffers and protected data.

    The last addition is the ability to do raw, encrypted
    sends and receives. The idea here is to send raw
    encrypted and compressed data and receive it exactly
    as is on a backup system. This means that the dataset
    on the receiving system is protected using the same
    user key that is in use on the sending side. By doing
    so, datasets can be efficiently backed up to an
    untrusted system without fear of data being
    compromised.

    Reviewed by: Matthew Ahrens <mahrens@delphix.com>
    Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
    Reviewed-by: Jorgen Lundman <lundman@lundman.net>
    Signed-off-by: Tom Caputi <tcaputi@datto.com>

Send / Recv Fixes following b52563

    This patch fixes several issues discovered after
    the encryption patch was merged:

    Fixed a bug where encrypted datasets could attempt
    to receive embedded data records.

    Fixed a bug where dirty records created by the recv
    code wasn't properly setting the dr_raw flag.

    Fixed a typo where a dmu_tx_commit() was changed to
    dmu_tx_abort()

    Fixed a few error handling bugs unrelated to the
    encryption patch in dmu_recv_stream()

    Signed-off-by: Tom Caputi <tcaputi@datto.com>

Encryption patch follow-up

* HKDF implementation moved to its own file and tests added to ensure
correctness.

* Ztest can now create and test encrypted datasets. This is currently
disabled until issue ZOL #6526 is resolved, but otherwise functions as
advertised.

* Several small bug fixes discovered after enabling ztest to run on
encrypted datasets.

* Fixed coverity defects added by the encryption patch.

* Updated man pages for encrypted send / receive behavior.

* Fixed a bug where encrypted datasets could receive
  DRR_WRITE_EMBEDDED records.

* Minor code cleanups / consolidation.

Disable crypto tests in ztest

* Includes fix in dmu_free_long_object_impl

Unless permission is given to compile the crypto framework in userland
the crypto tests in ztest are disabled on IllumOS.

@lundman lundman force-pushed the openzfsonosx:zol_crypto branch from 9bb6c76 to e77d6fd Oct 24, 2017

@ahrens

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

@lundman This PR is just the first encryption commit from ZoL, right? In that case please squash to one commit. To raise the visibility of this, you might open a new PR and mention in the description that you think this is ready to integrate, and also call for any additional help you might need (e.g. testing and code review?).

@lundman

This comment has been minimized.

Copy link
Author

commented Oct 31, 2017

@ahrens This is the first PR, followed by all fixes, two more PRs and fixes. It has everything in it that ZoL has. I don't think it is so much about what I need, but what you want to happen next.

@lundman

This comment has been minimized.

Copy link
Author

commented Nov 1, 2017

Closed in favour of #489

@lundman lundman closed this Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.