Re: ublk: kernel crash when killing SPDK application

Re: ublk: kernel crash when killing SPDK application

[Ming Lei]

On Tue, Apr 22, 2025 at 02:43:06PM +0300, Jared Holzman wrote:
> On 15/04/2025 15:56, Ming Lei wrote:
> > On Tue, Apr 15, 2025 at 10:58:37AM +0000, Guy Eisenberg wrote:

...

> 
> Hi Ming,
> 
> Unfortunately your patch did not solve the issue, it is still happening (6.14 Kernel)
> 
> I believe the issue is that ublk_cancel_cmd() is calling io_uring_cmd_done() on a uring_cmd that is currently scheduled as a task work by io_uring_cmd_complete_in_task()
> 
> I reproduced with the patch below and saw the warning I added shortly before the crash. The dmesg log is attached.
> 
> I'm not sure how to solve the issue though. Unless we wait for the task work to complete in ublk_cancel cmd. I can't see any way to cancel the task work
> 
> Would appreciate your assistance,
> 
> Regards,
> 
> Jared
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index ca9a67b5b537..d9f544206b36 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -72,6 +72,10 @@
>  	(UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \
>  	 UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED)
>  
> +#ifndef IORING_URING_CMD_TW_SCHED
> +        #define IORING_URING_CMD_TW_SCHED (1U << 31)
> +#endif
> +
>  struct ublk_rq_data {
>  	struct llist_node node;
>  
> @@ -1236,6 +1240,7 @@ static void ublk_rq_task_work_cb(struct io_uring_cmd *cmd, unsigned issue_flags)
>  	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>  	struct ublk_queue *ubq = pdu->ubq;
>  
> +	cmd->flags &= ~IORING_URING_CMD_TW_SCHED;
>  	ublk_forward_io_cmds(ubq, issue_flags);
>  }
>  
> @@ -1245,7 +1250,7 @@ static void ublk_queue_cmd(struct ublk_queue *ubq, struct request *rq)
>  
>  	if (llist_add(&data->node, &ubq->io_cmds)) {
>  		struct ublk_io *io = &ubq->ios[rq->tag];
> -
> +		io->cmd->flags |= IORING_URING_CMD_TW_SCHED;
>  		io_uring_cmd_complete_in_task(io->cmd, ublk_rq_task_work_cb);
>  	}
>  }
> @@ -1498,8 +1503,10 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>  		io->flags |= UBLK_IO_FLAG_CANCELED;
>  	spin_unlock(&ubq->cancel_lock);
>  
> -	if (!done)
> +	if (!done) {
> +		WARN_ON_ONCE(io->cmd->flags & IORING_URING_CMD_TW_SCHED);
>  		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
> +        }
>  }
>  
>  /*
> @@ -1925,6 +1932,7 @@ static inline int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
>  static void ublk_ch_uring_cmd_cb(struct io_uring_cmd *cmd,
>  		unsigned int issue_flags)
>  {
> +	cmd->flags &= ~IORING_URING_CMD_TW_SCHED;
>  	ublk_ch_uring_cmd_local(cmd, issue_flags);
>  }
>  
> @@ -1937,6 +1945,7 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
>  
>  	/* well-implemented server won't run into unlocked */
>  	if (unlikely(issue_flags & IO_URING_F_UNLOCKED)) {
> +		cmd->flags |= IORING_URING_CMD_TW_SCHED;
>  		io_uring_cmd_complete_in_task(cmd, ublk_ch_uring_cmd_cb);
>  		return -EIOCBQUEUED;
>  	}
> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
> index abd0c8bd950b..3ac2ef7bd99a 100644
> --- a/include/linux/io_uring/cmd.h
> +++ b/include/linux/io_uring/cmd.h
> @@ -7,6 +7,7 @@
>  
>  /* only top 8 bits of sqe->uring_cmd_flags for kernel internal use */
>  #define IORING_URING_CMD_CANCELABLE	(1U << 30)
> +#define IORING_URING_CMD_TW_SCHED	(1U << 31)
>  
>  struct io_uring_cmd {
>  	struct file	*file;
> 

Nice debug patch!

Your patch and the dmesg log has shown the race between io_uring_cmd_complete_in_task()
and io_uring_cmd_done() <- ublk_cancel_cmd().

In theory, io_uring should have the knowledge to cover it, but I guess it
might be a bit hard.

I will try to cook a ublk fix tomorrow for you to test.

Thanks,
Ming

Re: ublk: kernel crash when killing SPDK application

[Jared Holzman]

On 22/04/2025 16:42, Ming Lei wrote:
> On Tue, Apr 22, 2025 at 02:43:06PM +0300, Jared Holzman wrote:
>> On 15/04/2025 15:56, Ming Lei wrote:
>>> On Tue, Apr 15, 2025 at 10:58:37AM +0000, Guy Eisenberg wrote:
> 
> ...
> 
>>
>> Hi Ming,
>>
>> Unfortunately your patch did not solve the issue, it is still happening (6.14 Kernel)
>>
>> I believe the issue is that ublk_cancel_cmd() is calling io_uring_cmd_done() on a uring_cmd that is currently scheduled as a task work by io_uring_cmd_complete_in_task()
>>
>> I reproduced with the patch below and saw the warning I added shortly before the crash. The dmesg log is attached.
>>
>> I'm not sure how to solve the issue though. Unless we wait for the task work to complete in ublk_cancel cmd. I can't see any way to cancel the task work
>>
>> Would appreciate your assistance,
>>
>> Regards,
>>
>> Jared
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index ca9a67b5b537..d9f544206b36 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -72,6 +72,10 @@
>>  	(UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \
>>  	 UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED)
>>  
>> +#ifndef IORING_URING_CMD_TW_SCHED
>> +        #define IORING_URING_CMD_TW_SCHED (1U << 31)
>> +#endif
>> +
>>  struct ublk_rq_data {
>>  	struct llist_node node;
>>  
>> @@ -1236,6 +1240,7 @@ static void ublk_rq_task_work_cb(struct io_uring_cmd *cmd, unsigned issue_flags)
>>  	struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
>>  	struct ublk_queue *ubq = pdu->ubq;
>>  
>> +	cmd->flags &= ~IORING_URING_CMD_TW_SCHED;
>>  	ublk_forward_io_cmds(ubq, issue_flags);
>>  }
>>  
>> @@ -1245,7 +1250,7 @@ static void ublk_queue_cmd(struct ublk_queue *ubq, struct request *rq)
>>  
>>  	if (llist_add(&data->node, &ubq->io_cmds)) {
>>  		struct ublk_io *io = &ubq->ios[rq->tag];
>> -
>> +		io->cmd->flags |= IORING_URING_CMD_TW_SCHED;
>>  		io_uring_cmd_complete_in_task(io->cmd, ublk_rq_task_work_cb);
>>  	}
>>  }
>> @@ -1498,8 +1503,10 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, struct ublk_io *io,
>>  		io->flags |= UBLK_IO_FLAG_CANCELED;
>>  	spin_unlock(&ubq->cancel_lock);
>>  
>> -	if (!done)
>> +	if (!done) {
>> +		WARN_ON_ONCE(io->cmd->flags & IORING_URING_CMD_TW_SCHED);
>>  		io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0, issue_flags);
>> +        }
>>  }
>>  
>>  /*
>> @@ -1925,6 +1932,7 @@ static inline int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
>>  static void ublk_ch_uring_cmd_cb(struct io_uring_cmd *cmd,
>>  		unsigned int issue_flags)
>>  {
>> +	cmd->flags &= ~IORING_URING_CMD_TW_SCHED;
>>  	ublk_ch_uring_cmd_local(cmd, issue_flags);
>>  }
>>  
>> @@ -1937,6 +1945,7 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
>>  
>>  	/* well-implemented server won't run into unlocked */
>>  	if (unlikely(issue_flags & IO_URING_F_UNLOCKED)) {
>> +		cmd->flags |= IORING_URING_CMD_TW_SCHED;
>>  		io_uring_cmd_complete_in_task(cmd, ublk_ch_uring_cmd_cb);
>>  		return -EIOCBQUEUED;
>>  	}
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h
>> index abd0c8bd950b..3ac2ef7bd99a 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -7,6 +7,7 @@
>>  
>>  /* only top 8 bits of sqe->uring_cmd_flags for kernel internal use */
>>  #define IORING_URING_CMD_CANCELABLE	(1U << 30)
>> +#define IORING_URING_CMD_TW_SCHED	(1U << 31)
>>  
>>  struct io_uring_cmd {
>>  	struct file	*file;
>>
> 
> Nice debug patch!
> 
> Your patch and the dmesg log has shown the race between io_uring_cmd_complete_in_task()
> and io_uring_cmd_done() <- ublk_cancel_cmd().
> 
> In theory, io_uring should have the knowledge to cover it, but I guess it
> might be a bit hard.
> 
> I will try to cook a ublk fix tomorrow for you to test.
> 
> Thanks,
> Ming
> 

That would be great. Much appreciated!

Regards,
Jared