Skip to content

Commit

Permalink
loop: revert "make autoclear operation asynchronous"
Browse files Browse the repository at this point in the history
The kernel test robot is reporting that xfstest which does

  umount ext2 on xfs
  umount xfs

sequence started failing, for commit 322c429 ("loop: make
autoclear operation asynchronous") removed a guarantee that fput() of
backing file is processed before lo_release() from close() returns to
user mode.

And syzbot is reporting that deferring destroy_workqueue() from
__loop_clr_fd() to a WQ context did not help [1]. Revert that commit.

Link: https://syzkaller.appspot.com/bug?extid=831661966588c802aae9 [1]
Reported-by: kernel test robot <oliver.sang@intel.com>
Acked-by: Jan Kara <jack@suse.cz>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reported-by: syzbot <syzbot+831661966588c802aae9@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Link: https://lore.kernel.org/r/20220211071554.3424-1-penguin-kernel@I-love.SAKURA.ne.jp
Signed-off-by: Jens Axboe <axboe@kernel.dk>
  • Loading branch information
Tetsuo Handa authored and Jens Axboe committed Feb 11, 2022
1 parent 93e2c52 commit bf23747
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 37 deletions.
65 changes: 29 additions & 36 deletions drivers/block/loop.c
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
return error;
}

static void __loop_clr_fd(struct loop_device *lo)
static void __loop_clr_fd(struct loop_device *lo, bool release)
{
struct file *filp;
gfp_t gfp = lo->old_gfp_mask;
Expand Down Expand Up @@ -1144,59 +1144,53 @@ static void __loop_clr_fd(struct loop_device *lo)
/* let user-space know about this change */
kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
mapping_set_gfp_mask(filp->f_mapping, gfp);
/* This is safe: open() is still holding a reference. */
module_put(THIS_MODULE);
blk_mq_unfreeze_queue(lo->lo_queue);

disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);

if (lo->lo_flags & LO_FLAGS_PARTSCAN) {
int err;

mutex_lock(&lo->lo_disk->open_mutex);
/*
* open_mutex has been held already in release path, so don't
* acquire it if this function is called in such case.
*
* If the reread partition isn't from release path, lo_refcnt
* must be at least one and it can only become zero when the
* current holder is released.
*/
if (!release)
mutex_lock(&lo->lo_disk->open_mutex);
err = bdev_disk_changed(lo->lo_disk, false);
mutex_unlock(&lo->lo_disk->open_mutex);
if (!release)
mutex_unlock(&lo->lo_disk->open_mutex);
if (err)
pr_warn("%s: partition scan of loop%d failed (rc=%d)\n",
__func__, lo->lo_number, err);
/* Device is gone, no point in returning error */
}

/*
* lo->lo_state is set to Lo_unbound here after above partscan has
* finished. There cannot be anybody else entering __loop_clr_fd() as
* Lo_rundown state protects us from all the other places trying to
* change the 'lo' device.
*/
lo->lo_flags = 0;
if (!part_shift)
lo->lo_disk->flags |= GENHD_FL_NO_PART;

fput(filp);
}

static void loop_rundown_completed(struct loop_device *lo)
{
mutex_lock(&lo->lo_mutex);
lo->lo_state = Lo_unbound;
mutex_unlock(&lo->lo_mutex);
module_put(THIS_MODULE);
}

static void loop_rundown_workfn(struct work_struct *work)
{
struct loop_device *lo = container_of(work, struct loop_device,
rundown_work);
struct block_device *bdev = lo->lo_device;
struct gendisk *disk = lo->lo_disk;

__loop_clr_fd(lo);
kobject_put(&bdev->bd_device.kobj);
module_put(disk->fops->owner);
loop_rundown_completed(lo);
}

static void loop_schedule_rundown(struct loop_device *lo)
{
struct block_device *bdev = lo->lo_device;
struct gendisk *disk = lo->lo_disk;

__module_get(disk->fops->owner);
kobject_get(&bdev->bd_device.kobj);
INIT_WORK(&lo->rundown_work, loop_rundown_workfn);
queue_work(system_long_wq, &lo->rundown_work);
/*
* Need not hold lo_mutex to fput backing file. Calling fput holding
* lo_mutex triggers a circular lock dependency possibility warning as
* fput can take open_mutex which is usually taken before lo_mutex.
*/
fput(filp);
}

static int loop_clr_fd(struct loop_device *lo)
Expand Down Expand Up @@ -1228,8 +1222,7 @@ static int loop_clr_fd(struct loop_device *lo)
lo->lo_state = Lo_rundown;
mutex_unlock(&lo->lo_mutex);

__loop_clr_fd(lo);
loop_rundown_completed(lo);
__loop_clr_fd(lo, false);
return 0;
}

Expand Down Expand Up @@ -1754,7 +1747,7 @@ static void lo_release(struct gendisk *disk, fmode_t mode)
* In autoclear mode, stop the loop thread
* and remove configuration after last close.
*/
loop_schedule_rundown(lo);
__loop_clr_fd(lo, true);
return;
} else if (lo->lo_state == Lo_bound) {
/*
Expand Down
1 change: 0 additions & 1 deletion drivers/block/loop.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ struct loop_device {
struct gendisk *lo_disk;
struct mutex lo_mutex;
bool idr_visible;
struct work_struct rundown_work;
};

struct loop_cmd {
Expand Down

0 comments on commit bf23747

Please sign in to comment.