We would like to start prelim codereview for the ZFS Crypto project today. Why prelim rather than final ? There are a couple of issues that still need to be resolved (see the list of Bugster openbugs in development/zfs/kernel with keyword zfs-crypto). The bulk of the code is ready (and has been since around April) for codereview. ## CodeReview for Phase 1 of the ZFS Crypto project * Webrev: http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/ * Mercurial Workspace: http://src.opensolaris.org/source/xref/zfs-crypto/gate/ ## CodeReview for Phase 1 of the ZFS Crypto project * Webrev: [http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/](http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/) * OpenGrok: http://src.opensolaris.org/source/xref/zfs-crypto/gate/ ## Review timing Codereview starts *Friday 5th September 2008 at 1200 US/Pacific* and is scheduled to end on *Friday 3rd October 2008 at 2359 US/Pacific*. Comments recieved after this time will still be considered but unless there are serious in nature (data corruption, security issue, regression from existing ZFS) they may have to wait until post onnv-gate integration to be addressed; however every comment will be looked at and assessed on its own merit. ## How to send comments All source code review comments should be sent to zfs-crypto-discuss at opensolaris.org. If for any reason you feel you need to send comments confidentially then please email me directly (Darren.Moffat at Sun.COM). ## Updates during the review period During the codereview period, and right up until integration into onnv-gate, there will be changes made to address last minute bugs and the to address review comments. The original webrev will be left alone and new ones will be produced as we feel they will be useful. At the end of the review period there will be two new webrev''s produced: one against onnv-gate at the time and the other against the webrev at the start of the review period. ## Source overview * $SRC/uts/common/fs/zfs/zio\_crypto.c * $SRC/lib/libzfs/common/libzfs\_crypto.c This is where all the real crypto happens almost everything else calls into functions in these files in kernel and userland respectively. The rest of the files are already existing ZFS functionality. * Major ZFS functional areas with change: * [ZIO](http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/usr/src/uts/common/fs/zfs/zio.c.frames.html) * [ZIL](http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/usr/src/uts/common/fs/zfs/zfs_log.c.frames.html) * [ARC](http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/usr/src/uts/common/fs/zfs/arc.c.frames.html) * [ZFS IOCTL](http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/usr/src/uts/common/fs/zfs/zfs_ioctl.c.frames.html) * [DMU](http://cr.opensolaris.org/~darrenm/zfs-crypto-gate/webrev/usr/src/uts/common/fs/zfs/dmu_objset.c.frames.html) The changes to the zfs/zpool commands should be purely CLI related and the real work happens in functions in libzfs. ## Non ZFS files in the review webrev Note that there are changes to a few non ZFS files in the gate at this time notably the following: usr/src/cmd/cmd-crypto/decrypt/decrypt.c usr/src/cmd/cmd-crypto/digest/digest.c usr/src/lib/libcryptoutil/common/cryptoutil.h usr/src/lib/libcryptoutil/common/debug.c usr/src/lib/libcryptoutil/common/keyfile.c usr/src/lib/libcryptoutil/common/mapfile-vers usr/src/lib/libcryptoutil/common/tohexstr.c usr/src/common/crypto/modes/ccm.c These are to be reviewed by the crypto framework team and/or are already in the process of being integrated separately. All the changes in those files will be integrated before the ZFS Crypto project. -- Darren J Moffat
Darren,
Here are my first comments... more to follow.
Mark
svc/milestone/devices-local
---------------------------
mcp-0 line 85 Don''t you need: || exit
$SMF_EXIT_ERR_FATAL
cmd/zdb/zdb.c
-------------
mcp-1 lines 1078-1091 Add "{}" since that seems to be the
convention.
cmd/zfs/zfs_main.c
------------------
mcp-2 lines 2881,3305 Gratuitous spaces.
mcp-3 line 3395 s/dataset''s/datasets/
mcp-4 line 4147 You need to ''goto error'' in
case you get
-o propname=xxx -o badprop
cmd/zpool/zpool_main.c
----------------------
mcp-5 line 862,922,1328 The convention seems to be to pass boolean
variables rather than B_FALSE.
Helps readability.
mcp-6 line 1330 s/fails/falls/
mcp-7 line 3896,3910,3924 These functions could be static.
mcp-8 line 3918 This function appears to be unused.
mcp-9 line 3960 Missing code to create the property list
and check for duplicate properties.
mark powers wrote:> Darren, > > Here are my first comments... more to follow. > > Mark > > svc/milestone/devices-local > --------------------------- > mcp-0 line 85 Don''t you need: || exit $SMF_EXIT_ERR_FATALWe don''t want the inability to load the key to be a fatal error. The datasets (filesystems and ZVOLS) which aren''t encrypted will still be able to be mounted even if there was a problem loading the key. The volinit will also "pass over" any encrypted zvols for which the key isn''t present and won''t return an error so the existing fatal error on line 86 doesn''t cause a problem either.> cmd/zdb/zdb.c > ------------- > mcp-1 lines 1078-1091 Add "{}" since that seems to be the > convention.Done, and I''m surprised I didn''t have them there since I''m usually very particular about that! I notice there as a cstyle error there too, which I''ve fixed.> cmd/zfs/zfs_main.c > ------------------ > mcp-2 lines 2881,3305 Gratuitous spaces.Fixed.> mcp-3 line 3395 s/dataset''s/datasets/Fixed.> mcp-4 line 4147 You need to ''goto error'' in case you get > -o propname=xxx -o badpropFixed.> cmd/zpool/zpool_main.c > ---------------------- > mcp-5 line 862,922,1328 The convention seems to be to pass boolean > variables rather than B_FALSE. > Helps readability.Fixed, I added a boolean_t encrypted_only which is initialized to B_FALSE.> mcp-6 line 1330 s/fails/falls/Fixed.> mcp-7 line 3896,3910,3924 These functions could be static.Fixed.> mcp-8 line 3918 This function appears to be unused.Fixed - it was unused.> mcp-9 line 3960 Missing code to create the property list > and check for duplicate properties.I don''t believe we are missing any code there. This is the same style as is used in zpool_create and it works the same as it does there. add_prop_list() does the nvlist_alloc() and the validity and duplicate detection. # zpool key -c -o keysource=passphrase,prompt tank Enter new passphrase for ''tank'': Enter again: Key change successful. # zpool key -c -o keysource=passphrase,prompt -o keysource=raw,file tank property ''keysource'' specified multiple times # zpool key -c -o bogus=pocus tank property ''bogus'' is not a valid pool property -- Darren J Moffat
Here''s what I have so far. - Eric usr/src/cmd/zfs/zfs_main.c 4124: shouldn''t there be a "?" case in the getopt switch? 4270: I think you need to check for a NULL pointer here. superfluous new lines at 3355, 3422. usr/src/cmd/zpool/zpool_main.c 1328: "depending of" -> "depending on" 3526: Any reason to say "Initial?" 3900: "*zpool_get_libzfs_cry(zhp) =" seems wrong. Probably have a zpool_set_libzfs_cry() function instead. usr/src/common/zfs/zfs_prop.c 215: Can''t the keyscope property use ZFS_TYPE_DATASET here? usr/src/common/zfs/zpool_prop.c 22: The copyright is out of date here and in a couple of other files. 115: There are three values listed, but the table has four. usr/src/common/zfs/zprop_common.c 348: update comment (or why can''t a binary property be fixed width?) usr/src/lib/libzfs/Makefile.com: Why do you need OBJS_SHARED_UTS? usr/src/lib/libzfs/common/libzfs.h 165-174: structure prefixes would be good here usr/src/lib/libzfs/common/libzfs_crypto.c should use zfs_alloc instead of malloc/bzero 202, 223: missing gettext() call 477: strcmp on a string returned by a getenv call (inside of a library) seems dicey 495: STDIN_FILENO instead of zero? 484, 871 #defining Esomething to 255 would be better 556: should there be a "default" case with an assert? 896: are there any currently other clients other than zpool? if not I''d avoid this 793, 1579, 1669: any reason to bzero before the free? usr/src/lib/libzfs/common/libzfs_mount.c 1196: may as well use zfs_spa_version() usr/src/lib/libzfs/common/libzfs_pool.c 510: superfluous "break;" 628, 1012: superfluous blank lines usr/src/lib/libzfs/common/libzfs_util.c 565, 571, 577: move the brackets to the next line It seens like these should all be of type zfs_crypt_t * rather than zfs_crypt_t **. 1187: this should be taken care of by zfs_prop_readonly. usr/src/lib/libzfs/common/mapfile-vers 37: is there anyone outside of libzfs who calls valid_set_keysource_change? If so, it should probably have some sort of zfs prefix on the name. 45: is there anyone outside of libzfs who calls zfs_crypto_create? usr/src/uts/common/fs/zfs/sys/spa.h 137: diagram shows highest order byte being used, but the macro on lines 235-236 uses the lowest order byte. usr/src/uts/common/fs/zfs/spa.c 1952: why did you need to move this above the spa_prop_validate code? usr/src/uts/common/fs/zfs/zio.c 1590: lets -> let''s 1611: comment says ENOENT but code says EAGAIN 1625: call to printf() usr/src/uts/common/fs/zfs/zio_crypt.c 694, 1031: you can just use kmem_alloc here 718: use KM_SLEEP instead of "0" 777: spa_keystatus is called here and other places without the sk_lock being held. 1207: ever -> every -- This message posted from opensolaris.org
Eric C. Taylor wrote:> Here''s what I have so far. > > - Eric > > usr/src/cmd/zfs/zfs_main.c > > 4124: shouldn''t there be a "?" case in the getopt switch?ACCEPT. Yes there should, however it was actually caught by the else clause at 4263.> 4270: I think you need to check for a NULL pointer here.ACCEPT.> superfluous new lines at 3355, 3422.ACCEPT> usr/src/cmd/zpool/zpool_main.c > > 1328: "depending of" -> "depending on"ACCEPT> 3526: Any reason to say "Initial?"Guess not, it was just a hint that there may be future phases. Best to drop "Initial" from the description.> 3900: "*zpool_get_libzfs_cry(zhp) =" seems wrong. Probably > have a zpool_set_libzfs_cry() function instead.ACCEPT I''ve added a zpool_set_libzfs_cry() and removed the _get_ variant as well as the uncalled libzfs_get_crypt. I also added the corresponding zfs_set_libzfs_cry() and updated zfs_main.c. As well as making use of {zfs,zpool}_set_libzfs_cry() in libzfs_crypto.c functions. I also fixed the set_callback_key_{load,unload} so that they will return the error from zpool_{load,unload}_key. I don''t know why we were masking that way.> usr/src/common/zfs/zfs_prop.c > > 215: Can''t the keyscope property use ZFS_TYPE_DATASET here?ACCEPT.> usr/src/common/zfs/zpool_prop.c > > 22: The copyright is out of date here and in a couple of other files.ACCEPT> 115: There are three values listed, but the table has four.The missing value is "defined". However defined will never be seen by any thing looking at the property. See the comment in zio_crypt.h where we define zfs_crypt_key_status_t. * keystatus is partially persistent and partially temporary. * The are two states that persist on disk undefined and defined. * If the on disk state is defined we return the appropriate "in memory" * state of available or unavailable depending on wither or not the * key is in the keystore. * * Old pool versions and datasets with encryption=off always have * a keystatus of undefined.> usr/src/common/zfs/zprop_common.c > > 348: update comment (or why can''t a binary property be fixed width?)Because we don''t know what its length is, just like with strings. On the other hand they kind of are fixed length because zfs_prop_get() doesn''t actually allow getting a value for them so I guess they are fixed at width 0. I''ve changed PROP_TYPE_BINARY to be fixed.> usr/src/lib/libzfs/Makefile.com: > > Why do you need OBJS_SHARED_UTS?ACCEPT, we don''t any more that is left over from before zcrypt_common.c was split out from zio_crypt.c.> usr/src/lib/libzfs/common/libzfs.h > > 165-174: structure prefixes would be good hereACCEPT, using zc_> usr/src/lib/libzfs/common/libzfs_crypto.c > > should use zfs_alloc instead of malloc/bzeroACCEPT, I also changed many of these to use the new {zfs,zpool}_set_libzfs_cry() function instead of poking the struct directly.> 202, 223: missing gettext() callACCEPT.> 477: strcmp on a string returned by a getenv call (inside > of a library) seems diceyI does a bit but I believe this is safe. However since this is only relevant to zpool (1M) because of the call to ''zpool key -l -a'' that is in svc:/system/device/local I''ve moved the skip logic to zpool_main.c.> 495: STDIN_FILENO instead of zero?ACCEPT> 484, 871 #defining Esomething to 255 would be betterNo longer applicable given the move of the key load bypass stuff to zpool_main.c.> 556: should there be a "default" case with an assert?ACCEPT> 896: are there any currently other clients other than zpool? > if not I''d avoid thisYes, the Lockhart based GUI and another two know ones, one unbundled from ON that already has libzfs contracts and another that will be a follow on feature from the ZFS Crypto team. By "this" I assume you mean the call to zpool_create_zvol_links() We need to do that because the links for encrypted zvols shouldn''t have been created if the pool was imported with the key not present. I also arranged for the links to be removed for encrypted zvols when the key is removed.> 793, 1579, 1669: any reason to bzero before the free?Good security practice to zero key material as soon as possible after you are done with it to reduce the amount of time it lives in process memory.> usr/src/lib/libzfs/common/libzfs_mount.c > > 1196: may as well use zfs_spa_version()REJECT. I can''t because we have a zpool_handle_t and zfs_spa_version() takes a zfs_handle_t and spa_version() takes a spa_t.> usr/src/lib/libzfs/common/libzfs_pool.c > > 510: superfluous "break;"ACCEPT> 628, 1012: superfluous blank linesACCEPT> usr/src/lib/libzfs/common/libzfs_util.c > > 565, 571, 577: move the brackets to the next lineN/A as these don''t exist anymore replaced with _set_> It seens like these should all be of type zfs_crypt_t * > rather than zfs_crypt_t **.N/A as these don''t exist anymore replaced with _set_> 1187: this should be taken care of by zfs_prop_readonly.It should be except that the binary properties aren''t read-only. This is here because there is no userland API to set a binary property - since there is no consumer for it yet. I''ve rather leave this check in place than replacing it with an ASSERT (which is what I originally had in there).> usr/src/lib/libzfs/common/mapfile-vers > > 37: is there anyone outside of libzfs who calls > valid_set_keysource_change? If so, it should probably > have some sort of zfs prefix on the name.No there isn''t so it has been removed from the mapfile.> 45: is there anyone outside of libzfs who calls > zfs_crypto_create?Originally we thought there might be, there certainly isn''t just now so I''ve removed it from the mapfile.> usr/src/uts/common/fs/zfs/sys/spa.h > > 137: diagram shows highest order byte being used, but the > macro on lines 235-236 uses the lowest order byte.Fixed the diagram, and the corresponding version in $SRC/grub/ too.> usr/src/uts/common/fs/zfs/spa.c > > 1952: why did you need to move this above the spa_prop_validate code?http://defect.opensolaris.org/bz/show_bug.cgi?id=1631 Maybe it doesn''t need to be flipped any more I''ll investigate.> usr/src/uts/common/fs/zfs/zio.c > > 1590: lets -> let''sN/A After mega merge/rewrite with 6754011 (onnv_100) this code doesn''t exist anymore so the comment isn''t there to be correct.> 1611: comment says ENOENT but code says EAGAINFixed comment.> 1625: call to printf()ACCEPT, This had already been replaced by a DTRACE static probe.> usr/src/uts/common/fs/zfs/zio_crypt.c > > 694, 1031: you can just use kmem_alloc hereACCEPT, and 731 too.> 718: use KM_SLEEP instead of "0"ACCEPT, thats what I get for reading nvlist_alloc(3nvpair) and reading that it was reserved and should be 0 rather than reading nvlist_alloc(9f) where it is a kmflag field.> 777: spa_keystatus is called here and other places without the > sk_lock being held.ACCEPT, I''ve put the rw_enter(sk_lock, RW_READER) inside spa_keystatus. spa_keystatus() is called from dmu_objset.c and spa.c as well as zio_crypt.c functions.> 1207: ever -> everyACCEPT -- Darren J Moffat