On Thu 27-01-22 10:48:13, Jan Kara wrote:

On Wed 26-01-22 16:50:36, Christoph Hellwig wrote:

quoted

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@lst.de>

Yup, good idea. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

On a second thought I retract this... See below:

quoted

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 [off-list ref]
SUSE Labs, CR