8727 Native data and metadata encryption for zfs

Review Request #1996 — Created June 12, 2019 and submitted

jjelinek
illumos-gate
general

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.

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

  • 0
  • 0
  • 23
  • 5
  • 28
Description From Last Updated
igork
  1. Ship It!
    1. 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.

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

  2. 
      
seeemef@mac.com
  1. 
      
  2. usr/src/cmd/zinject/zinject.c (Diff revision 1)
     
     

    Tab missing?

  3. usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
     
     

    Should be tab?

  4. usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
     
     

    Should be tab?

  5. Does CRLF vs LF matter?

  6. Does CRLF vs LF matter?

  7. errbuf seems to be unused after this.

    1. this matches the ZoL code, but I went ahead and removed it

  8. errbuf seems unused after this.

  9. Typo — "inheriting" instead?

    1. this matches the ZoL code bug I went ahead and fixed it anyway

  10. Typo "unencryppted"

    1. this matches the ZoL code but I fixed it anyway

  11. Typo "unenecrypted"

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

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

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

    What does "this" refer to?

    1. The only possibility is the function name.

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

    Typo "vice"?

    1. this matches the ZoL code but I fixed it anyway

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

    Typos "elimiate" and "depedency"

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

    Typo double space

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

    Typo "blocks"

    1. same as ZoL but I fixed it anyway

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

    Typo "inheritting"

    1. same as ZoL but I fixed it anyway

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

    Typo "tarversing"

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

    Typo "encyrption"

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

    Typo "inheritting"

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

    Typo missing space (or superfluous space)

    1. same as ZoL, I left it as-is

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

    Typo "which which"

    1. same as ZoL but I fixed it anyway

  24. 
      
tsoome
  1. 
      
  2. usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
     
     

    space at the end of line introduced

  3. usr/src/cmd/zpool/zpool_main.c (Diff revision 1)
     
     

    whitespace error

  4. 
      
ptribble
  1. 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.
  2. 
      
ptribble
  1. 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.)
    1. fixed (I forgot the git add on that file)

  2. 
      
jjelinek
seeemef@mac.com
  1. LGTM

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

    whitespace error

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

    2. its shown by rb at line 5680, just after blkptr_t

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

  3. do we want to clear those?

    1. I'd like to keep this aligned with ZoL and not make spurious diffs.

  4. 
      
jbk
  1. 
      
  2. usr/src/uts/common/fs/zfs/dsl_crypt.c (Diff revision 1)
     
     

    Should this be ret = SET_ERROR(ENOMEM);?

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

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

  4. 
      
jjelinek
jjelinek
jbk
  1. Ship It!
  2. 
      
tsoome
  1. Ship It!
  2. 
      
tsoome
  1. 
      
  2. usr/src/lib/libzpool/common/kernel.c (Diff revision 4)
     
     

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

  3. 
      
jjelinek
andy_js
  1. Ship It!
  2. 
      
jjelinek
Review request changed

Status: Closed (submitted)

Loading...