Dina wrote: > ... > > 6722460 finish moving /dev/random and /dev/urandom seeding and usage to > libcryptoutil > 6666204 meta slot opens and closes /dev/urandom needlessly for every read > > The webrev is at http://cr.opensolaris.org/~dinak/random-hg >
My review comments are below. -Krishna ------------------------------------------------------------------------------------------- usr/src/lib/libcryptoutil/common/cryptoutil.h KY-1 - lines 144-145 Highly recommend renaming these to something like DEV_RANDOM and DEV_URANDOM. It is more clear and gets you away from defining "AVG_QUALITY". People already know about /dev/random and /dev/urandom. KY-2 - lines 151-152 Consider renaming these to pkcs11_get_random (or pkcs11_generate_random) and pkcs11_get_nzero_random. This makes them consistent with the other routines above. usr/src/lib/libcryptoutil/common/random.c KY-3 T3 lines 34-39 No need for _REENTRANT madness now that libpthread is folded in to libc in S10 (See PSARC 2002/217). It should be the default and lines 44-49 can go away. Recommend removing the RAND_LOCK macros also. KY-4 T2 lines 198 and 216, 223 and 241 This results in a performance problem for the multi-thread case since the global lock is held and released for each call. See the current meta_SeedRandom() for the solution. KY-5 T2 lines 214 and 215, 239 and 240 This results in a performance problem since you are opening and closing /dev/urandom needlessly for every seed. There are applications that make this call frequently. KY-6 T5 lines 203 and 213, 228 and 238 Remove the leftover comments. KY-7 T3 lines 253-254, 264 Comments need updating. KY-8 T2 line 265 Same issue as KY-4. You are holding and releasing a lock for each call to this routine by calling pkcs11_open_random. usr/src/lib/pkcs11/libpkcs11/common/metaGeneral.c KY-9 T3 line 198 Need to close the open fd's (if any) from /dev/random and /dev/urandom. This is needed even if they are closed in pkcs11_softtoken since there can be a configuration where softtoken slot is disabled. Once KY-5 is addressed, the fd to seed /dev/urandom needs to be closed here. Also, there is a change in behavior here that needs to be called out in comments here - With this change, libpkcs11 and pkcs11_softtoken share the same global fd. This can be a problem if there were ever a case where pkcs11_softtoken is finalized and closes the fd while libpkcs11 is still used (say using pkcs11_kernel slots). usr/src/lib/pkcs11/pkcs11_softtoken/common/softGeneral.c KY-10 T3 line 328 Once KY-5 is addressed, the fd to seed /dev/urandom needs to be closed here.
