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

["Harris, James R" <james.r.harris@intel.com>]

> On Jan 25, 2023, at 1:05 PM, Jens Axboe <axboe@kernel.dk> wrote:
> 
> 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.

Thanks Jens.  This patch fixes things for me.  If you and Ming decide to go forward
with this patch, feel free to add:

Tested-by: Jim Harris <james.r.harris@intel.com>

> 
> 
> 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
>