On Wed, Mar 08, 2017 at 09:39:30AM +0000, Reshetova, Elena wrote:

quoted

On Mon, Mar 06, 2017 at 04:20:57PM +0200, Elena Reshetova wrote:

quoted

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <redacted>
Signed-off-by: Kees Cook <redacted>
Signed-off-by: David Windsor <redacted>
---
 drivers/md/raid5-cache.c |  8 +++---
 drivers/md/raid5.c       | 66 ++++++++++++++++++++++++------------------------
 drivers/md/raid5.h       |  3 ++-
 3 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 3f307be..6c05e12 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c

snip

quoted

 	       sh->check_state, sh->reconstruct_state);

 	analyse_stripe(sh, &s);

@@ -4924,7 +4924,7 @@ static void activate_bit_delay(struct r5conf *conf,
 		struct stripe_head *sh = list_entry(head.next, struct

stripe_head, lru);

quoted

 		int hash;
 		list_del_init(&sh->lru);
-		atomic_inc(&sh->count);
+		refcount_inc(&sh->count);
 		hash = sh->hash_lock_index;
 		__release_stripe(conf, sh,

&temp_inactive_list[hash]);

quoted

 	}

@@ -5240,7 +5240,7 @@ static struct stripe_head *__get_priority_stripe(struct

r5conf *conf, int group)

quoted

 		sh->group = NULL;
 	}
 	list_del_init(&sh->lru);
-	BUG_ON(atomic_inc_return(&sh->count) != 1);
+	BUG_ON(refcount_inc_not_zero(&sh->count));

This changes the behavior. refcount_inc_not_zero doesn't inc if original value is 0

Hm.. So, you want to inc here in any case and BUG if the end result differs from 1. 
So essentially you want to only increment here from zero to one under normal conditions... This is a challenge for refcount_t and against the design.
Is it ok just to maybe do this here:

-	BUG_ON(atomic_inc_return(&sh->count) != 1);
+	BUG_ON(refcount_read(&sh->count) != 0);
+	refcount_set((&sh->count, 1);

this looks ok

Do we have an issue with locking in this case? Or maybe it is then better to leave this one to be atomic_t without protection since it isn't a real refcounter as it turns out. 

There is no lock issue, the count should be 0 in the list. It's a refcounter actually, so good to do the convert.

Thanks,
Shaohua