linux-kernel - Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150513125839.371ef677@notabene.brown>
Date:	Wed, 13 May 2015 12:58:39 +1000
From:	NeilBrown <neilb@...e.de>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, mingo@...nel.org,
	laijs@...fujitsu.com, dipankar@...ibm.com,
	akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
	josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org,
	dhowells@...hat.com, edumazet@...gle.com, dvhart@...ux.intel.com,
	fweisbec@...il.com, oleg@...hat.com, bobby.prani@...il.com,
	Patrick Marlier <patrick.marlier@...il.com>,
	wangyun@...ux.vnet.ibm.com
Subject: Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage

On Tue, 12 May 2015 22:38:53 -0400 Steven Rostedt <rostedt@...dmis.org> wrote:

> On Tue, 12 May 2015 15:46:26 -0700
> "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:
> 
> > From: Patrick Marlier <patrick.marlier@...il.com>
> > 
> > Signed-off-by: Patrick Marlier <patrick.marlier@...il.com>
> > Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> > ---
> >  drivers/md/bitmap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> > index 2bc56e2a3526..32901772e4ee 100644
> > --- a/drivers/md/bitmap.c
> > +++ b/drivers/md/bitmap.c
> > @@ -181,7 +181,7 @@ static struct md_rdev *next_active_rdev(struct md_rdev *rdev, struct mddev *mdde
> >  	rcu_read_lock();
> >  	if (rdev == NULL)
> >  		/* start at the beginning */
> > -		rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);
> > +		rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set);
> 
> Hmm, this changes the semantics.
> 
> The original code looks nasty, I first thought it was broken, but it
> seems to work out of sheer luck (or clever hack)

Definitely a clever hack - no question of "luck" here :-)

It might makes sense to change it to use list_for_each_entry_from_rcu()

  if (rdev == NULL)
     rdev = list_entry_rcu(mddev->disks.next, struct md_rdev, same_set);
  else {
     rdev_dec_pending(rdev, mddev);
     rdev = list_next_entry_rcu(rdev->same_set.next, struct md_rdev, same_set);
  }
  list_for_each_entry_from_rcu(rdev, ....)

but there isn't a "list_next_entry_rcu"....


Also, it would have been polity to at least 'cc' them Maintainer of this code
in the original patch - no?

Thanks,
NeilBrown

> 
> >  	else {
> >  		/* release the previous rdev and start from there. */
> >  		rdev_dec_pending(rdev, mddev);
> 
> 
> What comes after this is:
> 
> 	list_for_each_entry_continue_rcu(rdev, &mddev->disks, same_set) {
> 		if (rdev->raid_disk >= 0 &&
> 
> Now the original code had:
> 
>   rdev = list_entry_rcu(&mddev->disks, struct md_rdev, same_set);
> 
> Where &mddev->disks would return the address of the disks field of
> mddev which is a list head. Then it would get the 'same_set' offset,
> which is 0, and rdev is pointing to a makeshift md_rdev struct. But it
> isn't used, as the list_for_each_entry_continue_rcu() has:
> 
> #define list_for_each_entry_continue_rcu(pos, head, member) 		\
> 	for (pos = list_entry_rcu(pos->member.next, typeof(*pos), member); \
> 	     &pos->member != (head);	\
> 	     pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> 
> Thus the first use of pos is pos->member.next or:
> 
>   mddev->disks.next
> 
> But now you converted it to rdev = mddev->disks.next, which means the
> first use is:
> 
>   pos = mddev->disks.next->next
> 
> I think you are skipping the first element here.
> 
> -- Steve


Content of type "application/pgp-signature" skipped