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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditionally using GFP_KERNEL forces the io_uring submitter thread to sleep, breaking non-blocking semantics.

Pre-existing issues:
- [High] Using GFP_KERNEL while holding a block layer request tag introduces a tag exhaustion deadlock during memory reclaim.
--

--- Patch [2]: [PATCH 2/2] scsi: bsg: map io_uring user buffers with GFP_KERNEL ---
commit e89699f2c58014fd3728ce2e8bed2b71fc1c0abf
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>

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 c57ce01379de5..f90d19fdbd8ea 100644
> --- a/drivers/scsi/scsi_bsg.c
> +++ b/drivers/scsi/scsi_bsg.c
> @@ -76,9 +76,8 @@ static enum rq_end_io_ret scsi_bsg_uring_cmd_done(struct request *req,
>  
>  static int scsi_bsg_map_user_buffer(struct request *req,
>  				    struct io_uring_cmd *ioucmd,
> -				    unsigned int issue_flags, gfp_t gfp_mask,
> -				    bool is_write, u64 buf_addr,
> -				    unsigned long buf_len)
> +				    unsigned int issue_flags, bool is_write,
> +				    u64 buf_addr, unsigned long buf_len)
>  {
>  	struct iov_iter iter;
>  	int ret;
> @@ -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 while holding a
block request tag risk a tag exhaustion deadlock?

The request tag is allocated in scsi_alloc_request() prior to this mapping
step. If blk_rq_map_user_iov() triggers direct memory reclaim, and reclaim
attempts to write dirty pages to this same block device, it will need a tag.
If all tags are currently held by concurrent tasks stuck in this mapping
phase, won't the writeback block indefinitely?

>  	} 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);

[ ... ]

> @@ -170,8 +166,7 @@ static int scsi_bsg_uring_cmd(struct request_queue *q, struct io_uring_cmd *iouc
>  		unsigned long buf_len = is_write ? dout_xfer_len : din_xfer_len;
>  
>  		ret = scsi_bsg_map_user_buffer(req, ioucmd, issue_flags,
> -					       gfp_mask, is_write, buf_addr,
> -					       buf_len);
> +					       is_write, buf_addr, buf_len);

[Severity: High]
Since this changes the allocation to GFP_KERNEL unconditionally, won't this
illegally block the io_uring submission context when IO_URING_F_NONBLOCK is
set?

If a non-blocking context like SQPOLL is forced to block for memory
allocation or page faults during scsi_bsg_map_user_buffer(), doesn't it stall
all other rings sharing the thread?

Should this path attempt mapping with GFP_NOWAIT first, and return -EAGAIN on
-ENOMEM so the operation can be safely punted to an async io-wq worker?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720032338.461681-1-yangxiuwei@kylinos.cn?part=2