Re: [PATCH v7 04/19] s390/dasd: Snapshot intrc before freeing the request block

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Accessing `req->q` after the request has been freed by `blk_mq_end_request()`.
- [High] Accessing `req` inside `free_cp()` after it has been requeued to the block layer in `dasd_generic_requeue_all_requests()`.
- [High] `cqr->dq` is dereferenced after `cqr` is freed in `dasd_flush_block_queue()`.
--

--- Patch [4]: [PATCH v7 04/19] s390/dasd: Snapshot intrc before freeing the request block ---
commit 0c9b1e836f68d2761aa0c36acf690d7404a26d86
Author: Stefan Haberland <[email protected]>

s390/dasd: Snapshot intrc before freeing the request block

__dasd_cleanup_cqr() maps the completion result to a block status by
reading cqr->intrc, but only after discipline->free_cp() has returned the
request block to its memory pool. On SMP another CPU can reallocate that
block and overwrite cqr->intrc before it is read, completing the request
with the wrong error. This patch snapshots intrc before free_cp() is called.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index dbe3caa1e0b4a..f5585c549e09e 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -2687,17 +2687,23 @@ static void __dasd_cleanup_cqr(struct dasd_ccw_req *cqr)
>  	struct request *req;
>  	blk_status_t error = BLK_STS_OK;
>  	unsigned int proc_bytes;
> -	int status;
> +	int status, intrc;
>  
>  	req = (struct request *) cqr->callback_data;
>  	dasd_profile_end(cqr->block, cqr, req);
>  
> +	/*
> +	 * free_cp() returns the request block to its memory pool, so snapshot
> +	 * everything still needed from cqr before calling it - another CPU can
> +	 * reallocate and overwrite the block right after.
> +	 */
>  	proc_bytes = cqr->proc_bytes;
> +	intrc = cqr->intrc;
>  	status = cqr->block->base->discipline->free_cp(cqr, req);
>  	if (status < 0)
>  		error = errno_to_blk_status(status);
>  	else if (status == 0) {
> -		switch (cqr->intrc) {
> +		switch (intrc) {
>  		case -EPERM:
>  			/*
>  			 * DASD doesn't implement SCSI/NVMe reservations, but it

[Severity: High]
This isn't a bug introduced by this patch, but further down in
__dasd_cleanup_cqr() we have:

	if (error) {
		blk_mq_end_request(req, error);
		blk_mq_run_hw_queues(req->q, true);

Is it safe to dereference req->q here?

Once blk_mq_end_request() completes and frees the request back to the block
layer's tag pool, could another thread reallocate and modify req before we
access req->q?

The same pattern also appears in dasd_request_done():

	blk_mq_end_request(req, 0);
	blk_mq_run_hw_queues(req->q, true);


[Severity: High]
This is a pre-existing issue, but in dasd_flush_block_queue():

		spin_lock_irqsave(&cqr->dq->lock, flags);
		cqr->endclk = get_tod_clock();
		list_del_init(&cqr->blocklist);
		__dasd_cleanup_cqr(cqr);
		spin_unlock_irqrestore(&cqr->dq->lock, flags);

Since __dasd_cleanup_cqr(cqr) calls free_cp() which returns cqr to its
memory pool, is it safe to evaluate cqr->dq in the spin_unlock_irqrestore()
call right after?

If another CPU reallocates and overwrites cqr in the meantime, could
cqr->dq evaluate to NULL or point to unrelated memory causing lock
corruption?


[Severity: High]
This is a pre-existing issue, but looking at dasd_generic_requeue_all_requests():

		_dasd_requeue_request(cqr);
		list_del_init(&cqr->blocklist);
		cqr->block->base->discipline->free_cp(
			cqr, (struct request *) cqr->callback_data);

Since _dasd_requeue_request() passes ownership of the request back to the
block layer, could the block layer immediately dispatch and complete it?

If so, is it safe to pass req to free_cp() where it might iterate over the
request's bios via rq_for_each_segment() and potentially copy bounce buffers?

Could req be freed or its bios concurrently modified by the block layer before
free_cp() completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4