Re: [PATCH 6/8] block: drivers: complete request locally from blk_mq_tagset_busy_iter

From: Ming Lei <ming.lei@redhat.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	Khazhy Kumykov <khazhy@google.com>,
	Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>,
	Hannes Reinecke <hare@suse.de>,
	John Garry <john.garry@huawei.com>,
	David Jeffery <djeffery@redhat.com>
Subject: Re: [PATCH 6/8] block: drivers: complete request locally from blk_mq_tagset_busy_iter
Date: Tue, 27 Apr 2021 16:54:17 +0800	[thread overview]
Message-ID: <YIfRObi3RTEbA5vd@T590> (raw)
In-Reply-To: <YIZcuMkLV1+DKhLZ@T590>

On Mon, Apr 26, 2021 at 02:24:56PM +0800, Ming Lei wrote:
> On Sun, Apr 25, 2021 at 08:02:10PM -0700, Bart Van Assche wrote:
> > On 4/25/21 1:57 AM, Ming Lei wrote:
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index c289991ffaed..7cbaee282b6d 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -1568,7 +1568,11 @@ static void scsi_mq_done(struct scsi_cmnd *cmd)
> > >  	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))
> > >  		return;
> > >  	trace_scsi_dispatch_cmd_done(cmd);
> > > -	blk_mq_complete_request(cmd->request);
> > > +
> > > +	if (unlikely(host_byte(cmd->result) != DID_OK))
> > > +		blk_mq_complete_request_locally(cmd->request);
> > > +	else
> > > +		blk_mq_complete_request(cmd->request);
> > >  }
> > 
> > This change is so tricky that it deserves a comment.
> > 
> > An even better approach would be *not* to export
> > blk_mq_complete_request_locally() from the block layer to block drivers
> > and instead modify the block layer such that it completes a request on
> > the same CPU if request completion happens from inside the context of a
> > tag iteration function. That would save driver writers the trouble of
> > learning yet another block layer API.
> 
> Yeah, that is possible, and one request flag(eg. RQF_ITERATED) can be added.
> The flag is set before calling ->fn(), and evaluated in
> blk_mq_complete_request_remote().

Thinking of the issue further, we have grabbed rq->refcnt before calling
->fn(), not only request UAF is fixed, but also driver gets valid
request instance to check if the request has been completed, so we needn't
to consider the double completion issue[1] any more, which is supposed
to be covered by driver, such as, scsi uses SCMD_STATE_COMPLETE in
scsi_mq_done() for avoiding double completion.

[1] https://lore.kernel.org/linux-block/YIdWz8C5eklPvEiV@T590/T/#u

Thanks,
Ming