linear raid : mddev not protected in linear_add

linear raid : mddev not protected in linear_add

[SandeepKsinha]

Hi,

In linear_add( ), the mddev is gets changed.
Don't we require to lock/unlock around mddev inorder to avoid races?



-- 
Regards,
Sandeep.





 	
“To learn is to change. Education is a process that changes the learner.”
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linear raid : mddev not protected in linear_add

[Neil Brown]

On Tuesday May 19, sandeepksinha@gmail.com wrote:
> Hi,
> 
> In linear_add( ), the mddev is gets changed.
> Don't we require to lock/unlock around mddev inorder to avoid races?
> 

A good question.

To be able to answer it, we must also ask "what races do we need to
avoid".  Did you have some possible races in mind?

In linear add there are 3 lines which are in an important order:

	mddev->private = newconf;
	mddev->raid_disks++;
	....
	set_capacity(mddev->gendisk, mddev->array_sectors);

Until raid_disks in incremented, there will be no attempt to access
the extra information in ->private.
Until the set_capacity, there will be no attempt to send IO beyond the
reach of raid_disks.
So except that it might be wise to insert some barrier()s, it looks
OK....  or it did.

I just noticed that the new 'which_dev' checks mddev->raid_disks,
where as the old which_dev did not.

So if:
  which_dev takes a copy of mddev->private,
                               linear_add changes mddev->private
                               linear_add changes raid_disks
  which_dev takes a copy of mddev->raid_disks

then which_dev will have a raid_disks which points beyond the end of
conf->disks.  If the target sector were beyond the end of the original
array, we could fall off the end of that array which would be bad.
However as the sector number will have been tested again the size of
the array (in bio_check_eod from generic_make_request), that cannot
happen.  Unless which_dev was called from linear_mergeable_bvec...

I suspect in light of this it would be best to:
  1/ insert a barrier() between setting ->private and setting
      ->raid_disks
  2/ in which_dev, read ->raid_disks before ->private, and put a
      barrier in between.

We could possibly use rcu_dereference and rcu_assign_pointer, and
maybe even make use of rcu to free the old conf structure promptly
rather than keeping it around until the array is stopped.
That would require careful placement of rcu_read_{,un}lock around
access to ->private.

Probably best to just put barriers in for now, and leave the rcu
stuff.

NeilBrown

Re: linear raid : mddev not protected in linear_add

[SandeepKsinha]

Hi Neil,

On Wed, May 20, 2009 at 4:28 AM, Neil Brown <neilb@suse.de> wrote:
> On Tuesday May 19, sandeepksinha@gmail.com wrote:
>> Hi,
>>
>> In linear_add( ), the mddev is gets changed.
>> Don't we require to lock/unlock around mddev inorder to avoid races?
>>
>
> A good question.
>
> To be able to answer it, we must also ask "what races do we need to
> avoid".  Did you have some possible races in mind?
>

I see mddev being referenced in md.c (md layer) for lot of operations.
Actually for almost all operations we pass through it. i.e get array
info.

> In linear add there are 3 lines which are in an important order:
>
>        mddev->private = newconf;
>        mddev->raid_disks++;
>        ....
>        set_capacity(mddev->gendisk, mddev->array_sectors);
>
> Until raid_disks in incremented, there will be no attempt to access
> the extra information in ->private.
> Until the set_capacity, there will be no attempt to send IO beyond the
> reach of raid_disks.
> So except that it might be wise to insert some barrier()s, it looks
> OK....  or it did.
>
> I just noticed that the new 'which_dev' checks mddev->raid_disks,
> where as the old which_dev did not.
>
> So if:
>  which_dev takes a copy of mddev->private,
>                               linear_add changes mddev->private
>                               linear_add changes raid_disks
>  which_dev takes a copy of mddev->raid_disks
>
> then which_dev will have a raid_disks which points beyond the end of
> conf->disks.  If the target sector were beyond the end of the original
> array, we could fall off the end of that array which would be bad.
> However as the sector number will have been tested again the size of
> the array (in bio_check_eod from generic_make_request), that cannot
> happen.  Unless which_dev was called from linear_mergeable_bvec...
>
Yes, this is true.

> I suspect in light of this it would be best to:
>  1/ insert a barrier() between setting ->private and setting
>      ->raid_disks
>  2/ in which_dev, read ->raid_disks before ->private, and put a
>      barrier in between.
>

Do you suggest implementing barrier functions for linear raid?
Can you elaborate on this a bit more please?

> We could possibly use rcu_dereference and rcu_assign_pointer, and
> maybe even make use of rcu to free the old conf structure promptly
> rather than keeping it around until the array is stopped.
> That would require careful placement of rcu_read_{,un}lock around
> access to ->private.
>
> Probably best to just put barriers in for now, and leave the rcu
> stuff.
>
k.
> NeilBrown
>
>
>



-- 
Regards,
Sandeep.





 	
“To learn is to change. Education is a process that changes the learner.”
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linear raid : mddev not protected in linear_add

[Neil Brown]

On Wednesday May 20, sandeepksinha@gmail.com wrote:
> Hi Neil,
> 
> On Wed, May 20, 2009 at 4:28 AM, Neil Brown <neilb@suse.de> wrote:
> > On Tuesday May 19, sandeepksinha@gmail.com wrote:
> >> Hi,
> >>
> >> In linear_add( ), the mddev is gets changed.
> >> Don't we require to lock/unlock around mddev inorder to avoid races?
> >>
> >
> > A good question.
> >
> > To be able to answer it, we must also ask "what races do we need to
> > avoid".  Did you have some possible races in mind?
> >
> 
> I see mddev being referenced in md.c (md layer) for lot of operations.
> Actually for almost all operations we pass through it. i.e get array
> info.

True.  The only field of mddev which linear_add changes, and which
md.c looks at would be ->raid_disks.  Are there any time where it
depends on the value in a way that a race could make dangerous.  I
think not.

> > I suspect in light of this it would be best to:
> >  1/ insert a barrier() between setting ->private and setting
> >      ->raid_disks
> >  2/ in which_dev, read ->raid_disks before ->private, and put a
> >      barrier in between.
> >
> 
> Do you suggest implementing barrier functions for linear raid?
> Can you elaborate on this a bit more please?

I'm not talking about IO request barriers, though I agree the
terminology is confusing.  I'm talking about memory barriers.
See Documentation/memory-barriers.txt

If one processor writes new values into two variables, 'a' and 'b'.
At about the same time that another processor reads the two variables,
then the second process could obviously get the two old values, or
the two new values, or the new value of 'a' but the old value of 'b'.
What is less obvious is that some times it could get the old value of
'a' and the new value of 'b'.
You can remove some of this indeterminism with memory barriers.

NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html