From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shaohua Li Subject: Re: [PATCH 10/29] drivers, md: convert stripe_head.count from atomic_t to refcount_t Date: Thu, 9 Mar 2017 09:18:29 -0800 Message-ID: <20170309171829.33z7z6czwdivztp4@kernel.org> References: <1488810076-3754-1-git-send-email-elena.reshetova@intel.com> <1488810076-3754-11-git-send-email-elena.reshetova@intel.com> <20170307190759.jnrq66kfpkr4m7zl@kernel.org> <2236FBA76BA1254E88B949DDB74E612B41C56050@IRSMSX102.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <2236FBA76BA1254E88B949DDB74E612B41C56050@IRSMSX102.ger.corp.intel.com> Sender: linux-raid-owner@vger.kernel.org To: "Reshetova, Elena" Cc: "gregkh@linuxfoundation.org" , "linux-raid@vger.kernel.org" , Hans Liljestrand , Kees Cook , David Windsor List-Id: linux-raid.ids On Wed, Mar 08, 2017 at 09:39:30AM +0000, Reshetova, Elena wrote: > > On Mon, Mar 06, 2017 at 04:20:57PM +0200, Elena Reshetova wrote: > > > 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 > > > Signed-off-by: Hans Liljestrand > > > Signed-off-by: Kees Cook > > > Signed-off-by: David Windsor > > > --- > > > 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 > > > 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); > > > 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]); > > > } > > > @@ -5240,7 +5240,7 @@ static struct stripe_head *__get_priority_stripe(struct > > r5conf *conf, int group) > > > 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