| Message ID | 20220809091629.104682-4-ZiyangZhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk_drv: cleanup and bugfix | expand |
On Tue, Aug 9, 2022 at 5:17 PM ZiyangZhang <ZiyangZhang@linux.alibaba.com> 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> > --- > 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 bedef46f6abf..0b9bd9e02b53 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -725,6 +725,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx, > { > struct ublk_queue *ubq = hctx->driver_data; > struct request *rq = bd->rq; > + struct ublk_io *io = &ubq->ios[rq->tag]; The above line should be moved into branch of not using task work, otherwise this patch looks fine. Thanks,
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index bedef46f6abf..0b9bd9e02b53 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -725,6 +725,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx, { struct ublk_queue *ubq = hctx->driver_data; struct request *rq = bd->rq; + struct ublk_io *io = &ubq->ios[rq->tag]; blk_status_t res; /* fill iod to slot in io cmd buffer */ @@ -748,9 +749,24 @@ 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 io_uring_cmd *cmd = io->cmd; struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd); + /* + * If the check passes, 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); }
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(-)