Re: [PATCH] md/raid1: fix a race between removing rdev and access conf->mirrors[i].rdev

[Guoqing Jiang <guoqing.jiang@xxxxxxxxxxxxxxx>]

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.


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.

Thanks,
Guoqing