diff -up ./lib/softoken/lgglue.c.read-write-db-locks ./lib/softoken/lgglue.c --- ./lib/softoken/lgglue.c.read-write-db-locks 2023-06-04 10:42:53.000000000 +0200 +++ ./lib/softoken/lgglue.c 2023-06-21 10:59:44.940835332 +0200 @@ -184,13 +184,13 @@ sftkdb_encrypt_stub(PLArenaPool *arena, } /* not a key handle */ - if (handle == NULL || handle->passwordLock == NULL) { + if (handle == NULL || !sftkdb_passwordLockIsInited(handle)) { return SECFailure; } - PZ_Lock(handle->passwordLock); + sftkdb_passwordReaderLock(handle); if (handle->passwordKey.data == NULL) { - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); /* PORT_SetError */ return SECFailure; } @@ -208,7 +208,7 @@ sftkdb_encrypt_stub(PLArenaPool *arena, rv = sftkdb_EncryptAttribute(arena, handle, sdb, key, iterationCount, CK_INVALID_HANDLE, CKT_INVALID_TYPE, plainText, cipherText); - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); return rv; } @@ -235,13 +235,13 @@ sftkdb_decrypt_stub(SDB *sdb, SECItem *c } /* not a key handle */ - if (handle == NULL || handle->passwordLock == NULL) { + if (handle == NULL || !sftkdb_passwordLockIsInited(handle)) { return SECFailure; } - PZ_Lock(handle->passwordLock); + sftkdb_passwordReaderLock(handle); if (handle->passwordKey.data == NULL) { - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); /* PORT_SetError */ return SECFailure; } @@ -249,7 +249,7 @@ sftkdb_decrypt_stub(SDB *sdb, SECItem *c CK_INVALID_HANDLE, CKT_INVALID_TYPE, cipherText, plainText); - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); return rv; } diff -up ./lib/softoken/sftkdb.c.read-write-db-locks ./lib/softoken/sftkdb.c --- ./lib/softoken/sftkdb.c.read-write-db-locks 2023-06-04 10:42:53.000000000 +0200 +++ ./lib/softoken/sftkdb.c 2023-06-21 12:54:41.007208477 +0200 @@ -413,9 +413,9 @@ sftkdb_fixupTemplateOut(CK_ATTRIBUTE *te cipherText.data = ntemplate[i].pValue; cipherText.len = ntemplate[i].ulValueLen; - PZ_Lock(handle->passwordLock); + sftkdb_passwordReaderLock(handle); if (handle->passwordKey.data == NULL) { - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); template[i].ulValueLen = -1; crv = CKR_USER_NOT_LOGGED_IN; continue; @@ -425,7 +425,7 @@ sftkdb_fixupTemplateOut(CK_ATTRIBUTE *te objectID, ntemplate[i].type, &cipherText, &plainText); - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); if (rv != SECSuccess) { PORT_Memset(template[i].pValue, 0, template[i].ulValueLen); template[i].ulValueLen = -1; @@ -478,12 +478,12 @@ sftkdb_fixupTemplateOut(CK_ATTRIBUTE *te * we do a second check holding the lock just in case the user * loggout while we were trying to get the signature. */ - PZ_Lock(keyHandle->passwordLock); + sftkdb_passwordReaderLock(keyHandle); if (keyHandle->passwordKey.data == NULL) { /* if we are no longer logged in, no use checking the other * Signatures either. */ checkSig = PR_FALSE; - PZ_Unlock(keyHandle->passwordLock); + sftkdb_passwordReaderUnlock(keyHandle); continue; } @@ -491,7 +491,7 @@ sftkdb_fixupTemplateOut(CK_ATTRIBUTE *te &keyHandle->passwordKey, objectID, ntemplate[i].type, &plainText, &signText); - PZ_Unlock(keyHandle->passwordLock); + sftkdb_passwordReaderUnlock(keyHandle); if (rv != SECSuccess) { PORT_Memset(template[i].pValue, 0, template[i].ulValueLen); template[i].ulValueLen = -1; @@ -590,9 +590,9 @@ sftk_signTemplate(PLArenaPool *arena, SF plainText.data = template[i].pValue; plainText.len = template[i].ulValueLen; - PZ_Lock(keyHandle->passwordLock); + sftkdb_passwordReaderLock(keyHandle); if (keyHandle->passwordKey.data == NULL) { - PZ_Unlock(keyHandle->passwordLock); + sftkdb_passwordReaderUnlock(keyHandle); crv = CKR_USER_NOT_LOGGED_IN; goto loser; } @@ -601,7 +601,7 @@ sftk_signTemplate(PLArenaPool *arena, SF keyHandle->defaultIterationCount, objectID, template[i].type, &plainText, &signText); - PZ_Unlock(keyHandle->passwordLock); + sftkdb_passwordReaderUnlock(keyHandle); if (rv != SECSuccess) { crv = CKR_GENERAL_ERROR; /* better error code here? */ goto loser; @@ -770,9 +770,9 @@ sftk_ExtractTemplate(PLArenaPool *arena, plainText.data = tp->pValue; plainText.len = tp->ulValueLen; - PZ_Lock(handle->passwordLock); + sftkdb_passwordReaderLock(handle); if (handle->passwordKey.data == NULL) { - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); *crv = CKR_USER_NOT_LOGGED_IN; break; } @@ -782,7 +782,7 @@ sftk_ExtractTemplate(PLArenaPool *arena, objectID, tp->type, &plainText, &cipherText); - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); if (rv == SECSuccess) { tp->pValue = cipherText->data; tp->ulValueLen = cipherText->len; @@ -1638,15 +1638,15 @@ sftkdb_CloseDB(SFTKDBHandle *handle) } (*handle->db->sdb_Close)(handle->db); } - if (handle->passwordLock) { - PZ_Lock(handle->passwordLock); + if (sftkdb_passwordLockIsInited(handle)) { + sftkdb_passwordWriterLock(handle); } if (handle->passwordKey.data) { SECITEM_ZfreeItem(&handle->passwordKey, PR_FALSE); } - if (handle->passwordLock) { - PZ_Unlock(handle->passwordLock); - SKIP_AFTER_FORK(PZ_DestroyLock(handle->passwordLock)); + if (sftkdb_passwordLockIsInited(handle)) { + sftkdb_passwordWriterUnlock(handle); + SKIP_AFTER_FORK(sftkdb_passwordLockDestroy(handle)); } if (handle->updatePasswordKey) { SECITEM_ZfreeItem(handle->updatePasswordKey, PR_TRUE); @@ -2704,8 +2704,10 @@ sftk_NewDBHandle(SDB *sdb, int type, PRB handle->passwordKey.data = NULL; handle->passwordKey.len = 0; handle->passwordLock = NULL; + handle->passwordWriterCond = NULL; + handle->passwordReaderCond = NULL; if (type == SFTK_KEYDB_TYPE) { - handle->passwordLock = PZ_NewLock(nssILockAttribute); + (void) sftkdb_passwordLockInit(handle); } sdb->app_private = handle; return handle; diff -up ./lib/softoken/sftkdbti.h.read-write-db-locks ./lib/softoken/sftkdbti.h --- ./lib/softoken/sftkdbti.h.read-write-db-locks 2023-06-04 10:42:53.000000000 +0200 +++ ./lib/softoken/sftkdbti.h 2023-06-21 10:59:44.940835332 +0200 @@ -4,6 +4,7 @@ #ifndef SFTKDBTI_H #define SFTKDBTI_H 1 +#include /* * private defines @@ -19,6 +20,11 @@ struct SFTKDBHandleStr { SECItem *oldKey; SECItem *updatePasswordKey; PZLock *passwordLock; + PRCondVar *passwordWriterCond; + PRCondVar *passwordReaderCond; + PRBool passwordWriterActive; + int passwordWriters; + int passwordReaders; SFTKDBHandle *peerDB; SDB *update; char *updateID; @@ -79,4 +85,13 @@ sftkdb_DestroyAttributeSignature(SFTKDBH CK_OBJECT_HANDLE objectID, CK_ATTRIBUTE_TYPE type); +/* password lock functions */ +SECStatus sftkdb_passwordLockInit(SFTKDBHandle *keydb); +void sftkdb_passwordLockDestroy(SFTKDBHandle *keydb); +PRBool sftkdb_passwordLockIsInited(SFTKDBHandle *keydb); +void sftkdb_passwordReaderLock(SFTKDBHandle *keydb); +void sftkdb_passwordWriterLock(SFTKDBHandle *keydb); +void sftkdb_passwordReaderUnlock(SFTKDBHandle *keydb); +void sftkdb_passwordWriterUnlock(SFTKDBHandle *keydb); + #endif diff -up ./lib/softoken/sftkpwd.c.read-write-db-locks ./lib/softoken/sftkpwd.c --- ./lib/softoken/sftkpwd.c.read-write-db-locks 2023-06-04 10:42:53.000000000 +0200 +++ ./lib/softoken/sftkpwd.c 2023-06-21 10:59:44.940835332 +0200 @@ -632,6 +632,126 @@ loser: return rv; } +SECStatus +sftkdb_passwordLockInit(SFTKDBHandle *keydb) +{ + keydb->passwordLock = PZ_NewLock(nssILockAttribute); + if (keydb->passwordLock == NULL) { + return SECFailure; + } + keydb->passwordWriterCond = PR_NewCondVar(keydb->passwordLock); + if (keydb->passwordWriterCond == NULL) { + PZ_DestroyLock(keydb->passwordLock); + keydb->passwordLock = NULL; + return SECFailure; + } + keydb->passwordReaderCond = PR_NewCondVar(keydb->passwordLock); + if (keydb->passwordReaderCond == NULL) { + PR_DestroyCondVar(keydb->passwordWriterCond); + PZ_DestroyLock(keydb->passwordLock); + keydb->passwordWriterCond = NULL; + keydb->passwordLock = NULL; + return SECFailure; + } + keydb->passwordWriters = 0; + keydb->passwordReaders = 0; + keydb->passwordWriterActive = PR_FALSE; + return SECSuccess; +} + +void +sftkdb_passwordLockDestroy(SFTKDBHandle *keydb) +{ + PR_DestroyCondVar(keydb->passwordWriterCond); + keydb->passwordWriterCond = NULL; + PR_DestroyCondVar(keydb->passwordReaderCond); + keydb->passwordReaderCond = NULL; + PZ_DestroyLock(keydb->passwordLock); + keydb->passwordLock = NULL; +} + +PRBool +sftkdb_passwordLockIsInited(SFTKDBHandle *keydb) +{ + return (keydb->passwordLock) && (keydb->passwordWriterCond) + && (keydb->passwordReaderCond); +} + +/* we need reader/writer locks for the database because we have servers + * that use the database key in their transactions on multi-cpu systems. + * This means that all the other cpus doing private key operations become + * effectively single threaded. We implement our specific reader/writer + * locks because we want writer locks to interrupt reader streams to + * prevent starvation on the writer side. */ +void +sftkdb_passwordReaderLock(SFTKDBHandle *keydb) +{ + PZ_Lock(keydb->passwordLock); + /* if there is a writer waiting or running wait until the + * writer has completed before we continue. This prevents + * writer starvation in a loaded system */ + while (keydb->passwordWriters) { + PR_WaitCondVar(keydb->passwordReaderCond, PR_INTERVAL_NO_TIMEOUT); + } + /* allow multiple readers */ + keydb->passwordReaders++; + PORT_Assert(keydb->passwordWriterActive == PR_FALSE); + PZ_Unlock(keydb->passwordLock); +} + +/* We can only have 1 active writer or multiple active readers. + * Readers will wait for an active writer. This means lots of write + * operations can stall the readers. Write operations are rare, though and + * only happen on database update, or on initial login. + */ +void +sftkdb_passwordWriterLock(SFTKDBHandle *keydb) +{ + PZ_Lock(keydb->passwordLock); + keydb->passwordWriters++; + /* if we have any readers or writers, wait for the writer condition */ + while (keydb->passwordReaders || keydb->passwordWriterActive) { + PR_WaitCondVar(keydb->passwordWriterCond, PR_INTERVAL_NO_TIMEOUT); + } + /* only one writer allowed at a time */ + keydb->passwordWriterActive = PR_TRUE; + PZ_Unlock(keydb->passwordLock); +} + +/* unlock, decrements the reader counter, and if we have not active readers, + * notify any waiting writers. */ +void +sftkdb_passwordReaderUnlock(SFTKDBHandle *keydb) +{ + PZ_Lock(keydb->passwordLock); + PORT_Assert(keydb->passwordReaders); + keydb->passwordReaders--; + if (keydb->passwordReaders == 0) { + if (keydb->passwordWriters) { + PR_NotifyCondVar(keydb->passwordWriterCond); + } + } + PZ_Unlock(keydb->passwordLock); +} + +/* unlock, if there are more writers, wake one of them up. If there or no more + * writers but readers, wake up all the readers. */ +void +sftkdb_passwordWriterUnlock(SFTKDBHandle *keydb) +{ + PZ_Lock(keydb->passwordLock); + PORT_Assert(keydb->passwordWriters); + PORT_Assert(keydb->passwordWriterActive); + keydb->passwordWriterActive = PR_FALSE; + keydb->passwordWriters--; + if (keydb->passwordWriters) { + PR_NotifyCondVar(keydb->passwordWriterCond); + } else { + PR_NotifyAllCondVar(keydb->passwordReaderCond); + } + PZ_Unlock(keydb->passwordLock); +} + /* * safely swith the passed in key for the one caches in the keydb handle * @@ -645,13 +765,13 @@ sftkdb_switchKeys(SFTKDBHandle *keydb, S unsigned char *data; int len; - if (keydb->passwordLock == NULL) { + if (!sftkdb_passwordLockIsInited(keydb)) { PORT_Assert(keydb->type != SFTK_KEYDB_TYPE); return; } /* an atomic pointer set would be nice */ - SKIP_AFTER_FORK(PZ_Lock(keydb->passwordLock)); + SKIP_AFTER_FORK(sftkdb_passwordWriterLock(keydb)); data = keydb->passwordKey.data; len = keydb->passwordKey.len; keydb->passwordKey.data = passKey->data; @@ -659,7 +779,7 @@ sftkdb_switchKeys(SFTKDBHandle *keydb, S keydb->defaultIterationCount = iterationCount; passKey->data = data; passKey->len = len; - SKIP_AFTER_FORK(PZ_Unlock(keydb->passwordLock)); + SKIP_AFTER_FORK(sftkdb_passwordWriterUnlock(keydb)); } /* @@ -705,11 +825,11 @@ sftkdb_GetUpdatePasswordKey(SFTKDBHandle return NULL; } - PZ_Lock(handle->passwordLock); + sftkdb_passwordReaderLock(handle); if (handle->updatePasswordKey) { key = SECITEM_DupItem(handle->updatePasswordKey); } - PZ_Unlock(handle->passwordLock); + sftkdb_passwordReaderUnlock(handle); return key; } @@ -732,12 +852,12 @@ sftkdb_FreeUpdatePasswordKey(SFTKDBHandl return; } - PZ_Lock(handle->passwordLock); + sftkdb_passwordWriterLock(handle); if (handle->updatePasswordKey) { key = handle->updatePasswordKey; handle->updatePasswordKey = NULL; } - PZ_Unlock(handle->passwordLock); + sftkdb_passwordWriterUnlock(handle); if (key) { SECITEM_ZfreeItem(key, PR_TRUE); @@ -1004,7 +1124,7 @@ sftkdb_finishPasswordCheck(SFTKDBHandle * update, as we now have both required passwords. * */ - PZ_Lock(keydb->passwordLock); + sftkdb_passwordWriterLock(keydb); if (sftkdb_NeedUpdateDBPassword(keydb)) { /* Squirrel this special key away. * This has the side effect of turning sftkdb_NeedLegacyPW off, @@ -1012,7 +1132,7 @@ sftkdb_finishPasswordCheck(SFTKDBHandle * SFTK_GET_PW_DB (thus effecting both sftkdb_CheckPassword() * and sftkdb_HasPasswordSet()) */ keydb->updatePasswordKey = SECITEM_DupItem(key); - PZ_Unlock(keydb->passwordLock); + sftkdb_passwordWriterUnlock(keydb); if (keydb->updatePasswordKey == NULL) { /* PORT_Error set by SECITEM_DupItem */ rv = SECFailure; @@ -1077,7 +1197,7 @@ sftkdb_finishPasswordCheck(SFTKDBHandle * update case. */ } } else { - PZ_Unlock(keydb->passwordLock); + sftkdb_passwordWriterUnlock(keydb); } /* load the keys, so the keydb can parse it's key set */ sftkdb_switchKeys(keydb, key, iterationCount);