RE: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open

[Gulam Mohamed]

Hi Kuai,

> -----Original Message-----
> From: Yu Kuai <yukuai1@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 22, 2024 8:01 AM
> To: Jens Axboe <axboe@xxxxxxxxx>; Gulam Mohamed
> <gulam.mohamed@xxxxxxxxxx>; linux-block@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: shinichiro.kawasaki@xxxxxxx; chaitanyak@xxxxxxxxxx; hch@xxxxxx;
> yukuai (C) <yukuai3@xxxxxxxxxx>
> Subject: Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop
> detach and loop open
> 
> Hi,
> 
> 在 2024/05/22 9:39, Jens Axboe 写道:
> > On 5/21/24 4:42 PM, Gulam Mohamed wrote:
> >> Description
> >> ===========
> >>
> >> 1. Userspace sends the command "losetup -d" which uses the open() call
> >>     to open the device
> >> 2. Kernel receives the ioctl command "LOOP_CLR_FD" which calls the
> >>     function loop_clr_fd()
> >> 3. If LOOP_CLR_FD is the first command received at the time, then the
> >>     AUTOCLEAR flag is not set and deletion of the
> >>     loop device proceeds ahead and scans the partitions (drop/add
> >>     partitions)
> >>
> >> 	if (disk_openers(lo->lo_disk) > 1) {
> >> 		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
> >> 		loop_global_unlock(lo, true);
> >> 		return 0;
> >> 	}
> >>
> >>   4. Before scanning partitions, it will check to see if any partition of
> >>      the loop device is currently opened
> >>   5. If any partition is opened, then it will return EBUSY:
> >>
> >>      if (disk->open_partitions)
> >> 		return -EBUSY;
> >>   6. So, after receiving the "LOOP_CLR_FD" command and just before the
> above
> >>      check for open_partitions, if any other command
> >>      (like blkid) opens any partition of the loop device, then the partition
> >>      scan will not proceed and EBUSY is returned as shown in above code
> >>   7. But in "__loop_clr_fd()", this EBUSY error is not propagated
> >>   8. We have noticed that this is causing the partitions of the loop to
> >>      remain stale even after the loop device is detached resulting in the
> >>      IO errors on the partitions
> >>
> >> Fix
> >> ---
> >> Re-introduce the lo_open() call to restrict any process to open the
> >> loop device when its being detached
> >>
> >> Test case
> >> =========
> >> Test case involves the following two scripts:
> >>
> >> script1.sh
> >> ----------
> >> while [ 1 ];
> >> do
> >> 	losetup -P -f /home/opt/looptest/test10.img
> >> 	blkid /dev/loop0p1
> >> done
> >>
> >> script2.sh
> >> ----------
> >> while [ 1 ];
> >> do
> >> 	losetup -d /dev/loop0
> >> done
> >>
> >> Without fix, the following IO errors have been observed:
> >>
> >> kernel: __loop_clr_fd: partition scan of loop0 failed (rc=-16)
> >> kernel: I/O error, dev loop0, sector 20971392 op 0x0:(READ) flags 0x80700
> >>          phys_seg 1 prio class 0
> >> kernel: I/O error, dev loop0, sector 108868 op 0x0:(READ) flags 0x0
> >>          phys_seg 1 prio class 0
> >> kernel: Buffer I/O error on dev loop0p1, logical block 27201, async page
> >>          read
> >>
> >> V1->V2:
> >> 	Added a test case, 010, in blktests in tests/loop/
> >> Signed-off-by: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>
> >> ---
> >>   drivers/block/loop.c | 19 +++++++++++++++++++
> >>   1 file changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c index
> >> 28a95fd366fe..9a235d8c062d 100644
> >> --- a/drivers/block/loop.c
> >> +++ b/drivers/block/loop.c
> >> @@ -1717,6 +1717,24 @@ static int lo_compat_ioctl(struct block_device
> *bdev, blk_mode_t mode,
> >>   }
> >>   #endif
> >>
> >> +static int lo_open(struct gendisk *disk, blk_mode_t mode) {
> >> +        struct loop_device *lo = disk->private_data;
> >> +        int err;
> >> +
> >> +        if (!lo)
> >> +                return -ENXIO;
> >> +
> >> +        err = mutex_lock_killable(&lo->lo_mutex);
> >> +        if (err)
> >> +                return err;
> >> +
> >> +        if (lo->lo_state == Lo_rundown)
> >> +                err = -ENXIO;
> >> +        mutex_unlock(&lo->lo_mutex);
> 
> This doesn't fix the problem completely, there is still a race window.
> 
> lo_release
>   if (disk_openers(disk) > 0)
>    reutrn
>    -> no openers now
> 		lo_open
> 		 if (lo->lo_state == Lo_rundown)
> 		 -> no set yet
> 		 open succeed
>   mutex_lock(lo_mutex)
>   lo->lo_state = Lo_rundown
>   mutex_unlock(lo_mutex)
>   __loop_clr_fd
We have noticed that, at block layer, both open() and release() are protected by gendisk->open_mutex.
So, this race may not happen. Can you please let me know if I understand correctly?
> 
> And with the respect, loop_clr_fd() has the same problem.
> 
> I think probably loop need a open counter for itself.
We are looking to see how to handle this case
> 
> Thanks,
> Kuai
> 
> >> +	return err;
> >> +}
> >
> > Most of this function uses spaces rather than tabs.
> >