On Thu 27-01-22 10:48:13, Jan Kara wrote:
> On Wed 26-01-22 16:50:36, Christoph Hellwig wrote:
> > lo_refcnt is only incremented in lo_open and decremented in lo_release,
> > and thus protected by open_mutex. Only take lo_mutex when lo_release
> > actually takes action for the final release.
> >
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Yup, good idea. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
On a second thought I retract this... See below:
> > index d3a7f281ce1b6..43980ec69dfdd 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1740,10 +1740,10 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
> > {
> > struct loop_device *lo = disk->private_data;
> >
> > - mutex_lock(&lo->lo_mutex);
> > - if (atomic_dec_return(&lo->lo_refcnt))
> > - goto out_unlock;
> > + if (!atomic_dec_and_test(&lo->lo_refcnt))
> > + return;
> >
> > + mutex_lock(&lo->lo_mutex);
There's a subtle race here like:
Thread 1 Thread2 (mount)
lo_release()
if (!atomic_dec_and_test(&lo->lo_refcnt))
- sees we are last one
lo_open()
mutex_lock_killable(&lo->lo_mutex);
atomic_inc(&lo->lo_refcnt);
mutex_unlock(&lo->lo_mutex);
ioctl(LOOP_GET_STATUS)
sees everything is fine
mutex_lock(&lo->lo_mutex);
if (lo->lo_flags & LO_FLAGS_AUTOCLEAR) {
cleans up the device
}
is unhappy, mount fails
Just after writing this I have realized that the above sequence is not
actually possible due to disk->open_mutex protecting us and serializing
lo_release() with lo_open() but it needs at least a comment to explain that
we rely on disk->open_mutex to avoid races with lo_open().
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
