kern/130286: [patch] hifn(4) changes
Patrick Lamaiziere
patfbsd at davenulle.org
Wed Jan 7 21:20:02 PST 2009
>Number: 130286
>Category: kern
>Synopsis: [patch] hifn(4) changes
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: change-request
>Submitter-Id: current-users
>Arrival-Date: Thu Jan 08 05:20:01 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator: Patrick Lamaiziere
>Release: 8.0-CURRENT/i386
>Organization:
>Environment:
FreeBSD malpractice.lamaiziere.net 8.0-CURRENT FreeBSD 8.0-CURRENT #5: Mon Dec 29 21:20:22 CET 2008 root@:/usr/obj/usr/src/sys/NET5501 i386
>Description:
I would like to suggest few small changes in the hifn(4) driver:
Please see the attached patch.
Description.
The main change is the use of a rwlock (sc->sc_sessions_lock) to protect the sessions.
We don't need to use the main driver lock for this. While i'm here, fix small others problems.
hifn_attach()
- Useless bzero of softc
hifn_detach()
- Return EBUSY if there is active session
- Free the sessions
- Use callout_drain() instead callout_stop()
hifn_rng()
- Remove the unused macro RANDOM_BITS
hifn_newsession()
- Add and use a rwlock to lock the sessions (sc->sc_sessions_lock).
- Remove useless bzero of the sessions (use malloc with M_ZERO)
- Use arc4rand() instead read_random()
hifn_freesession()
- Use a rwlock to lock the sessions
hifn_process()
- Use a rwlock to lock the sessions. In the current driver there is
no lock to protect the sessions in hifn_process() but the sessions
can be reallocated in hifn_newsession().
I think this is the cause of PR kern/91407:
http://www.FreeBSD.org/cgi/query-pr.cgi?pr=kern/91407
Anyway we should protect the sessions in hifn_process().
hifn_callback()
- Use a rwlock to lock the sessions.
I've tested on a Soekris vpn 1411 (hifn 7955) with ipsec and openssl.
Regards.
(with big thanks to Mike Tancsa)
>How-To-Repeat:
>Fix:
Patch attached with submission follows:
--- /home/patrick/src/sys/dev/hifn/hifn7751.c 2007-03-21 04:42:49.000000000 +0100
+++ /usr/src/sys/dev/hifn/hifn7751.c 2009-01-01 02:46:18.000000000 +0100
@@ -58,6 +58,7 @@ __FBSDID("$FreeBSD: src/sys/dev/hifn/hif
#include <sys/mbuf.h>
#include <sys/lock.h>
#include <sys/mutex.h>
+#include <sys/rwlock.h>
#include <sys/sysctl.h>
#include <vm/vm.h>
@@ -362,9 +363,9 @@ hifn_attach(device_t dev)
u_int16_t ena, rev;
KASSERT(sc != NULL, ("hifn_attach: null software carrier!"));
- bzero(sc, sizeof (*sc));
sc->sc_dev = dev;
+ rw_init(&sc->sc_sessions_lock, device_get_nameunit(dev));
mtx_init(&sc->sc_mtx, device_get_nameunit(dev), "hifn driver", MTX_DEF);
/* XXX handle power management */
@@ -637,6 +638,7 @@ fail_io1:
fail_io0:
bus_release_resource(dev, SYS_RES_MEMORY, HIFN_BAR0, sc->sc_bar0res);
fail_pci:
+ rw_destroy(&sc->sc_sessions_lock);
mtx_destroy(&sc->sc_mtx);
return (ENXIO);
}
@@ -648,15 +650,32 @@ static int
hifn_detach(device_t dev)
{
struct hifn_softc *sc = device_get_softc(dev);
+ struct hifn_session *ses;
+ int i;
KASSERT(sc != NULL, ("hifn_detach: null software carrier!"));
+ rw_wlock(&sc->sc_sessions_lock);
+ for (i = 0; i < sc->sc_nsessions; i++) {
+ ses = &sc->sc_sessions[i];
+ if (ses->hs_used) {
+ rw_wunlock(&sc->sc_sessions_lock);
+ device_printf(dev,
+ "cannot detach, sessions still active.\n");
+ return (EBUSY);
+ }
+ }
+ if (sc->sc_sessions != NULL)
+ free(sc->sc_sessions, M_DEVBUF);
+ rw_wunlock(&sc->sc_sessions_lock);
+ crypto_unregister_all(sc->sc_cid);
+
/* disable interrupts */
WRITE_REG_1(sc, HIFN_1_DMA_IER, 0);
/*XXX other resources */
- callout_stop(&sc->sc_tickto);
- callout_stop(&sc->sc_rngto);
+ callout_drain(&sc->sc_tickto);
+ callout_drain(&sc->sc_rngto);
#ifdef HIFN_RNDTEST
if (sc->sc_rndtest)
rndtest_detach(sc->sc_rndtest);
@@ -666,8 +685,6 @@ hifn_detach(device_t dev)
WRITE_REG_1(sc, HIFN_1_DMA_CNFG, HIFN_DMACNFG_MSTRESET |
HIFN_DMACNFG_DMARESET | HIFN_DMACNFG_MODE);
- crypto_unregister_all(sc->sc_cid);
-
bus_generic_detach(dev); /*XXX should be no children, right? */
bus_teardown_intr(dev, sc->sc_irq, sc->sc_intrhand);
@@ -682,6 +699,7 @@ hifn_detach(device_t dev)
bus_release_resource(dev, SYS_RES_MEMORY, HIFN_BAR1, sc->sc_bar1res);
bus_release_resource(dev, SYS_RES_MEMORY, HIFN_BAR0, sc->sc_bar0res);
+ rw_destroy(&sc->sc_sessions_lock);
mtx_destroy(&sc->sc_mtx);
return (0);
@@ -817,7 +835,6 @@ hifn_init_pubrng(struct hifn_softc *sc)
static void
hifn_rng(void *vsc)
{
-#define RANDOM_BITS(n) (n)*sizeof (u_int32_t), (n)*sizeof (u_int32_t)*NBBY, 0
struct hifn_softc *sc = vsc;
u_int32_t sts, num[2];
int i;
@@ -2354,12 +2371,12 @@ hifn_newsession(device_t dev, u_int32_t
if (sidp == NULL || cri == NULL || sc == NULL)
return (EINVAL);
- HIFN_LOCK(sc);
+ rw_wlock(&sc->sc_sessions_lock);
if (sc->sc_sessions == NULL) {
ses = sc->sc_sessions = (struct hifn_session *)malloc(
- sizeof(*ses), M_DEVBUF, M_NOWAIT);
+ sizeof(*ses), M_DEVBUF, M_NOWAIT | M_ZERO);
if (ses == NULL) {
- HIFN_UNLOCK(sc);
+ rw_wunlock(&sc->sc_sessions_lock);
return (ENOMEM);
}
sesn = 0;
@@ -2375,9 +2392,9 @@ hifn_newsession(device_t dev, u_int32_t
if (ses == NULL) {
sesn = sc->sc_nsessions;
ses = (struct hifn_session *)malloc((sesn + 1) *
- sizeof(*ses), M_DEVBUF, M_NOWAIT);
+ sizeof(*ses), M_DEVBUF, M_NOWAIT | M_ZERO);
if (ses == NULL) {
- HIFN_UNLOCK(sc);
+ rw_wunlock(&sc->sc_sessions_lock);
return (ENOMEM);
}
bcopy(sc->sc_sessions, ses, sesn * sizeof(*ses));
@@ -2388,9 +2405,7 @@ hifn_newsession(device_t dev, u_int32_t
sc->sc_nsessions++;
}
}
- HIFN_UNLOCK(sc);
- bzero(ses, sizeof(*ses));
ses->hs_used = 1;
for (c = cri; c != NULL; c = c->cri_next) {
@@ -2399,8 +2414,10 @@ hifn_newsession(device_t dev, u_int32_t
case CRYPTO_SHA1:
case CRYPTO_MD5_HMAC:
case CRYPTO_SHA1_HMAC:
- if (mac)
+ if (mac) {
+ rw_wunlock(&sc->sc_sessions_lock);
return (EINVAL);
+ }
mac = 1;
ses->hs_mlen = c->cri_mlen;
if (ses->hs_mlen == 0) {
@@ -2419,20 +2436,23 @@ hifn_newsession(device_t dev, u_int32_t
case CRYPTO_DES_CBC:
case CRYPTO_3DES_CBC:
case CRYPTO_AES_CBC:
- /* XXX this may read fewer, does it matter? */
- read_random(ses->hs_iv,
- c->cri_alg == CRYPTO_AES_CBC ?
- HIFN_AES_IV_LENGTH : HIFN_IV_LENGTH);
+ arc4rand(ses->hs_iv,
+ c->cri_alg == CRYPTO_AES_CBC ?
+ HIFN_AES_IV_LENGTH : HIFN_IV_LENGTH, 0);
/*FALLTHROUGH*/
case CRYPTO_ARC4:
- if (cry)
+ if (cry) {
+ rw_wunlock(&sc->sc_sessions_lock);
return (EINVAL);
+ }
cry = 1;
break;
default:
+ rw_wunlock(&sc->sc_sessions_lock);
return (EINVAL);
}
}
+ rw_wunlock(&sc->sc_sessions_lock);
if (mac == 0 && cry == 0)
return (EINVAL);
@@ -2457,14 +2477,14 @@ hifn_freesession(device_t dev, u_int64_t
if (sc == NULL)
return (EINVAL);
- HIFN_LOCK(sc);
session = HIFN_SESSION(sid);
+ rw_wlock(&sc->sc_sessions_lock);
if (session < sc->sc_nsessions) {
bzero(&sc->sc_sessions[session], sizeof(struct hifn_session));
error = 0;
} else
error = EINVAL;
- HIFN_UNLOCK(sc);
+ rw_wunlock(&sc->sc_sessions_lock);
return (error);
}
@@ -2483,11 +2503,17 @@ hifn_process(device_t dev, struct crypto
}
session = HIFN_SESSION(crp->crp_sid);
- if (sc == NULL || session >= sc->sc_nsessions) {
+ if (sc == NULL) {
err = EINVAL;
goto errout;
}
-
+ rw_rlock(&sc->sc_sessions_lock);
+ if (session >= sc->sc_nsessions) {
+ rw_runlock(&sc->sc_sessions_lock);
+ err = EINVAL;
+ goto errout;
+ }
+ rw_runlock(&sc->sc_sessions_lock);
cmd = malloc(sizeof(struct hifn_command), M_DEVBUF, M_NOWAIT | M_ZERO);
if (cmd == NULL) {
hifnstats.hst_nomem++;
@@ -2597,10 +2623,12 @@ hifn_process(device_t dev, struct crypto
if (enccrd->crd_flags & CRD_F_ENCRYPT) {
if (enccrd->crd_flags & CRD_F_IV_EXPLICIT)
bcopy(enccrd->crd_iv, cmd->iv, ivlen);
- else
+ else {
+ rw_rlock(&sc->sc_sessions_lock);
bcopy(sc->sc_sessions[session].hs_iv,
cmd->iv, ivlen);
-
+ rw_runlock(&sc->sc_sessions_lock);
+ }
if ((enccrd->crd_flags & CRD_F_IV_PRESENT)
== 0) {
crypto_copyback(crp->crp_flags,
@@ -2856,9 +2884,11 @@ hifn_callback(struct hifn_softc *sc, str
continue;
ivlen = ((crd->crd_alg == CRYPTO_AES_CBC) ?
HIFN_AES_IV_LENGTH : HIFN_IV_LENGTH);
+ rw_rlock(&sc->sc_sessions_lock);
crypto_copydata(crp->crp_flags, crp->crp_buf,
crd->crd_skip + crd->crd_len - ivlen, ivlen,
cmd->softc->sc_sessions[cmd->session_num].hs_iv);
+ rw_runlock(&sc->sc_sessions_lock);
break;
}
}
@@ -2873,7 +2903,9 @@ hifn_callback(struct hifn_softc *sc, str
crd->crd_alg != CRYPTO_SHA1_HMAC) {
continue;
}
+ rw_rlock(&sc->sc_sessions_lock);
len = cmd->softc->sc_sessions[cmd->session_num].hs_mlen;
+ rw_runlock(&sc->sc_sessions_lock);
crypto_copyback(crp->crp_flags, crp->crp_buf,
crd->crd_inject, len, macbuf);
break;
--- /home/patrick/src/sys/dev/hifn/hifn7751var.h 2007-03-21 04:42:49.000000000 +0100
+++ /usr/src/sys/dev/hifn/hifn7751var.h 2008-12-29 18:04:24.000000000 +0100
@@ -161,6 +161,7 @@ struct hifn_softc {
int sc_maxses;
int sc_nsessions;
struct hifn_session *sc_sessions;
+ struct rwlock sc_sessions_lock;/* sessions lock */
int sc_ramsize;
int sc_flags;
#define HIFN_HAS_RNG 0x1 /* includes random number generator */
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list