Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

loop: revert "make autoclear operation asynchronous"

The kernel test robot is reporting that xfstest which does

umount ext2 on xfs
umount xfs

sequence started failing, for commit 322c4293ecc58110 ("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>

authored by

Tetsuo Handa and committed by
Jens Axboe
bf23747e 93e2c52d

+29 -37
+29 -36
drivers/block/loop.c
··· 1082 1082 return error; 1083 1083 } 1084 1084 1085 - static void __loop_clr_fd(struct loop_device *lo) 1085 + static void __loop_clr_fd(struct loop_device *lo, bool release) 1086 1086 { 1087 1087 struct file *filp; 1088 1088 gfp_t gfp = lo->old_gfp_mask; ··· 1144 1144 /* let user-space know about this change */ 1145 1145 kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE); 1146 1146 mapping_set_gfp_mask(filp->f_mapping, gfp); 1147 + /* This is safe: open() is still holding a reference. */ 1148 + module_put(THIS_MODULE); 1147 1149 blk_mq_unfreeze_queue(lo->lo_queue); 1148 1150 1149 1151 disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE); ··· 1153 1151 if (lo->lo_flags & LO_FLAGS_PARTSCAN) { 1154 1152 int err; 1155 1153 1156 - mutex_lock(&lo->lo_disk->open_mutex); 1154 + /* 1155 + * open_mutex has been held already in release path, so don't 1156 + * acquire it if this function is called in such case. 1157 + * 1158 + * If the reread partition isn't from release path, lo_refcnt 1159 + * must be at least one and it can only become zero when the 1160 + * current holder is released. 1161 + */ 1162 + if (!release) 1163 + mutex_lock(&lo->lo_disk->open_mutex); 1157 1164 err = bdev_disk_changed(lo->lo_disk, false); 1158 - mutex_unlock(&lo->lo_disk->open_mutex); 1165 + if (!release) 1166 + mutex_unlock(&lo->lo_disk->open_mutex); 1159 1167 if (err) 1160 1168 pr_warn("%s: partition scan of loop%d failed (rc=%d)\n", 1161 1169 __func__, lo->lo_number, err); 1162 1170 /* Device is gone, no point in returning error */ 1163 1171 } 1164 1172 1173 + /* 1174 + * lo->lo_state is set to Lo_unbound here after above partscan has 1175 + * finished. There cannot be anybody else entering __loop_clr_fd() as 1176 + * Lo_rundown state protects us from all the other places trying to 1177 + * change the 'lo' device. 1178 + */ 1165 1179 lo->lo_flags = 0; 1166 1180 if (!part_shift) 1167 1181 lo->lo_disk->flags |= GENHD_FL_NO_PART; 1168 - 1169 - fput(filp); 1170 - } 1171 - 1172 - static void loop_rundown_completed(struct loop_device *lo) 1173 - { 1174 1182 mutex_lock(&lo->lo_mutex); 1175 1183 lo->lo_state = Lo_unbound; 1176 1184 mutex_unlock(&lo->lo_mutex); 1177 - module_put(THIS_MODULE); 1178 - } 1179 1185 1180 - static void loop_rundown_workfn(struct work_struct *work) 1181 - { 1182 - struct loop_device *lo = container_of(work, struct loop_device, 1183 - rundown_work); 1184 - struct block_device *bdev = lo->lo_device; 1185 - struct gendisk *disk = lo->lo_disk; 1186 - 1187 - __loop_clr_fd(lo); 1188 - kobject_put(&bdev->bd_device.kobj); 1189 - module_put(disk->fops->owner); 1190 - loop_rundown_completed(lo); 1191 - } 1192 - 1193 - static void loop_schedule_rundown(struct loop_device *lo) 1194 - { 1195 - struct block_device *bdev = lo->lo_device; 1196 - struct gendisk *disk = lo->lo_disk; 1197 - 1198 - __module_get(disk->fops->owner); 1199 - kobject_get(&bdev->bd_device.kobj); 1200 - INIT_WORK(&lo->rundown_work, loop_rundown_workfn); 1201 - queue_work(system_long_wq, &lo->rundown_work); 1186 + /* 1187 + * Need not hold lo_mutex to fput backing file. Calling fput holding 1188 + * lo_mutex triggers a circular lock dependency possibility warning as 1189 + * fput can take open_mutex which is usually taken before lo_mutex. 1190 + */ 1191 + fput(filp); 1202 1192 } 1203 1193 1204 1194 static int loop_clr_fd(struct loop_device *lo) ··· 1222 1228 lo->lo_state = Lo_rundown; 1223 1229 mutex_unlock(&lo->lo_mutex); 1224 1230 1225 - __loop_clr_fd(lo); 1226 - loop_rundown_completed(lo); 1231 + __loop_clr_fd(lo, false); 1227 1232 return 0; 1228 1233 } 1229 1234 ··· 1747 1754 * In autoclear mode, stop the loop thread 1748 1755 * and remove configuration after last close. 1749 1756 */ 1750 - loop_schedule_rundown(lo); 1757 + __loop_clr_fd(lo, true); 1751 1758 return; 1752 1759 } else if (lo->lo_state == Lo_bound) { 1753 1760 /*
-1
drivers/block/loop.h
··· 56 56 struct gendisk *lo_disk; 57 57 struct mutex lo_mutex; 58 58 bool idr_visible; 59 - struct work_struct rundown_work; 60 59 }; 61 60 62 61 struct loop_cmd {