From a3e8fa0499ea7d779dfeb36fe916445aebac0ceb Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 01:41:13 +0200 Subject: Make some session functions static --- ncr-int.h | 3 --- ncr-sessions.c | 8 +++++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/ncr-int.h b/ncr-int.h index 5e86aff..9ae0ea1 100644 --- a/ncr-int.h +++ b/ncr-int.h @@ -186,9 +186,6 @@ int ncr_key_storage_unwrap(struct ncr_lists *lst, struct nlattr *tb[]); /* sessions */ -struct session_item_st* ncr_session_new(struct ncr_lists *lst); -void _ncr_sessions_item_put( struct session_item_st* item); -struct session_item_st* ncr_sessions_item_get(struct ncr_lists *lst, ncr_session_t desc); void ncr_sessions_list_deinit(struct ncr_lists *lst); int ncr_session_init(struct ncr_lists *lists, diff --git a/ncr-sessions.c b/ncr-sessions.c index 41f8a4c..ee32e69 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -31,6 +31,7 @@ #include #include +static void _ncr_sessions_item_put(struct session_item_st *item); static int _ncr_session_update_key(struct ncr_lists *lists, ncr_session_t ses, struct nlattr *tb[]); static void _ncr_session_remove(struct ncr_lists *lst, ncr_session_t desc); @@ -54,7 +55,8 @@ void ncr_sessions_list_deinit(struct ncr_lists *lst) } /* returns the data item corresponding to desc */ -struct session_item_st* ncr_sessions_item_get(struct ncr_lists *lst, ncr_session_t desc) +static struct session_item_st *ncr_sessions_item_get(struct ncr_lists *lst, + ncr_session_t desc) { struct session_item_st* item; @@ -71,7 +73,7 @@ struct session_item_st* item; return NULL; } -void _ncr_sessions_item_put( struct session_item_st* item) +static void _ncr_sessions_item_put(struct session_item_st *item) { if (atomic_dec_and_test(&item->refcnt)) { cryptodev_cipher_deinit(&item->cipher); @@ -85,7 +87,7 @@ void _ncr_sessions_item_put( struct session_item_st* item) } } -struct session_item_st* ncr_session_new(struct ncr_lists *lst) +static struct session_item_st *ncr_session_new(struct ncr_lists *lst) { struct session_item_st* sess; -- cgit From a3198b9448140afd15169745c5260cf828e1a82f Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:00:12 +0200 Subject: Separate session ID allocation from object creation --- ncr-sessions.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 15 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index ee32e69..7230c34 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -54,6 +54,49 @@ void ncr_sessions_list_deinit(struct ncr_lists *lst) mutex_unlock(&lst->session_idr_mutex); } +/* Allocate a descriptor without making a sesssion available to userspace. */ +static ncr_session_t session_alloc_desc(struct ncr_lists *lst) +{ + int ret, desc; + + mutex_lock(&lst->session_idr_mutex); + if (idr_pre_get(&lst->session_idr, GFP_KERNEL) == 0) { + ret = -ENOMEM; + goto end; + } + /* idr_pre_get() should preallocate enough, and, due to + session_idr_mutex, nobody else can use the preallocated data. + Therefore the loop recommended in idr_get_new() documentation is not + necessary. */ + ret = idr_get_new(&lst->session_idr, NULL, &desc); + if (ret != 0) + goto end; + ret = desc; +end: + mutex_unlock(&lst->session_idr_mutex); + return ret; +} + +/* Drop a pre-allocated, unpublished session descriptor */ +static void session_drop_desc(struct ncr_lists *lst, ncr_session_t desc) +{ + mutex_lock(&lst->session_idr_mutex); + idr_remove(&lst->session_idr, desc); + mutex_unlock(&lst->session_idr_mutex); +} + +/* Make a session descriptor visible in user-space */ +static void session_publish(struct ncr_lists *lst, struct session_item_st *sess) +{ + void *old; + + atomic_inc(&sess->refcnt); + mutex_lock(&lst->session_idr_mutex); + old = idr_replace(&lst->session_idr, sess, sess->desc); + mutex_unlock(&lst->session_idr_mutex); + BUG_ON(old != NULL); +} + /* returns the data item corresponding to desc */ static struct session_item_st *ncr_sessions_item_get(struct ncr_lists *lst, ncr_session_t desc) @@ -61,6 +104,7 @@ static struct session_item_st *ncr_sessions_item_get(struct ncr_lists *lst, struct session_item_st* item; mutex_lock(&lst->session_idr_mutex); + /* item may be NULL for pre-allocated session IDs. */ item = idr_find(&lst->session_idr, desc); if (item != NULL) { atomic_inc(&item->refcnt); @@ -87,7 +131,7 @@ static void _ncr_sessions_item_put(struct session_item_st *item) } } -static struct session_item_st *ncr_session_new(struct ncr_lists *lst) +static struct session_item_st *ncr_session_new(ncr_session_t desc) { struct session_item_st* sess; @@ -108,19 +152,8 @@ static struct session_item_st *ncr_session_new(struct ncr_lists *lst) } mutex_init(&sess->mem_mutex); - atomic_set(&sess->refcnt, 2); /* One for lst->list, one for "sess" */ - - mutex_lock(&lst->session_idr_mutex); - /* idr_pre_get() should preallocate enough, and, due to - session_idr_mutex, nobody else can use the preallocated data. - Therefore the loop recommended in idr_get_new() documentation is not - necessary. */ - if (idr_pre_get(&lst->session_idr, GFP_KERNEL) == 0 || - idr_get_new(&lst->session_idr, sess, &sess->desc) != 0) { - mutex_unlock(&lst->session_idr_mutex); - goto err_sess; - } - mutex_unlock(&lst->session_idr_mutex); + atomic_set(&sess->refcnt, 1); + sess->desc = desc; return sess; @@ -263,15 +296,23 @@ static int _ncr_session_init(struct ncr_lists *lists, ncr_crypto_op_t op, struct nlattr *tb[]) { const struct nlattr *nla; + ncr_session_t desc; struct session_item_st* ns = NULL; int ret; const struct algo_properties_st *sign_hash; - ns = ncr_session_new(lists); + desc = session_alloc_desc(lists); + if (desc < 0) { + err(); + return desc; + } + ns = ncr_session_new(desc); if (ns == NULL) { err(); + session_drop_desc(lists, desc); return -ENOMEM; } + session_publish(lists, ns); ns->op = op; ns->algorithm = _ncr_nla_to_properties(tb[NCR_ATTR_ALGORITHM]); @@ -529,6 +570,7 @@ static void _ncr_session_remove(struct ncr_lists *lst, ncr_session_t desc) struct session_item_st * item; mutex_lock(&lst->session_idr_mutex); + /* item may be NULL for pre-allocated session IDs. */ item = idr_find(&lst->session_idr, desc); if (item != NULL) idr_remove(&lst->session_idr, desc); /* Steal the reference */ -- cgit From d50cc4f8c117aa971960e498536335b6d6194787 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:05:33 +0200 Subject: Publish sessions only after initialization --- ncr-sessions.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index 7230c34..885b630 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -312,7 +312,6 @@ static int _ncr_session_init(struct ncr_lists *lists, ncr_crypto_op_t op, session_drop_desc(lists, desc); return -ENOMEM; } - session_publish(lists, ns); ns->op = op; ns->algorithm = _ncr_nla_to_properties(tb[NCR_ATTR_ALGORITHM]); @@ -490,11 +489,12 @@ static int _ncr_session_init(struct ncr_lists *lists, ncr_crypto_op_t op, goto fail; } + session_publish(lists, ns); ret = ns->desc; fail: if (ret < 0) { - _ncr_session_remove(lists, ns->desc); + session_drop_desc(lists, desc); } _ncr_sessions_item_put(ns); -- cgit From 24a20d546d227083a5304a3c82cba69d8334f80c Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:27:16 +0200 Subject: Move session lookup and locking out of low-level functions While doing this, also lock mem_mutex during _ncr_session_update_key. --- ncr-sessions.c | 83 ++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 49 insertions(+), 34 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index 885b630..d711ba7 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -32,7 +32,8 @@ #include static void _ncr_sessions_item_put(struct session_item_st *item); -static int _ncr_session_update_key(struct ncr_lists *lists, ncr_session_t ses, +static int _ncr_session_update_key(struct ncr_lists *lists, + struct session_item_st *sess, struct nlattr *tb[]); static void _ncr_session_remove(struct ncr_lists *lst, ncr_session_t desc); @@ -702,34 +703,21 @@ static int get_userbuf2(struct session_item_st *ses, struct nlattr *tb[], } /* Called when userspace buffers are used */ -static int _ncr_session_update(struct ncr_lists *lists, ncr_session_t ses, +static int _ncr_session_update(struct session_item_st *sess, struct nlattr *tb[], int compat) { int ret; - struct session_item_st* sess; struct scatterlist *isg = NULL; struct scatterlist *osg = NULL; unsigned osg_cnt=0, isg_cnt=0; size_t isg_size = 0, osg_size; struct ncr_session_output_buffer out; - sess = ncr_sessions_item_get(lists, ses); - if (sess == NULL) { - err(); - return -EINVAL; - } - - if (mutex_lock_interruptible(&sess->mem_mutex)) { - err(); - _ncr_sessions_item_put(sess); - return -ERESTARTSYS; - } - ret = get_userbuf2(sess, tb, &isg, &isg_cnt, &isg_size, &out, &osg, &osg_cnt, compat); if (ret < 0) { err(); - goto fail; + return ret; } switch(sess->op) { @@ -811,8 +799,6 @@ fail: release_user_pages(sess->pages, sess->available_pages); sess->available_pages = 0; } - mutex_unlock(&sess->mem_mutex); - _ncr_sessions_item_put(sess); return ret; } @@ -820,12 +806,32 @@ fail: static int try_session_update(struct ncr_lists *lists, ncr_session_t ses, struct nlattr *tb[], int compat) { + struct session_item_st *sess; + int ret; + + sess = ncr_sessions_item_get(lists, ses); + if (sess == NULL) { + err(); + return -EINVAL; + } + + if (mutex_lock_interruptible(&sess->mem_mutex)) { + err(); + ret = -ERESTARTSYS; + goto end; + } if (tb[NCR_ATTR_UPDATE_INPUT_KEY_AS_DATA] != NULL) - return _ncr_session_update_key(lists, ses, tb); + ret = _ncr_session_update_key(lists, sess, tb); else if (tb[NCR_ATTR_UPDATE_INPUT_DATA] != NULL) - return _ncr_session_update(lists, ses, tb, compat); + ret = _ncr_session_update(sess, tb, compat); + else + ret = 0; + mutex_unlock(&sess->mem_mutex); - return 0; +end: + _ncr_sessions_item_put(sess); + + return ret; } static int _ncr_session_final(struct ncr_lists *lists, ncr_session_t ses, @@ -1003,25 +1009,19 @@ fail: } /* Direct with key: Allows to hash a key */ -static int _ncr_session_update_key(struct ncr_lists *lists, ncr_session_t ses, +static int _ncr_session_update_key(struct ncr_lists *lists, + struct session_item_st* sess, struct nlattr *tb[]) { int ret; - struct session_item_st* sess; struct key_item_st* key = NULL; - sess = ncr_sessions_item_get(lists, ses); - if (sess == NULL) { - err(); - return -EINVAL; - } - /* read key */ ret = key_item_get_nla_read(&key, lists, tb[NCR_ATTR_UPDATE_INPUT_KEY_AS_DATA]); if (ret < 0) { err(); - goto fail; + return ret; } if (key->type != NCR_KEY_TYPE_SECRET) { @@ -1054,8 +1054,7 @@ static int _ncr_session_update_key(struct ncr_lists *lists, ncr_session_t ses, ret = 0; fail: - if (key) _ncr_key_item_put(key); - _ncr_sessions_item_put(sess); + _ncr_key_item_put(key); return ret; } @@ -1064,14 +1063,30 @@ int ncr_session_update(struct ncr_lists *lists, const struct ncr_session_update *op, struct nlattr *tb[], int compat) { + struct session_item_st *sess; int ret; + sess = ncr_sessions_item_get(lists, op->ses); + if (sess == NULL) { + err(); + return -EINVAL; + } + + if (mutex_lock_interruptible(&sess->mem_mutex)) { + err(); + ret = -ERESTARTSYS; + goto end; + } if (tb[NCR_ATTR_UPDATE_INPUT_DATA] != NULL) - ret = _ncr_session_update(lists, op->ses, tb, compat); + ret = _ncr_session_update(sess, tb, compat); else if (tb[NCR_ATTR_UPDATE_INPUT_KEY_AS_DATA] != NULL) - ret = _ncr_session_update_key(lists, op->ses, tb); + ret = _ncr_session_update_key(lists, sess, tb); else ret = -EINVAL; + mutex_unlock(&sess->mem_mutex); + +end: + _ncr_sessions_item_put(sess); if (unlikely(ret)) { err(); -- cgit From b6ddae6aab4f799f0df64865079ffdc5f1f24b6f Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:33:00 +0200 Subject: Avoid duplicit lookup and locking in _try_session_update --- ncr-sessions.c | 42 +++++++++++------------------------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index d711ba7..d5ae1dd 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -803,35 +803,16 @@ fail: return ret; } -static int try_session_update(struct ncr_lists *lists, ncr_session_t ses, - struct nlattr *tb[], int compat) +static int try_session_update(struct ncr_lists *lists, + struct session_item_st *sess, struct nlattr *tb[], + int compat) { - struct session_item_st *sess; - int ret; - - sess = ncr_sessions_item_get(lists, ses); - if (sess == NULL) { - err(); - return -EINVAL; - } - - if (mutex_lock_interruptible(&sess->mem_mutex)) { - err(); - ret = -ERESTARTSYS; - goto end; - } if (tb[NCR_ATTR_UPDATE_INPUT_KEY_AS_DATA] != NULL) - ret = _ncr_session_update_key(lists, sess, tb); + return _ncr_session_update_key(lists, sess, tb); else if (tb[NCR_ATTR_UPDATE_INPUT_DATA] != NULL) - ret = _ncr_session_update(sess, tb, compat); + return _ncr_session_update(sess, tb, compat); else - ret = 0; - mutex_unlock(&sess->mem_mutex); - -end: - _ncr_sessions_item_put(sess); - - return ret; + return 0; } static int _ncr_session_final(struct ncr_lists *lists, ncr_session_t ses, @@ -850,17 +831,16 @@ static int _ncr_session_final(struct ncr_lists *lists, ncr_session_t ses, return -EINVAL; } - ret = try_session_update(lists, ses, tb, compat); - if (ret < 0) { + if (mutex_lock_interruptible(&sess->mem_mutex)) { err(); _ncr_sessions_item_put(sess); - return ret; + return -ERESTARTSYS; } - if (mutex_lock_interruptible(&sess->mem_mutex)) { + ret = try_session_update(lists, sess, tb, compat); + if (ret < 0) { err(); - _ncr_sessions_item_put(sess); - return -ERESTARTSYS; + goto fail; } switch(sess->op) { -- cgit From 9cbe1a7c208e170cca07e735099842308bedc317 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:44:35 +0200 Subject: Remove redundant deinitializations. _ncr_sessions_item_put() clears this all on last put. --- ncr-sessions.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index d5ae1dd..7010cf7 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -975,13 +975,6 @@ fail: mutex_unlock(&sess->mem_mutex); kfree(buffer); - cryptodev_hash_deinit(&sess->hash); - if (sess->algorithm->is_symmetric) { - cryptodev_cipher_deinit(&sess->cipher); - } else { - ncr_pk_cipher_deinit(&sess->pk); - } - _ncr_sessions_item_put(sess); _ncr_session_remove(lists, ses); -- cgit From 6aca7d2bca9cab65aa5b84de3aa735667478c289 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:45:09 +0200 Subject: Move session lookup and locking out of _ncr_session_final --- ncr-sessions.c | 74 ++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index 7010cf7..1c12d82 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -815,32 +815,20 @@ static int try_session_update(struct ncr_lists *lists, return 0; } -static int _ncr_session_final(struct ncr_lists *lists, ncr_session_t ses, - struct nlattr *tb[], int compat) +static int _ncr_session_final(struct ncr_lists *lists, + struct session_item_st *sess, struct nlattr *tb[], + int compat) { const struct nlattr *nla; int ret; - struct session_item_st* sess; int digest_size; uint8_t digest[NCR_HASH_MAX_OUTPUT_SIZE]; void *buffer = NULL; - sess = ncr_sessions_item_get(lists, ses); - if (sess == NULL) { - err(); - return -EINVAL; - } - - if (mutex_lock_interruptible(&sess->mem_mutex)) { - err(); - _ncr_sessions_item_put(sess); - return -ERESTARTSYS; - } - ret = try_session_update(lists, sess, tb, compat); if (ret < 0) { err(); - goto fail; + return ret; } switch(sess->op) { @@ -972,12 +960,8 @@ static int _ncr_session_final(struct ncr_lists *lists, ncr_session_t ses, } fail: - mutex_unlock(&sess->mem_mutex); kfree(buffer); - _ncr_sessions_item_put(sess); - _ncr_session_remove(lists, ses); - return ret; } @@ -1073,26 +1057,60 @@ int ncr_session_final(struct ncr_lists *lists, const struct ncr_session_final *op, struct nlattr *tb[], int compat) { - return _ncr_session_final(lists, op->ses, tb, compat); + struct session_item_st *sess; + int ret; + + sess = ncr_sessions_item_get(lists, op->ses); + if (sess == NULL) { + err(); + return -EINVAL; + } + + if (mutex_lock_interruptible(&sess->mem_mutex)) { + err(); + _ncr_sessions_item_put(sess); + return -ERESTARTSYS; + } + + ret = _ncr_session_final(lists, sess, tb, compat); + + mutex_unlock(&sess->mem_mutex); + _ncr_sessions_item_put(sess); + _ncr_session_remove(lists, op->ses); + + return ret; } int ncr_session_once(struct ncr_lists *lists, const struct ncr_session_once *once, struct nlattr *tb[], int compat) { - int ret; + struct session_item_st *sess; + int ret, desc; - ret = _ncr_session_init(lists, once->op, tb); - if (ret < 0) { + desc = _ncr_session_init(lists, once->op, tb); + if (desc < 0) { err(); - return ret; + return desc; } - ret = _ncr_session_final(lists, ret, tb, compat); - if (ret < 0) { + sess = ncr_sessions_item_get(lists, desc); + if (sess == NULL) { err(); - return ret; + return -EINVAL; } + if (mutex_lock_interruptible(&sess->mem_mutex)) { + err(); + _ncr_sessions_item_put(sess); + return -ERESTARTSYS; + } + + ret = _ncr_session_final(lists, sess, tb, compat); + + mutex_unlock(&sess->mem_mutex); + _ncr_sessions_item_put(sess); + _ncr_session_remove(lists, desc); + return ret; } -- cgit From daa80bfb63be6fcab80f2d7f526976b3000572f2 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 02:56:52 +0200 Subject: Move ID handling out of _ncr_session_init --- ncr-sessions.c | 57 +++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index 1c12d82..ab4c997 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -293,25 +293,20 @@ static int key_item_get_nla_read(struct key_item_st **st, return ret; } -static int _ncr_session_init(struct ncr_lists *lists, ncr_crypto_op_t op, - struct nlattr *tb[]) +static struct session_item_st *_ncr_session_init(struct ncr_lists *lists, + ncr_session_t desc, + ncr_crypto_op_t op, + struct nlattr *tb[]) { const struct nlattr *nla; - ncr_session_t desc; - struct session_item_st* ns = NULL; + struct session_item_st *ns; int ret; const struct algo_properties_st *sign_hash; - desc = session_alloc_desc(lists); - if (desc < 0) { - err(); - return desc; - } ns = ncr_session_new(desc); if (ns == NULL) { err(); - session_drop_desc(lists, desc); - return -ENOMEM; + return ERR_PTR(-ENOMEM); } ns->op = op; @@ -490,23 +485,38 @@ static int _ncr_session_init(struct ncr_lists *lists, ncr_crypto_op_t op, goto fail; } - session_publish(lists, ns); - ret = ns->desc; + return ns; fail: - if (ret < 0) { - session_drop_desc(lists, desc); - } _ncr_sessions_item_put(ns); - return ret; + return ERR_PTR(ret); } int ncr_session_init(struct ncr_lists *lists, const struct ncr_session_init *session, struct nlattr *tb[]) { - return _ncr_session_init(lists, session->op, tb); + ncr_session_t desc; + struct session_item_st *sess; + + desc = session_alloc_desc(lists); + if (desc < 0) { + err(); + return desc; + } + + sess = _ncr_session_init(lists, desc, session->op, tb); + if (IS_ERR(sess)) { + err(); + session_drop_desc(lists, desc); + return PTR_ERR(sess); + } + + session_publish(lists, sess); + _ncr_sessions_item_put(sess); + + return desc; } static int _ncr_session_encrypt(struct session_item_st* sess, const struct scatterlist* input, unsigned input_cnt, @@ -1088,18 +1098,21 @@ int ncr_session_once(struct ncr_lists *lists, struct session_item_st *sess; int ret, desc; - desc = _ncr_session_init(lists, once->op, tb); + desc = session_alloc_desc(lists); if (desc < 0) { err(); return desc; } - sess = ncr_sessions_item_get(lists, desc); - if (sess == NULL) { + sess = _ncr_session_init(lists, desc, once->op, tb); + if (IS_ERR(sess)) { err(); - return -EINVAL; + session_drop_desc(lists, desc); + return PTR_ERR(sess); } + session_publish(lists, sess); + if (mutex_lock_interruptible(&sess->mem_mutex)) { err(); _ncr_sessions_item_put(sess); -- cgit From 638b47ddcec13d384c6122f56f8e336019382e2b Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 03:08:13 +0200 Subject: Don't allocate session IDs for *_ONCE. Also skip locking the mutex - the session is private to the calling thread, nothing else can access it. --- ncr-sessions.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index ab4c997..c0ef7e0 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -1096,34 +1096,17 @@ int ncr_session_once(struct ncr_lists *lists, int compat) { struct session_item_st *sess; - int ret, desc; - - desc = session_alloc_desc(lists); - if (desc < 0) { - err(); - return desc; - } + int ret; - sess = _ncr_session_init(lists, desc, once->op, tb); + sess = _ncr_session_init(lists, -1, once->op, tb); if (IS_ERR(sess)) { err(); - session_drop_desc(lists, desc); return PTR_ERR(sess); } - session_publish(lists, sess); - - if (mutex_lock_interruptible(&sess->mem_mutex)) { - err(); - _ncr_sessions_item_put(sess); - return -ERESTARTSYS; - } - ret = _ncr_session_final(lists, sess, tb, compat); - mutex_unlock(&sess->mem_mutex); _ncr_sessions_item_put(sess); - _ncr_session_remove(lists, desc); return ret; } -- cgit From 3959e5b137db6e81820a9d729ba3c61fc269b3f0 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 03:14:05 +0200 Subject: Simplify refcount handling in session_init --- ncr-sessions.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index c0ef7e0..098b35f 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -86,12 +86,12 @@ static void session_drop_desc(struct ncr_lists *lst, ncr_session_t desc) mutex_unlock(&lst->session_idr_mutex); } -/* Make a session descriptor visible in user-space */ -static void session_publish(struct ncr_lists *lst, struct session_item_st *sess) +/* Make a session descriptor visible in user-space, stealing the reference */ +static void session_publish_ref(struct ncr_lists *lst, + struct session_item_st *sess) { void *old; - atomic_inc(&sess->refcnt); mutex_lock(&lst->session_idr_mutex); old = idr_replace(&lst->session_idr, sess, sess->desc); mutex_unlock(&lst->session_idr_mutex); @@ -513,8 +513,7 @@ int ncr_session_init(struct ncr_lists *lists, return PTR_ERR(sess); } - session_publish(lists, sess); - _ncr_sessions_item_put(sess); + session_publish_ref(lists, sess); return desc; } -- cgit From 4bc7ded8cb7e7e77c28a2b476ca1f3e0391b646b Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 03:15:15 +0200 Subject: Rename ncr_sessions_item_get() to session_get_ref() ... for consistency with the other session ID handlers. --- ncr-sessions.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index 098b35f..beb5271 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -99,8 +99,8 @@ static void session_publish_ref(struct ncr_lists *lst, } /* returns the data item corresponding to desc */ -static struct session_item_st *ncr_sessions_item_get(struct ncr_lists *lst, - ncr_session_t desc) +static struct session_item_st *session_get_ref(struct ncr_lists *lst, + ncr_session_t desc) { struct session_item_st* item; @@ -1032,7 +1032,7 @@ int ncr_session_update(struct ncr_lists *lists, struct session_item_st *sess; int ret; - sess = ncr_sessions_item_get(lists, op->ses); + sess = session_get_ref(lists, op->ses); if (sess == NULL) { err(); return -EINVAL; @@ -1069,7 +1069,7 @@ int ncr_session_final(struct ncr_lists *lists, struct session_item_st *sess; int ret; - sess = ncr_sessions_item_get(lists, op->ses); + sess = session_get_ref(lists, op->ses); if (sess == NULL) { err(); return -EINVAL; -- cgit From b89b8e504e89d8d3cbad36d6d1abea2b7c285c22 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 03:30:45 +0200 Subject: Unpublish session ID at start of ncr_session_final. This means that ncr_session_final() can't be called on one ID twice, ensuring that the ID is still unique throughout the runtime of ncr_session_final(). (Note that this is not guaranteed for ncr_session_update(): a concurrent thread can call ncr_session_final() on the ID and reuse it before ncr_session_update() finishes.) --- ncr-sessions.c | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index beb5271..ae1aa69 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -35,7 +35,6 @@ static void _ncr_sessions_item_put(struct session_item_st *item); static int _ncr_session_update_key(struct ncr_lists *lists, struct session_item_st *sess, struct nlattr *tb[]); -static void _ncr_session_remove(struct ncr_lists *lst, ncr_session_t desc); static int session_list_deinit_fn(int id, void *item, void *unused) { @@ -118,6 +117,23 @@ struct session_item_st* item; return NULL; } +/* Find a session, stealing the reference, but keep the descriptor allocated. */ +static struct session_item_st *session_unpublish_ref(struct ncr_lists *lst, + ncr_session_t desc) +{ + struct session_item_st *sess; + + mutex_lock(&lst->session_idr_mutex); + /* sess may be NULL for pre-allocated session IDs. */ + sess = idr_replace(&lst->session_idr, NULL, desc); + mutex_unlock(&lst->session_idr_mutex); + if (sess != NULL && !IS_ERR(sess)) + return sess; + + err(); + return NULL; +} + static void _ncr_sessions_item_put(struct session_item_st *item) { if (atomic_dec_and_test(&item->refcnt)) { @@ -575,21 +591,6 @@ int ret; return 0; } -static void _ncr_session_remove(struct ncr_lists *lst, ncr_session_t desc) -{ - struct session_item_st * item; - - mutex_lock(&lst->session_idr_mutex); - /* item may be NULL for pre-allocated session IDs. */ - item = idr_find(&lst->session_idr, desc); - if (item != NULL) - idr_remove(&lst->session_idr, desc); /* Steal the reference */ - mutex_unlock(&lst->session_idr_mutex); - - if (item != NULL) - _ncr_sessions_item_put(item); -} - static int _ncr_session_grow_pages(struct session_item_st *ses, int pagecount) { struct scatterlist *sg; @@ -1069,7 +1070,10 @@ int ncr_session_final(struct ncr_lists *lists, struct session_item_st *sess; int ret; - sess = session_get_ref(lists, op->ses); + /* Make the session inaccessible atomically to avoid concurrent + session_final() callers, but keep the ID allocated to keep audit + information unambiguous. */ + sess = session_unpublish_ref(lists, op->ses); if (sess == NULL) { err(); return -EINVAL; @@ -1077,15 +1081,18 @@ int ncr_session_final(struct ncr_lists *lists, if (mutex_lock_interruptible(&sess->mem_mutex)) { err(); - _ncr_sessions_item_put(sess); + /* Other threads may observe the session descriptor + disappearing and reappearing - but then they should not be + accessing it anyway if it is being freed. + session_unpublish_ref keeps the ID allocated for us. */ + session_publish_ref(lists, sess); return -ERESTARTSYS; } - ret = _ncr_session_final(lists, sess, tb, compat); - mutex_unlock(&sess->mem_mutex); + _ncr_sessions_item_put(sess); - _ncr_session_remove(lists, op->ses); + session_drop_desc(lists, op->ses); return ret; } -- cgit From ddd535966fc356f99b72af5eda678575f6fabebb Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 03:35:07 +0200 Subject: Move session_item_st definition to ncr-sessions.c It is not used anywhere else. --- ncr-int.h | 26 -------------------------- ncr-sessions.c | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/ncr-int.h b/ncr-int.h index 9ae0ea1..6bada32 100644 --- a/ncr-int.h +++ b/ncr-int.h @@ -56,32 +56,6 @@ struct algo_properties_st { ncr_key_type_t key_type; }; -struct session_item_st { - const struct algo_properties_st *algorithm; - ncr_crypto_op_t op; - - /* contexts for various options. - * simpler to have them like that than - * in a union. - */ - struct cipher_data cipher; - struct ncr_pk_ctx pk; - struct hash_data hash; - - struct scatterlist *sg; - struct page **pages; - unsigned array_size; - unsigned available_pages; - struct mutex mem_mutex; /* down when the - * values above are changed. - */ - - struct key_item_st* key; - - atomic_t refcnt; - ncr_session_t desc; -}; - struct key_item_st { /* This object is also not protected from concurrent access. */ diff --git a/ncr-sessions.c b/ncr-sessions.c index ae1aa69..2db972e 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -31,6 +31,32 @@ #include #include +struct session_item_st { + const struct algo_properties_st *algorithm; + ncr_crypto_op_t op; + + /* contexts for various options. + * simpler to have them like that than + * in a union. + */ + struct cipher_data cipher; + struct ncr_pk_ctx pk; + struct hash_data hash; + + struct scatterlist *sg; + struct page **pages; + unsigned array_size; + unsigned available_pages; + struct mutex mem_mutex; /* down when the + * values above are changed. + */ + + struct key_item_st* key; + + atomic_t refcnt; + ncr_session_t desc; +}; + static void _ncr_sessions_item_put(struct session_item_st *item); static int _ncr_session_update_key(struct ncr_lists *lists, struct session_item_st *sess, -- cgit From 371746ebd171a83172b328375992f03f6c336bd4 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Wed, 25 Aug 2010 03:51:36 +0200 Subject: Document locking. Document how members of session_item_st are protected, and what assumptions are made by internal functions. --- ncr-sessions.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/ncr-sessions.c b/ncr-sessions.c index 2db972e..2521f2e 100644 --- a/ncr-sessions.c +++ b/ncr-sessions.c @@ -32,8 +32,15 @@ #include struct session_item_st { + atomic_t refcnt; + /* Constant values throughout the life of this object follow. */ + ncr_session_t desc; const struct algo_properties_st *algorithm; ncr_crypto_op_t op; + struct key_item_st *key; + + /* Variable values, protected usually by mutex, follow. */ + struct mutex mutex; /* contexts for various options. * simpler to have them like that than @@ -47,14 +54,6 @@ struct session_item_st { struct page **pages; unsigned array_size; unsigned available_pages; - struct mutex mem_mutex; /* down when the - * values above are changed. - */ - - struct key_item_st* key; - - atomic_t refcnt; - ncr_session_t desc; }; static void _ncr_sessions_item_put(struct session_item_st *item); @@ -193,7 +192,7 @@ static struct session_item_st *ncr_session_new(ncr_session_t desc) err(); goto err_sess; } - mutex_init(&sess->mem_mutex); + mutex_init(&sess->mutex); atomic_set(&sess->refcnt, 1); sess->desc = desc; @@ -350,6 +349,8 @@ static struct session_item_st *_ncr_session_init(struct ncr_lists *lists, err(); return ERR_PTR(-ENOMEM); } + /* ns is the only reference throughout this function, so no locking + is necessary. */ ns->op = op; ns->algorithm = _ncr_nla_to_properties(tb[NCR_ATTR_ALGORITHM]); @@ -560,6 +561,7 @@ int ncr_session_init(struct ncr_lists *lists, return desc; } +/* The caller is responsible for locking of the session. */ static int _ncr_session_encrypt(struct session_item_st* sess, const struct scatterlist* input, unsigned input_cnt, size_t input_size, void *output, unsigned output_cnt, size_t *output_size) { @@ -588,6 +590,7 @@ int ret; return 0; } +/* The caller is responsible for locking of the session. */ static int _ncr_session_decrypt(struct session_item_st* sess, const struct scatterlist* input, unsigned input_cnt, size_t input_size, struct scatterlist *output, unsigned output_cnt, size_t *output_size) @@ -617,6 +620,7 @@ int ret; return 0; } +/* The caller is responsible for locking of the session. */ static int _ncr_session_grow_pages(struct session_item_st *ses, int pagecount) { struct scatterlist *sg; @@ -648,7 +652,8 @@ static int _ncr_session_grow_pages(struct session_item_st *ses, int pagecount) } /* Make NCR_ATTR_UPDATE_INPUT_DATA and NCR_ATTR_UPDATE_OUTPUT_BUFFER available - in scatterlists */ + in scatterlists. + The caller is responsible for locking of the session. */ static int get_userbuf2(struct session_item_st *ses, struct nlattr *tb[], struct scatterlist **src_sg, unsigned *src_cnt, size_t *src_size, struct ncr_session_output_buffer *dst, @@ -738,7 +743,8 @@ static int get_userbuf2(struct session_item_st *ses, struct nlattr *tb[], return 0; } -/* Called when userspace buffers are used */ +/* Called when userspace buffers are used. + The caller is responsible for locking of the session. */ static int _ncr_session_update(struct session_item_st *sess, struct nlattr *tb[], int compat) { @@ -839,6 +845,7 @@ fail: return ret; } +/* The caller is responsible for locking of the session. */ static int try_session_update(struct ncr_lists *lists, struct session_item_st *sess, struct nlattr *tb[], int compat) @@ -851,6 +858,9 @@ static int try_session_update(struct ncr_lists *lists, return 0; } +/* The caller is responsible for locking of the session. + Note that one or more _ncr_session_update()s may still be blocked on + sess->mutex and will execute after this function! */ static int _ncr_session_final(struct ncr_lists *lists, struct session_item_st *sess, struct nlattr *tb[], int compat) @@ -1001,7 +1011,8 @@ fail: return ret; } -/* Direct with key: Allows to hash a key */ +/* Direct with key: Allows to hash a key. + The caller is responsible for locking of the session. */ static int _ncr_session_update_key(struct ncr_lists *lists, struct session_item_st* sess, struct nlattr *tb[]) @@ -1065,7 +1076,10 @@ int ncr_session_update(struct ncr_lists *lists, return -EINVAL; } - if (mutex_lock_interruptible(&sess->mem_mutex)) { + /* Note that op->ses may be reallocated from now on, making the audit + information confusing. */ + + if (mutex_lock_interruptible(&sess->mutex)) { err(); ret = -ERESTARTSYS; goto end; @@ -1076,7 +1090,7 @@ int ncr_session_update(struct ncr_lists *lists, ret = _ncr_session_update_key(lists, sess, tb); else ret = -EINVAL; - mutex_unlock(&sess->mem_mutex); + mutex_unlock(&sess->mutex); end: _ncr_sessions_item_put(sess); @@ -1105,7 +1119,7 @@ int ncr_session_final(struct ncr_lists *lists, return -EINVAL; } - if (mutex_lock_interruptible(&sess->mem_mutex)) { + if (mutex_lock_interruptible(&sess->mutex)) { err(); /* Other threads may observe the session descriptor disappearing and reappearing - but then they should not be @@ -1115,7 +1129,7 @@ int ncr_session_final(struct ncr_lists *lists, return -ERESTARTSYS; } ret = _ncr_session_final(lists, sess, tb, compat); - mutex_unlock(&sess->mem_mutex); + mutex_unlock(&sess->mutex); _ncr_sessions_item_put(sess); session_drop_desc(lists, op->ses); @@ -1136,6 +1150,7 @@ int ncr_session_once(struct ncr_lists *lists, return PTR_ERR(sess); } + /* No locking of sess necessary, "sess" is the only reference. */ ret = _ncr_session_final(lists, sess, tb, compat); _ncr_sessions_item_put(sess); -- cgit