Re: [PATCH v2 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL

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

New issues:
- [High] Using GFP_KERNEL in the non-blocking io_uring issue path introduces a permanent task_work deadlock under memory pressure.

Pre-existing issues:
- [High] Allocating memory with GFP_KERNEL while holding a block layer request tag causes a tag exhaustion deadlock if memory reclaim needs to write to the same device.
--

--- Patch [2]: [PATCH v2 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL ---
commit 9c93dc072bebb5f4c7db406ce91a1fb2ffa4a9e7
Author: Yang Xiuwei <[email protected]>

scsi: bsg: map io_uring user buffers with GFP_KERNEL

IO_URING_F_NONBLOCK only needs BLK_MQ_REQ_NOWAIT for request
allocation. Mapping the user buffer may sleep, so use GFP_KERNEL
instead of GFP_NOWAIT.

> diff --git a/drivers/scsi/scsi_bsg.c b/drivers/scsi/scsi_bsg.c
> index 7758c5f22a7e9..ddb33ae684837 100644
> --- a/drivers/scsi/scsi_bsg.c
> +++ b/drivers/scsi/scsi_bsg.c
> @@ -89,10 +88,10 @@ static int scsi_bsg_map_user_buffer(struct request *req,
>  						&iter, ioucmd, issue_flags);
>  		if (ret < 0)
>  			return ret;
> -		ret = blk_rq_map_user_iov(req->q, req, NULL, &iter, gfp_mask);
> +		ret = blk_rq_map_user_iov(req->q, req, NULL, &iter, GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does using GFP_KERNEL here cause a tag
exhaustion deadlock?

Prior to this call, scsi_alloc_request() consumes a hardware tag. If
GFP_KERNEL enters direct reclaim and needs to write dirty pages back to the
same block device, the filesystem will attempt to allocate another tag.

If all tags are currently held by blocked BSG tasks, won't the system deadlock?

>  	} else {
>  		ret = blk_rq_map_user(req->q, req, NULL, uptr64(buf_addr),
> -				      buf_len, gfp_mask);
> +				      buf_len, GFP_KERNEL);
>  	}
>  
>  	return ret;

[ ... ]

> @@ -132,10 +130,8 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
>  	if (cmd->dout_iovec_count > 0 || cmd->din_iovec_count > 0)
>  		return -EOPNOTSUPP;
>  
> -	if (issue_flags & IO_URING_F_NONBLOCK) {
> +	if (issue_flags & IO_URING_F_NONBLOCK)
>  		blk_flags = BLK_MQ_REQ_NOWAIT;
> -		gfp_mask = GFP_NOWAIT;
> -	}
>  
>  	req = scsi_alloc_request(q, dout_xfer_len ?
>  				 REQ_OP_DRV_OUT : REQ_OP_DRV_IN, blk_flags);

[Severity: High]
Does removing the GFP_NOWAIT fallback for the non-blocking io_uring issue
path introduce a permanent task_work deadlock?

If a user submits more than 256 IORING_OP_URING_CMD operations in a single
batch, the bio allocations will exhaust the fs_bio_set mempool. By passing
GFP_KERNEL to blk_rq_map_user_iov(), mempool_alloc() will put the submitter
thread to sleep waiting for a bio to be freed.

Since these bios are freed when their completions run via task_work
(scsi_bsg_uring_task_cb), which must be executed by the very submitter thread
now permanently blocked in mempool_alloc(), does this create an unresolvable
circular dependency?

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