On Thu, Jun 17, 2021 at 03:19:11PM +0200, Alexander Bluhm wrote:
> On Thu, Jun 17, 2021 at 10:09:47AM +0200, Martin Pieuchot wrote:
> > Could you annotate which field is being protected by the KERNEL_LOCK()?
>
> No. I do not want to invest into fine grained crypto locking. I
> need a stable test machine.
Now my machine is stable again, I can do some annotations.
- remove unused variable cryptodesc_pool
- document global variables in crypto.c
- assert kernel lock where needed
- remove dead code from crypto_get_driverid()
- move crypto_init() prototype into header file
ok?
bluhm
Index: crypto/crypto.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/crypto/crypto.c,v
retrieving revision 1.82
diff -u -p -r1.82 crypto.c
--- crypto/crypto.c 30 Mar 2020 17:48:39 -0000 1.82
+++ crypto/crypto.c 21 Jun 2021 21:00:19 -0000
@@ -27,16 +27,23 @@
#include <crypto/cryptodev.h>
-void crypto_init(void);
-
-struct cryptocap *crypto_drivers = NULL;
-int crypto_drivers_num = 0;
-
-struct pool cryptop_pool;
-struct pool cryptodesc_pool;
+/*
+ * Locks used to protect struct members in this file:
+ * A allocated during driver attach, no hotplug, no detach
+ * I initialized by main()
+ * K modified with kernel lock
+ */
-struct taskq *crypto_taskq;
-struct taskq *crypto_taskq_mpsafe;
+struct cryptocap *crypto_drivers; /* [A] array allocated by driver
+ [K] driver data and session count */
+int crypto_drivers_num = 0; /* [A] attached drivers array size */
+
+struct pool cryptop_pool; /* [I] set of crypto descriptors */
+
+struct taskq *crypto_taskq; /* [I] run crypto_invoke() and callback
+ with kernel lock */
+struct taskq *crypto_taskq_mpsafe; /* [I] run crypto_invoke()
+ without kernel lock */
/*
* Create a new session.
@@ -52,6 +59,8 @@ crypto_newsession(u_int64_t *sid, struct
if (crypto_drivers == NULL)
return EINVAL;
+ KERNEL_ASSERT_LOCKED();
+
s = splvm();
/*
@@ -186,6 +195,8 @@ crypto_freesession(u_int64_t sid)
if (hid >= crypto_drivers_num)
return ENOENT;
+ KERNEL_ASSERT_LOCKED();
+
s = splvm();
if (crypto_drivers[hid].cc_sessions)
@@ -215,6 +226,9 @@ crypto_get_driverid(u_int8_t flags)
{
struct cryptocap *newdrv;
int i, s;
+
+ /* called from attach routines */
+ KERNEL_ASSERT_LOCKED();
s = splvm();
@@ -241,39 +255,33 @@ crypto_get_driverid(u_int8_t flags)
}
/* Out of entries, allocate some more. */
- if (i == crypto_drivers_num) {
- if (crypto_drivers_num >= CRYPTO_DRIVERS_MAX) {
- splx(s);
- return -1;
- }
-
- newdrv = mallocarray(crypto_drivers_num,
- 2 * sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT);
- if (newdrv == NULL) {
- splx(s);
- return -1;
- }
+ if (crypto_drivers_num >= CRYPTO_DRIVERS_MAX) {
+ splx(s);
+ return -1;
+ }
- memcpy(newdrv, crypto_drivers,
- crypto_drivers_num * sizeof(struct cryptocap));
- bzero(&newdrv[crypto_drivers_num],
- crypto_drivers_num * sizeof(struct cryptocap));
+ newdrv = mallocarray(crypto_drivers_num,
+ 2 * sizeof(struct cryptocap), M_CRYPTO_DATA, M_NOWAIT);
+ if (newdrv == NULL) {
+ splx(s);
+ return -1;
+ }
- newdrv[i].cc_sessions = 1; /* Mark */
- newdrv[i].cc_flags = flags;
+ memcpy(newdrv, crypto_drivers,
+ crypto_drivers_num * sizeof(struct cryptocap));
+ bzero(&newdrv[crypto_drivers_num],
+ crypto_drivers_num * sizeof(struct cryptocap));
- free(crypto_drivers, M_CRYPTO_DATA,
- crypto_drivers_num * sizeof(struct cryptocap));
+ newdrv[i].cc_sessions = 1; /* Mark */
+ newdrv[i].cc_flags = flags;
- crypto_drivers_num *= 2;
- crypto_drivers = newdrv;
- splx(s);
- return i;
- }
+ free(crypto_drivers, M_CRYPTO_DATA,
+ crypto_drivers_num * sizeof(struct cryptocap));
- /* Shouldn't really get here... */
+ crypto_drivers_num *= 2;
+ crypto_drivers = newdrv;
splx(s);
- return -1;
+ return i;
}
/*
@@ -287,11 +295,13 @@ crypto_register(u_int32_t driverid, int
{
int s, i;
-
if (driverid >= crypto_drivers_num || alg == NULL ||
crypto_drivers == NULL)
return EINVAL;
+ /* called from attach routines */
+ KERNEL_ASSERT_LOCKED();
+
s = splvm();
for (i = 0; i <= CRYPTO_ALGORITHM_MAX; i++) {
@@ -326,6 +336,9 @@ crypto_unregister(u_int32_t driverid, in
{
int i = CRYPTO_ALGORITHM_MAX + 1, s;
u_int32_t ses;
+
+ /* may be called from detach routines, but not used */
+ KERNEL_ASSERT_LOCKED();
s = splvm();
Index: crypto/cryptodev.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/crypto/cryptodev.h,v
retrieving revision 1.71
diff -u -p -r1.71 cryptodev.h
--- crypto/cryptodev.h 10 Aug 2017 18:57:20 -0000 1.71
+++ crypto/cryptodev.h 21 Jun 2021 20:59:08 -0000
@@ -216,6 +216,7 @@ struct cryptocap {
int (*cc_freesession) (u_int64_t);
};
+void crypto_init(void);
int crypto_newsession(u_int64_t *, struct cryptoini *, int);
int crypto_freesession(u_int64_t);
Index: kern/init_main.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/init_main.c,v
retrieving revision 1.307
diff -u -p -r1.307 init_main.c
--- kern/init_main.c 2 Jun 2021 13:56:28 -0000 1.307
+++ kern/init_main.c 21 Jun 2021 20:59:08 -0000
@@ -145,7 +145,6 @@ long __guard_local __attribute__((sectio
int main(void *);
void check_console(struct proc *);
void start_init(void *);
-void crypto_init(void);
void db_ctf_init(void);
void prof_init(void);
void init_exec(void);
