| Message ID | 1550763223-21764-3-git-send-email-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | loop: fix two issues introduced by prior commit | expand |
On Thu 21-02-19 23:33:43, Dongli Zhang wrote: > Commit 0da03cab87e6 > ("loop: Fix deadlock when calling blkdev_reread_part()") moves > blkdev_reread_part() out of the loop_ctl_mutex. However, > GENHD_FL_NO_PART_SCAN is set before __blkdev_reread_part(). As a result, > __blkdev_reread_part() will fail the check of GENHD_FL_NO_PART_SCAN and > will not rescan the loop device to delete all partitions. > > Below are steps to reproduce the issue: > > step1 # dd if=/dev/zero of=tmp.raw bs=1M count=100 > step2 # losetup -P /dev/loop0 tmp.raw > step3 # parted /dev/loop0 mklabel gpt > step4 # parted -a none -s /dev/loop0 mkpart primary 64s 1 > step5 # losetup -d /dev/loop0 > > Step5 will not be able to delete /dev/loop0p1 (introduced by step4) and > there is below kernel warning message: > > [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) > > This patch sets GENHD_FL_NO_PART_SCAN after blkdev_reread_part(). > > Fixes: 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > Changed since v1: > * move the setting of lo->lo_state to Lo_unbound after partscan has finished as well > (suggested by Jan Kara) > > drivers/block/loop.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) Thanks the patch looks good! Just one nit below: > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 7908673..a13f5dc 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1034,6 +1034,16 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer, > return err; > } > > +static void loop_disable_partscan(struct loop_device *lo) > +{ I don't think there's any benefit in having this small function with a single caller and furthermore with the subtle sideeffect that it changes lo_state. So I'd just put the code inline in __loop_clr_fd(). With that you can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On 02/22/2019 07:47 PM, Jan Kara wrote: > On Thu 21-02-19 23:33:43, Dongli Zhang wrote: >> Commit 0da03cab87e6 >> ("loop: Fix deadlock when calling blkdev_reread_part()") moves >> blkdev_reread_part() out of the loop_ctl_mutex. However, >> GENHD_FL_NO_PART_SCAN is set before __blkdev_reread_part(). As a result, >> __blkdev_reread_part() will fail the check of GENHD_FL_NO_PART_SCAN and >> will not rescan the loop device to delete all partitions. >> >> Below are steps to reproduce the issue: >> >> step1 # dd if=/dev/zero of=tmp.raw bs=1M count=100 >> step2 # losetup -P /dev/loop0 tmp.raw >> step3 # parted /dev/loop0 mklabel gpt >> step4 # parted -a none -s /dev/loop0 mkpart primary 64s 1 >> step5 # losetup -d /dev/loop0 >> >> Step5 will not be able to delete /dev/loop0p1 (introduced by step4) and >> there is below kernel warning message: >> >> [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) >> >> This patch sets GENHD_FL_NO_PART_SCAN after blkdev_reread_part(). >> >> Fixes: 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") >> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> >> --- >> Changed since v1: >> * move the setting of lo->lo_state to Lo_unbound after partscan has finished as well >> (suggested by Jan Kara) >> >> drivers/block/loop.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) > > Thanks the patch looks good! Just one nit below: > >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >> index 7908673..a13f5dc 100644 >> --- a/drivers/block/loop.c >> +++ b/drivers/block/loop.c >> @@ -1034,6 +1034,16 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer, >> return err; >> } >> >> +static void loop_disable_partscan(struct loop_device *lo) >> +{ > > I don't think there's any benefit in having this small function with a single > caller and furthermore with the subtle sideeffect that it changes lo_state. > So I'd just put the code inline in __loop_clr_fd(). With that you can add: > > Reviewed-by: Jan Kara <jack@suse.cz> Thank you very much! I will send out v3 with the Reviewed-by and put the code inline in __loop_clr_fd(). Dongli Zhang > > Honza >
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 7908673..a13f5dc 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1034,6 +1034,16 @@ loop_init_xfer(struct loop_device *lo, struct loop_func_table *xfer, return err; } +static void loop_disable_partscan(struct loop_device *lo) +{ + mutex_lock(&loop_ctl_mutex); + lo->lo_flags = 0; + if (!part_shift) + lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; + lo->lo_state = Lo_unbound; + mutex_unlock(&loop_ctl_mutex); +} + static int __loop_clr_fd(struct loop_device *lo, bool release) { struct file *filp = NULL; @@ -1089,16 +1099,12 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE); } mapping_set_gfp_mask(filp->f_mapping, gfp); - lo->lo_state = Lo_unbound; /* This is safe: open() is still holding a reference. */ module_put(THIS_MODULE); blk_mq_unfreeze_queue(lo->lo_queue); partscan = lo->lo_flags & LO_FLAGS_PARTSCAN && bdev; lo_number = lo->lo_number; - lo->lo_flags = 0; - if (!part_shift) - lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN; loop_unprepare_queue(lo); out_unlock: mutex_unlock(&loop_ctl_mutex); @@ -1121,6 +1127,18 @@ static int __loop_clr_fd(struct loop_device *lo, bool release) /* Device is gone, no point in returning error */ err = 0; } + + /* + * lo->lo_state is set to Lo_unbound inside loop_disable_partscan() + * here after above partscan has finished. + * + * There cannot be anybody else entering __loop_clr_fd() as + * lo->lo_backing_file is already cleared and Lo_rundown state + * protects us from all the other places trying to change the 'lo' + * device. + */ + loop_disable_partscan(lo); + /* * Need not hold loop_ctl_mutex to fput backing file. * Calling fput holding loop_ctl_mutex triggers a circular
Commit 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") moves blkdev_reread_part() out of the loop_ctl_mutex. However, GENHD_FL_NO_PART_SCAN is set before __blkdev_reread_part(). As a result, __blkdev_reread_part() will fail the check of GENHD_FL_NO_PART_SCAN and will not rescan the loop device to delete all partitions. Below are steps to reproduce the issue: step1 # dd if=/dev/zero of=tmp.raw bs=1M count=100 step2 # losetup -P /dev/loop0 tmp.raw step3 # parted /dev/loop0 mklabel gpt step4 # parted -a none -s /dev/loop0 mkpart primary 64s 1 step5 # losetup -d /dev/loop0 Step5 will not be able to delete /dev/loop0p1 (introduced by step4) and there is below kernel warning message: [ 464.414043] __loop_clr_fd: partition scan of loop0 failed (rc=-22) This patch sets GENHD_FL_NO_PART_SCAN after blkdev_reread_part(). Fixes: 0da03cab87e6 ("loop: Fix deadlock when calling blkdev_reread_part()") Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- Changed since v1: * move the setting of lo->lo_state to Lo_unbound after partscan has finished as well (suggested by Jan Kara) drivers/block/loop.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)