tech-kern archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
(partly working) patch to make opencrypto use mutex/condvar
The enclosed doesn't work for me (disappointing, since I wrote it!)
with the cryptosoft backend, but one of my coworkers swears it works
fine for him with a hardware driver. Comments? This is the first
code I've tried to convert to our modern synchronization primitives.
Clearly, there could, and probably should, be two separate locks on
the two separate queues used for symmetric and asymmetric requests.
Also, this does little good if we can't mark the device MPSAFE so
we can submit multiple request streams via /dev/crypto, and I haven't
even thought about what that would require.
Comments very, very welcome.
--
Thor Lancelot Simon
tls%rek.tjls.com@localhost
"The inconsistency is startling, though admittedly, if consistency is to
be abandoned or transcended, there is no problem." - Noam Chomsky
Index: crypto.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/crypto.c,v
retrieving revision 1.22
diff -u -r1.22 crypto.c
--- crypto.c 1 Feb 2008 04:52:35 -0000 1.22
+++ crypto.c 1 Feb 2008 18:28:31 -0000
@@ -43,8 +43,10 @@
#include <opencrypto/cryptodev.h>
#include <opencrypto/xform.h> /* XXX for M_XDATA */
- #define splcrypto splnet
- /* below is kludges to check whats still missing */
+kcondvar_t cryptoret_cv;
+kmutex_t crypto_mtx;
+
+/* below are kludges for residual code wrtitten to FreeBSD interfaces */
#define SWI_CRYPTO 17
#define register_swi(lvl, fn) \
softint_establish(SOFTINT_NET, (void (*)(void*))fn, NULL)
@@ -89,7 +91,6 @@
*/
struct pool cryptop_pool;
struct pool cryptodesc_pool;
-int crypto_pool_initialized = 0;
int crypto_usercrypto = 1; /* userland may open /dev/crypto */
int crypto_userasymcrypto = 1; /* userland may do asym crypto reqs */
@@ -178,7 +179,12 @@
{
int error;
-
+ mutex_init(&crypto_mtx, MUTEX_DEFAULT, IPL_NET);
+ cv_init(&cryptoret_cv, "crypto_wait");
+ pool_init(&cryptop_pool, sizeof(struct cryptop), 0, 0,
+ 0, "cryptop", NULL, IPL_NET);
+ pool_init(&cryptodesc_pool, sizeof(struct cryptodesc), 0, 0,
+ 0, "cryptodesc", NULL, IPL_NET);
crypto_drivers = malloc(CRYPTO_DRIVERS_INITIAL *
sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT | M_ZERO);
if (crypto_drivers == NULL) {
@@ -225,9 +231,8 @@
struct cryptoini *cr;
u_int32_t hid, lid;
int err = EINVAL;
- int s;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
if (crypto_drivers == NULL)
goto done;
@@ -288,7 +293,7 @@
}
}
done:
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -301,9 +306,8 @@
{
u_int32_t hid;
int err = 0;
- int s;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
if (crypto_drivers == NULL) {
err = EINVAL;
@@ -337,7 +341,7 @@
bzero(&crypto_drivers[hid], sizeof(struct cryptocap));
done:
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -349,11 +353,11 @@
crypto_get_driverid(u_int32_t flags)
{
struct cryptocap *newdrv;
- int i, s;
+ int i;
- crypto_init();
+ crypto_init(); /* XXX oh, this is foul! */
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx); /* XXX alloc_mtx? */
for (i = 0; i < crypto_drivers_num; i++)
if (crypto_drivers[i].cc_process == NULL &&
(crypto_drivers[i].cc_flags & CRYPTOCAP_F_CLEANUP) == 0 &&
@@ -364,7 +368,7 @@
if (i == crypto_drivers_num) {
/* Be careful about wrap-around. */
if (2 * crypto_drivers_num <= crypto_drivers_num) {
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
printf("crypto: driver count wraparound!\n");
return -1;
}
@@ -372,7 +376,7 @@
newdrv = malloc(2 * crypto_drivers_num *
sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
if (newdrv == NULL) {
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
printf("crypto: no space to expand driver table!\n");
return -1;
}
@@ -393,7 +397,7 @@
if (bootverbose)
printf("crypto: assign driver %u, flags %u\n", i, flags);
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return i;
}
@@ -415,11 +419,10 @@
int (*kprocess)(void*, struct cryptkop *, int),
void *karg)
{
- int s;
struct cryptocap *cap;
int err;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
cap = crypto_checkdriver(driverid);
if (cap != NULL &&
@@ -446,7 +449,7 @@
} else
err = EINVAL;
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -463,9 +466,9 @@
void *arg)
{
struct cryptocap *cap;
- int s, err;
+ int err;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
cap = crypto_checkdriver(driverid);
/* NB: algorithms are in the range [1..max] */
@@ -498,7 +501,7 @@
} else
err = EINVAL;
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -511,11 +514,11 @@
int
crypto_unregister(u_int32_t driverid, int alg)
{
- int i, err, s;
+ int i, err;
u_int32_t ses;
struct cryptocap *cap;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
cap = crypto_checkdriver(driverid);
if (cap != NULL &&
@@ -544,7 +547,7 @@
} else
err = EINVAL;
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -554,14 +557,19 @@
* around so that subsequent calls using those sessions will
* correctly detect the driver has been unregistered and reroute
* requests.
+ *
+ * XXX careful. Don't change this to call crypto_unregister() for each
+ * XXX registered algorithm unless you drop the mutex across the calls;
+ * XXX you can't take it recursively.
*/
int
crypto_unregister_all(u_int32_t driverid)
{
- int i, err, s = splcrypto();
+ int i, err;
u_int32_t ses;
struct cryptocap *cap;
+ mutex_spin_enter(&crypto_mtx);
cap = crypto_checkdriver(driverid);
if (cap != NULL) {
for (i = CRYPTO_ALGORITHM_MIN; i <= CRYPTO_ALGORITHM_MAX; i++) {
@@ -581,7 +589,7 @@
} else
err = EINVAL;
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -593,9 +601,9 @@
crypto_unblock(u_int32_t driverid, int what)
{
struct cryptocap *cap;
- int needwakeup, err, s;
+ int needwakeup, err;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
cap = crypto_checkdriver(driverid);
if (cap != NULL) {
needwakeup = 0;
@@ -608,12 +616,13 @@
cap->cc_kqblocked = 0;
}
if (needwakeup) {
+ mutex_spin_exit(&crypto_mtx);
setsoftcrypto(softintr_cookie);
}
err = 0;
} else
err = EINVAL;
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return err;
}
@@ -626,9 +635,9 @@
crypto_dispatch(struct cryptop *crp)
{
u_int32_t hid = SESID2HID(crp->crp_sid);
- int s, result;
+ int result;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
cryptostats.cs_ops++;
@@ -645,6 +654,7 @@
*/
cap = crypto_checkdriver(hid);
if (cap && !cap->cc_qblocked) {
+ mutex_spin_exit(&crypto_mtx);
result = crypto_invoke(crp, 0);
if (result == ERESTART) {
/*
@@ -652,6 +662,7 @@
* driver ``blocked'' for cryptop's and put
* the op on the queue.
*/
+ mutex_spin_enter(&crypto_mtx);
crypto_drivers[hid].cc_qblocked = 1;
TAILQ_INSERT_HEAD(&crp_q, crp, crp_next);
cryptostats.cs_blocks++;
@@ -674,12 +685,13 @@
*/
TAILQ_INSERT_TAIL(&crp_q, crp, crp_next);
if (wasempty) {
+ mutex_spin_exit(&crypto_mtx);
setsoftcrypto(softintr_cookie);
}
result = 0;
}
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return result;
}
@@ -692,13 +704,14 @@
crypto_kdispatch(struct cryptkop *krp)
{
struct cryptocap *cap;
- int s, result;
+ int result;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
cryptostats.cs_kops++;
cap = crypto_checkdriver(krp->krp_hid);
if (cap && !cap->cc_kqblocked) {
+ mutex_spin_exit(&crypto_mtx);
result = crypto_kinvoke(krp, 0);
if (result == ERESTART) {
/*
@@ -706,6 +719,7 @@
* driver ``blocked'' for cryptop's and put
* the op on the queue.
*/
+ mutex_spin_enter(&crypto_mtx);
crypto_drivers[krp->krp_hid].cc_kqblocked = 1;
TAILQ_INSERT_HEAD(&crp_kq, krp, krp_next);
cryptostats.cs_kblocks++;
@@ -718,7 +732,7 @@
TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next);
result = 0;
}
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
return result;
}
@@ -857,20 +871,15 @@
crypto_freereq(struct cryptop *crp)
{
struct cryptodesc *crd;
- int s;
if (crp == NULL)
return;
- s = splcrypto();
-
while ((crd = crp->crp_desc) != NULL) {
crp->crp_desc = crd->crd_next;
pool_put(&cryptodesc_pool, crd);
}
-
pool_put(&cryptop_pool, crp);
- splx(s);
}
/*
@@ -881,21 +890,9 @@
{
struct cryptodesc *crd;
struct cryptop *crp;
- int s;
-
- s = splcrypto();
-
- if (crypto_pool_initialized == 0) {
- pool_init(&cryptop_pool, sizeof(struct cryptop), 0, 0,
- 0, "cryptop", NULL, IPL_NET);
- pool_init(&cryptodesc_pool, sizeof(struct cryptodesc), 0, 0,
- 0, "cryptodesc", NULL, IPL_NET);
- crypto_pool_initialized = 1;
- }
crp = pool_get(&cryptop_pool, 0);
if (crp == NULL) {
- splx(s);
return NULL;
}
bzero(crp, sizeof(struct cryptop));
@@ -903,7 +900,6 @@
while (num--) {
crd = pool_get(&cryptodesc_pool, 0);
if (crd == NULL) {
- splx(s);
crypto_freereq(crp);
return NULL;
}
@@ -913,7 +909,6 @@
crp->crp_desc = crd;
}
- splx(s);
return crp;
}
@@ -934,7 +929,7 @@
* has done its tsleep().
*/
{
- int s, wasempty;
+ int wasempty;
/*
* Normal case; queue the callback for the thread.
*
@@ -943,12 +938,12 @@
* back to mark operations completed. Thus we need
* to mask both while manipulating the return queue.
*/
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
wasempty = TAILQ_EMPTY(&crp_ret_q);
TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next);
if (wasempty)
- wakeup_one(&crp_ret_q);
- splx(s);
+ cv_signal(&cryptoret_cv);
+ mutex_spin_exit(&crypto_mtx);
}
}
@@ -958,7 +953,7 @@
void
crypto_kdone(struct cryptkop *krp)
{
- int s, wasempty;
+ int wasempty;
if (krp->krp_status != 0)
cryptostats.cs_kerrs++;
@@ -968,21 +963,20 @@
* back to mark operations completed. Thus we need
* to mask both while manipulating the return queue.
*/
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
wasempty = TAILQ_EMPTY(&crp_ret_kq);
TAILQ_INSERT_TAIL(&crp_ret_kq, krp, krp_next);
if (wasempty)
- wakeup_one(&crp_ret_q);
- splx(s);
+ cv_signal(&cryptoret_cv);
+ mutex_spin_exit(&crypto_mtx);
}
int
crypto_getfeat(int *featp)
{
int hid, kalg, feat = 0;
- int s;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
if (crypto_userasymcrypto == 0)
goto out;
@@ -1000,7 +994,7 @@
feat |= 1 << kalg;
}
out:
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
*featp = feat;
return (0);
}
@@ -1014,11 +1008,11 @@
struct cryptop *crp, *submit;
struct cryptkop *krp;
struct cryptocap *cap;
- int result, hint, s;
+ int result, hint;
printf("crypto softint\n");
cryptostats.cs_intrs++;
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
do {
/*
* Find the first element in the queue that can be
@@ -1059,7 +1053,11 @@
}
if (submit != NULL) {
TAILQ_REMOVE(&crp_q, submit, crp_next);
+ mutex_spin_exit(&crypto_mtx);
result = crypto_invoke(submit, hint);
+ /* we must take here as the TAILQ op or kinvoke
+ may need this mutex below. sigh. */
+ mutex_spin_enter(&crypto_mtx);
if (result == ERESTART) {
/*
* The driver ran out of resources, mark the
@@ -1089,7 +1087,10 @@
}
if (krp != NULL) {
TAILQ_REMOVE(&crp_kq, krp, krp_next);
+ mutex_spin_exit(&crypto_mtx);
result = crypto_kinvoke(krp, 0);
+ /* the next iteration will want the mutex. :-/ */
+ mutex_spin_enter(&crypto_mtx);
if (result == ERESTART) {
/*
* The driver ran out of resources, mark the
@@ -1107,7 +1108,7 @@
}
}
} while (submit != NULL || krp != NULL);
- splx(s);
+ mutex_spin_exit(&crypto_mtx);
}
/*
@@ -1118,9 +1119,15 @@
{
struct cryptop *crp;
struct cryptkop *krp;
- int s;
- s = splcrypto();
+ /*
+ * We take the mutex here the first time through. On subsequent
+ * entries to the infinite loop below, we will still have the
+ * mutex from the prior iteration.
+ */
+
+ mutex_spin_enter(&crypto_mtx);
+
for (;;) {
crp = TAILQ_FIRST(&crp_ret_q);
if (crp != NULL)
@@ -1130,7 +1137,7 @@
TAILQ_REMOVE(&crp_ret_kq, krp, krp_next);
if (crp != NULL || krp != NULL) {
- splx(s); /* lower ipl for callbacks */
+ mutex_spin_exit(&crypto_mtx); /* drop for callbacks */
if (crp != NULL) {
#ifdef CRYPTO_TIMING
if (crypto_timing) {
@@ -1149,14 +1156,10 @@
}
if (krp != NULL)
krp->krp_callback(krp);
- s = splcrypto();
+ mutex_spin_enter(&crypto_mtx);
} else {
- (void) tsleep(&crp_ret_q, PLOCK, "crypto_wait", 0);
+ cv_wait(&cryptoret_cv, &crypto_mtx);
cryptostats.cs_rets++;
}
}
}
-
-
-
-
Index: cryptodev.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptodev.c,v
retrieving revision 1.31
diff -u -r1.31 cryptodev.c
--- cryptodev.c 1 Feb 2008 04:52:35 -0000 1.31
+++ cryptodev.c 1 Feb 2008 18:28:33 -0000
@@ -55,7 +55,6 @@
#include <opencrypto/cryptodev.h>
#include <opencrypto/xform.h>
- #define splcrypto splnet
#ifdef CRYPTO_DEBUG
#define DPRINTF(a) uprintf a
#else
@@ -368,7 +367,7 @@
{
struct cryptop *crp = NULL;
struct cryptodesc *crde = NULL, *crda = NULL;
- int error, s;
+ int error;
if (cop->len > 256*1024-4)
return (E2BIG);
@@ -474,11 +473,21 @@
crp->crp_mac=cse->tmp_mac;
}
- s = splcrypto(); /* NB: only needed with CRYPTO_F_CBIMM */
+ /*
+ * XXX there was a comment here which said that we went to
+ * XXX splcrypto() but needed to only if CRYPTO_F_CBIMM,
+ * XXX disabled on NetBSD since 1.6O due to a race condition.
+ * XXX But crypto_dispatch went to splcrypto() itself! (And
+ * XXX now takes the crypto_mtx mutex itself). We do, however,
+ * XXX need to hold the mutex across the call to mtsleep().
+ * XXX (should we arrange for crypto_dispatch to return to
+ * XXX us with it held? it seems quite ugly to do so.)
+ */
error = crypto_dispatch(crp);
+ mutex_spin_enter(&crypto_mtx);
if (error == 0 && (crp->crp_flags & CRYPTO_F_DONE) == 0)
- error = tsleep(crp, PSOCK, "crydev", 0);
- splx(s);
+ error = mtsleep(crp, PSOCK, "crydev", 0, &crypto_mtx);
+ mutex_spin_exit(&crypto_mtx);
if (error) {
goto bail;
}
@@ -519,7 +528,9 @@
cse->error = crp->crp_etype;
if (crp->crp_etype == EAGAIN)
return crypto_dispatch(crp);
+ mutex_spin_enter(&crypto_mtx);
wakeup_one(crp);
+ mutex_spin_exit(&crypto_mtx);
return (0);
}
@@ -528,7 +539,9 @@
{
struct cryptkop *krp = (struct cryptkop *) op;
+ mutex_spin_enter(&crypto_mtx);
wakeup_one(krp);
+ mutex_spin_exit(&crypto_mtx);
return (0);
}
@@ -621,7 +634,9 @@
error = crypto_kdispatch(krp);
if (error == 0)
- error = tsleep(krp, PSOCK, "crydev", 0);
+ mutex_spin_enter(&crypto_mtx);
+ error = mtsleep(krp, PSOCK, "crydev", 0, &crypto_mtx);
+ mutex_spin_exit(&crypto_mtx);
if (error)
goto fail;
Index: cryptodev.h
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptodev.h,v
retrieving revision 1.10
diff -u -r1.10 cryptodev.h
--- cryptodev.h 1 Feb 2008 04:52:35 -0000 1.10
+++ cryptodev.h 1 Feb 2008 18:28:33 -0000
@@ -391,7 +391,11 @@
extern int crypto_userasymcrypto; /* userland may do asym crypto reqs */
extern int crypto_devallowsoft; /* only use hardware crypto */
+/*
+ * Mutual exclusion and its unwelcome friends.
+ */
+extern kmutex_t crypto_mtx;
/*
* initialize the crypto framework subsystem (not the pseudo-device).
* This must be called very early in boot, so the framework is ready
Index: cryptosoft_xform.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptosoft_xform.c,v
retrieving revision 1.5
diff -u -r1.5 cryptosoft_xform.c
--- cryptosoft_xform.c 4 Mar 2007 06:03:40 -0000 1.5
+++ cryptosoft_xform.c 1 Feb 2008 18:28:33 -0000
@@ -537,7 +537,7 @@
int err;
MALLOC(*sched, u_int8_t *, sizeof(rijndael_ctx), M_CRYPTO_DATA,
- M_WAITOK);
+ M_NOWAIT);
if (*sched != NULL) {
bzero(*sched, sizeof(rijndael_ctx));
rijndael_set_key((rijndael_ctx *) *sched, key, len * 8);
Home |
Main Index |
Thread Index |
Old Index