From owner-freebsd-current@FreeBSD.ORG Mon Mar 10 22:02:49 2008 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7A5561065673; Mon, 10 Mar 2008 22:02:49 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from speedfactory.net (mail.speedfactory.net [66.23.216.219]) by mx1.freebsd.org (Postfix) with ESMTP id 71C008FC1A; Mon, 10 Mar 2008 22:02:48 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from server.baldwin.cx (unverified [66.23.211.162]) by speedfactory.net (SurgeMail 3.8s) with ESMTP id 234995148-1834499 for multiple; Mon, 10 Mar 2008 18:04:18 -0400 Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.14.2/8.14.2) with ESMTP id m2AM2T6M090038; Mon, 10 Mar 2008 18:02:38 -0400 (EDT) (envelope-from jhb@FreeBSD.org) From: John Baldwin To: freebsd-current@FreeBSD.org Date: Mon, 10 Mar 2008 17:27:19 -0400 User-Agent: KMail/1.9.7 References: <47CCB187.8070808@FreeBSD.org> <723D012C-7907-4CFC-B134-C5E5A0B486D9@mac.com> <47CCDA8A.60004@errno.com> In-Reply-To: <47CCDA8A.60004@errno.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200803101727.20378.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Mon, 10 Mar 2008 18:02:39 -0400 (EDT) X-Virus-Scanned: ClamAV 0.91.2/6192/Mon Mar 10 10:54:00 2008 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: Marcel Moolenaar , re@FreeBSD.org, "George V. Neville-Neil" Subject: Re: IPSEC/crypto is broken in FreeBSD/powerpc 7.0-RELEASE! X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Mar 2008 22:02:49 -0000 On Tuesday 04 March 2008 12:13:46 am Sam Leffler wrote: > Marcel Moolenaar wrote: > > > > On Mar 3, 2008, at 6:18 PM, Maxim Sobolev wrote: > > > >> Hi, > >> > >> It appears to be that "options IPSEC" along with "device crypto" > >> breaks FreeBSD/powerpc kernel badly. When enabling these options, > >> apparently kernel doesn't perform any initialization tasks (I don't > >> see usual probe/init sequence output) but jumps straight into root fs > >> mounting after initing crypto(4) and ipsec(4), which is not usable > >> since no devices has been attached. Keyboard is not working either. > > > > The problem is with device crypto. It attaches to nexus(4) and > > expects to be the only child. As you can see from the log, all > > children of nexus suddenly become instantiations of cryptosoft(4) > > rather then the usual drivers that attach. > > > > The swcr_probe() function should check that the device it gets > > is really the one created for it. > > > > Don't know about "expects to be the only child" but I did was jhb said > was right. If you know otherwise please fix it. My mistake was assuming that all platforms treated nexus(4) the same in that each child of nexus has a fixed devclass when it is created. BTW, it looks like the common crypto code doesn't really need a full on device_t, it really just wants a 'kobj' that uses the class and a name of of some sort. Other places in the kernel use a plain old switch table (e.g. struct ifnet and struct cdev with cdevsw) for such things since kobj has some additional overhead. Is there any reason the crypto stuff can't use a separate object to present a crypto device that is separate from the device_t? The least code churn might be to stay with kobj. Hmm, you could actually reuse the device_t as the kobj for hardware devices. All the MI crypto code needs is a name, so you would split out the name instead of using device_get_nameunit(). Something like this would be a start. (The hardware drivers would simply pass device_get_nameunit(dev) as the second arg to crypto_get_driverid()). The one routine needs to be changed to return the crypto device name (cc_name) in a caller-supplied buffer rather than the device_t, but other than those changes (and updating function prototypes in headers) this should be most of what you would need. It would of course be cleaner to not reuse the device_t and have cryptocaps be a bit stronger of an object (maybe it becomes a kobj and crypto_get_driverid() takes a class pointer rather than a kobj). Index: opencrypto/crypto.c =================================================================== RCS file: /usr/cvs/src/sys/opencrypto/crypto.c,v retrieving revision 1.28 diff -u -r1.28 crypto.c --- opencrypto/crypto.c 20 Oct 2007 23:23:22 -0000 1.28 +++ opencrypto/crypto.c 10 Mar 2008 21:05:03 -0000 @@ -99,7 +99,8 @@ * Not tagged fields are read-only. */ struct cryptocap { - device_t cc_dev; /* (d) device/driver */ + struct kobj *cc_dev; /* (d) device/driver */ + const char *cc_name; /* (d) device name */ u_int32_t cc_sessions; /* (d) # of sessions */ u_int32_t cc_koperations; /* (d) # os asym operations */ /* @@ -476,7 +477,7 @@ * support for the algorithms they handle. */ int32_t -crypto_get_driverid(device_t dev, int flags) +crypto_get_driverid(struct kobj *dev, const char *name, int flags) { struct cryptocap *newdrv; int i; @@ -538,7 +539,7 @@ /* * Lookup a driver by name. We match against the full device * name and unit, and against just the name. The latter gives - * us a simple widlcarding by device name. On success return the + * us a simple wildcarding by device name. On success return the * driver/hardware identifier; otherwise return -1. */ int @@ -548,12 +549,17 @@ CRYPTO_DRIVER_LOCK(); for (i = 0; i < crypto_drivers_num; i++) { - device_t dev = crypto_drivers[i].cc_dev; - if (dev == NULL || + if (crypto_drivers[i].cc_dev == NULL || (crypto_drivers[i].cc_flags & CRYPTOCAP_F_CLEANUP)) continue; - if (strncmp(match, device_get_nameunit(dev), len) == 0 || - strncmp(match, device_get_name(dev), len) == 0) + /* + * Previously this did two checks. However, the second + * check was never relevant. If the user asked for just + * "foo" then 'len' will be 3 and the 'strncmp()' will + * match "foo0" just fine without requiring a separate + * match against "foo". + */ + if (strncmp(match, cc->cc_name, len) == 0) break; } CRYPTO_DRIVER_UNLOCK(); @@ -563,6 +569,8 @@ /* * Return the device_t for the specified driver or NULL * if the driver identifier is invalid. + * + * XXX: This needs to change to copy the name into a caller-supplied buffer. */ device_t crypto_find_device_byhid(int hid) @@ -605,7 +613,7 @@ cap->cc_kalg[kalg] = flags | CRYPTO_ALG_FLAG_SUPPORTED; if (bootverbose) printf("crypto: %s registers key alg %u flags %u\n" - , device_get_nameunit(cap->cc_dev) + , cap->cc_name , kalg , flags ); @@ -644,7 +652,7 @@ cap->cc_max_op_len[alg] = maxoplen; if (bootverbose) printf("crypto: %s registers alg %u flags %u maxoplen %u\n" - , device_get_nameunit(cap->cc_dev) + , cap->cc_name , alg , flags , maxoplen @@ -1454,7 +1462,7 @@ if (cap->cc_dev == NULL) continue; db_printf("%-12s %4u %4u %08x %2u %2u\n" - , device_get_nameunit(cap->cc_dev) + , cap->cc_name , cap->cc_sessions , cap->cc_koperations , cap->cc_flags Index: opencrypto/cryptosoft.c =================================================================== RCS file: /usr/cvs/src/sys/opencrypto/cryptosoft.c,v retrieving revision 1.19 diff -u -r1.19 cryptosoft.c --- opencrypto/cryptosoft.c 9 May 2007 19:37:02 -0000 1.19 +++ opencrypto/cryptosoft.c 10 Mar 2008 21:22:08 -0000 @@ -61,7 +61,7 @@ static int swcr_encdec(struct cryptodesc *, struct swcr_data *, caddr_t, int); static int swcr_authcompute(struct cryptodesc *, struct swcr_data *, caddr_t, int); static int swcr_compdec(struct cryptodesc *, struct swcr_data *, caddr_t, int); -static int swcr_freesession(device_t dev, u_int64_t tid); +static int swcr_freesession(struct kobj *kobj, u_int64_t tid); /* * Apply a symmetric encryption/decryption algorithm. @@ -584,7 +584,7 @@ * Generate a new software session. */ static int -swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri) +swcr_newsession(struct kobj *kobj, u_int32_t *sid, struct cryptoini *cri) { struct swcr_data **swd; struct auth_hash *axf; @@ -793,7 +793,7 @@ * Free a session. */ static int -swcr_freesession(device_t dev, u_int64_t tid) +swcr_freesession(struct kobj *kobj, u_int64_t tid) { struct swcr_data *swd; struct enc_xform *txf; @@ -882,7 +882,7 @@ * Process a software request. */ static int -swcr_process(device_t dev, struct cryptop *crp, int hint) +swcr_process(struct kobj *kobj, struct cryptop *crp, int hint) { struct cryptodesc *crd; struct swcr_data *sw; @@ -976,33 +976,33 @@ return 0; } -static void -swcr_identify(device_t *dev, device_t parent) -{ - /* NB: order 10 is so we get attached after h/w devices */ - if (device_find_child(parent, "cryptosoft", -1) == NULL && - BUS_ADD_CHILD(parent, 10, "cryptosoft", -1) == 0) - panic("cryptosoft: could not attach"); -} +static struct kobj_method swcr_methods[] = { + KOBJMETHOD(cryptodev_newsession, swcr_newsession), + KOBJMETHOD(cryptodev_freesession, swcr_freesession), + KOBJMETHOD(cryptodev_process, swcr_process), -static int -swcr_probe(device_t dev) -{ - device_set_desc(dev, "software crypto"); - return (0); -} + {0, 0}, +}; -static int -swcr_attach(device_t dev) +DEFINE_CLASS(cryptosoft, swcr_methods, 0); + +static struct kobj *swcr_kobj; + +static void +swcr_init(void) { + + kobj_class_compile(&cryptosoft_class); + kobj_init(&swcr_kobj, &cryptosoft_class); + swcr_kobj = kobj_create(&cryptosoft_class, M_CRYPTO_DATA, M_WAITOK); memset(hmac_ipad_buffer, HMAC_IPAD_VAL, HMAC_MAX_BLOCK_LEN); memset(hmac_opad_buffer, HMAC_OPAD_VAL, HMAC_MAX_BLOCK_LEN); - swcr_id = crypto_get_driverid(dev, + swcr_id = crypto_get_driverid(swcr_kobj, "cryptosoft0", CRYPTOCAP_F_SOFTWARE | CRYPTOCAP_F_SYNC); if (swcr_id < 0) { - device_printf(dev, "cannot initialize!"); - return ENOMEM; + printf("cryptosoft0: cannot initialize!"); + return; } #define REGISTER(alg) \ crypto_register(swcr_id, alg, 0,0) @@ -1027,38 +1027,21 @@ REGISTER(CRYPTO_CAMELLIA_CBC); REGISTER(CRYPTO_DEFLATE_COMP); #undef REGISTER - - return 0; } static void -swcr_detach(device_t dev) +swcr_destroy(void) { crypto_unregister_all(swcr_id); if (swcr_sessions != NULL) FREE(swcr_sessions, M_CRYPTO_DATA); + if (swcr_kobj != NULL) { + kobj_delete(swcr_kobj, M_CRYPTO_DATA); + swcr_kobj = NULL; + kobj_class_free(&cryptosoft_class); + } } -static device_method_t swcr_methods[] = { - DEVMETHOD(device_identify, swcr_identify), - DEVMETHOD(device_probe, swcr_probe), - DEVMETHOD(device_attach, swcr_attach), - DEVMETHOD(device_detach, swcr_detach), - - DEVMETHOD(cryptodev_newsession, swcr_newsession), - DEVMETHOD(cryptodev_freesession,swcr_freesession), - DEVMETHOD(cryptodev_process, swcr_process), - - {0, 0}, -}; - -static driver_t swcr_driver = { - "cryptosoft", - swcr_methods, - 0, /* NB: no softc */ -}; -static devclass_t swcr_devclass; - /* * NB: We explicitly reference the crypto module so we * get the necessary ordering when built as a loadable @@ -1067,7 +1050,35 @@ * normal module dependencies would handle things). */ extern int crypto_modevent(struct module *, int, void *); -/* XXX where to attach */ -DRIVER_MODULE(cryptosoft, nexus, swcr_driver, swcr_devclass, crypto_modevent,0); + +static int +swcr_modevent(struct module *module, int cmd, void *arg) +{ + int error; + + switch (cmd) { + case MOD_LOAD: + error = crypto_modevent(module, cmd, arg); + if (error) + return (error); + swcr_init(); + return (0); + case MOD_UNLOAD: + swcr_destroy(); + return (crypto_modevent(module, cmd, arg)); + case MOD_QUIESCE: + return (crypto_modevent(module, cmd, arg)); + default: + return (EOPNOTSUPP); + } +} + +static moduledata_t cryptosoft_module = { + "cryptosoft", + swcr_modevent, + 0 +}; + +DECLARE_MODULE(cryptosoft, cryptosoft_module, SI_SUB_DRIVER, SI_ORDER_ANY); MODULE_VERSION(cryptosoft, 1); MODULE_DEPEND(cryptosoft, crypto, 1, 1, 1); -- John Baldwin