- Download Diff

Summary:

8727 Native data and metadata encryption for zfs

Review Request #1996 — Created June 12, 2019 and submitted June 28, 2019, 4:47 p.m.

Information
Owner:	jjelinek
Repository:	illumos-gate
Branch:
Bugs:
Depends On:
Reviewers
Groups:	general
People:

Description

Work by Jörgen Lundman, any many others over the past couple of years to port the ZoL encryption support to illumos. The full set of ZoL commits included in the changeset are in the bug report.

I modified the following test and commented out parts of it which will not work on illumos
usr/src/test/zfs-tests/tests/functional/rsend/send_encrypted_files.ksh

The zpool_import_errata3 and zpool_import_errata4 tests do not currently work on illumos and I left those out of the runfiles.

Testing Done

Lots of misc. testing over the past couple of years. A zfs-test run completes successfully, including the new and updated tests.

Issues

0
0
23
5
All Issues: 28

Description	From	Last Updated
Tab missing?	seeemef@mac.com	June 13, 2019, 8:27 p.m.
Should be tab?	seeemef@mac.com	June 13, 2019, 8:27 p.m.
space at the end of line introduced	tsoome	June 13, 2019, 8:54 p.m.
Should be tab?	seeemef@mac.com	June 13, 2019, 8:28 p.m.
whitespace error	tsoome	June 13, 2019, 8:54 p.m.
Does CRLF vs LF matter?	seeemef@mac.com	June 13, 2019, 8:34 p.m.
errbuf seems to be unused after this.	seeemef@mac.com	June 13, 2019, 8:36 p.m.
errbuf seems unused after this.	seeemef@mac.com	June 13, 2019, 8:37 p.m.
Typo — "inheriting" instead?	seeemef@mac.com	June 13, 2019, 8:38 p.m.
Typo "unencryppted"	seeemef@mac.com	June 13, 2019, 8:40 p.m.
Typo "unenecrypted"	seeemef@mac.com	June 13, 2019, 8:41 p.m.
"\n " seems odd. Saw "\n\t" at other places in this patch.	seeemef@mac.com	June 13, 2019, 8:43 p.m.
What does "this" refer to?	seeemef@mac.com	June 13, 2019, 8:44 p.m.
Typo "vice"?	seeemef@mac.com	June 13, 2019, 8:46 p.m.
Typos "elimiate" and "depedency"	seeemef@mac.com	June 13, 2019, 8:47 p.m.
Typo double space	seeemef@mac.com	June 13, 2019, 8:48 p.m.
Typo "blocks"	seeemef@mac.com	June 13, 2019, 8:49 p.m.
Should this be ret = SET_ERROR(ENOMEM);?	jbk	June 17, 2019, 4:26 p.m.
Typo "inheritting"	seeemef@mac.com	June 13, 2019, 8:50 p.m.
Typo "tarversing"	seeemef@mac.com	June 13, 2019, 8:51 p.m.
Typo "encyrption"	seeemef@mac.com	June 13, 2019, 8:51 p.m.
Typo "inheritting"	seeemef@mac.com	June 13, 2019, 8:52 p.m.
Typo missing space (or superfluous space)	seeemef@mac.com	June 13, 2019, 8:53 p.m.
Typo "which which"	seeemef@mac.com	June 13, 2019, 8:54 p.m.
IIRC, these are very specific to the Linux kernel's module mechanism. Any reason we can't delete them?	jbk	June 17, 2019, 4:27 p.m.
whitespace error	tsoome	June 17, 2019, 4:20 p.m.
do we want to clear those?	tsoome	June 17, 2019, 4:23 p.m.
we return int, therefore we should have 0 here. Sorry for missing it.	tsoome	June 22, 2019, 5:57 p.m.

Ship it!

Ship It!

jlevon June 12, 2019, 9:37 p.m.

Please don't do this. A review on here means that you looked carefully at the code for errors, issues, and concerns. You clearly have not done this, as there is no physical way you could have done this in time. This is not the right place to vote for code.

igork June 12, 2019, 9:40 p.m.

i looked to list of commits + several updates what i did with ports "Native ZFS" to DilOS (it's done several weeks ago) and i know what i do.

usr/src/cmd/zinject/zinject.c (Diff revision 1)
Show all issues
```
Tab missing?
```
usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
Show all issues
```
Should be tab?
```
usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
Show all issues
```
Should be tab?
```
usr/src/lib/libzfs/common/libzfs_crypto.c (Diff revision 1)
```
Does CRLF vs LF matter?
```
1. jjelinek June 13, 2019, 8:56 p.m.
  fixed
usr/src/lib/libzfs/common/libzfs_crypto.c (Diff revision 1)
Show all issues
```
Does CRLF vs LF matter?
```
usr/src/lib/libzfs/common/libzfs_crypto.c (Diff revision 1)
Show all issues
```
errbuf seems to be unused after this.
```
1. jjelinek June 13, 2019, 8:56 p.m.
  this matches the ZoL code, but I went ahead and removed it
usr/src/lib/libzfs/common/libzfs_crypto.c (Diff revision 1)
Show all issues
```
errbuf seems unused after this.
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above
usr/src/lib/libzfs/common/libzfs_crypto.c (Diff revision 1)
Show all issues
```
Typo — "inheriting" instead?
```
1. jjelinek June 13, 2019, 8:56 p.m.
  this matches the ZoL code bug I went ahead and fixed it anyway
usr/src/lib/libzfs/common/libzfs_sendrecv.c (Diff revision 1)
Show all issues
```
Typo "unencryppted"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  this matches the ZoL code but I fixed it anyway
usr/src/lib/libzfs/common/libzfs_sendrecv.c (Diff revision 1)
Show all issues
```
Typo "unenecrypted"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above

usr/src/lib/libzfs/common/libzfs_sendrecv.c (Diff revision 1)

Show all issues

"\n " seems odd. Saw "\n\t" at other places in this patch.

jjelinek June 13, 2019, 8:56 p.m.

I've left this as-is since it matches the original ZoL code.

usr/src/lib/libzpool/common/kernel.c (Diff revision 1)
Show all issues
```
What does "this" refer to?
```
1. jjelinek June 13, 2019, 8:56 p.m.
  The only possibility is the function name.
usr/src/uts/common/fs/zfs/arc.c (Diff revision 1)
Show all issues
```
Typo "vice"?
```
1. jjelinek June 13, 2019, 8:56 p.m.
  this matches the ZoL code but I fixed it anyway
usr/src/uts/common/fs/zfs/arc.c (Diff revision 1)
Show all issues
```
Typos "elimiate" and "depedency"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above
usr/src/uts/common/fs/zfs/arc.c (Diff revision 1)
Show all issues
```
Typo double space
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above
usr/src/uts/common/fs/zfs/dbuf.c (Diff revision 1)
Show all issues
```
Typo "blocks"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as ZoL but I fixed it anyway
usr/src/uts/common/fs/zfs/dsl_crypt.c (Diff revision 1)
Show all issues
```
Typo "inheritting"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as ZoL but I fixed it anyway
usr/src/uts/common/fs/zfs/dsl_crypt.c (Diff revision 1)
Show all issues
```
Typo "tarversing"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above
usr/src/uts/common/fs/zfs/dsl_crypt.c (Diff revision 1)
Show all issues
```
Typo "encyrption"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above
usr/src/uts/common/fs/zfs/dsl_crypt.c (Diff revision 1)
Show all issues
```
Typo "inheritting"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as above
usr/src/uts/common/fs/zfs/zio_crypt.c (Diff revision 1)
Show all issues
```
Typo missing space (or superfluous space)
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as ZoL, I left it as-is
usr/src/uts/common/fs/zfs/zio_crypt.c (Diff revision 1)
Show all issues
```
Typo "which which"
```
1. jjelinek June 13, 2019, 8:56 p.m.
  same as ZoL but I fixed it anyway

usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
Show all issues
```
space at the end of line introduced
```
usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
Show all issues
```
whitespace error
```

The change to

usr/src/cmd/mdb/intel/amd64/zfs/Makefile

that adds

\-I../../../../../common/zfs

to CPPFLAGS, needs propagating to sparc, here:

usr/src/cmd/mdb/sparc/v9/zfs/Makefile

With this, I get a clean build on sparc.

jjelinek June 13, 2019, 8:56 p.m.
```
fixed
```

The manifest

usr/src/pkg/manifests/system-test-zfstest.mf

adds

file path=opt/zfs-tests/tests/functional/rsend/send_mixed_raw mode=0555

but this file does not appear to be supplied, causing packaging to
fail:

==== Validating manifests against proto area ====

Entries present in manifests but not proto area:
        file NOHASH group=bin mode=0555 owner=root path=opt/zfs-tests/tests/func
tional/rsend/send_mixed_raw

(This is on sparc, but I'm not sure how that would make any difference.)

jjelinek June 13, 2019, 8:56 p.m.

fixed (I forgot the git add on that file)

Change Summary:

fixed first round of CR feedback

Diff:

Revision 2 (+19918 -1519)

Show changes

	usr/src/cmd/availdevs/availdevs.c
	usr/src/cmd/mdb/common/modules/zfs/zfs.c
	usr/src/cmd/mdb/intel/amd64/libzpool/Makefile
	usr/src/cmd/mdb/intel/amd64/zfs/Makefile
	usr/src/cmd/mdb/intel/ia32/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v7/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/zfs/Makefile
	189 more

Ship it!

```
LGTM
```

usr/src/uts/common/fs/zfs/arc.c (Diff revision 2)

Show all issues

whitespace error

jjelinek June 17, 2019, 4:23 p.m.

I don't see any whitespace error here and cstyle also does not report any problem. I'm going to close this unless you can be more specific.

tsoome June 17, 2019, 4:44 p.m.

its shown by rb at line 5680, just after blkptr_t

jjelinek June 17, 2019, 5:34 p.m.

Thanks, for some reason RB is showing your comment on line 5681. I fixed this.

usr/src/uts/common/fs/zfs/sys/dmu_impl.h (Diff revision 2)
Show all issues
```
do we want to clear those?
```
1. jjelinek June 17, 2019, 4:23 p.m.
  I'd like to keep this aligned with ZoL and not make spurious diffs.

usr/src/uts/common/fs/zfs/dsl_crypt.c (Diff revision 1)
Show all issues
```
Should this be ret = SET_ERROR(ENOMEM);?
```

usr/src/uts/common/fs/zfs/zio_crypt.c (Diff revision 1)

Show all issues

IIRC, these are very specific to the Linux kernel's module mechanism. Any reason we can't delete them?

Change Summary:

more code review updates

Diff:

Revision 3 (+19910 -1519)

Show changes

	usr/src/cmd/availdevs/availdevs.c
	usr/src/cmd/mdb/common/modules/zfs/zfs.c
	usr/src/cmd/mdb/intel/amd64/libzpool/Makefile
	usr/src/cmd/mdb/intel/amd64/zfs/Makefile
	usr/src/cmd/mdb/intel/ia32/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v7/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/zfs/Makefile
	189 more

Change Summary:

Latest code review updates, and a fix for a failing test case only under a DEBUG build.

Diff:

Revision 4 (+19918 -1520)

Show changes

	usr/src/cmd/availdevs/availdevs.c
	usr/src/cmd/mdb/common/modules/zfs/zfs.c
	usr/src/cmd/mdb/intel/amd64/libzpool/Makefile
	usr/src/cmd/mdb/intel/amd64/zfs/Makefile
	usr/src/cmd/mdb/intel/ia32/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v7/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/zfs/Makefile
	189 more

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

usr/src/lib/libzpool/common/kernel.c (Diff revision 4)

Show all issues

we return int, therefore we should have 0 here. Sorry for missing it.

Change Summary:

Latest rebase, and fix for Tommas' return nit.

Diff:

Revision 5 (+19965 -1526)

Show changes

	usr/src/cmd/availdevs/availdevs.c
	usr/src/cmd/mdb/common/modules/zfs/zfs.c
	usr/src/cmd/mdb/intel/amd64/libzpool/Makefile
	usr/src/cmd/mdb/intel/amd64/zfs/Makefile
	usr/src/cmd/mdb/intel/ia32/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v7/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/libzpool/Makefile
	usr/src/cmd/mdb/sparc/v9/zfs/Makefile
	189 more

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

8727 Native data and metadata encryption for zfs

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)