On 2019/7/29 19:54, Guoqing Jiang wrote:
On 7/29/19 1:36 PM, Yufen Yu wrote:
I don't think this can fix the race condition completely.
- p->rdev = NULL;
if (!test_bit(RemoveSynchronized, &rdev->flags)) {
synchronize_rcu();
+ p->rdev = NULL;
if (atomic_read(&rdev->nr_pending)) {
If we access conf->mirrors[i].rdev (e.g. raid1_write_request()) after
RCU grace period,
synchronize_rcu() will not wait the reader. Then, it also can cause
NULL pointer dereference.
That is the reason why we add the new flag 'WantRemove'. It can
prevent the reader to access
the 'rdev' after RCU grace period.
Sorry for my wrong description. It is ** after RCU grace start **, not
'after RCU grace period'.
How about move it to the else branch?
@@ -1825,7 +1828,6 @@ static int raid1_remove_disk(struct mddev
*mddev, struct md_rdev *rdev)
err = -EBUSY;
goto abort;
}
- p->rdev = NULL;
if (!test_bit(RemoveSynchronized, &rdev->flags)) {
synchronize_rcu();
if (atomic_read(&rdev->nr_pending)) {
@@ -1833,8 +1835,10 @@ static int raid1_remove_disk(struct mddev
*mddev, struct md_rdev *rdev)
err = -EBUSY;
p->rdev = rdev;
goto abort;
- }
- }
+ } else
+ p->rdev = NULL;
+ } else
+ p->rdev = NULL;
After rcu period, the nr_pending should be not zero in your case.
I also don't think this can work.
+ +
| |
| |
| +--->Reader | +--->Reader nr_pending++
| |
| |
| |
+ +
start rcu period end rcu period
call synchronize_rcu() return synchronize_rcu()
If the reader try to read conf->mirrors[i].rdev after rcu peroid start,
synchronize_rcu() will not wait the reader. We assume the current
value of nr_pending is 0. Then, raid1_remove_disk will set 'p->rdev = NULL'.
After that the reader add 'nr_pending' to 1 and try to access
conf->mirrors[i].rdev,
It can cause NULL pointer dereference.
Adding the new flag 'WantRemove' can prevent the reader to access
conf->mirrors[i].rdev after ** start rcu period **.
Yufen
Thank a lot
Thanks,
Guoqing
