atomic_t variables are currently used to implement reference counters with the following properties: - counter is initialized to 1 using atomic_set() - a resource is freed upon counter reaching zero - once counter reaches zero, its further increments aren't allowed - counter schema uses basic atomic operations (set, inc, inc_not_zero, dec_and_test, etc.) Such atomic variables should be converted to a newly provided refcount_t type and API that prevents accidental counter overflows and underflows. This is important since overflows and underflows can lead to use-after-free situation and be exploitable. The variable mb_cache_entry.e_refcnt is used as pure reference counter. Convert it to refcount_t and fix up the operations. **Important note for maintainers: Some functions from refcount_t API defined in lib/refcount.c have different memory ordering guarantees than their atomic counterparts. The full comparison can be seen in https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon in state to be merged to the documentation tree. Normally the differences should not matter since refcount_t provides enough guarantees to satisfy the refcounting use cases, but in some rare cases it might matter. Please double check that you don't have some undocumented memory guarantees for this variable usage. For the mb_cache_entry.e_refcnt it might make a difference in following places: - mb_cache_destroy(), mb_cache_shrink() and mb_cache_entry_delete(): decrement in refcount_dec() provides RELEASE ordering vs. fully unordered atomic counterpart. Since the change is for better, it should not matter for these cases. - mb_cache_entry_put(): decrement in refcount_dec_and_test() only provides RELEASE ordering and control dependency on success vs. fully ordered atomic counterpart Suggested-by: Kees Cook <keescook@xxxxxxxxxxxx> Reviewed-by: David Windsor <dwindsor@xxxxxxxxx> Reviewed-by: Hans Liljestrand <ishkamiel@xxxxxxxxx> Signed-off-by: Elena Reshetova <elena.reshetova@xxxxxxxxx> --- fs/mbcache.c | 16 ++++++++-------- include/linux/mbcache.h | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/fs/mbcache.c b/fs/mbcache.c index d818fd2..8bdb52b 100644 --- a/fs/mbcache.c +++ b/fs/mbcache.c @@ -90,7 +90,7 @@ int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key, INIT_LIST_HEAD(&entry->e_list); /* One ref for hash, one ref returned */ - atomic_set(&entry->e_refcnt, 1); + refcount_set(&entry->e_refcnt, 1); entry->e_key = key; entry->e_value = value; entry->e_reusable = reusable; @@ -109,7 +109,7 @@ int mb_cache_entry_create(struct mb_cache *cache, gfp_t mask, u32 key, spin_lock(&cache->c_list_lock); list_add_tail(&entry->e_list, &cache->c_list); /* Grab ref for LRU list */ - atomic_inc(&entry->e_refcnt); + refcount_inc(&entry->e_refcnt); cache->c_entry_count++; spin_unlock(&cache->c_list_lock); @@ -141,7 +141,7 @@ static struct mb_cache_entry *__entry_find(struct mb_cache *cache, entry = hlist_bl_entry(node, struct mb_cache_entry, e_hash_list); if (entry->e_key == key && entry->e_reusable) { - atomic_inc(&entry->e_refcnt); + refcount_inc(&entry->e_refcnt); goto out; } node = node->next; @@ -204,7 +204,7 @@ struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *cache, u32 key, hlist_bl_lock(head); hlist_bl_for_each_entry(entry, node, head, e_hash_list) { if (entry->e_key == key && entry->e_value == value) { - atomic_inc(&entry->e_refcnt); + refcount_inc(&entry->e_refcnt); goto out; } } @@ -239,7 +239,7 @@ void mb_cache_entry_delete(struct mb_cache *cache, u32 key, u64 value) if (!list_empty(&entry->e_list)) { list_del_init(&entry->e_list); cache->c_entry_count--; - atomic_dec(&entry->e_refcnt); + refcount_dec(&entry->e_refcnt); } spin_unlock(&cache->c_list_lock); mb_cache_entry_put(cache, entry); @@ -300,7 +300,7 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache, hlist_bl_lock(head); if (!hlist_bl_unhashed(&entry->e_hash_list)) { hlist_bl_del_init(&entry->e_hash_list); - atomic_dec(&entry->e_refcnt); + refcount_dec(&entry->e_refcnt); } hlist_bl_unlock(head); if (mb_cache_entry_put(cache, entry)) @@ -397,11 +397,11 @@ void mb_cache_destroy(struct mb_cache *cache) list_for_each_entry_safe(entry, next, &cache->c_list, e_list) { if (!hlist_bl_unhashed(&entry->e_hash_list)) { hlist_bl_del_init(&entry->e_hash_list); - atomic_dec(&entry->e_refcnt); + refcount_dec(&entry->e_refcnt); } else WARN_ON(1); list_del(&entry->e_list); - WARN_ON(atomic_read(&entry->e_refcnt) != 1); + WARN_ON(refcount_read(&entry->e_refcnt) != 1); mb_cache_entry_put(cache, entry); } kfree(cache->c_hash); diff --git a/include/linux/mbcache.h b/include/linux/mbcache.h index 20f1e3f..0bf2af6 100644 --- a/include/linux/mbcache.h +++ b/include/linux/mbcache.h @@ -5,7 +5,7 @@ #include <linux/hash.h> #include <linux/list_bl.h> #include <linux/list.h> -#include <linux/atomic.h> +#include <linux/refcount.h> #include <linux/fs.h> struct mb_cache; @@ -15,7 +15,7 @@ struct mb_cache_entry { struct list_head e_list; /* Hash table list - protected by hash chain bitlock */ struct hlist_bl_node e_hash_list; - atomic_t e_refcnt; + refcount_t e_refcnt; /* Key in hash - stable during lifetime of the entry */ u32 e_key; u32 e_referenced:1; @@ -33,7 +33,7 @@ void __mb_cache_entry_free(struct mb_cache_entry *entry); static inline int mb_cache_entry_put(struct mb_cache *cache, struct mb_cache_entry *entry) { - if (!atomic_dec_and_test(&entry->e_refcnt)) + if (!refcount_dec_and_test(&entry->e_refcnt)) return 0; __mb_cache_entry_free(entry); return 1; -- 2.7.4
