Module Name: src Committed By: knakahara Date: Tue Jun 6 01:45:57 UTC 2017
Modified Files: src/sys/opencrypto: crypto.c cryptodev.h Log Message: restructure locks(1/2): make relation between lock and data explicit. + crypto_drv_mtx protects - whole crypto_drivers + crypto_drivers[i].cc_lock (new) protects - crypto_drivers[i] itself - member of crypto_drivers[i] + crypto_q_mtx protects - crp_q - crp_kq + crypto_ret_q_mtx protects - crp_ret_q - crp_ret_kq - crypto_exit_flag I will add locking note later. To generate a diff of this commit: cvs rdiff -u -r1.80 -r1.81 src/sys/opencrypto/crypto.c cvs rdiff -u -r1.34 -r1.35 src/sys/opencrypto/cryptodev.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Modified files: Index: src/sys/opencrypto/crypto.c diff -u src/sys/opencrypto/crypto.c:1.80 src/sys/opencrypto/crypto.c:1.81 --- src/sys/opencrypto/crypto.c:1.80 Mon Jun 5 09:09:13 2017 +++ src/sys/opencrypto/crypto.c Tue Jun 6 01:45:57 2017 @@ -1,4 +1,4 @@ -/* $NetBSD: crypto.c,v 1.80 2017/06/05 09:09:13 knakahara Exp $ */ +/* $NetBSD: crypto.c,v 1.81 2017/06/06 01:45:57 knakahara Exp $ */ /* $FreeBSD: src/sys/opencrypto/crypto.c,v 1.4.2.5 2003/02/26 00:14:05 sam Exp $ */ /* $OpenBSD: crypto.c,v 1.41 2002/07/17 23:52:38 art Exp $ */ @@ -53,7 +53,7 @@ */ #include <sys/cdefs.h> -__KERNEL_RCSID(0, "$NetBSD: crypto.c,v 1.80 2017/06/05 09:09:13 knakahara Exp $"); +__KERNEL_RCSID(0, "$NetBSD: crypto.c,v 1.81 2017/06/06 01:45:57 knakahara Exp $"); #include <sys/param.h> #include <sys/reboot.h> @@ -374,8 +374,11 @@ static int crypto_destroy(bool); static int crypto_invoke(struct cryptop *crp, int hint); static int crypto_kinvoke(struct cryptkop *krp, int hint); -static struct cryptocap *crypto_checkdriver(u_int32_t); +static struct cryptocap *crypto_checkdriver_lock(u_int32_t); static struct cryptocap *crypto_checkdriver_uninit(u_int32_t); +static void crypto_driver_lock(struct cryptocap *); +static void crypto_driver_unlock(struct cryptocap *); +static void crypto_driver_clear(struct cryptocap *); static struct cryptostats cryptostats; #ifdef CRYPTO_TIMING @@ -438,26 +441,33 @@ crypto_destroy(bool exit_kthread) if (exit_kthread) { struct cryptocap *cap = NULL; - mutex_spin_enter(&crypto_ret_q_mtx); - /* if we have any in-progress requests, don't unload */ + mutex_spin_enter(&crypto_q_mtx); if (!TAILQ_EMPTY(&crp_q) || !TAILQ_EMPTY(&crp_kq)) { - mutex_spin_exit(&crypto_ret_q_mtx); + mutex_spin_exit(&crypto_q_mtx); return EBUSY; } + mutex_spin_exit(&crypto_q_mtx); + /* FIXME: + * prohibit enqueue to crp_q and crp_kq after here. + */ + mutex_enter(&crypto_drv_mtx); for (i = 0; i < crypto_drivers_num; i++) { cap = crypto_checkdriver_uninit(i); if (cap == NULL) continue; - if (cap->cc_sessions != 0) - break; - } - if (cap != NULL) { - mutex_spin_exit(&crypto_ret_q_mtx); - return EBUSY; + if (cap->cc_sessions != 0) { + mutex_exit(&crypto_drv_mtx); + return EBUSY; + } } + mutex_exit(&crypto_drv_mtx); + /* FIXME: + * prohibit touch crypto_drivers[] and each element after here. + */ + mutex_spin_enter(&crypto_ret_q_mtx); /* kick the cryptoret thread and wait for it to exit */ crypto_exit_flag = 1; cv_signal(&cryptoret_cv); @@ -516,20 +526,28 @@ crypto_newsession(u_int64_t *sid, struct if (cap == NULL) continue; + crypto_driver_lock(cap); + /* * If it's not initialized or has remaining sessions * referencing it, skip. */ if (cap->cc_newsession == NULL || - (cap->cc_flags & CRYPTOCAP_F_CLEANUP)) + (cap->cc_flags & CRYPTOCAP_F_CLEANUP)) { + crypto_driver_unlock(cap); continue; + } /* Hardware required -- ignore software drivers. */ - if (hard > 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE)) + if (hard > 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE)) { + crypto_driver_unlock(cap); continue; + } /* Software required -- ignore hardware drivers. */ - if (hard < 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE) == 0) + if (hard < 0 && (cap->cc_flags & CRYPTOCAP_F_SOFTWARE) == 0) { + crypto_driver_unlock(cap); continue; + } /* See if all the algorithms are supported. */ for (cr = cri; cr; cr = cr->cri_next) @@ -560,9 +578,12 @@ crypto_newsession(u_int64_t *sid, struct DPRINTF("crypto_drivers[%d].cc_newsession() failed. error=%d\n", hid, err); } + crypto_driver_unlock(cap); goto done; /*break;*/ } + + crypto_driver_unlock(cap); } done: mutex_exit(&crypto_drv_mtx); @@ -579,14 +600,10 @@ crypto_freesession(u_int64_t sid) struct cryptocap *cap; int err = 0; - mutex_enter(&crypto_drv_mtx); - /* Determine two IDs. */ - cap = crypto_checkdriver(CRYPTO_SESID2HID(sid)); - if (cap == NULL) { - err = ENOENT; - goto done; - } + cap = crypto_checkdriver_lock(CRYPTO_SESID2HID(sid)); + if (cap == NULL) + return ENOENT; if (cap->cc_sessions) (cap->cc_sessions)--; @@ -602,10 +619,9 @@ crypto_freesession(u_int64_t sid) * make the entry available for reuse. */ if ((cap->cc_flags & CRYPTOCAP_F_CLEANUP) && cap->cc_sessions == 0) - memset(cap, 0, sizeof(struct cryptocap)); + crypto_driver_clear(cap); -done: - mutex_exit(&crypto_drv_mtx); + crypto_driver_unlock(cap); return err; } @@ -665,6 +681,7 @@ crypto_get_driverid(u_int32_t flags) /* NB: state is zero'd on free */ cap->cc_sessions = 1; /* Mark */ cap->cc_flags = flags; + mutex_init(&cap->cc_lock, MUTEX_DEFAULT, IPL_NET); if (bootverbose) printf("crypto: assign driver %u, flags %u\n", i, flags); @@ -675,12 +692,18 @@ crypto_get_driverid(u_int32_t flags) } static struct cryptocap * -crypto_checkdriver(u_int32_t hid) +crypto_checkdriver_lock(u_int32_t hid) { + struct cryptocap *cap; KASSERT(crypto_drivers != NULL); - return (hid >= crypto_drivers_num ? NULL : &crypto_drivers[hid]); + if (hid >= crypto_drivers_num) + return NULL; + + cap = &crypto_drivers[hid]; + mutex_enter(&cap->cc_lock); + return cap; } /* @@ -693,12 +716,56 @@ static struct cryptocap * crypto_checkdriver_uninit(u_int32_t hid) { + KASSERT(mutex_owned(&crypto_drv_mtx)); + if (crypto_drivers == NULL) return NULL; return (hid >= crypto_drivers_num ? NULL : &crypto_drivers[hid]); } +static inline void +crypto_driver_lock(struct cryptocap *cap) +{ + + KASSERT(cap != NULL); + + mutex_enter(&cap->cc_lock); +} + +static inline void +crypto_driver_unlock(struct cryptocap *cap) +{ + + KASSERT(cap != NULL); + + mutex_exit(&cap->cc_lock); +} + +static void +crypto_driver_clear(struct cryptocap *cap) +{ + + if (cap == NULL) + return; + + KASSERT(mutex_owned(&cap->cc_lock)); + + cap->cc_sessions = 0; + memset(&cap->cc_max_op_len, 0, sizeof(cap->cc_max_op_len)); + memset(&cap->cc_alg, 0, sizeof(cap->cc_alg)); + memset(&cap->cc_kalg, 0, sizeof(cap->cc_kalg)); + cap->cc_flags = 0; + cap->cc_qblocked = 0; + cap->cc_kqblocked = 0; + + cap->cc_arg = NULL; + cap->cc_newsession = NULL; + cap->cc_process = NULL; + cap->cc_freesession = NULL; + cap->cc_kprocess = NULL; +} + /* * Register support for a key-related algorithm. This routine * is called once for each algorithm supported a driver. @@ -713,7 +780,7 @@ crypto_kregister(u_int32_t driverid, int mutex_enter(&crypto_drv_mtx); - cap = crypto_checkdriver(driverid); + cap = crypto_checkdriver_lock(driverid); if (cap != NULL && (CRK_ALGORITM_MIN <= kalg && kalg <= CRK_ALGORITHM_MAX)) { /* @@ -759,12 +826,12 @@ crypto_register(u_int32_t driverid, int struct cryptocap *cap; int err; - mutex_enter(&crypto_drv_mtx); + cap = crypto_checkdriver_lock(driverid); + if (cap == NULL) + return EINVAL; - cap = crypto_checkdriver(driverid); /* NB: algorithms are in the range [1..max] */ - if (cap != NULL && - (CRYPTO_ALGORITHM_MIN <= alg && alg <= CRYPTO_ALGORITHM_MAX)) { + if (CRYPTO_ALGORITHM_MIN <= alg && alg <= CRYPTO_ALGORITHM_MAX) { /* * XXX Do some performance testing to determine placing. * XXX We probably need an auxiliary data structure that @@ -794,25 +861,25 @@ crypto_register(u_int32_t driverid, int } else err = EINVAL; - mutex_exit(&crypto_drv_mtx); + crypto_driver_unlock(cap); + return err; } static int -crypto_unregister_locked(u_int32_t driverid, int alg, bool all) +crypto_unregister_locked(struct cryptocap *cap, int alg, bool all) { int i; u_int32_t ses; - struct cryptocap *cap; bool lastalg = true; - KASSERT(mutex_owned(&crypto_drv_mtx)); + KASSERT(cap != NULL); + KASSERT(mutex_owned(&cap->cc_lock)); if (alg < CRYPTO_ALGORITHM_MIN || CRYPTO_ALGORITHM_MAX < alg) return EINVAL; - cap = crypto_checkdriver(driverid); - if (cap == NULL || (!all && cap->cc_alg[alg] == 0)) + if (!all && cap->cc_alg[alg] == 0) return EINVAL; cap->cc_alg[alg] = 0; @@ -831,7 +898,7 @@ crypto_unregister_locked(u_int32_t drive } if (lastalg) { ses = cap->cc_sessions; - memset(cap, 0, sizeof(struct cryptocap)); + crypto_driver_clear(cap); if (ses != 0) { /* * If there are pending sessions, just mark as invalid. @@ -854,10 +921,11 @@ int crypto_unregister(u_int32_t driverid, int alg) { int err; + struct cryptocap *cap; - mutex_enter(&crypto_drv_mtx); - err = crypto_unregister_locked(driverid, alg, false); - mutex_exit(&crypto_drv_mtx); + cap = crypto_checkdriver_lock(driverid); + err = crypto_unregister_locked(cap, alg, false); + crypto_driver_unlock(cap); return err; } @@ -873,14 +941,15 @@ int crypto_unregister_all(u_int32_t driverid) { int err, i; + struct cryptocap *cap; - mutex_enter(&crypto_drv_mtx); + cap = crypto_checkdriver_lock(driverid); for (i = CRYPTO_ALGORITHM_MIN; i <= CRYPTO_ALGORITHM_MAX; i++) { - err = crypto_unregister_locked(driverid, i, true); + err = crypto_unregister_locked(cap, i, true); if (err) break; } - mutex_exit(&crypto_drv_mtx); + crypto_driver_unlock(cap); return err; } @@ -895,12 +964,9 @@ crypto_unblock(u_int32_t driverid, int w struct cryptocap *cap; int needwakeup = 0; - mutex_spin_enter(&crypto_q_mtx); - cap = crypto_checkdriver(driverid); - if (cap == NULL) { - mutex_spin_exit(&crypto_q_mtx); + cap = crypto_checkdriver_lock(driverid); + if (cap == NULL) return EINVAL; - } if (what & CRYPTO_SYMQ) { needwakeup |= cap->cc_qblocked; @@ -910,7 +976,7 @@ crypto_unblock(u_int32_t driverid, int w needwakeup |= cap->cc_kqblocked; cap->cc_kqblocked = 0; } - mutex_spin_exit(&crypto_q_mtx); + crypto_driver_unlock(cap); if (needwakeup) setsoftcrypto(softintr_cookie); @@ -956,9 +1022,7 @@ crypto_dispatch(struct cryptop *crp) return 0; } - mutex_spin_enter(&crypto_q_mtx); - - cap = crypto_checkdriver(CRYPTO_SESID2HID(crp->crp_sid)); + cap = crypto_checkdriver_lock(CRYPTO_SESID2HID(crp->crp_sid)); /* * TODO: * If we can ensure the driver has been valid until the driver is @@ -969,22 +1033,20 @@ crypto_dispatch(struct cryptop *crp) * The driver must be detached, so this request will migrate * to other drivers in cryptointr() later. */ + mutex_spin_enter(&crypto_q_mtx); TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); mutex_spin_exit(&crypto_q_mtx); return 0; } - /* - * TODO: - * cap->cc_qblocked should be protected by a spin lock other than - * crypto_q_mtx. - */ if (cap->cc_qblocked != 0) { + crypto_driver_unlock(cap); /* * The driver is blocked, just queue the op until * it unblocks and the swi thread gets kicked. */ + mutex_spin_enter(&crypto_q_mtx); TAILQ_INSERT_TAIL(&crp_q, crp, crp_next); mutex_spin_exit(&crypto_q_mtx); @@ -996,7 +1058,7 @@ crypto_dispatch(struct cryptop *crp) * immediately; dispatch it directly to the * driver unless the driver is currently blocked. */ - mutex_spin_exit(&crypto_q_mtx); + crypto_driver_unlock(cap); result = crypto_invoke(crp, 0); if (result == ERESTART) { /* @@ -1004,8 +1066,10 @@ crypto_dispatch(struct cryptop *crp) * driver ``blocked'' for cryptop's and put * the op on the queue. */ - mutex_spin_enter(&crypto_q_mtx); + crypto_driver_lock(cap); cap->cc_qblocked = 1; + crypto_driver_unlock(cap); + mutex_spin_enter(&crypto_q_mtx); TAILQ_INSERT_HEAD(&crp_q, crp, crp_next); cryptostats.cs_blocks++; mutex_spin_exit(&crypto_q_mtx); @@ -1033,16 +1097,16 @@ crypto_kdispatch(struct cryptkop *krp) KASSERT(krp != NULL); - mutex_spin_enter(&crypto_q_mtx); cryptostats.cs_kops++; - cap = crypto_checkdriver(krp->krp_hid); + cap = crypto_checkdriver_lock(krp->krp_hid); /* * TODO: * If we can ensure the driver has been valid until the driver is * done crypto_unregister(), this migrate operation is not required. */ if (cap == NULL) { + mutex_spin_enter(&crypto_q_mtx); TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); mutex_spin_exit(&crypto_q_mtx); @@ -1050,17 +1114,19 @@ crypto_kdispatch(struct cryptkop *krp) } if (cap->cc_kqblocked != 0) { + crypto_driver_unlock(cap); /* * The driver is blocked, just queue the op until * it unblocks and the swi thread gets kicked. */ + mutex_spin_enter(&crypto_q_mtx); TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); mutex_spin_exit(&crypto_q_mtx); return 0; } - mutex_spin_exit(&crypto_q_mtx); + crypto_driver_unlock(cap); result = crypto_kinvoke(krp, 0); if (result == ERESTART) { /* @@ -1068,8 +1134,10 @@ crypto_kdispatch(struct cryptkop *krp) * driver ``blocked'' for cryptop's and put * the op on the queue. */ - mutex_spin_enter(&crypto_q_mtx); + crypto_driver_lock(cap); cap->cc_kqblocked = 1; + crypto_driver_unlock(cap); + mutex_spin_enter(&crypto_q_mtx); TAILQ_INSERT_HEAD(&crp_kq, krp, krp_next); cryptostats.cs_kblocks++; mutex_spin_exit(&crypto_q_mtx); @@ -1109,27 +1177,34 @@ crypto_kinvoke(struct cryptkop *krp, int cap = crypto_checkdriver_uninit(hid); if (cap == NULL) continue; + crypto_driver_lock(cap); if ((cap->cc_flags & CRYPTOCAP_F_SOFTWARE) && - crypto_devallowsoft == 0) + crypto_devallowsoft == 0) { + crypto_driver_unlock(cap); continue; - if (cap->cc_kprocess == NULL) + } + if (cap->cc_kprocess == NULL) { + crypto_driver_unlock(cap); continue; + } if ((cap->cc_kalg[krp->krp_op] & - CRYPTO_ALG_FLAG_SUPPORTED) == 0) + CRYPTO_ALG_FLAG_SUPPORTED) == 0) { + crypto_driver_unlock(cap); continue; + } break; } + mutex_exit(&crypto_drv_mtx); if (cap != NULL) { int (*process)(void *, struct cryptkop *, int); void *arg; process = cap->cc_kprocess; arg = cap->cc_karg; - mutex_exit(&crypto_drv_mtx); krp->krp_hid = hid; + crypto_driver_unlock(cap); error = (*process)(arg, krp, hint); } else { - mutex_exit(&crypto_drv_mtx); error = ENODEV; } @@ -1188,7 +1263,7 @@ crypto_invoke(struct cryptop *crp, int h return 0; } - cap = crypto_checkdriver(CRYPTO_SESID2HID(crp->crp_sid)); + cap = crypto_checkdriver_lock(CRYPTO_SESID2HID(crp->crp_sid)); if (cap != NULL && (cap->cc_flags & CRYPTOCAP_F_CLEANUP) == 0) { int (*process)(void *, struct cryptop *, int); void *arg; @@ -1200,11 +1275,15 @@ crypto_invoke(struct cryptop *crp, int h * Invoke the driver to process the request. */ DPRINTF("calling process for %p\n", crp); + crypto_driver_unlock(cap); return (*process)(arg, crp, hint); } else { struct cryptodesc *crd; u_int64_t nid = 0; + if (cap != NULL) + crypto_driver_unlock(cap); + /* * Driver has unregistered; migrate the session and return * an error to the caller so they'll resubmit the op. @@ -1472,14 +1551,19 @@ crypto_getfeat(int *featp) if ((cap->cc_flags & CRYPTOCAP_F_SOFTWARE) && crypto_devallowsoft == 0) { + crypto_driver_unlock(cap); continue; } - if (cap->cc_kprocess == NULL) + if (cap->cc_kprocess == NULL) { + crypto_driver_unlock(cap); continue; + } for (kalg = 0; kalg < CRK_ALGORITHM_MAX; kalg++) if ((cap->cc_kalg[kalg] & CRYPTO_ALG_FLAG_SUPPORTED) != 0) feat |= 1 << kalg; + + crypto_driver_unlock(cap); } mutex_exit(&crypto_drv_mtx); @@ -1510,8 +1594,10 @@ cryptointr(void) hint = 0; TAILQ_FOREACH_SAFE(crp, &crp_q, crp_next, cnext) { u_int32_t hid = CRYPTO_SESID2HID(crp->crp_sid); - cap = crypto_checkdriver(hid); + cap = crypto_checkdriver_lock(hid); if (cap == NULL || cap->cc_process == NULL) { + if (cap != NULL) + crypto_driver_unlock(cap); /* Op needs to be migrated, process it. */ submit = crp; break; @@ -1520,8 +1606,11 @@ cryptointr(void) /* * skip blocked crp regardless of CRYPTO_F_BATCH */ - if (cap->cc_qblocked != 0) + if (cap->cc_qblocked != 0) { + crypto_driver_unlock(cap); continue; + } + crypto_driver_unlock(cap); /* * skip batch crp until the end of crp_q @@ -1566,12 +1655,13 @@ cryptointr(void) * it at the end does not work. */ /* validate sid again */ - cap = crypto_checkdriver(CRYPTO_SESID2HID(submit->crp_sid)); + cap = crypto_checkdriver_lock(CRYPTO_SESID2HID(submit->crp_sid)); if (cap == NULL) { /* migrate again, sigh... */ TAILQ_INSERT_TAIL(&crp_q, submit, crp_next); } else { cap->cc_qblocked = 1; + crypto_driver_unlock(cap); TAILQ_INSERT_HEAD(&crp_q, submit, crp_next); cryptostats.cs_blocks++; } @@ -1580,13 +1670,18 @@ cryptointr(void) /* As above, but for key ops */ TAILQ_FOREACH_SAFE(krp, &crp_kq, krp_next, knext) { - cap = crypto_checkdriver(krp->krp_hid); + cap = crypto_checkdriver_lock(krp->krp_hid); if (cap == NULL || cap->cc_kprocess == NULL) { + if (cap != NULL) + crypto_driver_unlock(cap); /* Op needs to be migrated, process it. */ break; } - if (!cap->cc_kqblocked) + if (!cap->cc_kqblocked) { + crypto_driver_unlock(cap); break; + } + crypto_driver_unlock(cap); } if (krp != NULL) { TAILQ_REMOVE(&crp_kq, krp, krp_next); @@ -1605,12 +1700,13 @@ cryptointr(void) * it at the end does not work. */ /* validate sid again */ - cap = crypto_checkdriver(krp->krp_hid); + cap = crypto_checkdriver_lock(krp->krp_hid); if (cap == NULL) { /* migrate again, sigh... */ TAILQ_INSERT_TAIL(&crp_kq, krp, krp_next); } else { cap->cc_kqblocked = 1; + crypto_driver_unlock(cap); TAILQ_INSERT_HEAD(&crp_kq, krp, krp_next); cryptostats.cs_kblocks++; } Index: src/sys/opencrypto/cryptodev.h diff -u src/sys/opencrypto/cryptodev.h:1.34 src/sys/opencrypto/cryptodev.h:1.35 --- src/sys/opencrypto/cryptodev.h:1.34 Thu May 25 05:24:57 2017 +++ src/sys/opencrypto/cryptodev.h Tue Jun 6 01:45:57 2017 @@ -1,4 +1,4 @@ -/* $NetBSD: cryptodev.h,v 1.34 2017/05/25 05:24:57 knakahara Exp $ */ +/* $NetBSD: cryptodev.h,v 1.35 2017/06/06 01:45:57 knakahara Exp $ */ /* $FreeBSD: src/sys/opencrypto/cryptodev.h,v 1.2.2.6 2003/07/02 17:04:50 sam Exp $ */ /* $OpenBSD: cryptodev.h,v 1.33 2002/07/17 23:52:39 art Exp $ */ @@ -560,6 +560,8 @@ struct cryptocap { int (*cc_freesession) (void*, u_int64_t); void *cc_karg; /* callback argument */ int (*cc_kprocess) (void*, struct cryptkop *, int); + + kmutex_t cc_lock; }; /*