[PATCH 0/2] ublk: two fixes

[PATCH 0/2] ublk: two fixes

[Ming Lei]

Hello Jens,

The 1st patch fixes one kernel panic issue when running IO vs. remove
devices on ublk zc.

The 2nd one fixes one regression introduced in this cycle.

Thanks,


Ming Lei (2):
  ublk: fix handling recovery & reissue in ublk_abort_queue()
  ublk: don't fail request for recovery & reissue in case of
    ubq->canceling

 drivers/block/ublk_drv.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

-- 
2.47.0

[PATCH 1/2] ublk: fix handling recovery & reissue in ublk_abort_queue()

[Ming Lei]

Commit 8284066946e6 ("ublk: grab request reference when the request is handled
by userspace") doesn't grab request reference in case of recovery reissue.
Then the request can be requeued & re-dispatch & failed when canceling
uring command.

If it is one zc request, the request can be freed before io_uring
returns the zc buffer back, then cause kernel panic:

[  126.773061] BUG: kernel NULL pointer dereference, address: 00000000000000c8
[  126.773657] #PF: supervisor read access in kernel mode
[  126.774052] #PF: error_code(0x0000) - not-present page
[  126.774455] PGD 0 P4D 0
[  126.774698] Oops: Oops: 0000 [#1] SMP NOPTI
[  126.775034] CPU: 13 UID: 0 PID: 1612 Comm: kworker/u64:55 Not tainted 6.14.0_blk+ #182 PREEMPT(full)
[  126.775676] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39 04/01/2014
[  126.776275] Workqueue: iou_exit io_ring_exit_work
[  126.776651] RIP: 0010:ublk_io_release+0x14/0x130 [ublk_drv]

Fixes it by always grabbing request reference for aborting the request.

Reported-by: Caleb Sander Mateos <csander@purestorage.com>
Closes: https://lore.kernel.org/linux-block/CADUfDZodKfOGUeWrnAxcZiLT+puaZX8jDHoj_sfHZCOZwhzz6A@mail.gmail.com/
Fixes: 8284066946e6 ("ublk: grab request reference when the request is handled by userspace")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 2fd05c1bd30b..41bed67508f2 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1140,6 +1140,25 @@ static void ublk_complete_rq(struct kref *ref)
 	__ublk_complete_rq(req);
 }
 
+static void ublk_do_fail_rq(struct request *req)
+{
+	struct ublk_queue *ubq = req->mq_hctx->driver_data;
+
+	if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
+		blk_mq_requeue_request(req, false);
+	else
+		__ublk_complete_rq(req);
+}
+
+static void ublk_fail_rq_fn(struct kref *ref)
+{
+	struct ublk_rq_data *data = container_of(ref, struct ublk_rq_data,
+			ref);
+	struct request *req = blk_mq_rq_from_pdu(data);
+
+	ublk_do_fail_rq(req);
+}
+
 /*
  * Since ublk_rq_task_work_cb always fails requests immediately during
  * exiting, __ublk_fail_req() is only called from abort context during
@@ -1153,10 +1172,13 @@ static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io,
 {
 	WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE);
 
-	if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
-		blk_mq_requeue_request(req, false);
-	else
-		ublk_put_req_ref(ubq, req);
+	if (ublk_need_req_ref(ubq)) {
+		struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
+
+		kref_put(&data->ref, ublk_fail_rq_fn);
+	} else {
+		ublk_do_fail_rq(req);
+	}
 }
 
 static void ubq_complete_io_cmd(struct ublk_io *io, int res,
-- 
2.47.0

[PATCH 2/2] ublk: don't fail request for recovery & reissue in case of ubq->canceling

[Ming Lei]

ubq->canceling is set with request queue quiesced when io_uring context is
exiting. Recovery & reissue(UBLK_F_USER_RECOVERY_REISSUE) requires
request to be re-queued and re-dispatch after device is recovered.

However commit d796cea7b9f3 ("ublk: implement ->queue_rqs()") still may
fail any request in case of ubq->canceling, this way breaks
UBLK_F_USER_RECOVERY_REISSUE.

Fix it by calling __ublk_abort_rq() in case of ubq->canceling.

Reported-by: Uday Shankar <ushankar@purestorage.com>
Closes: https://lore.kernel.org/linux-block/Z%2FQkkTRHfRxtN%2FmB@dev-ushankar.dev.purestorage.com/
Fixes: commit d796cea7b9f3 ("ublk: implement ->queue_rqs()")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 41bed67508f2..d6ca2f1097ad 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1371,7 +1371,8 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq)
 	return BLK_EH_RESET_TIMER;
 }
 
-static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq)
+static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq,
+				  bool check_cancel)
 {
 	blk_status_t res;
 
@@ -1390,7 +1391,7 @@ static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq)
 	if (ublk_nosrv_should_queue_io(ubq) && unlikely(ubq->force_abort))
 		return BLK_STS_IOERR;
 
-	if (unlikely(ubq->canceling))
+	if (check_cancel && unlikely(ubq->canceling))
 		return BLK_STS_IOERR;
 
 	/* fill iod to slot in io cmd buffer */
@@ -1409,7 +1410,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct request *rq = bd->rq;
 	blk_status_t res;
 
-	res = ublk_prep_req(ubq, rq);
+	res = ublk_prep_req(ubq, rq, false);
 	if (res != BLK_STS_OK)
 		return res;
 
@@ -1441,7 +1442,7 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
 			ublk_queue_cmd_list(ubq, &submit_list);
 		ubq = this_q;
 
-		if (ublk_prep_req(ubq, req) == BLK_STS_OK)
+		if (ublk_prep_req(ubq, req, true) == BLK_STS_OK)
 			rq_list_add_tail(&submit_list, req);
 		else
 			rq_list_add_tail(&requeue_list, req);
-- 
2.47.0

Re: [PATCH 2/2] ublk: don't fail request for recovery & reissue in case of ubq->canceling

[Uday Shankar]

On Tue, Apr 08, 2025 at 03:24:38PM +0800, Ming Lei wrote:
> ubq->canceling is set with request queue quiesced when io_uring context is
> exiting. Recovery & reissue(UBLK_F_USER_RECOVERY_REISSUE) requires
> request to be re-queued and re-dispatch after device is recovered.
> 
> However commit d796cea7b9f3 ("ublk: implement ->queue_rqs()") still may
> fail any request in case of ubq->canceling, this way breaks
> UBLK_F_USER_RECOVERY_REISSUE.

This change actually affects UBLK_F_USER_RECOVERY as long as FAIL_IO
isn't set (regardless of RECOVERY_REISSUE). RECOVERY_REISSUE only
changes behavior for requests that are already in the ublk server when
the ublk server dies, but the code change only affects requests that
come in after ublk server death is already detected. At that point, both
plain USER_RECOVERY and USER_RECOVERY|USER_RECOVERY_REISSUE should see
requests queue instead of completing with error. See below code
snippets:

static inline void __ublk_abort_rq(struct ublk_queue *ubq,
		struct request *rq)
{
	/* We cannot process this rq so just requeue it. */
	if (ublk_nosrv_dev_should_queue_io(ubq->dev))
		blk_mq_requeue_request(rq, false);
	else
		blk_mq_end_request(rq, BLK_STS_IOERR);
}

/*
 * Should I/O issued while there is no ublk server queue? If not, I/O
 * issued while there is no ublk server will get errors.
 */
static inline bool ublk_nosrv_dev_should_queue_io(struct ublk_device *ub)
{
	return (ub->dev_info.flags & UBLK_F_USER_RECOVERY) &&
	       !(ub->dev_info.flags & UBLK_F_USER_RECOVERY_FAIL_IO);
}

> 
> Fix it by calling __ublk_abort_rq() in case of ubq->canceling.
> 
> Reported-by: Uday Shankar <ushankar@purestorage.com>
> Closes: https://lore.kernel.org/linux-block/Z%2FQkkTRHfRxtN%2FmB@dev-ushankar.dev.purestorage.com/
> Fixes: commit d796cea7b9f3 ("ublk: implement ->queue_rqs()")

Will this upset anything parsing these tags? The syntax I've usually
seen doesn't have "commit," i.e.

Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")

> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Code change looks good.

Reviewed-by: Uday Shankar <ushankar@purestorage.com>

> ---
>  drivers/block/ublk_drv.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 41bed67508f2..d6ca2f1097ad 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1371,7 +1371,8 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq)
>  	return BLK_EH_RESET_TIMER;
>  }
>  
> -static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq)
> +static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq,
> +				  bool check_cancel)
>  {
>  	blk_status_t res;
>  
> @@ -1390,7 +1391,7 @@ static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq)
>  	if (ublk_nosrv_should_queue_io(ubq) && unlikely(ubq->force_abort))
>  		return BLK_STS_IOERR;
>  
> -	if (unlikely(ubq->canceling))
> +	if (check_cancel && unlikely(ubq->canceling))
>  		return BLK_STS_IOERR;
>  
>  	/* fill iod to slot in io cmd buffer */
> @@ -1409,7 +1410,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	struct request *rq = bd->rq;
>  	blk_status_t res;
>  
> -	res = ublk_prep_req(ubq, rq);
> +	res = ublk_prep_req(ubq, rq, false);
>  	if (res != BLK_STS_OK)
>  		return res;
>  
> @@ -1441,7 +1442,7 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
>  			ublk_queue_cmd_list(ubq, &submit_list);
>  		ubq = this_q;
>  
> -		if (ublk_prep_req(ubq, req) == BLK_STS_OK)
> +		if (ublk_prep_req(ubq, req, true) == BLK_STS_OK)
>  			rq_list_add_tail(&submit_list, req);
>  		else
>  			rq_list_add_tail(&requeue_list, req);
> -- 
> 2.47.0
> 

Re: [PATCH 2/2] ublk: don't fail request for recovery & reissue in case of ubq->canceling

[Ming Lei]

On Tue, Apr 08, 2025 at 01:11:52PM -0600, Uday Shankar wrote:
> On Tue, Apr 08, 2025 at 03:24:38PM +0800, Ming Lei wrote:
> > ubq->canceling is set with request queue quiesced when io_uring context is
> > exiting. Recovery & reissue(UBLK_F_USER_RECOVERY_REISSUE) requires
> > request to be re-queued and re-dispatch after device is recovered.
> > 
> > However commit d796cea7b9f3 ("ublk: implement ->queue_rqs()") still may
> > fail any request in case of ubq->canceling, this way breaks
> > UBLK_F_USER_RECOVERY_REISSUE.
> 
> This change actually affects UBLK_F_USER_RECOVERY as long as FAIL_IO
> isn't set (regardless of RECOVERY_REISSUE). RECOVERY_REISSUE only
> changes behavior for requests that are already in the ublk server when
> the ublk server dies, but the code change only affects requests that
> come in after ublk server death is already detected. At that point, both
> plain USER_RECOVERY and USER_RECOVERY|USER_RECOVERY_REISSUE should see
> requests queue instead of completing with error. See below code
> snippets:
> 
> static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> 		struct request *rq)
> {
> 	/* We cannot process this rq so just requeue it. */
> 	if (ublk_nosrv_dev_should_queue_io(ubq->dev))
> 		blk_mq_requeue_request(rq, false);
> 	else
> 		blk_mq_end_request(rq, BLK_STS_IOERR);
> }
> 
> /*
>  * Should I/O issued while there is no ublk server queue? If not, I/O
>  * issued while there is no ublk server will get errors.
>  */
> static inline bool ublk_nosrv_dev_should_queue_io(struct ublk_device *ub)
> {
> 	return (ub->dev_info.flags & UBLK_F_USER_RECOVERY) &&
> 	       !(ub->dev_info.flags & UBLK_F_USER_RECOVERY_FAIL_IO);
> }

Indeed, will update commit log.

> 
> > 
> > Fix it by calling __ublk_abort_rq() in case of ubq->canceling.
> > 
> > Reported-by: Uday Shankar <ushankar@purestorage.com>
> > Closes: https://lore.kernel.org/linux-block/Z%2FQkkTRHfRxtN%2FmB@dev-ushankar.dev.purestorage.com/
> > Fixes: commit d796cea7b9f3 ("ublk: implement ->queue_rqs()")
> 
> Will this upset anything parsing these tags? The syntax I've usually
> seen doesn't have "commit," i.e.
> 
> Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")

Will fix it in V2.

Thanks
Ming

Re: [PATCH 1/2] ublk: fix handling recovery & reissue in ublk_abort_queue()

[Caleb Sander Mateos]

On Tue, Apr 8, 2025 at 12:25 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> Commit 8284066946e6 ("ublk: grab request reference when the request is handled
> by userspace") doesn't grab request reference in case of recovery reissue.
> Then the request can be requeued & re-dispatch & failed when canceling
> uring command.
>
> If it is one zc request, the request can be freed before io_uring
> returns the zc buffer back, then cause kernel panic:
>
> [  126.773061] BUG: kernel NULL pointer dereference, address: 00000000000000c8
> [  126.773657] #PF: supervisor read access in kernel mode
> [  126.774052] #PF: error_code(0x0000) - not-present page
> [  126.774455] PGD 0 P4D 0
> [  126.774698] Oops: Oops: 0000 [#1] SMP NOPTI
> [  126.775034] CPU: 13 UID: 0 PID: 1612 Comm: kworker/u64:55 Not tainted 6.14.0_blk+ #182 PREEMPT(full)
> [  126.775676] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39 04/01/2014
> [  126.776275] Workqueue: iou_exit io_ring_exit_work
> [  126.776651] RIP: 0010:ublk_io_release+0x14/0x130 [ublk_drv]
>
> Fixes it by always grabbing request reference for aborting the request.
>
> Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> Closes: https://lore.kernel.org/linux-block/CADUfDZodKfOGUeWrnAxcZiLT+puaZX8jDHoj_sfHZCOZwhzz6A@mail.gmail.com/
> Fixes: 8284066946e6 ("ublk: grab request reference when the request is handled by userspace")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 2fd05c1bd30b..41bed67508f2 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1140,6 +1140,25 @@ static void ublk_complete_rq(struct kref *ref)
>         __ublk_complete_rq(req);
>  }
>
> +static void ublk_do_fail_rq(struct request *req)
> +{
> +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> +
> +       if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
> +               blk_mq_requeue_request(req, false);
> +       else
> +               __ublk_complete_rq(req);
> +}
> +
> +static void ublk_fail_rq_fn(struct kref *ref)
> +{
> +       struct ublk_rq_data *data = container_of(ref, struct ublk_rq_data,
> +                       ref);
> +       struct request *req = blk_mq_rq_from_pdu(data);
> +
> +       ublk_do_fail_rq(req);
> +}
> +
>  /*
>   * Since ublk_rq_task_work_cb always fails requests immediately during
>   * exiting, __ublk_fail_req() is only called from abort context during
> @@ -1153,10 +1172,13 @@ static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io,
>  {
>         WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE);
>
> -       if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
> -               blk_mq_requeue_request(req, false);
> -       else
> -               ublk_put_req_ref(ubq, req);
> +       if (ublk_need_req_ref(ubq)) {
> +               struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
> +
> +               kref_put(&data->ref, ublk_fail_rq_fn);

I think this looks good, just a small question. If __ublk_fail_req()
is called but there is at least 1 other reference, ublk_do_fail_rq()
won't get called here. When the last reference is dropped, it will
call __ublk_complete_rq() instead. That checks for io->flags &
UBLK_IO_FLAG_ABORTED and will terminate the I/O with BLK_STS_IOERR.
But is that the desired behavior in the
ublk_nosrv_should_reissue_outstanding() case? I would think it should
call blk_mq_requeue_request() instead.

Best,
Caleb

> +       } else {
> +               ublk_do_fail_rq(req);
> +       }
>  }
>
>  static void ubq_complete_io_cmd(struct ublk_io *io, int res,
> --
> 2.47.0
>

Re: [PATCH 1/2] ublk: fix handling recovery & reissue in ublk_abort_queue()

[Ming Lei]

On Tue, Apr 08, 2025 at 06:47:20PM -0700, Caleb Sander Mateos wrote:
> On Tue, Apr 8, 2025 at 12:25 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Commit 8284066946e6 ("ublk: grab request reference when the request is handled
> > by userspace") doesn't grab request reference in case of recovery reissue.
> > Then the request can be requeued & re-dispatch & failed when canceling
> > uring command.
> >
> > If it is one zc request, the request can be freed before io_uring
> > returns the zc buffer back, then cause kernel panic:
> >
> > [  126.773061] BUG: kernel NULL pointer dereference, address: 00000000000000c8
> > [  126.773657] #PF: supervisor read access in kernel mode
> > [  126.774052] #PF: error_code(0x0000) - not-present page
> > [  126.774455] PGD 0 P4D 0
> > [  126.774698] Oops: Oops: 0000 [#1] SMP NOPTI
> > [  126.775034] CPU: 13 UID: 0 PID: 1612 Comm: kworker/u64:55 Not tainted 6.14.0_blk+ #182 PREEMPT(full)
> > [  126.775676] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39 04/01/2014
> > [  126.776275] Workqueue: iou_exit io_ring_exit_work
> > [  126.776651] RIP: 0010:ublk_io_release+0x14/0x130 [ublk_drv]
> >
> > Fixes it by always grabbing request reference for aborting the request.
> >
> > Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> > Closes: https://lore.kernel.org/linux-block/CADUfDZodKfOGUeWrnAxcZiLT+puaZX8jDHoj_sfHZCOZwhzz6A@mail.gmail.com/
> > Fixes: 8284066946e6 ("ublk: grab request reference when the request is handled by userspace")
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 2fd05c1bd30b..41bed67508f2 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -1140,6 +1140,25 @@ static void ublk_complete_rq(struct kref *ref)
> >         __ublk_complete_rq(req);
> >  }
> >
> > +static void ublk_do_fail_rq(struct request *req)
> > +{
> > +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> > +
> > +       if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
> > +               blk_mq_requeue_request(req, false);
> > +       else
> > +               __ublk_complete_rq(req);
> > +}
> > +
> > +static void ublk_fail_rq_fn(struct kref *ref)
> > +{
> > +       struct ublk_rq_data *data = container_of(ref, struct ublk_rq_data,
> > +                       ref);
> > +       struct request *req = blk_mq_rq_from_pdu(data);
> > +
> > +       ublk_do_fail_rq(req);
> > +}
> > +
> >  /*
> >   * Since ublk_rq_task_work_cb always fails requests immediately during
> >   * exiting, __ublk_fail_req() is only called from abort context during
> > @@ -1153,10 +1172,13 @@ static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io,
> >  {
> >         WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE);
> >
> > -       if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
> > -               blk_mq_requeue_request(req, false);
> > -       else
> > -               ublk_put_req_ref(ubq, req);
> > +       if (ublk_need_req_ref(ubq)) {
> > +               struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
> > +
> > +               kref_put(&data->ref, ublk_fail_rq_fn);
> 
> I think this looks good, just a small question. If __ublk_fail_req()
> is called but there is at least 1 other reference, ublk_do_fail_rq()
> won't get called here. When the last reference is dropped, it will
> call __ublk_complete_rq() instead. That checks for io->flags &
> UBLK_IO_FLAG_ABORTED and will terminate the I/O with BLK_STS_IOERR.
> But is that the desired behavior in the
> ublk_nosrv_should_reissue_outstanding() case? I would think it should
> call blk_mq_requeue_request() instead.

Good catch, I'd suggest to fix the kernel panic first, and this behavior
for ublk_nosrv_should_reissue_outstanding() is less serious and can be
fixed as one followup.

Especially, Uday's approach "[PATCH v3] ublk: improve detection and handling of ublk server exit"[1]
may simplify this area lot, with which requests aborting is moved to
ublk_ch_release() after all uring_cmd are completed. Then we can get
correct behavior for ublk_nosrv_should_reissue_outstanding() without any
change basically.


[1] https://lore.kernel.org/linux-block/20250403-ublk_timeout-v3-1-aa09f76c7451@purestorage.com/


thanks,
Ming

Re: [PATCH 1/2] ublk: fix handling recovery & reissue in ublk_abort_queue()

[Caleb Sander Mateos]

On Tue, Apr 8, 2025 at 8:17 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> On Tue, Apr 08, 2025 at 06:47:20PM -0700, Caleb Sander Mateos wrote:
> > On Tue, Apr 8, 2025 at 12:25 AM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > Commit 8284066946e6 ("ublk: grab request reference when the request is handled
> > > by userspace") doesn't grab request reference in case of recovery reissue.
> > > Then the request can be requeued & re-dispatch & failed when canceling
> > > uring command.
> > >
> > > If it is one zc request, the request can be freed before io_uring
> > > returns the zc buffer back, then cause kernel panic:
> > >
> > > [  126.773061] BUG: kernel NULL pointer dereference, address: 00000000000000c8
> > > [  126.773657] #PF: supervisor read access in kernel mode
> > > [  126.774052] #PF: error_code(0x0000) - not-present page
> > > [  126.774455] PGD 0 P4D 0
> > > [  126.774698] Oops: Oops: 0000 [#1] SMP NOPTI
> > > [  126.775034] CPU: 13 UID: 0 PID: 1612 Comm: kworker/u64:55 Not tainted 6.14.0_blk+ #182 PREEMPT(full)
> > > [  126.775676] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-1.fc39 04/01/2014
> > > [  126.776275] Workqueue: iou_exit io_ring_exit_work
> > > [  126.776651] RIP: 0010:ublk_io_release+0x14/0x130 [ublk_drv]
> > >
> > > Fixes it by always grabbing request reference for aborting the request.
> > >
> > > Reported-by: Caleb Sander Mateos <csander@purestorage.com>
> > > Closes: https://lore.kernel.org/linux-block/CADUfDZodKfOGUeWrnAxcZiLT+puaZX8jDHoj_sfHZCOZwhzz6A@mail.gmail.com/
> > > Fixes: 8284066946e6 ("ublk: grab request reference when the request is handled by userspace")
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 30 ++++++++++++++++++++++++++----
> > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 2fd05c1bd30b..41bed67508f2 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1140,6 +1140,25 @@ static void ublk_complete_rq(struct kref *ref)
> > >         __ublk_complete_rq(req);
> > >  }
> > >
> > > +static void ublk_do_fail_rq(struct request *req)
> > > +{
> > > +       struct ublk_queue *ubq = req->mq_hctx->driver_data;
> > > +
> > > +       if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
> > > +               blk_mq_requeue_request(req, false);
> > > +       else
> > > +               __ublk_complete_rq(req);
> > > +}
> > > +
> > > +static void ublk_fail_rq_fn(struct kref *ref)
> > > +{
> > > +       struct ublk_rq_data *data = container_of(ref, struct ublk_rq_data,
> > > +                       ref);
> > > +       struct request *req = blk_mq_rq_from_pdu(data);
> > > +
> > > +       ublk_do_fail_rq(req);
> > > +}
> > > +
> > >  /*
> > >   * Since ublk_rq_task_work_cb always fails requests immediately during
> > >   * exiting, __ublk_fail_req() is only called from abort context during
> > > @@ -1153,10 +1172,13 @@ static void __ublk_fail_req(struct ublk_queue *ubq, struct ublk_io *io,
> > >  {
> > >         WARN_ON_ONCE(io->flags & UBLK_IO_FLAG_ACTIVE);
> > >
> > > -       if (ublk_nosrv_should_reissue_outstanding(ubq->dev))
> > > -               blk_mq_requeue_request(req, false);
> > > -       else
> > > -               ublk_put_req_ref(ubq, req);
> > > +       if (ublk_need_req_ref(ubq)) {
> > > +               struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
> > > +
> > > +               kref_put(&data->ref, ublk_fail_rq_fn);
> >
> > I think this looks good, just a small question. If __ublk_fail_req()
> > is called but there is at least 1 other reference, ublk_do_fail_rq()
> > won't get called here. When the last reference is dropped, it will
> > call __ublk_complete_rq() instead. That checks for io->flags &
> > UBLK_IO_FLAG_ABORTED and will terminate the I/O with BLK_STS_IOERR.
> > But is that the desired behavior in the
> > ublk_nosrv_should_reissue_outstanding() case? I would think it should
> > call blk_mq_requeue_request() instead.
>
> Good catch, I'd suggest to fix the kernel panic first, and this behavior
> for ublk_nosrv_should_reissue_outstanding() is less serious and can be
> fixed as one followup.
>
> Especially, Uday's approach "[PATCH v3] ublk: improve detection and handling of ublk server exit"[1]
> may simplify this area lot, with which requests aborting is moved to
> ublk_ch_release() after all uring_cmd are completed. Then we can get
> correct behavior for ublk_nosrv_should_reissue_outstanding() without any
> change basically.

Sounds like a good plan to me.

Thanks,
Caleb