Re: [PATCH 1/2] mtd: ubi: block: forbid direct reclaim I/O during synchronous read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] mtd: block: uninitialized use of 'ret' on 0-byte read
--

commit d98a6ab0cd0bd3200e8fcacff751a85e2f9ee372
Author: Ye Linfei <yelinfei@ruijie.com.cn>

mtd: ubi: block: forbid direct reclaim I/O during synchronous read

This commit marks the entire ubiblock_read operation with
memalloc_noio_save() and memalloc_noio_restore() to forbid direct reclaim
I/O while the driver executes. This prevents a deadlock where synchronous
swap writeback could be triggered via direct reclaim while the task is
already holding the SRCU read lock in the blk-mq synchronous dispatch path.

Cc: stable@vger.kernel.org

> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> index 29c0d6941a818..c9d42181bf648 100644
> --- a/drivers/mtd/ubi/block.c
> +++ b/drivers/mtd/ubi/block.c

[ ... ]

> @@ -189,6 +190,7 @@ static blk_status_t ubiblock_read(struct request *req)
>  	int leb = pos;
>  	struct req_iterator iter;
>  	struct bio_vec bvec;
> +	unsigned int noio_flags;
>  	int ret;
>  
>  	blk_mq_start_request(req);
> @@ -201,6 +203,18 @@ static blk_status_t ubiblock_read(struct request *req)
>  	ubi_sgl_init(&pdu->usgl);
>  	blk_rq_map_sg(req, pdu->usgl.sg);
>  
> +	/*
> +	 * This entire callback runs in the synchronous dispatch path of
> +	 * blk-mq: submit_bio_noacct_nocheck() only queues a bio submitted
> +	 * from here onto current->bio_list, and the sole dispatcher of
> +	 * that list is this very task, busy inside ->queue_rq().  Any
> +	 * block I/O issued from below would therefore deadlock against
> +	 * itself (e.g. synchronous swap writeback reached via direct
> +	 * reclaim).  Forbid it for the whole read instead of relying on
> +	 * every callee using safe allocation flags.
> +	 */
> +	noio_flags = memalloc_noio_save();
> +
>  	while (bytes_left) {
>  		/*
>  		 * We can only read one LEB at a time. Therefore if the read
> @@ -221,6 +235,7 @@ static blk_status_t ubiblock_read(struct request *req)
>  
>  	rq_for_each_segment(bvec, req, iter)
>  		flush_dcache_page(bvec.bv_page);
> +	memalloc_noio_restore(noio_flags);
>  
>  	blk_mq_end_request(req, errno_to_blk_status(ret));

[Severity: Low]
Does ubiblock_read() in drivers/mtd/ubi/block.c leave ret uninitialized if a
0-byte read request is submitted?

If blk_rq_bytes(req) evaluates to 0, bytes_left is 0 and the loop is bypassed:

    int to_read = blk_rq_bytes(req);
    int bytes_left = to_read;
    ...
    int ret;
    ...
    while (bytes_left) { // bypassed completely
        ...
        ret = ubi_read_sg(...);
    }

This leaves the uninitialized ret value on the stack to be consumed here by
errno_to_blk_status(), returning an unpredictable block completion status.

While 0-byte reads might be rare in practice, the block layer does not
structurally filter out dataless REQ_OP_READ bios.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792046946.git.yelinfei@ruijie.com.cn?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/