[PATCH V2 0/3] ublk_drv: cleanup and bugfix

[PATCH V2 0/3] ublk_drv: cleanup and bugfix

[ZiyangZhang]

The following 3 patches are cleanup and bugfix. Patch 1 and 2
simply inline a function and update comments for ublk_drv's
aborting machemism.

Patch 3 fix a null-deref bug reported by myself. Ming gives out a
patch and I integrate it with more comments on this bug.

From V1:
- reserve comment on __ublk_fail_req() to notify that aborted
  requests may be issued again.
- only reference ublk_io in branch of not using task_work_add()

ZiyangZhang (3):
  ublk_drv: check ubq_daemon_is_dying() in __ublk_rq_task_work()
  ublk_drv: update comment for __ublk_fail_req()
  ublk_drv: do not add a re-issued request aborted previously to
    ioucmd's task_work

 drivers/block/ublk_drv.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

-- 
2.27.0

[PATCH V2 1/3] ublk_drv: check ubq_daemon_is_dying() in __ublk_rq_task_work()

[ZiyangZhang]

Replace direct check on PF_EXITING in __ublk_rq_task_work() by the
existing wrapper. Also inline ubq_daemon_is_dying().

Reviewed-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
---
 drivers/block/ublk_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 5d8c7234639c..17896172b0fe 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -555,7 +555,7 @@ static inline struct ublk_uring_cmd_pdu *ublk_get_uring_cmd_pdu(
 	return (struct ublk_uring_cmd_pdu *)&ioucmd->pdu;
 }
 
-static bool ubq_daemon_is_dying(struct ublk_queue *ubq)
+static inline bool ubq_daemon_is_dying(struct ublk_queue *ubq)
 {
 	return ubq->ubq_daemon->flags & PF_EXITING;
 }
@@ -644,8 +644,7 @@ static inline void __ublk_rq_task_work(struct request *req)
 	struct ublk_device *ub = ubq->dev;
 	int tag = req->tag;
 	struct ublk_io *io = &ubq->ios[tag];
-	bool task_exiting = current != ubq->ubq_daemon ||
-		(current->flags & PF_EXITING);
+	bool task_exiting = current != ubq->ubq_daemon || ubq_daemon_is_dying(ubq);
 	unsigned int mapped_bytes;
 
 	pr_devel("%s: complete: op %d, qid %d tag %d io_flags %x addr %llx\n",
-- 
2.27.0

[PATCH V2 2/3] ublk_drv: update comment for __ublk_fail_req()

[ZiyangZhang]

Since __ublk_rq_task_work always fails requests immediately during
exiting, __ublk_fail_req() is only called from abort context during
exiting. So lock is unnecessary.

Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
---
 drivers/block/ublk_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 17896172b0fe..685a43b7ae6e 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -605,8 +605,9 @@ static void ublk_complete_rq(struct request *req)
 }
 
 /*
- * __ublk_fail_req() may be called from abort context or ->ubq_daemon
- * context during exiting, so lock is required.
+ * Since __ublk_rq_task_work always fails requests immediately during
+ * exiting, __ublk_fail_req() is only called from abort context during
+ * exiting. So lock is unnecessary.
  *
  * Also aborting may not be started yet, keep in mind that one failed
  * request may be issued by block layer again.
-- 
2.27.0

Re: [PATCH V2 2/3] ublk_drv: update comment for __ublk_fail_req()

[Ming Lei]

On Mon, Aug 15, 2022 at 10:36:32AM +0800, ZiyangZhang wrote:
> Since __ublk_rq_task_work always fails requests immediately during
> exiting, __ublk_fail_req() is only called from abort context during
> exiting. So lock is unnecessary.
> 
> Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
> ---
>  drivers/block/ublk_drv.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 17896172b0fe..685a43b7ae6e 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -605,8 +605,9 @@ static void ublk_complete_rq(struct request *req)
>  }
>  
>  /*
> - * __ublk_fail_req() may be called from abort context or ->ubq_daemon
> - * context during exiting, so lock is required.
> + * Since __ublk_rq_task_work always fails requests immediately during
> + * exiting, __ublk_fail_req() is only called from abort context during
> + * exiting. So lock is unnecessary.
>   *
>   * Also aborting may not be started yet, keep in mind that one failed
>   * request may be issued by block layer again.
> -- 
> 2.27.0
> 

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

-- 
Ming

[PATCH V2 3/3] ublk_drv: do not add a re-issued request aborted previously to ioucmd's task_work

[ZiyangZhang]

In ublk_queue_rq(), Assume current request is a re-issued request aborted
previously in monitor_work because the ubq_daemon(ioucmd's task) is
PF_EXITING. For this request, we cannot call
io_uring_cmd_complete_in_task() anymore because at that moment io_uring
context may be freed in case that no inflight ioucmd exists. Otherwise,
we may cause null-deref in ctx->fallback_work.

Add a check on UBLK_IO_FLAG_ABORTED to prevent the above situation. This
check is safe and makes sense.

Note: monitor_work sets UBLK_IO_FLAG_ABORTED and ends this request
(releasing the tag). Then the request is restarted(allocating the tag)
and we are here. Since releasing/allocating a tag implies smp_mb(),
finding UBLK_IO_FLAG_ABORTED guarantees that here is a re-issued request
aborted previously.

Suggested-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>
---
 drivers/block/ublk_drv.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 685a43b7ae6e..6a4a94b4cdf4 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -756,9 +756,25 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx,
 		if (task_work_add(ubq->ubq_daemon, &data->work, notify_mode))
 			goto fail;
 	} else {
-		struct io_uring_cmd *cmd = ubq->ios[rq->tag].cmd;
+		struct ublk_io *io = &ubq->ios[rq->tag];
+		struct io_uring_cmd *cmd = io->cmd;
 		struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
 
+		/*
+		 * If the check pass, we know that this is a re-issued request aborted
+		 * previously in monitor_work because the ubq_daemon(cmd's task) is
+		 * PF_EXITING. We cannot call io_uring_cmd_complete_in_task() anymore
+		 * because this ioucmd's io_uring context may be freed now if no inflight
+		 * ioucmd exists. Otherwise we may cause null-deref in ctx->fallback_work.
+		 *
+		 * Note: monitor_work sets UBLK_IO_FLAG_ABORTED and ends this request(releasing
+		 * the tag). Then the request is re-started(allocating the tag) and we are here.
+		 * Since releasing/allocating a tag implies smp_mb(), finding UBLK_IO_FLAG_ABORTED
+		 * guarantees that here is a re-issued request aborted previously.
+		 */
+		if ((io->flags & UBLK_IO_FLAG_ABORTED))
+			goto fail;
+
 		pdu->req = rq;
 		io_uring_cmd_complete_in_task(cmd, ublk_rq_task_work_cb);
 	}
-- 
2.27.0

Re: [PATCH V2 3/3] ublk_drv: do not add a re-issued request aborted previously to ioucmd's task_work

[Ming Lei]

On Mon, Aug 15, 2022 at 10:36:33AM +0800, ZiyangZhang wrote:
> In ublk_queue_rq(), Assume current request is a re-issued request aborted
> previously in monitor_work because the ubq_daemon(ioucmd's task) is
> PF_EXITING. For this request, we cannot call
> io_uring_cmd_complete_in_task() anymore because at that moment io_uring
> context may be freed in case that no inflight ioucmd exists. Otherwise,
> we may cause null-deref in ctx->fallback_work.
> 
> Add a check on UBLK_IO_FLAG_ABORTED to prevent the above situation. This
> check is safe and makes sense.
> 
> Note: monitor_work sets UBLK_IO_FLAG_ABORTED and ends this request
> (releasing the tag). Then the request is restarted(allocating the tag)
> and we are here. Since releasing/allocating a tag implies smp_mb(),
> finding UBLK_IO_FLAG_ABORTED guarantees that here is a re-issued request
> aborted previously.
> 
> Suggested-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com>

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


Thanks,
Ming

Re: [PATCH V2 0/3] ublk_drv: cleanup and bugfix

[Jens Axboe]

On Mon, 15 Aug 2022 10:36:30 +0800, ZiyangZhang wrote:
> The following 3 patches are cleanup and bugfix. Patch 1 and 2
> simply inline a function and update comments for ublk_drv's
> aborting machemism.
> 
> Patch 3 fix a null-deref bug reported by myself. Ming gives out a
> patch and I integrate it with more comments on this bug.
> 
> [...]

Applied, thanks!

[1/3] ublk_drv: check ubq_daemon_is_dying() in __ublk_rq_task_work()
      commit: 966120b51a245c9ff5857c5b169310c248e0ae87
[2/3] ublk_drv: update comment for __ublk_fail_req()
      commit: bb24174754afc5a7d185ca5406dcfbc608cdf157
[3/3] ublk_drv: do not add a re-issued request aborted previously to ioucmd's task_work
      commit: e6190dd0031d335c22586d34ef898301ed20f230

Best regards,
-- 
Jens Axboe