[PATCH] block: ublk: improve handling device deletion

[PATCH] block: ublk: improve handling device deletion

[Ming Lei]

Inside ublk_ctrl_del_dev(), when the device is removed, we wait
until the device number is freed with holding global lock of
ublk_ctl_mutex, this way isn't friendly from user viewpoint:

1) if device is in-use, the current delete command hangs in
ublk_ctrl_del_dev(), and user can't break from the handling
because wait_event() is used

2) global lock is held, so any new device can't be added and
other old devices can't be removed.

Improve the deleting handling by the following way, suggested by
Nadav:

1) wait without holding the global lock

2) replace wait_event() with wait_event_interruptible()

Reported-by: Nadav Amit <nadav.amit@gmail.com>
Suggested-by: Nadav Amit <nadav.amit@gmail.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index d83fe2c2b3ba..e6eceee44366 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -150,6 +150,7 @@ struct ublk_device {
 
 #define UB_STATE_OPEN		0
 #define UB_STATE_USED		1
+#define UB_STATE_DELETED	2
 	unsigned long		state;
 	int			ub_number;
 
@@ -1804,20 +1805,33 @@ static int ublk_ctrl_del_dev(struct ublk_device **p_ub)
 	if (ret)
 		return ret;
 
-	ublk_remove(ub);
+	if (!test_bit(UB_STATE_DELETED, &ub->state)) {
+		ublk_remove(ub);
+		set_bit(UB_STATE_DELETED, &ub->state);
+	}
 
 	/* Mark the reference as consumed */
 	*p_ub = NULL;
 	ublk_put_device(ub);
+	mutex_unlock(&ublk_ctl_mutex);
 
 	/*
 	 * Wait until the idr is removed, then it can be reused after
 	 * DEL_DEV command is returned.
+	 *
+	 * If we returns because of user interrupt, future delete command
+	 * may come:
+	 *
+	 * - the device number isn't freed, this device won't or needn't
+	 *   be deleted again, since UB_STATE_DELETED is set, and device
+	 *   will be released after the last reference is dropped
+	 *
+	 * - the device number is freed already, we will not find this
+	 *   device via ublk_get_device_from_id()
 	 */
-	wait_event(ublk_idr_wq, ublk_idr_freed(idx));
-	mutex_unlock(&ublk_ctl_mutex);
+	wait_event_interruptible(ublk_idr_wq, ublk_idr_freed(idx));
 
-	return ret;
+	return 0;
 }
 
 static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
-- 
2.38.1

Re: [PATCH] block: ublk: improve handling device deletion

[Ziyang Zhang]

On 2023/2/7 23:07, Ming Lei wrote:
> Inside ublk_ctrl_del_dev(), when the device is removed, we wait
> until the device number is freed with holding global lock of
> ublk_ctl_mutex, this way isn't friendly from user viewpoint:
> 
> 1) if device is in-use, the current delete command hangs in
> ublk_ctrl_del_dev(), and user can't break from the handling
> because wait_event() is used
> 
> 2) global lock is held, so any new device can't be added and
> other old devices can't be removed.
> 
> Improve the deleting handling by the following way, suggested by
> Nadav:
> 
> 1) wait without holding the global lock
> 
> 2) replace wait_event() with wait_event_interruptible()
> 
> Reported-by: Nadav Amit <nadav.amit@gmail.com>
> Suggested-by: Nadav Amit <nadav.amit@gmail.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Hi Ming,

I tried this patch. And the folloing NPE bug was trigged by:

(0) dd if=/dev/zero of=/root/img bs=4096 count=1024768
(1) ublk add -t loop -f /root/img
(2) mkdir -p /root/ublk
(3) mount /dev/ublkb0 /root/ublk
(4) echo "hello" > /root/ublk
(5) ./ublk del -n 0

So I delete the ublk device while it is mounting as
an ext4 filesystem. I think ublk should handle this
by (1) returning -EBUSY or (2) blocking incoming IO.

[  119.115252] EXT4-fs (ublkb0): recovery complete
[  119.115379] EXT4-fs (ublkb0): mounted filesystem c12f15a9-6cb3-44b4-ae39-cbb91e8fd7b8 with ordered data mode. Quota mode: none.
[  144.400128] BUG: kernel NULL pointer dereference, address: 0000000000000038
[  144.400189] #PF: supervisor read access in kernel mode
[  144.400228] #PF: error_code(0x0000) - not-present page
[  144.400266] PGD 105abc067 P4D 105abc067 PUD 1c331b067 PMD 0 
[  144.400312] Oops: 0000 [#1] PREEMPT SMP NOPTI
[  144.400348] CPU: 24 PID: 2388 Comm: node Kdump: loaded Tainted: G            E      6.2.0-rc6+ #2
[  144.400412] Hardware name: Alibaba Cloud Alibaba Cloud ECS, BIOS 9e9f1cc 04/01/2014
[  144.400469] RIP: 0010:bio_associate_blkg_from_css+0xe2/0x320
[  144.400515] Code: 03 75 65 65 48 ff 00 e8 dc 44 b9 ff e8 d7 44 b9 ff eb 3e 48 8b 45 08 48 8b 80 48 03 00 00 48 8b 98 a0 02 00 00 e8 be f3 b8 ff <48> 8b 43 38 a8 03 0f 85 0b 02 00 00 65 48 ff 00 e8 a9 44 b9 ff 48
[  144.400643] RSP: 0018:ffffb819c1f7f9f0 EFLAGS: 00010202
[  144.400682] RAX: 0000000000000002 RBX: 0000000000000000 RCX: 0000000000000c00
[  144.400734] RDX: ffff95b54bf0df40 RSI: ffffffffb0186a80 RDI: ffff95b511fc3800
[  144.400785] RBP: ffff95b511fc3800 R08: ffff95b511fc3800 R09: 0000000000003000
[  144.400837] R10: ffff95f2fffd5000 R11: 0000000000038e80 R12: 0000000000000000
[  144.400889] R13: 0000000000000c00 R14: ffff95b9027a7080 R15: ffffffffb0186038
[  144.400941] FS:  00007f3f789f8640(0000) GS:ffff95f203400000(0000) knlGS:0000000000000000
[  144.400999] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  144.401042] CR2: 0000000000000038 CR3: 00000001f84be004 CR4: 00000000007706e0
[  144.401094] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  144.401146] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  144.401198] PKRU: 55555554
[  144.401220] Call Trace:
[  144.401241]  <TASK>
[  144.401262]  bio_associate_blkg+0x2c/0x60
[  144.401294]  bio_alloc_bioset+0x17f/0x560
[  144.401327]  ? __getblk_gfp+0xbf/0xd0
[  144.401357]  submit_bh_wbc+0x9b/0x130
[  144.401396]  ext4_read_bh_lock+0x3d/0x60
[  144.401430]  ext4_bread_batch+0xee/0x170
[  144.401466]  __ext4_find_entry+0x147/0x3e0
[  144.401499]  ? d_alloc+0x87/0xa0
[  144.401527]  ? preempt_count_add+0x51/0xa0
[  144.401563]  ext4_lookup.part.0+0x48/0x1a0
[  144.401596]  __lookup_slow+0x72/0x120
[  144.401627]  walk_component+0xe5/0x160
[  144.401657]  path_lookupat+0x6e/0x1c0
[  144.401687]  filename_lookup+0xc0/0x1b0
[  144.401718]  ? schedule+0x6b/0xe0
[  144.401747]  ? futex_wait_queue+0x64/0x90
[  144.401781]  vfs_statx+0x7d/0x150
[  144.401809]  do_statx+0x31/0x60
[  144.401836]  ? __check_object_size.part.0+0x47/0xd0
[  144.401873]  ? strncpy_from_user+0x52/0x150
[  144.402059]  ? getname_flags.part.0+0x48/0x1b0
[  144.402226]  __x64_sys_statx+0x68/0x90
[  144.402387]  do_syscall_64+0x3c/0x90
[  144.402553]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  144.402717] RIP: 0033:0x7f3f7c23ee5d
[  144.402868] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 93 af 1b 00 f7 d8 64 89 01 48
[  144.403253] RSP: 002b:00007f3f789f7b78 EFLAGS: 00000246 ORIG_RAX: 000000000000014c
[  144.403445] RAX: ffffffffffffffda RBX: 00000000061f6a68 RCX: 00007f3f7c23ee5d
[  144.403633] RDX: 0000000000000100 RSI: 00000000060aba90 RDI: 00000000ffffff9c
[  144.403815] RBP: 00007f3f789f7b80 R08: 00007f3f789f7b90 R09: 00007f3f789f7ca0
[  144.403996] R10: 0000000000000fff R11: 0000000000000246 R12: 00007f3f789f8548
[  144.404174] R13: 00000000061f6b48 R14: 00007f3f7c29f550 R15: 00000000061f6b48
[  144.404349]  </TASK>
[  144.404496] Modules linked in: ublk_drv(E) tcp_diag(E) inet_diag(E) rfkill(E) mousedev(E) intel_rapl_msr(E) intel_rapl_common(E) nfit(E) intel_powerclamp(E) rapl(E) psmouse(E) i2c_piix4(E) virtio_console(E) virtio_balloon(E) pcspkr(E) fuse(E) ip_tables(E) xfs(E) libcrc32c(E) cirrus(E) drm_shmem_helper(E) drm_kms_helper(E) syscopyarea(E) sysfillrect(E) sysimgblt(E) crct10dif_pclmul(E) crc32_pclmul(E) crc32c_intel(E) ghash_clmulni_intel(E) virtio_net(E) drm(E) net_failover(E) serio_raw(E) failover(E) i2c_core(E)
[  144.405467] CR2: 0000000000000038

Re: [PATCH] block: ublk: improve handling device deletion

[Ming Lei]

On Wed, Feb 08, 2023 at 01:57:42PM +0800, Ziyang Zhang wrote:
> On 2023/2/7 23:07, Ming Lei wrote:
> > Inside ublk_ctrl_del_dev(), when the device is removed, we wait
> > until the device number is freed with holding global lock of
> > ublk_ctl_mutex, this way isn't friendly from user viewpoint:
> > 
> > 1) if device is in-use, the current delete command hangs in
> > ublk_ctrl_del_dev(), and user can't break from the handling
> > because wait_event() is used
> > 
> > 2) global lock is held, so any new device can't be added and
> > other old devices can't be removed.
> > 
> > Improve the deleting handling by the following way, suggested by
> > Nadav:
> > 
> > 1) wait without holding the global lock
> > 
> > 2) replace wait_event() with wait_event_interruptible()
> > 
> > Reported-by: Nadav Amit <nadav.amit@gmail.com>
> > Suggested-by: Nadav Amit <nadav.amit@gmail.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> 
> Hi Ming,
> 
> I tried this patch. And the folloing NPE bug was trigged by:
> 
> (0) dd if=/dev/zero of=/root/img bs=4096 count=1024768
> (1) ublk add -t loop -f /root/img
> (2) mkdir -p /root/ublk
> (3) mount /dev/ublkb0 /root/ublk
> (4) echo "hello" > /root/ublk
> (5) ./ublk del -n 0
> 
> So I delete the ublk device while it is mounting as
> an ext4 filesystem. I think ublk should handle this
> by (1) returning -EBUSY or (2) blocking incoming IO.

That isn't ublk unique, and is one recent block layer regression,
see the following report:

https://lore.kernel.org/linux-block/20230208063552.GA15030@lst.de/T/#u

Thanks,
Ming

Re: [PATCH] block: ublk: improve handling device deletion

[Jens Axboe]

On Tue, 07 Feb 2023 23:07:00 +0800, Ming Lei wrote:
> Inside ublk_ctrl_del_dev(), when the device is removed, we wait
> until the device number is freed with holding global lock of
> ublk_ctl_mutex, this way isn't friendly from user viewpoint:
> 
> 1) if device is in-use, the current delete command hangs in
> ublk_ctrl_del_dev(), and user can't break from the handling
> because wait_event() is used
> 
> [...]

Applied, thanks!

[1/1] block: ublk: improve handling device deletion
      commit: 0abe39dec065133e3f92a52219c3728fe7d7617f

Best regards,
-- 
Jens Axboe