On Mon, Aug 29, 2022 at 02:13:12PM +0800, Ziyang Zhang wrote:

On 2022/8/29 13:40, Ming Lei wrote:

quoted

On Wed, Aug 24, 2022 at 01:47:39PM +0800, ZiyangZhang wrote:

quoted

If one rq is handled by io_uring_cmd_complete_in_task(), after a crash
this rq is actually handled by an io_uring fallback wq. We have to
end(abort) this rq since this fallback wq is a task other than the
crashed task. However, current code does not call io_uring_cmd_done()
at the same time but do it in ublk_cancel_queue(). With current design,
this does work because ublk_cancel_queue() is called AFTER del_gendisk(),
which waits for the rq ended(aborted) in fallback wq. This implies that
fallback wq on this rq is scheduled BEFORE calling io_uring_cmd_done()
on the corresponding ioucmd in ublk_cancel_queue().

Right.

quoted

However, while considering recovery feature, we cannot rely on
del_gendisk() or blk_mq_freeze_queue() to wait for completion of all
rqs because we may not want any aborted rq. Besides, io_uring does not
provide "flush fallback" machenism so we cannot trace this ioucmd.

Why not?

If user recovery is enabled, del_gendisk() can be replaced with
blk_mq_quiesce_queue(), then let abort work function do:

- cancel all in-flight requests by holding them into requeue list
  instead of finishing them as before, and this way is safe because
  abort worker does know the ubq daemon is dying
- cancel pending commands as before, because the situation is same
  with disk deleted or queue frozen

The problem is: we cannot control when fallback wq is scheduled.
So we are unsafe to call io_uring_cmd_done() in another process.

What is the other process?

It can't be fallback wq since any ublk request is aborted at the beginning
of __ublk_rq_task_work().

It shouldn't be the process calling ublk_cancel_dev(), since it is
safe to call io_uring_cmd_done() if ubq->nr_io_ready > 0.

Or others?

Otherwise, there is a UAF, just as
(5804987b7272f437299011c76b7363b8df6f8515: ublk_drv: do not add a
re-issued request aborted previously to ioucmd's task_work).

As I mentioned, del_gendisk() can be replaced with
blk_mq_quiesce_queue() in case of user recovery, then no any new
request can be queued after blk_mq_quiesce_queue() returns.

Yeah I know the answer is very simple: flush the fallback wq.
But here are two more questions:

I don't see why we need to flush fallback wq, care to provide some
details?

(1) Should ublk_drv rely on the fallback wq machenism?
    IMO, ublk_drv should not know detail of io_uring_cmd_complete_in_task()
    because its implementation may change in the future.
    BTW, I think current ublk_rq_task_work_cb() is not correct because
    it does not always call io_uring_cmd_done() before returning.
    nvme_uring_cmd_end_io() always calls io_uring_cmd_done() for each ioucmd
    no matter the rq succeeds or fails.

(2) Suppose io_uring does export the symbol 'flush_fallback_work', should we call
    it before starting a new process(recovery)?
    What if fallback wq is not scheduled immediately if there are many processes
    running and the system overhead is heavy. In this case the recovery process
    may wait for too long. Really we should not depend on fallback wq and please
    let the fallback wq complete the ioucmd itself.

quoted

With this way, the current abort logic won't be changed much.

And user recovery should only be started _after_ ublk device is found
as aborted.

START_RECOVERY will check if all ubq_daemons(the process) are PF_EXITING.

That is different. If START_RECOVERY is only run on aborted device, the
recovery handler could be simplified.

quoted

quoted

The recovery machenism needs to complete all ioucmds of a dying ubq
to avoid leaking io_uring ctx. But as talked above, we are unsafe
to call io_uring_cmd_done() in the recovery task if fallback wq happens
to run simultaneously. This is a UAF case because io_uring ctx may be
freed. Actually a similar case happens in
(5804987b7272f437299011c76b7363b8df6f8515: ublk_drv: do not add a
re-issued request aborted previously to ioucmd's task_work).

If you take the above approach, I guess there isn't such problem because
abort can handle the case well as before.

Ming, we did think this approach(quiesce, requeue rq/complete ioucmd)
at the very beginning. But we decided to drop it because we don not want
rely on 'flush fallback wq' machenism, which
makes ublk_drv rely on io_uring's internal implementation.

Then the focus is 'flush fallback wq', please see my above question.


Thanks,
Ming