Re: IPSEC/crypto is broken in FreeBSD/powerpc 7.0-RELEASE!

From: John Baldwin <jhb_at_FreeBSD.org>
Date: Mon, 10 Mar 2008 17:27:19 -0400
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
_at__at_ -99,7 +99,8 _at__at_
  * 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 */
 	/*
_at__at_ -476,7 +477,7 _at__at_
  * 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;
_at__at_ -538,7 +539,7 _at__at_
 /*
  * 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
_at__at_ -548,12 +549,17 _at__at_
 
 	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();
_at__at_ -563,6 +569,8 _at__at_
 /*
  * 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)
_at__at_ -605,7 +613,7 _at__at_
 		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
 			);
_at__at_ -644,7 +652,7 _at__at_
 		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
_at__at_ -1454,7 +1462,7 _at__at_
 		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
_at__at_ -61,7 +61,7 _at__at_
 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.
_at__at_ -584,7 +584,7 _at__at_
  * 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;
_at__at_ -793,7 +793,7 _at__at_
  * 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;
_at__at_ -882,7 +882,7 _at__at_
  * 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;
_at__at_ -976,33 +976,33 _at__at_
 	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)
_at__at_ -1027,38 +1027,21 _at__at_
  	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
_at__at_ -1067,7 +1050,35 _at__at_
  * 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
Received on Mon Mar 10 2008 - 21:02:49 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:28 UTC