* [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
@ 2024-05-21 22:42 Gulam Mohamed
2024-05-21 22:42 ` [PATCH 2/2] loop: Test to detect a race condition between loop detach and open Gulam Mohamed
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Gulam Mohamed @ 2024-05-21 22:42 UTC (permalink / raw)
To: linux-block, linux-kernel; +Cc: axboe, shinichiro.kawasaki, chaitanyak, hch
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@oracle.com>
---
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);
+ return err;
+}
+
static void lo_release(struct gendisk *disk)
{
struct loop_device *lo = disk->private_data;
@@ -1752,6 +1770,7 @@ static void lo_free_disk(struct gendisk *disk)
static const struct block_device_operations lo_fops = {
.owner = THIS_MODULE,
+ .open = lo_open,
.release = lo_release,
.ioctl = lo_ioctl,
#ifdef CONFIG_COMPAT
--
2.39.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] loop: Test to detect a race condition between loop detach and open
2024-05-21 22:42 [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Gulam Mohamed
@ 2024-05-21 22:42 ` Gulam Mohamed
2024-05-23 8:12 ` Christoph Hellwig
2024-05-23 11:14 ` Shinichiro Kawasaki
2024-05-22 1:39 ` [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Jens Axboe
2024-05-23 8:12 ` Christoph Hellwig
2 siblings, 2 replies; 12+ messages in thread
From: Gulam Mohamed @ 2024-05-21 22:42 UTC (permalink / raw)
To: linux-block, linux-kernel; +Cc: axboe, shinichiro.kawasaki, chaitanyak, hch
When one process opens a loop device partition and another process detaches
it, there will be a race condition due to which stale loop partitions are
created causing IO errors. This test will detect the race
Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
---
tests/loop/010 | 90 ++++++++++++++++++++++++++++++++++++++++++++++
tests/loop/010.out | 2 ++
2 files changed, 92 insertions(+)
create mode 100755 tests/loop/010
create mode 100644 tests/loop/010.out
diff --git a/tests/loop/010 b/tests/loop/010
new file mode 100755
index 000000000000..ea93a120d51a
--- /dev/null
+++ b/tests/loop/010
@@ -0,0 +1,90 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024, Oracle and/or its affiliates.
+#
+# Test to detect a race between loop detach and loop open which creates
+# stale loop partitions when one process opens the loop partition and
+# another process detaches the loop device
+#
+. tests/loop/rc
+DESCRIPTION="check stale loop partition"
+TIMED=1
+
+requires() {
+ _have_program parted
+ _have_program mkfs.xfs
+}
+
+image_file="/tmp/loopImg"
+line1="partition scan of loop0 failed (rc=-16)"
+
+function create_loop()
+{
+ while [ 1 ]
+ do
+ loop_device="$(losetup -P -f --show "${image_file}")"
+ blkid /dev/loop0p1 >> /dev/null 2>&1
+ done
+}
+
+function detach_loop()
+{
+ while [ 1 ]
+ do
+ if [ -e /dev/loop0 ]; then
+ losetup -d /dev/loop0 > /dev/null 2>&1
+ fi
+ done
+}
+
+test() {
+ echo "Running ${TEST_NAME}"
+ local failure="/tmp/failure"
+ touch $failure
+ echo 0 > $failure
+ dmesg -c > /dev/null 2>&1
+
+ truncate -s 2G "${image_file}"
+ parted -a none -s "${image_file}" mklabel gpt
+ loop_device="$(losetup -P -f --show "${image_file}")"
+ parted -a none -s "${loop_device}" mkpart primary 64s 109051s
+
+ udevadm settle
+ if [ ! -e "${loop_device}" ]; then
+ return 1
+ fi
+
+ mkfs.xfs -f "${loop_device}p1" > /dev/null 2>&1
+
+ losetup -d "${loop_device}" > /dev/null 2>&1
+
+ create_loop &
+ create_pid=$!
+ detach_loop &
+ detach_pid=$!
+
+ sleep "${TIMEOUT:-180}"
+ {
+ kill -9 $create_pid
+ kill -9 $detach_pid
+ wait
+ sleep 1
+ } 2>/dev/null
+
+ losetup -D > /dev/null 2>&1
+ dmesg | while IFS= read -r line2
+ do
+ match=$(echo "$line2" | grep -o "$line1")
+ if [ "$line1" == "$match" ]; then
+ echo 1 > "/tmp/failure"
+ break
+ fi
+ done
+ failed=$(cat $failure)
+ if [ $failed -eq 0 ]; then
+ echo "Test complete"
+ else
+ echo "Test failed"
+ fi
+ rm -f $failure
+}
diff --git a/tests/loop/010.out b/tests/loop/010.out
new file mode 100644
index 000000000000..64a6aee00b8a
--- /dev/null
+++ b/tests/loop/010.out
@@ -0,0 +1,2 @@
+Running loop/010
+Test complete
--
2.39.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-21 22:42 [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Gulam Mohamed
2024-05-21 22:42 ` [PATCH 2/2] loop: Test to detect a race condition between loop detach and open Gulam Mohamed
@ 2024-05-22 1:39 ` Jens Axboe
2024-05-22 2:31 ` Yu Kuai
2024-05-22 19:11 ` Gulam Mohamed
2024-05-23 8:12 ` Christoph Hellwig
2 siblings, 2 replies; 12+ messages in thread
From: Jens Axboe @ 2024-05-22 1:39 UTC (permalink / raw)
To: Gulam Mohamed, linux-block, linux-kernel
Cc: shinichiro.kawasaki, chaitanyak, hch
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@oracle.com>
> ---
> 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);
> + return err;
> +}
Most of this function uses spaces rather than tabs.
--
Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-22 1:39 ` [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Jens Axboe
@ 2024-05-22 2:31 ` Yu Kuai
2024-05-22 19:12 ` Gulam Mohamed
2024-05-22 19:11 ` Gulam Mohamed
1 sibling, 1 reply; 12+ messages in thread
From: Yu Kuai @ 2024-05-22 2:31 UTC (permalink / raw)
To: Jens Axboe, Gulam Mohamed, linux-block, linux-kernel
Cc: shinichiro.kawasaki, chaitanyak, hch, yukuai (C)
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@oracle.com>
>> ---
>> 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
And with the respect, loop_clr_fd() has the same problem.
I think probably loop need a open counter for itself.
Thanks,
Kuai
>> + return err;
>> +}
>
> Most of this function uses spaces rather than tabs.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-22 1:39 ` [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Jens Axboe
2024-05-22 2:31 ` Yu Kuai
@ 2024-05-22 19:11 ` Gulam Mohamed
1 sibling, 0 replies; 12+ messages in thread
From: Gulam Mohamed @ 2024-05-22 19:11 UTC (permalink / raw)
To: Jens Axboe, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: shinichiro.kawasaki@wdc.com, chaitanyak@nvidia.com, hch@lst.de
Hi Jens,
> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Wednesday, May 22, 2024 7:10 AM
> To: Gulam Mohamed <gulam.mohamed@oracle.com>; linux-
> block@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: shinichiro.kawasaki@wdc.com; chaitanyak@nvidia.com; hch@lst.de
> Subject: Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop
> detach and loop open
>
> 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@oracle.com>
> > ---
> > 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);
> > + return err;
> > +}
>
> Most of this function uses spaces rather than tabs.
I am sorry I should have checked that. Will address the issue in next version
>
> --
> Jens Axboe
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-22 2:31 ` Yu Kuai
@ 2024-05-22 19:12 ` Gulam Mohamed
2024-05-23 1:13 ` Yu Kuai
0 siblings, 1 reply; 12+ messages in thread
From: Gulam Mohamed @ 2024-05-22 19:12 UTC (permalink / raw)
To: Yu Kuai, Jens Axboe, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: shinichiro.kawasaki@wdc.com, chaitanyak@nvidia.com, hch@lst.de,
yukuai (C)
Hi Kuai,
> -----Original Message-----
> From: Yu Kuai <yukuai1@huaweicloud.com>
> Sent: Wednesday, May 22, 2024 8:01 AM
> To: Jens Axboe <axboe@kernel.dk>; Gulam Mohamed
> <gulam.mohamed@oracle.com>; linux-block@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: shinichiro.kawasaki@wdc.com; chaitanyak@nvidia.com; hch@lst.de;
> yukuai (C) <yukuai3@huawei.com>
> 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@oracle.com>
> >> ---
> >> 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.
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-22 19:12 ` Gulam Mohamed
@ 2024-05-23 1:13 ` Yu Kuai
0 siblings, 0 replies; 12+ messages in thread
From: Yu Kuai @ 2024-05-23 1:13 UTC (permalink / raw)
To: Gulam Mohamed, Yu Kuai, Jens Axboe, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: shinichiro.kawasaki@wdc.com, chaitanyak@nvidia.com, hch@lst.de,
yukuai (C)
Hi,
在 2024/05/23 3:12, Gulam Mohamed 写道:
> Hi Kuai,
>
>> -----Original Message-----
>> From: Yu Kuai <yukuai1@huaweicloud.com>
>> Sent: Wednesday, May 22, 2024 8:01 AM
>> To: Jens Axboe <axboe@kernel.dk>; Gulam Mohamed
>> <gulam.mohamed@oracle.com>; linux-block@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Cc: shinichiro.kawasaki@wdc.com; chaitanyak@nvidia.com; hch@lst.de;
>> yukuai (C) <yukuai3@huawei.com>
>> 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@oracle.com>
>>>> ---
>>>> 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?
Yes, __loop_clr_fd from lo_release can't concurrent with lo_open.
>>
>> And with the respect, loop_clr_fd() has the same problem.
Did you check __loop_clr_fd from lo_ioctl?
Thanks,
Kuai
>>
>> 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.
>>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-21 22:42 [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Gulam Mohamed
2024-05-21 22:42 ` [PATCH 2/2] loop: Test to detect a race condition between loop detach and open Gulam Mohamed
2024-05-22 1:39 ` [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Jens Axboe
@ 2024-05-23 8:12 ` Christoph Hellwig
2024-05-23 18:39 ` Gulam Mohamed
2 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2024-05-23 8:12 UTC (permalink / raw)
To: Gulam Mohamed
Cc: linux-block, linux-kernel, axboe, shinichiro.kawasaki, chaitanyak,
hch
On Tue, May 21, 2024 at 10:42:48PM +0000, Gulam Mohamed wrote:
> Description
> ===========
That's a weird way to format a patch. Between this and the odd subject
not matching patch 2 I was tricked into thinking this was just a cover
letter and patch 1 was missing for a while. Please take a look at other
patches/commit and try to word it similarly.
> V1->V2:
> Added a test case, 010, in blktests in tests/loop/
These kind of patch revision changelogs belong after the --- so that they
don't go into git history. Or even better into the cover letter, which
is missing here.
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> ---
> 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;
->private_data is never cleared, so the NULL check here doesn't
make sense.
> + err = mutex_lock_killable(&lo->lo_mutex);
> + if (err)
> + return err;
> +
> + if (lo->lo_state == Lo_rundown)
> + err = -ENXIO;
> + mutex_unlock(&lo->lo_mutex);
What if we race with setting Lo_rundown and that gets set right
after we unlock here?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] loop: Test to detect a race condition between loop detach and open
2024-05-21 22:42 ` [PATCH 2/2] loop: Test to detect a race condition between loop detach and open Gulam Mohamed
@ 2024-05-23 8:12 ` Christoph Hellwig
2024-05-23 11:14 ` Shinichiro Kawasaki
1 sibling, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2024-05-23 8:12 UTC (permalink / raw)
To: Gulam Mohamed
Cc: linux-block, linux-kernel, axboe, shinichiro.kawasaki, chaitanyak,
hch
On Tue, May 21, 2024 at 10:42:49PM +0000, Gulam Mohamed wrote:
> When one process opens a loop device partition and another process detaches
> it, there will be a race condition due to which stale loop partitions are
> created causing IO errors. This test will detect the race
This isn't really a 2/2, but a single patch for a separate repository.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] loop: Test to detect a race condition between loop detach and open
2024-05-21 22:42 ` [PATCH 2/2] loop: Test to detect a race condition between loop detach and open Gulam Mohamed
2024-05-23 8:12 ` Christoph Hellwig
@ 2024-05-23 11:14 ` Shinichiro Kawasaki
2024-05-27 9:56 ` [PATCH blktests] " Gulam Mohamed
1 sibling, 1 reply; 12+ messages in thread
From: Shinichiro Kawasaki @ 2024-05-23 11:14 UTC (permalink / raw)
To: Gulam Mohamed
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
axboe@kernel.dk, chaitanyak@nvidia.com, hch@lst.de
On May 21, 2024 / 22:42, Gulam Mohamed wrote:
> When one process opens a loop device partition and another process detaches
> it, there will be a race condition due to which stale loop partitions are
> created causing IO errors. This test will detect the race
Hello Gulam, thanks for the patch. I ran the new test case on my system and
observed it failed. It looks working as expected.
This is the patch not for Linux kernel but for blktests, then it's the better to
separate it out. I suggest to add the subject title prefix "[PATCH blktests]"
to this patch so that reviewers can tell the target repository easily.
I suggest to take a look in the "new" script. It has some guides for blktests,
such as "Indent with tabs" or the global variable "TMPDIR". Also, please run
"make check" which runs shellcheck.
$ make check
shellcheck -x -e SC2119 -f gcc check common/* \
tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
tests/loop/010:23:17: note: Instead of '[ 1 ]', use 'true'. [SC2161]
tests/loop/010:32:17: note: Instead of '[ 1 ]', use 'true'. [SC2161]
tests/loop/010:84:7: note: Double quote to prevent globbing and word splitting. [SC2086]
make: *** [Makefile:21: check] Error 1
Please address the shellcheck warnings above. It will help us to keep script
quality at some level.
>
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> ---
> tests/loop/010 | 90 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/loop/010.out | 2 ++
> 2 files changed, 92 insertions(+)
> create mode 100755 tests/loop/010
> create mode 100644 tests/loop/010.out
>
> diff --git a/tests/loop/010 b/tests/loop/010
> new file mode 100755
> index 000000000000..ea93a120d51a
> --- /dev/null
> +++ b/tests/loop/010
> @@ -0,0 +1,90 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024, Oracle and/or its affiliates.
> +#
> +# Test to detect a race between loop detach and loop open which creates
> +# stale loop partitions when one process opens the loop partition and
> +# another process detaches the loop device
> +#
> +. tests/loop/rc
> +DESCRIPTION="check stale loop partition"
> +TIMED=1
> +
> +requires() {
> + _have_program parted
> + _have_program mkfs.xfs
> +}
> +
> +image_file="/tmp/loopImg"
$TMPDIR is recommended instead of /tmp. $TMPDIR is cleaned up after test run.
> +line1="partition scan of loop0 failed (rc=-16)"
> +
> +function create_loop()
Nit: the "function" keyword is not used in blktests (there is one exception
though).
> +{
> + while [ 1 ]
> + do
> + loop_device="$(losetup -P -f --show "${image_file}")"
> + blkid /dev/loop0p1 >> /dev/null 2>&1
> + done
> +}
> +
> +function detach_loop()
> +{
> + while [ 1 ]
> + do
> + if [ -e /dev/loop0 ]; then
> + losetup -d /dev/loop0 > /dev/null 2>&1
> + fi
> + done
> +}
> +
> +test() {
> + echo "Running ${TEST_NAME}"
> + local failure="/tmp/failure"
Nit: some local variables are declared with "local" keyword, and some other are
not.
> + touch $failure
> + echo 0 > $failure
Why the file is required to keep the fail/pass status? Just a bash variable
looks enough.
> + dmesg -c > /dev/null 2>&1
This kernel ring buffer clear will likely affect test harnesses which run
blktests. To check the kernel message generated during each test case run,
please use _dmesg_since_test_start(): ref nbd/004, nvme/003.
> +
> + truncate -s 2G "${image_file}"
Other test cases in loop group have 1G file size. Is 2G required for this test?
If not, 1G size would be the better to reduce disk shortage risk.
> + parted -a none -s "${image_file}" mklabel gpt
> + loop_device="$(losetup -P -f --show "${image_file}")"
> + parted -a none -s "${loop_device}" mkpart primary 64s 109051s
> +
> + udevadm settle
> + if [ ! -e "${loop_device}" ]; then
> + return 1
> + fi
> +
> + mkfs.xfs -f "${loop_device}p1" > /dev/null 2>&1
> +
> + losetup -d "${loop_device}" > /dev/null 2>&1
> +
> + create_loop &
> + create_pid=$!
> + detach_loop &
> + detach_pid=$!
> +
> + sleep "${TIMEOUT:-180}"
The default 180 seconds sounds a bit long. How about 30 seconds?
> + {
> + kill -9 $create_pid
> + kill -9 $detach_pid
> + wait
> + sleep 1
> + } 2>/dev/null
> +
> + losetup -D > /dev/null 2>&1
> + dmesg | while IFS= read -r line2
> + do
> + match=$(echo "$line2" | grep -o "$line1")
> + if [ "$line1" == "$match" ]; then
> + echo 1 > "/tmp/failure"
> + break
> + fi
> + done
> + failed=$(cat $failure)
> + if [ $failed -eq 0 ]; then
> + echo "Test complete"
> + else
> + echo "Test failed"
> + fi
> + rm -f $failure
> +}
> diff --git a/tests/loop/010.out b/tests/loop/010.out
> new file mode 100644
> index 000000000000..64a6aee00b8a
> --- /dev/null
> +++ b/tests/loop/010.out
> @@ -0,0 +1,2 @@
> +Running loop/010
> +Test complete
> --
> 2.39.3
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open
2024-05-23 8:12 ` Christoph Hellwig
@ 2024-05-23 18:39 ` Gulam Mohamed
0 siblings, 0 replies; 12+ messages in thread
From: Gulam Mohamed @ 2024-05-23 18:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
axboe@kernel.dk, shinichiro.kawasaki@wdc.com,
chaitanyak@nvidia.com
Hi,
> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Thursday, May 23, 2024 1:42 PM
> To: Gulam Mohamed <gulam.mohamed@oracle.com>
> Cc: linux-block@vger.kernel.org; linux-kernel@vger.kernel.org;
> axboe@kernel.dk; shinichiro.kawasaki@wdc.com; chaitanyak@nvidia.com;
> hch@lst.de
> Subject: Re: [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop
> detach and loop open
>
> On Tue, May 21, 2024 at 10:42:48PM +0000, Gulam Mohamed wrote:
> > Description
> > ===========
>
> That's a weird way to format a patch. Between this and the odd subject not
> matching patch 2 I was tricked into thinking this was just a cover letter and
> patch 1 was missing for a while. Please take a look at other patches/commit
> and try to word it similarly.
I will take care of this in the next version.
>
> > V1->V2:
> > Added a test case, 010, in blktests in tests/loop/
>
> These kind of patch revision changelogs belong after the --- so that they don't
> go into git history. Or even better into the cover letter, which is missing here.
>
Sure. I will take care of this in the next version.
> > Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> > ---
> > 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;
>
> ->private_data is never cleared, so the NULL check here doesn't
> make sense.
>
> > + err = mutex_lock_killable(&lo->lo_mutex);
> > + if (err)
> > + return err;
> > +
> > + if (lo->lo_state == Lo_rundown)
> > + err = -ENXIO;
> > + mutex_unlock(&lo->lo_mutex);
>
> What if we race with setting Lo_rundown and that gets set right after we
> unlock here?
Similar race was mentioned by Kuai in his comments. We think these race conditions can be resolved by bringing back the "lo->refcnt" ,
by reverting the commit a0e286b6a5b61d4da01bdf865071c4da417046d6 plus the above Lo_rundown check in lo_open.
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH blktests] loop: Test to detect a race condition between loop detach and open
2024-05-23 11:14 ` Shinichiro Kawasaki
@ 2024-05-27 9:56 ` Gulam Mohamed
0 siblings, 0 replies; 12+ messages in thread
From: Gulam Mohamed @ 2024-05-27 9:56 UTC (permalink / raw)
To: Shinichiro Kawasaki
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
axboe@kernel.dk, chaitanyak@nvidia.com, hch@lst.de
Hi,
> -----Original Message-----
> From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Sent: Thursday, May 23, 2024 4:45 PM
> To: Gulam Mohamed <gulam.mohamed@oracle.com>
> Cc: linux-block@vger.kernel.org; linux-kernel@vger.kernel.org;
> axboe@kernel.dk; chaitanyak@nvidia.com; hch@lst.de
> Subject: Re: [PATCH 2/2] loop: Test to detect a race condition between loop
> detach and open
>
> On May 21, 2024 / 22:42, Gulam Mohamed wrote:
> > When one process opens a loop device partition and another process
> > detaches it, there will be a race condition due to which stale loop
> > partitions are created causing IO errors. This test will detect the
> > race
>
> Hello Gulam, thanks for the patch. I ran the new test case on my system and
> observed it failed. It looks working as expected.
>
> This is the patch not for Linux kernel but for blktests, then it's the better to
> separate it out. I suggest to add the subject title prefix "[PATCH blktests]"
> to this patch so that reviewers can tell the target repository easily.
>
Done. Changed the subject line prefix to "[PATCH blktests]"
> I suggest to take a look in the "new" script. It has some guides for blktests,
> such as "Indent with tabs" or the global variable "TMPDIR". Also, please run
> "make check" which runs shellcheck.
>
> $ make check
> shellcheck -x -e SC2119 -f gcc check common/* \
> tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
> tests/loop/010:23:17: note: Instead of '[ 1 ]', use 'true'. [SC2161]
> tests/loop/010:32:17: note: Instead of '[ 1 ]', use 'true'. [SC2161]
> tests/loop/010:84:7: note: Double quote to prevent globbing and word
> splitting. [SC2086]
> make: *** [Makefile:21: check] Error 1
>
> Please address the shellcheck warnings above. It will help us to keep script
> quality at some level.
Done. Resolved the warnings
>
> >
> > Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
> > ---
> > tests/loop/010 | 90
> ++++++++++++++++++++++++++++++++++++++++++++++
> > tests/loop/010.out | 2 ++
> > 2 files changed, 92 insertions(+)
> > create mode 100755 tests/loop/010
> > create mode 100644 tests/loop/010.out
> >
> > diff --git a/tests/loop/010 b/tests/loop/010 new file mode 100755
> > index 000000000000..ea93a120d51a
> > --- /dev/null
> > +++ b/tests/loop/010
> > @@ -0,0 +1,90 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-3.0+
> > +# Copyright (C) 2024, Oracle and/or its affiliates.
> > +#
> > +# Test to detect a race between loop detach and loop open which
> > +creates # stale loop partitions when one process opens the loop
> > +partition and # another process detaches the loop device # .
> > +tests/loop/rc DESCRIPTION="check stale loop partition"
> > +TIMED=1
> > +
> > +requires() {
> > + _have_program parted
> > + _have_program mkfs.xfs
> > +}
> > +
> > +image_file="/tmp/loopImg"
>
> $TMPDIR is recommended instead of /tmp. $TMPDIR is cleaned up after test
> run.
Done. Using $TMPDIR now.
>
> > +line1="partition scan of loop0 failed (rc=-16)"
> > +
> > +function create_loop()
>
> Nit: the "function" keyword is not used in blktests (there is one exception
> though).
>
Done. Removed the keyword "function" now
> > +{
> > + while [ 1 ]
> > + do
> > + loop_device="$(losetup -P -f --show "${image_file}")"
> > + blkid /dev/loop0p1 >> /dev/null 2>&1
> > + done
> > +}
> > +
> > +function detach_loop()
> > +{
> > + while [ 1 ]
> > + do
> > + if [ -e /dev/loop0 ]; then
> > + losetup -d /dev/loop0 > /dev/null 2>&1
> > + fi
> > + done
> > +}
> > +
> > +test() {
> > + echo "Running ${TEST_NAME}"
> > + local failure="/tmp/failure"
>
> Nit: some local variables are declared with "local" keyword, and some other
> are not.
Done. Using the "local" keyword for all local variables now
>
> > + touch $failure
> > + echo 0 > $failure
>
> Why the file is required to keep the fail/pass status? Just a bash variable looks
> enough.
In the beginning I tried with a bash variable but it was not working as the variable need to be accessed by other two child threads also.
>
> > + dmesg -c > /dev/null 2>&1
>
> This kernel ring buffer clear will likely affect test harnesses which run blktests.
> To check the kernel message generated during each test case run, please use
> _dmesg_since_test_start(): ref nbd/004, nvme/003.
Done. Used the above function
>
> > +
> > + truncate -s 2G "${image_file}"
>
> Other test cases in loop group have 1G file size. Is 2G required for this test?
> If not, 1G size would be the better to reduce disk shortage risk.
>
Using 1G is not an issue. Done
> > + parted -a none -s "${image_file}" mklabel gpt
> > + loop_device="$(losetup -P -f --show "${image_file}")"
> > + parted -a none -s "${loop_device}" mkpart primary 64s 109051s
> > +
> > + udevadm settle
> > + if [ ! -e "${loop_device}" ]; then
> > + return 1
> > + fi
> > +
> > + mkfs.xfs -f "${loop_device}p1" > /dev/null 2>&1
> > +
> > + losetup -d "${loop_device}" > /dev/null 2>&1
> > +
> > + create_loop &
> > + create_pid=$!
> > + detach_loop &
> > + detach_pid=$!
> > +
> > + sleep "${TIMEOUT:-180}"
>
> The default 180 seconds sounds a bit long. How about 30 seconds?
Using 180 seconds is just to give more time to reproduce the issue as 30 seconds may not be sufficient for the race condition to occur
Will send the version 2 as soon as possible, with all these changes
>
> > + {
> > + kill -9 $create_pid
> > + kill -9 $detach_pid
> > + wait
> > + sleep 1
> > + } 2>/dev/null
> > +
> > + losetup -D > /dev/null 2>&1
> > + dmesg | while IFS= read -r line2
> > + do
> > + match=$(echo "$line2" | grep -o "$line1")
> > + if [ "$line1" == "$match" ]; then
> > + echo 1 > "/tmp/failure"
> > + break
> > + fi
> > + done
> > + failed=$(cat $failure)
> > + if [ $failed -eq 0 ]; then
> > + echo "Test complete"
> > + else
> > + echo "Test failed"
> > + fi
> > + rm -f $failure
> > +}
> > diff --git a/tests/loop/010.out b/tests/loop/010.out new file mode
> > 100644 index 000000000000..64a6aee00b8a
> > --- /dev/null
> > +++ b/tests/loop/010.out
> > @@ -0,0 +1,2 @@
> > +Running loop/010
> > +Test complete
> > --
> > 2.39.3
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-05-27 9:56 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-21 22:42 [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Gulam Mohamed
2024-05-21 22:42 ` [PATCH 2/2] loop: Test to detect a race condition between loop detach and open Gulam Mohamed
2024-05-23 8:12 ` Christoph Hellwig
2024-05-23 11:14 ` Shinichiro Kawasaki
2024-05-27 9:56 ` [PATCH blktests] " Gulam Mohamed
2024-05-22 1:39 ` [PATCH V2 for-6.10/block 1/2] loop: Fix a race between loop detach and loop open Jens Axboe
2024-05-22 2:31 ` Yu Kuai
2024-05-22 19:12 ` Gulam Mohamed
2024-05-23 1:13 ` Yu Kuai
2024-05-22 19:11 ` Gulam Mohamed
2024-05-23 8:12 ` Christoph Hellwig
2024-05-23 18:39 ` Gulam Mohamed
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox