[PATCH] mbcache: convert mb_cache_entry.e_refcnt from atomic_t to refcount_t

[Elena Reshetova]

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