Re: kernel oops when rmmod'ing ublk_drv w/ missing UBLK_CMD_START_DEV

[Jens Axboe <axboe@kernel.dk>]

On 1/25/23 12:50?PM, Harris, James R wrote:
> Hi,
> 
> I can reliably hit a kernel oops with ublk_drv using the following abnormal sequence of events (repro .c file at end of this e-mail):
> 
> 1) modprobe ublk_drv
> 2) run test app which basically does:
>   a) submit UBLK_CMD_ADD_DEV
>   b) submit UBLK_CMD_SET_PARAMS
>   c) wait for completions
>   d) do *not* submit UBLK_CMD_START_DEV
>   e) exit
> 3) rmmod ublk_drv
> 
> Reproduces on 6.2-rc5, 6.1.5 and 6.1.

Something like this may do it, but I'll let Ming sort out the details
on what's the most appropriate fix.


diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 17b677b5d3b2..dacc13e2a476 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1149,11 +1149,17 @@ static void ublk_unquiesce_dev(struct ublk_device *ub)
 	blk_mq_kick_requeue_list(ub->ub_disk->queue);
 }
 
-static void ublk_stop_dev(struct ublk_device *ub)
+/*
+ * Returns if the device was live or not
+ */
+static bool ublk_stop_dev(struct ublk_device *ub)
 {
+	bool was_live = false;
+
 	mutex_lock(&ub->mutex);
 	if (ub->dev_info.state == UBLK_S_DEV_DEAD)
 		goto unlock;
+	was_live = true;
 	if (ublk_can_use_recovery(ub)) {
 		if (ub->dev_info.state == UBLK_S_DEV_LIVE)
 			__ublk_quiesce_dev(ub);
@@ -1168,6 +1174,7 @@ static void ublk_stop_dev(struct ublk_device *ub)
 	ublk_cancel_dev(ub);
 	mutex_unlock(&ub->mutex);
 	cancel_delayed_work_sync(&ub->monitor_work);
+	return was_live;
 }
 
 /* device can only be started after all IOs are ready */
@@ -1470,7 +1477,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
-	ublk_stop_dev(ub);
+	if (!ublk_stop_dev(ub))
+		return;
 	cancel_work_sync(&ub->stop_work);
 	cancel_work_sync(&ub->quiesce_work);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
@@ -1771,7 +1779,8 @@ static int ublk_ctrl_stop_dev(struct io_uring_cmd *cmd)
 	if (!ub)
 		return -EINVAL;
 
-	ublk_stop_dev(ub);
+	if (!ublk_stop_dev(ub))
+		return -EINVAL;
 	cancel_work_sync(&ub->stop_work);
 	cancel_work_sync(&ub->quiesce_work);
 

-- 
Jens Axboe