tech-crypto archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

(less busted) patch to make opencrypto use mutex/condvar



Here's a better patch.  Note the (ugly, for testing) restructuring of
the tests before the cv_wait() in cryptodev_op(), and the DPRINTF:
with the traditional 4.4BSD scheduler (though seemingly not always
if using SCHED_M2) we seem to *always* pick up the request here before
cryptoret runs: the CRYPTO_F_DONE printf fires with the request in the
queued state.

I'm not entirely surprised: for the cryptosoft backend, after all, it's
not clear why we'd get preempted on the way down or back up and let
cryptoret run.  I think FreeBSD _may_ have this bug but never or almost
never see it because of the use of CBIMM for software requests, skipping
the enqueueing of the requests on the retq in crypto_done.  I am not
very happy with my workaround (and didn't turn it on for kop requests
yet -- it's really too ugly to stay).

Now what's left is that if process A has the device open and is submitting
requests, process B's requests all stick waiting on crydev, even after
process A exits. So, for example, if you do:

        sysctl -w kern.cryptodevallowsoft=0
        openssl speed -engine cryptodev -evp des-ede3-cbc & \
        openssl speed -engine cryptodev -evp des-ede3-cbc

You'll find that one of the processes sticks waiting on crydev, which
means it's cv_wait()ing on some request's crp->crp_cv. 

It seems possible that I broke this in my recent change that deprecated
CRIOGET in the frontend -- I'll look at it more later this weekend -- but,
of course, I see no obvious candidate for how.  Oh well; it's still some
progress.  And Pavel found a bug I hadn't even tripped over yet...

Patch attached and at http://www.panix.com/~tls/ocf-mtx5.diff

Thor
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    2 Feb 2008 10:52:01 -0000
@@ -26,9 +26,6 @@
 #include <sys/cdefs.h>
 __KERNEL_RCSID(0, "$NetBSD: crypto.c,v 1.22 2008/02/01 04:52:35 tls Exp $");
 
-/* XXX FIXME: should be defopt'ed */
-#define CRYPTO_TIMING                  /* enable cryptop timing stuff */
-
 #include <sys/param.h>
 #include <sys/reboot.h>
 #include <sys/systm.h>
@@ -40,11 +37,14 @@
 #include <sys/sysctl.h>
 #include <sys/intr.h>
 
+#include "opt_ocf.h"
 #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)
@@ -78,18 +78,55 @@
  * but have two to avoid type futzing (cryptop vs. cryptkop).  See below
  * for how synchronization is handled.
  */
-static TAILQ_HEAD(,cryptop) crp_ret_q =        /* callback queues */
+static TAILQ_HEAD(crprethead, cryptop) crp_ret_q =     /* callback queues */
                TAILQ_HEAD_INITIALIZER(crp_ret_q);
-static TAILQ_HEAD(,cryptkop) crp_ret_kq =
+static TAILQ_HEAD(krprethead, cryptkop) crp_ret_kq =
                TAILQ_HEAD_INITIALIZER(crp_ret_kq);
 
 /*
+ * XXX these functions are ghastly hacks for when the submission
+ * XXX routines discover a request that was not CBIMM is already
+ * XXX done, and must be yanked from the retq (where _done) put it
+ * XXX as cryptoret won't get the chance.  The queue is walked backwards
+ * XXX as the request is generally the last one queued.
+ *
+ *      call with the lock held, or else.
+ */
+int
+crypto_ret_q_remove(struct cryptop *crp)
+{
+       struct cryptop * acrp;
+
+       TAILQ_FOREACH_REVERSE(acrp, &crp_ret_q, crprethead, crp_next) {
+               if (acrp == crp) {
+                       TAILQ_REMOVE(&crp_ret_q, crp, crp_next);
+                       return 1;
+               }
+       }
+       return 0;
+}
+
+int
+crypto_ret_kq_remove(struct cryptkop *krp)
+{
+       struct cryptkop * akrp;
+
+       TAILQ_FOREACH_REVERSE(akrp, &crp_ret_kq, krprethead, krp_next) {
+               if (akrp == krp) {
+                       TAILQ_REMOVE(&crp_ret_kq, krp, krp_next);
+                       return 1;
+               }
+       }
+       return 0;
+}
+
+/*
  * Crypto op and desciptor data structures are allocated
  * from separate private zones(FreeBSD)/pools(netBSD/OpenBSD) .
  */
 struct pool cryptop_pool;
 struct pool cryptodesc_pool;
-int crypto_pool_initialized = 0;
+struct pool cryptkop_pool;
 
 int    crypto_usercrypto = 1;          /* userland may open /dev/crypto */
 int    crypto_userasymcrypto = 1;      /* userland may do asym crypto reqs */
@@ -170,14 +207,23 @@
 static int crypto_kinvoke(struct cryptkop *krp, int hint);
 
 static struct cryptostats cryptostats;
+#ifdef CRYPTO_TIMING
 static int crypto_timing = 0;
-
+#endif
 
 static int
 crypto_init0(void)
 {
        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);
+       pool_init(&cryptkop_pool, sizeof(struct cryptkop), 0, 0,
+                 0, "cryptkop", NULL, IPL_NET);
 
        crypto_drivers = malloc(CRYPTO_DRIVERS_INITIAL *
            sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT | M_ZERO);
@@ -225,9 +271,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 +333,7 @@
                }
        }
 done:
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -301,9 +346,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 +381,7 @@
                bzero(&crypto_drivers[hid], sizeof(struct cryptocap));
 
 done:
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -349,11 +393,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);
        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 +408,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 +416,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 +437,7 @@
        if (bootverbose)
                printf("crypto: assign driver %u, flags %u\n", i, flags);
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 
        return i;
 }
@@ -415,11 +459,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 +489,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -463,9 +506,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 +541,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -511,11 +554,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 +587,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -554,14 +597,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 +629,7 @@
        } else
                err = EINVAL;
 
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        return err;
 }
 
@@ -593,9 +641,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 +656,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 +675,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 +694,7 @@
                 */
                cap = crypto_checkdriver(hid);
                if (cap && !cap->cc_qblocked) {
+                       mutex_spin_exit(&crypto_mtx);
                        result = crypto_invoke(crp, 0);
                        if (result == ERESTART) {
                                /*
@@ -652,10 +702,12 @@
                                 * 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++;
                        }
+                       goto out_released;
                } else {
                        /*
                         * The driver is blocked, just queue the op until
@@ -674,13 +726,17 @@
                 */
                TAILQ_INSERT_TAIL(&crp_q, crp, crp_next);
                if (wasempty) {
+                       mutex_spin_exit(&crypto_mtx);
                        setsoftcrypto(softintr_cookie);
+                       result = 0;
+                       goto out_released;
                }
 
                result = 0;
        }
-       splx(s);
 
+       mutex_spin_exit(&crypto_mtx);
+out_released:
        return result;
 }
 
@@ -692,13 +748,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 +763,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 +776,7 @@
                TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next);
                result = 0;
        }
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 
        return result;
 }
@@ -736,7 +794,7 @@
        if (krp == NULL)
                return EINVAL;
        if (krp->krp_callback == NULL) {
-               free(krp, M_XDATA);             /* XXX allocated in cryptodev */
+               pool_put(&cryptkop_pool, krp);
                return EINVAL;
        }
 
@@ -846,6 +904,7 @@
                /*
                 * Invoke the driver to process the request.
                 */
+               DPRINTF(("calling process for %08x\n", (uint32_t)crp));
                return (*process)(crypto_drivers[hid].cc_arg, crp, hint);
        }
 }
@@ -857,20 +916,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,29 +935,17 @@
 {
        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));
+       cv_init(&crp->crp_cv, "crydev");
 
        while (num--) {
                crd = pool_get(&cryptodesc_pool, 0);
                if (crd == NULL) {
-                       splx(s);
                        crypto_freereq(crp);
                        return NULL;
                }
@@ -913,7 +955,6 @@
                crp->crp_desc = crd;
        }
 
-       splx(s);
        return crp;
 }
 
@@ -923,6 +964,8 @@
 void
 crypto_done(struct cryptop *crp)
 {
+       int wasempty;
+
        if (crp->crp_etype != 0)
                cryptostats.cs_errs++;
 #ifdef CRYPTO_TIMING
@@ -930,26 +973,30 @@
                crypto_tstat(&cryptostats.cs_done, &crp->crp_tstamp);
 #endif
        /*
-        * On netbsd 1.6O, CBIMM does its wake_one() before the requestor
-        * has done its tsleep().
+        * Normal case; queue the callback for the thread.
+        *
+        * The return queue is manipulated by the swi thread
+        * and, potentially, by crypto device drivers calling
+        * back to mark operations completed.  Thus we need
+        * to mask both while manipulating the return queue.
         */
-       {
-               int s, wasempty;
+       mutex_spin_enter(&crypto_mtx);
+       wasempty = TAILQ_EMPTY(&crp_ret_q);
+       DPRINTF(("crypto_done: queueing %08x\n", (uint32_t)crp));
+       TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next);
+       if (wasempty) {
+               DPRINTF(("crypto_done: waking cryptoret, %08x " \
+                       "hit empty queue\n.", (uint32_t)crp));
                /*
-                * Normal case; queue the callback for the thread.
-                *
-                * The return queue is manipulated by the swi thread
-                * and, potentially, by crypto device drivers calling
-                * back to mark operations completed.  Thus we need
-                * to mask both while manipulating the return queue.
+                * We set the formerly unused DONE flag so we
+                * don't race with the cv_wait, in cryptodev_op,
+                * on the per-request callback cv, which in turn
+                * is signalled from cryptoret, which we signal here.
                 */
-               s = splcrypto();
-               wasempty = TAILQ_EMPTY(&crp_ret_q);
-               TAILQ_INSERT_TAIL(&crp_ret_q, crp, crp_next);
-               if (wasempty)
-                       wakeup_one(&crp_ret_q);
-               splx(s);
+               crp->crp_flags |= CRYPTO_F_DONE;
+               cv_signal(&cryptoret_cv);
        }
+       mutex_spin_exit(&crypto_mtx);
 }
 
 /*
@@ -958,7 +1005,7 @@
 void
 crypto_kdone(struct cryptkop *krp)
 {
-       int s, wasempty;
+       int wasempty;
 
        if (krp->krp_status != 0)
                cryptostats.cs_kerrs++;
@@ -968,21 +1015,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 +1046,7 @@
                                feat |=  1 << kalg;
        }
 out:
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
        *featp = feat;
        return (0);
 }
@@ -1014,11 +1060,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 +1105,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 +1139,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 +1160,7 @@
                        }
                }
        } while (submit != NULL || krp != NULL);
-       splx(s);
+       mutex_spin_exit(&crypto_mtx);
 }
 
 /*
@@ -1118,10 +1171,10 @@
 {
        struct cryptop *crp;
        struct cryptkop *krp;
-       int s;
 
-       s = splcrypto();
        for (;;) {
+               mutex_spin_enter(&crypto_mtx);
+
                crp = TAILQ_FIRST(&crp_ret_q);
                if (crp != NULL)
                        TAILQ_REMOVE(&crp_ret_q, crp, crp_next);
@@ -1129,8 +1182,9 @@
                if (krp != NULL)
                        TAILQ_REMOVE(&crp_ret_kq, krp, krp_next);
 
+               /* drop before calling any callbacks. */
+               mutex_spin_exit(&crypto_mtx);
                if (crp != NULL || krp != NULL) {
-                       splx(s);                /* lower ipl for callbacks */
                        if (crp != NULL) {
 #ifdef CRYPTO_TIMING
                                if (crypto_timing) {
@@ -1145,18 +1199,23 @@
                                        crypto_tstat(&cryptostats.cs_finis, &t);
                                } else
 #endif
+                               {
+#if 0          /* too loud */
+                                       DCPRINTF(("cryptoret: calling back " \
+                                               "%08x for crp %08x\n", \
+                                               (uint32_t)crp->crp_callback, \
+                                               (uint32_t)crp));
+#endif
                                        crp->crp_callback(crp);
+                               }
                        }
                        if (krp != NULL)
                                krp->krp_callback(krp);
-                       s  = splcrypto();
                } else {
-                       (void) tsleep(&crp_ret_q, PLOCK, "crypto_wait", 0);
+                       mutex_spin_enter(&crypto_mtx);
+                       cv_wait(&cryptoret_cv, &crypto_mtx);
+                       mutex_spin_exit(&crypto_mtx);
                        cryptostats.cs_rets++;
                }
        }
 }
-
-
-
-
Index: cryptodev.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptodev.c,v
retrieving revision 1.32
diff -u -r1.32 cryptodev.c
--- cryptodev.c 2 Feb 2008 02:39:00 -0000       1.32
+++ cryptodev.c 2 Feb 2008 10:52:03 -0000
@@ -52,16 +52,10 @@
 #include <sys/device.h>
 #include <sys/kauth.h>
 
+#include "opt_ocf.h"
 #include <opencrypto/cryptodev.h>
 #include <opencrypto/xform.h>
 
-  #define splcrypto splnet
-#ifdef CRYPTO_DEBUG
-#define DPRINTF(a) uprintf a
-#else
-#define DPRINTF(a)
-#endif
-
 struct csession {
        TAILQ_ENTRY(csession) next;
        u_int64_t       sid;
@@ -374,7 +368,7 @@
 {
        struct cryptop *crp = NULL;
        struct cryptodesc *crde = NULL, *crda = NULL;
-       int error, s;
+       int error;
 
        if (cop->len > 256*1024-4)
                return (E2BIG);
@@ -480,14 +474,33 @@
                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 cv_wait().
+        * 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);
-       if (error == 0 && (crp->crp_flags & CRYPTO_F_DONE) == 0)
-               error = tsleep(crp, PSOCK, "crydev", 0);
-       splx(s);
-       if (error) {
+       mutex_spin_enter(&crypto_mtx);
+       if (error != 0) {
+               DPRINTF(("cryptodev_op: not waiting, error.\n"));
+               mutex_spin_exit(&crypto_mtx);
                goto bail;
+       } else if (crp->crp_flags & CRYPTO_F_DONE) {
+               int qed;
+               qed = crypto_ret_q_remove(crp);
+               DPRINTF(("cryptodev_op: not waiting, DONE request (%s).\n",
+                       qed ? "on queue" : "not on queue!"));
+       } else {
+               DPRINTF(("cryptodev_op: sleeping on cv %08x for crp %08x\n", \
+                       (uint32_t)&crp->crp_cv, (uint32_t)crp));
+               cv_wait(&crp->crp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
        }
+       mutex_spin_exit(&crypto_mtx);
 
        if (crp->crp_etype != 0) {
                error = crp->crp_etype;
@@ -521,11 +534,23 @@
 {
        struct cryptop *crp = (struct cryptop *) op;
        struct csession *cse = (struct csession *)crp->crp_opaque;
+       int error = 0;
 
+#if 0  /* too loud */
+       DCPRINTF(("cryptodev_cb: crp %08x\n", (uint32_t)crp));
+#endif
        cse->error = crp->crp_etype;
        if (crp->crp_etype == EAGAIN)
-               return crypto_dispatch(crp);
-       wakeup_one(crp);
+               error = crypto_dispatch(crp);
+       mutex_spin_enter(&crypto_mtx);
+       if (error != 0 || (crp->crp_flags & CRYPTO_F_DONE)) {
+#if 0
+               DCPRINTF(("cryptodev_cb: waking cv %08x for crp %08x\n", \
+                       (uint32_t)&crp->crp_cv, (uint32_t)crp));
+#endif
+               cv_signal(&crp->crp_cv);
+       }
+       mutex_spin_exit(&crypto_mtx);
        return (0);
 }
 
@@ -534,7 +559,9 @@
 {
        struct cryptkop *krp = (struct cryptkop *) op;
 
-       wakeup_one(krp);
+       mutex_spin_enter(&crypto_mtx);
+       cv_signal(&krp->krp_cv);
+       mutex_spin_exit(&crypto_mtx);
        return (0);
 }
 
@@ -600,10 +627,11 @@
                return (EINVAL);
        }
 
-       krp = (struct cryptkop *)malloc(sizeof *krp, M_XDATA, M_WAITOK);
+       krp = pool_get(&cryptkop_pool, PR_WAITOK);
        if (!krp)
                return (ENOMEM);
        bzero(krp, sizeof *krp);
+       cv_init(&krp->krp_cv, "crykdev");
        krp->krp_op = kop->crk_op;
        krp->krp_status = kop->crk_status;
        krp->krp_iparams = kop->crk_iparams;
@@ -626,11 +654,15 @@
        }
 
        error = crypto_kdispatch(krp);
-       if (error == 0)
-               error = tsleep(krp, PSOCK, "crydev", 0);
-       if (error)
+       if (error == 0) {
+               mutex_spin_enter(&crypto_mtx);
+               cv_wait(&krp->krp_cv, &crypto_mtx);     /* XXX cv_wait_sig? */
+               mutex_spin_exit(&crypto_mtx);
+       }
+#if 0
+       if (error)                      /* see XXX above -- wait intr? */
                goto fail;
-
+#endif
        if (krp->krp_status != 0) {
                error = krp->krp_status;
                goto fail;
@@ -652,7 +684,7 @@
                        if (krp->krp_param[i].crp_p)
                                FREE(krp->krp_param[i].crp_p, M_XDATA);
                }
-               free(krp, M_XDATA);
+               pool_put(&cryptkop_pool, krp);
        }
        return (error);
 }
Index: cryptodev.h
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptodev.h,v
retrieving revision 1.11
diff -u -r1.11 cryptodev.h
--- cryptodev.h 2 Feb 2008 02:39:00 -0000       1.11
+++ cryptodev.h 2 Feb 2008 10:52:03 -0000
@@ -291,6 +291,7 @@
 
        void *          crp_mac;
        struct timespec crp_tstamp;     /* performance time stamp */
+       kcondvar_t      crp_cv;
 };
 
 #define CRYPTO_BUF_CONTIG      0x0
@@ -315,6 +316,7 @@
        u_int32_t       krp_hid;
        struct crparam  krp_param[CRK_MAXPARAM];        /* kvm */
        int             (*krp_callback)(struct cryptkop *);
+       kcondvar_t      krp_cv;
 };
 
 /* Crypto capabilities structure */
@@ -387,6 +389,8 @@
 int    cuio_apply(struct uio *, int, int,
            int (*f)(void *, void *, unsigned int), void *);
 
+extern int crypto_ret_q_remove(struct cryptop *);
+extern int crypto_ret_kq_remove(struct cryptkop *);
 extern void crypto_freereq(struct cryptop *crp);
 extern struct cryptop *crypto_getreq(int num);
 
@@ -394,6 +398,17 @@
 extern int crypto_userasymcrypto;      /* userland may do asym crypto reqs */
 extern int crypto_devallowsoft;        /* only use hardware crypto */
 
+/*
+ * Asymmetric operations are allocated in cryptodev.c but can be
+ * freed in crypto.c.
+ */
+extern struct pool     cryptkop_pool;
+
+/*
+ * Mutual exclusion and its unwelcome friends.
+ */
+
+extern kmutex_t        crypto_mtx;
 
 /*
  * initialize the crypto framework subsystem (not the pseudo-device).
@@ -418,6 +433,13 @@
 extern void cuio_copyback(struct uio* uio, int off, int len, void *cp);
 extern int     cuio_getptr(struct uio *, int loc, int *off);
 
+#ifdef CRYPTO_DEBUG
+#define DPRINTF(a) uprintf a
+#define DCPRINTF(a) printf a
+#else
+#define DPRINTF(a)
+#define DCPRINTF(a)
+#endif
 
 #endif /* _KERNEL */
 #endif /* _CRYPTO_CRYPTO_H_ */
Index: cryptosoft.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptosoft.c,v
retrieving revision 1.19
diff -u -r1.19 cryptosoft.c
--- cryptosoft.c        2 Feb 2008 04:46:29 -0000       1.19
+++ cryptosoft.c        2 Feb 2008 10:52:03 -0000
@@ -33,6 +33,7 @@
 #include <sys/sysctl.h>
 #include <sys/errno.h>
 
+#include "opt_ocf.h"
 #include <opencrypto/cryptodev.h>
 #include <opencrypto/cryptosoft.h>
 #include <opencrypto/xform.h>
@@ -1008,6 +1009,7 @@
        }
 
 done:
+       DPRINTF(("request %08x done\n", (uint32_t)crp));
        crypto_done(crp);
        return 0;
 }
Index: cryptosoft_xform.c
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/cryptosoft_xform.c,v
retrieving revision 1.7
diff -u -r1.7 cryptosoft_xform.c
--- cryptosoft_xform.c  2 Feb 2008 02:39:00 -0000       1.7
+++ cryptosoft_xform.c  2 Feb 2008 10:52:03 -0000
@@ -326,9 +326,8 @@
        int err;
 
        MALLOC(p, des_key_schedule *, sizeof (des_key_schedule),
-               M_CRYPTO_DATA, M_NOWAIT);
+               M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
        if (p != NULL) {
-               bzero(p, sizeof(des_key_schedule));
                des_set_key((des_cblock *)__UNCONST(key), p[0]);
                err = 0;
        } else
@@ -370,9 +369,8 @@
        int err;
 
        MALLOC(p, des_key_schedule *, 3*sizeof (des_key_schedule),
-               M_CRYPTO_DATA, M_NOWAIT);
+               M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
        if (p != NULL) {
-               bzero(p, 3*sizeof(des_key_schedule));
                des_set_key((des_cblock *)__UNCONST(key +  0), p[0]);
                des_set_key((des_cblock *)__UNCONST(key +  8), p[1]);
                des_set_key((des_cblock *)__UNCONST(key + 16), p[2]);
@@ -395,22 +393,14 @@
 blf_encrypt(void *key, u_int8_t *blk)
 {
 
-#if defined(__NetBSD__)
        BF_ecb_encrypt(blk, blk, (BF_KEY *)key, 1);
-#else
-       blf_ecb_encrypt((blf_ctx *) key, blk, 8);
-#endif
 }
 
 static void
 blf_decrypt(void *key, u_int8_t *blk)
 {
 
-#if defined(__NetBSD__)
        BF_ecb_encrypt(blk, blk, (BF_KEY *)key, 0);
-#else
-       blf_ecb_decrypt((blf_ctx *) key, blk, 8);
-#endif
 }
 
 static int
@@ -418,21 +408,10 @@
 {
        int err;
 
-#if defined(__FreeBSD__) || defined(__NetBSD__)
-#define        BLF_SIZ sizeof(BF_KEY)
-#else
-#define        BLF_SIZ sizeof(blf_ctx)
-#endif
-
-       MALLOC(*sched, u_int8_t *, BLF_SIZ,
-               M_CRYPTO_DATA, M_NOWAIT);
+       MALLOC(*sched, u_int8_t *, sizeof(BF_KEY),
+               M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
        if (*sched != NULL) {
-               bzero(*sched, BLF_SIZ);
-#if defined(__FreeBSD__) || defined(__NetBSD__)
                BF_set_key((BF_KEY *) *sched, len, key);
-#else
-               blf_key((blf_ctx *)*sched, key, len);
-#endif
                err = 0;
        } else
                err = ENOMEM;
@@ -442,7 +421,7 @@
 static void
 blf_zerokey(u_int8_t **sched)
 {
-       bzero(*sched, BLF_SIZ);
+       bzero(*sched, sizeof(BF_KEY));
        FREE(*sched, M_CRYPTO_DATA);
        *sched = NULL;
 }
@@ -465,9 +444,8 @@
        int err;
 
        MALLOC(*sched, u_int8_t *, sizeof(cast128_key), M_CRYPTO_DATA,
-              M_NOWAIT);
+              M_NOWAIT|M_ZERO);
        if (*sched != NULL) {
-               bzero(*sched, sizeof(cast128_key));
                cast128_setkey((cast128_key *)*sched, key, len);
                err = 0;
        } else
@@ -505,7 +483,7 @@
         * Will this break a pdp-10, Cray-1, or GE-645 port?
         */
        MALLOC(*sched, u_int8_t *, 10 * (sizeof(u_int8_t *) + 0x100),
-               M_CRYPTO_DATA, M_NOWAIT);
+               M_CRYPTO_DATA, M_NOWAIT|M_ZERO);
 
        if (*sched != NULL) {
 
@@ -513,8 +491,6 @@
                u_int8_t* table = (u_int8_t*) &key_tables[10];
                int k;
 
-               bzero(*sched, 10 * sizeof(u_int8_t *)+0x100);
-
                for (k = 0; k < 10; k++) {
                        key_tables[k] = table;
                        table += 0x100;
@@ -553,9 +529,8 @@
        int err;
 
        MALLOC(*sched, u_int8_t *, sizeof(rijndael_ctx), M_CRYPTO_DATA,
-           M_NOWAIT);
+           M_NOWAIT|M_ZERO);
        if (*sched != NULL) {
-               bzero(*sched, sizeof(rijndael_ctx));
                rijndael_set_key((rijndael_ctx *) *sched, key, len * 8);
                err = 0;
        } else
Index: files.opencrypto
===================================================================
RCS file: /cvsroot/src/sys/opencrypto/files.opencrypto,v
retrieving revision 1.18
diff -u -r1.18 files.opencrypto
--- files.opencrypto    27 Oct 2006 21:20:48 -0000      1.18
+++ files.opencrypto    2 Feb 2008 10:52:03 -0000
@@ -23,3 +23,5 @@
 # (and thus crypto hardware accelerators).
 defpseudo crypto: opencrypto
 file   opencrypto/cryptodev.c          crypto
+
+defflag opt_ocf.h      CRYPTO_DEBUG CRYPTO_TIMING


Home | Main Index | Thread Index | Old Index