| Message ID | 20220824054744.77812-3-ZiyangZhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk_drv: add USER_RECOVERY support | expand |
On Wed, Aug 24, 2022 at 01:47:37PM +0800, ZiyangZhang wrote: > Assume only a few FETCH_REQ ioucmds are sent to ublk_drv, then the > ubq_daemon exits, We have to call io_uring_cmd_done() for all ioucmds > received so that io_uring ctx will not leak. > > ublk_cancel_queue() may be called before START_DEV or after STOP_DEV, > we decrease ubq->nr_io_ready and clear UBLK_IO_FLAG_ACTIVE so that we > won't call io_uring_cmd_done() twice for one ioucmd to avoid UAF. Also > clearing UBLK_IO_FLAG_ACTIVE makes the code more reasonable. > > Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com> > --- > drivers/block/ublk_drv.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index c39b67d7133d..e08f636b0b9d 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -967,18 +967,23 @@ static void ublk_cancel_queue(struct ublk_queue *ubq) > { > int i; > > - if (!ublk_queue_ready(ubq)) > + if (!ubq->nr_io_ready) > return; > > for (i = 0; i < ubq->q_depth; i++) { > struct ublk_io *io = &ubq->ios[i]; > > - if (io->flags & UBLK_IO_FLAG_ACTIVE) > + if (io->flags & UBLK_IO_FLAG_ACTIVE) { > + pr_devel("%s: done old cmd: qid %d tag %d\n", > + __func__, ubq->q_id, i); > io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0); > + io->flags &= ~UBLK_IO_FLAG_ACTIVE; > + ubq->nr_io_ready--; > + } > } > > /* all io commands are canceled */ > - ubq->nr_io_ready = 0; > + WARN_ON_ONCE(ubq->nr_io_ready); The change looks fine, but suggest to add comment like the following given the above WARN_ON_ONCE() change isn't obvious. ``` 1) ublk_cancel_dev() is called before sending START_DEV(), ->mutex provides protection on above update. 2) ublk_cancel_dev() is called after sending START_DEV(), disk is deleted first, UBLK_IO_RES_ABORT is returned so that any new io command can't be issued to driver, so updating on io flags and nr_io_ready is safe here Also ->nr_io_ready is guaranteed to become zero after ublk_cance_queue returns since request queue is either frozen or not present in both two cases. ``` Thanks, Ming
On 2022/8/29 11:01, Ming Lei wrote: > On Wed, Aug 24, 2022 at 01:47:37PM +0800, ZiyangZhang wrote: >> Assume only a few FETCH_REQ ioucmds are sent to ublk_drv, then the >> ubq_daemon exits, We have to call io_uring_cmd_done() for all ioucmds >> received so that io_uring ctx will not leak. >> >> ublk_cancel_queue() may be called before START_DEV or after STOP_DEV, >> we decrease ubq->nr_io_ready and clear UBLK_IO_FLAG_ACTIVE so that we >> won't call io_uring_cmd_done() twice for one ioucmd to avoid UAF. Also >> clearing UBLK_IO_FLAG_ACTIVE makes the code more reasonable. >> >> Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com> >> --- >> drivers/block/ublk_drv.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c >> index c39b67d7133d..e08f636b0b9d 100644 >> --- a/drivers/block/ublk_drv.c >> +++ b/drivers/block/ublk_drv.c >> @@ -967,18 +967,23 @@ static void ublk_cancel_queue(struct ublk_queue *ubq) >> { >> int i; >> >> - if (!ublk_queue_ready(ubq)) >> + if (!ubq->nr_io_ready) >> return; >> >> for (i = 0; i < ubq->q_depth; i++) { >> struct ublk_io *io = &ubq->ios[i]; >> >> - if (io->flags & UBLK_IO_FLAG_ACTIVE) >> + if (io->flags & UBLK_IO_FLAG_ACTIVE) { >> + pr_devel("%s: done old cmd: qid %d tag %d\n", >> + __func__, ubq->q_id, i); >> io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0); >> + io->flags &= ~UBLK_IO_FLAG_ACTIVE; >> + ubq->nr_io_ready--; >> + } >> } >> >> /* all io commands are canceled */ >> - ubq->nr_io_ready = 0; >> + WARN_ON_ONCE(ubq->nr_io_ready); > > The change looks fine, but suggest to add comment like the > following given the above WARN_ON_ONCE() change isn't obvious. > > ``` > 1) ublk_cancel_dev() is called before sending START_DEV(), ->mutex > provides protection on above update. > > 2) ublk_cancel_dev() is called after sending START_DEV(), disk is > deleted first, UBLK_IO_RES_ABORT is returned so that any new io > command can't be issued to driver, so updating on io flags and > nr_io_ready is safe here > > Also ->nr_io_ready is guaranteed to become zero after ublk_cance_queue > returns since request queue is either frozen or not present in both two cases. > > ``` > Thanks for your advice, Ming. Regards, Zhang
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index c39b67d7133d..e08f636b0b9d 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -967,18 +967,23 @@ static void ublk_cancel_queue(struct ublk_queue *ubq) { int i; - if (!ublk_queue_ready(ubq)) + if (!ubq->nr_io_ready) return; for (i = 0; i < ubq->q_depth; i++) { struct ublk_io *io = &ubq->ios[i]; - if (io->flags & UBLK_IO_FLAG_ACTIVE) + if (io->flags & UBLK_IO_FLAG_ACTIVE) { + pr_devel("%s: done old cmd: qid %d tag %d\n", + __func__, ubq->q_id, i); io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0); + io->flags &= ~UBLK_IO_FLAG_ACTIVE; + ubq->nr_io_ready--; + } } /* all io commands are canceled */ - ubq->nr_io_ready = 0; + WARN_ON_ONCE(ubq->nr_io_ready); } /* Cancel all pending commands, must be called after del_gendisk() returns */
Assume only a few FETCH_REQ ioucmds are sent to ublk_drv, then the ubq_daemon exits, We have to call io_uring_cmd_done() for all ioucmds received so that io_uring ctx will not leak. ublk_cancel_queue() may be called before START_DEV or after STOP_DEV, we decrease ubq->nr_io_ready and clear UBLK_IO_FLAG_ACTIVE so that we won't call io_uring_cmd_done() twice for one ioucmd to avoid UAF. Also clearing UBLK_IO_FLAG_ACTIVE makes the code more reasonable. Signed-off-by: ZiyangZhang <ZiyangZhang@linux.alibaba.com> --- drivers/block/ublk_drv.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)