On 21/06/21(Mon) 23:45, Alexander Bluhm wrote:
> 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
Diff is ok mpi@.
The annotations are not really coherent with the rest of the tree but this
can be improved in tree, especially if we start using a mutex to replace
the various splvm/splx dances and protect the cc_* fields.
>
> 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()
This doesn't tell if a lock is needed or not. Other data structure use:
"I immutable after creation"
Is it what you meant? That the variables aren't marked as 'const'
because they are initialized once but after that never change?
> + * K modified with kernel lock
Can we use the same wording as other files:
"K kernel lock"
> + */
>
> -struct taskq *crypto_taskq;
> -struct taskq *crypto_taskq_mpsafe;
> +struct cryptocap *crypto_drivers; /* [A] array allocated by driver
Isn't it possible to have a USB driver calling crypto_get_driverid()?
If that's the case what's protecting the structure is the KERNEL_LOCK().
If we want to simplify things and prevent hotpluging then maybe we
should assert for `cold'?
> + [K] driver data and session count */
Until now we've been annotating the fields in the data structure
definitions, you're referring to `cc_sessions', `cc_newsession', etc,
right?
> +int crypto_drivers_num = 0; /* [A] attached drivers array size */
> +
> +struct pool cryptop_pool; /* [I] set of crypto descriptors */
Pool are protected by their own lock, not sure we should start annotating
them.
> +
> +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);
>
