| Message ID | 20250408072440.1977943-2-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ublk: two fixes | expand |
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 >
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
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
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,
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(-)