Re: [PATCH v3 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] Unconditionally using GFP_KERNEL for memory allocation when IO_URING_F_NONBLOCK is set allows the io_uring submitter thread to sleep, violating non-blocking semantics.

Pre-existing issues:
- [High] Allocating memory with GFP_KERNEL while holding a block request tag can trigger a tag exhaustion deadlock in the block layer.
--

--- Patch [2]: [PATCH v3 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL ---
commit 4107274feb8462924a88001746916a4f70f777fe
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);
>  	} else {
>  		ret = blk_rq_map_user(req->q, req, NULL, uptr64(buf_addr),
> -				      buf_len, gfp_mask);
> +				      buf_len, GFP_KERNEL);
>  	}

[Severity: High]
This is a pre-existing issue, but does this allocation trigger a tag
exhaustion deadlock?

In scsi_bsg_uring_cmd(), scsi_alloc_request() has already been called, which
consumes a hardware tag from the block device's limited pool.

Calling blk_rq_map_user_iov() with GFP_KERNEL immediately after allows the
allocation to sleep in memory reclaim. If concurrent bsg io_uring commands
allocate all available hardware tags and then enter direct reclaim, and the
reclaim path attempts to write dirty pages back to the same block device, it
will require a hardware tag.

Since all tags would be held by threads waiting for memory, the reclaim
writeback cannot proceed. Shouldn't allocations performed while holding a
block tag use GFP_NOIO or GFP_NOWAIT to prevent block IO recursion?

[ ... ]

> @@ -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_mask downgrade here violate io_uring non-blocking
semantics?

When IO_URING_F_NONBLOCK is set, the io_uring core expects the issue handler
to be strictly non-blocking. If it cannot allocate memory without sleeping, it
should return -EAGAIN so the request can be deferred to an asynchronous io-wq
worker thread.

By removing GFP_NOWAIT and unconditionally using GFP_KERNEL for mapping the
buffer, won't this allow the submission thread (such as the SQPOLL thread)
to block in direct reclaim, stalling other independent operations on the
same ring?

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