Re: WARNING in md_ioctl
> > > diff --git i/drivers/md/md.c w/drivers/md/md.c > > > index 6072782070230..49442a3f4605b 100644 > > > --- i/drivers/md/md.c > > > +++ w/drivers/md/md.c > > > @@ -7591,8 +7591,10 @@ static int md_ioctl(struct block_device *bdev, > > > fmode_t mode, > > > err = -EBUSY; > > > goto out; > > > } > > > - WARN_ON_ONCE(test_bit(MD_CLOSING, >flags)); > > > - set_bit(MD_CLOSING, >flags); > > > + if (test_and_set_bit(MD_CLOSING, >flags)) { > > > + err = -EBUSY; > > > + goto out; > > > + } > > > did_set_md_closing = true; > > > mutex_unlock(>open_mutex); > > > sync_blockdev(bdev); > > > > > Good catch! The fix looks good. Would you like to submit a patch for it? Sure. I will send a patch soon. Best regards, Dae R. Jeong.
Re: WARNING in md_ioctl
On Mon, Oct 19, 2020 at 12:03 AM Dae R. Jeong wrote: > > > diff --git i/drivers/md/md.c w/drivers/md/md.c > > index 6072782070230..49442a3f4605b 100644 > > --- i/drivers/md/md.c > > +++ w/drivers/md/md.c > > @@ -7591,8 +7591,10 @@ static int md_ioctl(struct block_device *bdev, > > fmode_t mode, > > err = -EBUSY; > > goto out; > > } > > - WARN_ON_ONCE(test_bit(MD_CLOSING, >flags)); > > - set_bit(MD_CLOSING, >flags); > > + if (test_and_set_bit(MD_CLOSING, >flags)) { > > + err = -EBUSY; > > + goto out; > > + } > > did_set_md_closing = true; > > mutex_unlock(>open_mutex); > > sync_blockdev(bdev); > > > > Could you please test whether this fixes the issue? > > Since >open_mutex is held when testing a bit of mddev->flags, I > modified the code just a little bit by putting mutex_unlock() as > belows. Good catch! The fix looks good. Would you like to submit a patch for it? Thanks, Song
Re: WARNING in md_ioctl
> diff --git i/drivers/md/md.c w/drivers/md/md.c > index 6072782070230..49442a3f4605b 100644 > --- i/drivers/md/md.c > +++ w/drivers/md/md.c > @@ -7591,8 +7591,10 @@ static int md_ioctl(struct block_device *bdev, > fmode_t mode, > err = -EBUSY; > goto out; > } > - WARN_ON_ONCE(test_bit(MD_CLOSING, >flags)); > - set_bit(MD_CLOSING, >flags); > + if (test_and_set_bit(MD_CLOSING, >flags)) { > + err = -EBUSY; > + goto out; > + } > did_set_md_closing = true; > mutex_unlock(>open_mutex); > sync_blockdev(bdev); > > Could you please test whether this fixes the issue? Since >open_mutex is held when testing a bit of mddev->flags, I modified the code just a little bit by putting mutex_unlock() as belows. diff --git a/drivers/md/md.c b/drivers/md/md.c index 98bac4f304ae..643f7f5be49b 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -7590,8 +7590,11 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode, err = -EBUSY; goto out; } - WARN_ON_ONCE(test_bit(MD_CLOSING, >flags)); - set_bit(MD_CLOSING, >flags); + if (test_and_set_bit(MD_CLOSING, >flags)) { + mutex_unlock(>open_mutex); + err = -EBUSY; + goto out; + } did_set_md_closing = true; mutex_unlock(>open_mutex); sync_blockdev(bdev); The warning no longer recurs (of course, we removed WARN_ON_ONCE()). As I am not familiar with this code, I do not see any other problem. Best regards, Dae R. Jeong
Re: WARNING in md_ioctl
On Sat, Oct 17, 2020 at 4:13 AM Dae R. Jeong wrote: > > Hi, > > I looked into the warning "WARNING in md_ioctl" found by Syzkaller. > (https://syzkaller.appspot.com/bug?id=fbf9eaea2e65bfcabb4e2750c3ab0892867edea1) > I suspect that it is caused by a race between two concurrenct ioctl()s as > belows. > > CPU1 (md_ioctl()) CPU2 (md_ioctl()) > -- -- > set_bit(MD_CLOSING, >flags); > did_set_md_closing = true; >WARN_ON_ONCE(test_bit(MD_CLOSING, > >flags)); > > if(did_set_md_closing) > clear_bit(MD_CLOSING, >flags); > > If the above is correct, this warning is introduced > in the commit 065e519e("md: MD_CLOSING needs to be cleared after called > md_set_readonly or do_md_stop"). > Could you please take a look into this? This is an interesting case. We try to protect against concurrent ioctl via mddev->openers: if (mddev->pers && atomic_read(>openers) > 1) { mutex_unlock(>open_mutex); err = -EBUSY; goto out; } But in this case, we are sending multiple ioctl from the same fd, so openers == 1. We can probably do something like: diff --git i/drivers/md/md.c w/drivers/md/md.c index 6072782070230..49442a3f4605b 100644 --- i/drivers/md/md.c +++ w/drivers/md/md.c @@ -7591,8 +7591,10 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode, err = -EBUSY; goto out; } - WARN_ON_ONCE(test_bit(MD_CLOSING, >flags)); - set_bit(MD_CLOSING, >flags); + if (test_and_set_bit(MD_CLOSING, >flags)) { + err = -EBUSY; + goto out; + } did_set_md_closing = true; mutex_unlock(>open_mutex); sync_blockdev(bdev); Could you please test whether this fixes the issue? Thanks, Song