Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 Aug 2022 13:40:36 +0800
From:   Ming Lei <ming.lei@redhat.com>
To:     ZiyangZhang <ZiyangZhang@linux.alibaba.com>
Cc:     axboe@kernel.dk, xiaoguang.wang@linux.alibaba.com,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        joseph.qi@linux.alibaba.com, ming.lei@redhat.com
Subject: Re: [RFC PATCH 4/9] ublk_drv: refactor __ublk_rq_task_work() and
 aborting machenism
Message-ID: <YwxRVEQlIw3oWmwE@T590>
References: <20220824054744.77812-1-ZiyangZhang@linux.alibaba.com>
 <20220824054744.77812-5-ZiyangZhang@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220824054744.77812-5-ZiyangZhang@linux.alibaba.com>
Precedence: bulk

On Wed, Aug 24, 2022 at 01:47:39PM +0800, ZiyangZhang wrote:
> 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.

> 
> 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

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.

> 
> 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.

> 
> Besides, in order to implement recovery machenism, in ublk_queue_rq()
> and __ublk_rq_task_work(), we should not end(abort) current rq while
> ubq_daemon is dying.

Right, I believe one helper of ublk_abort_request() is helpful here.


Thanks, 
Ming