Re: [PATCH 3/3] mtdchar: keep OOB chunks aligned with their data pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtdchar: incomplete OOB transfer when combined request
  exhausts req.len
--

commit 148431ec8c48f9426f60a2b52dc5e2f9aab56347
Author: James Hilliard <james.hilliard1@gmail.com>

mtdchar: keep OOB chunks aligned with their data pages

This commit rounds non-final OOB chunks down to whole pages, caps the data
chunk to the corresponding page count when OOB is the limiting buffer, and
advances req.start by the number of OOB pages transferred on OOB-only
operations.

> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index 189e462a9df0f..85e98905f7320 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -589,6 +589,30 @@ static void adjust_oob_length(struct mtd_info *mtd, uint64_t start,
>  	ops->ooblen = min_t(u64, ops->ooblen, pages * oob_per_page);
>  }
>  
> +static int mtdchar_adjust_io_lengths(struct mtd_info *mtd, uint64_t start,
> +				     struct mtd_oob_ops *ops, uint64_t remaining_oob)
> +{
> +	u32 oob_per_page = mtd_oobavail(mtd, ops);
> +	u64 len;
> +
> +	if (!ops->ooblen)
> +		return 0;
> +	if (!oob_per_page)
> +		return -EINVAL;
> +
> +	if (ops->ooblen < remaining_oob) {
> +		/* Keep a page's OOB together when another chunk follows. */
> +		ops->ooblen = rounddown(ops->ooblen, oob_per_page);
> +		if (ops->len) {
> +			len = (u64)(ops->ooblen / oob_per_page) * mtd->writesize;
> +			len -= mtd_mod_by_ws(start, mtd);
> +			ops->len = min_t(u64, ops->len, len);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static noinline_for_stack int
>  mtdchar_write_ioctl(struct mtd_info *mtd, struct mtd_write_req __user *argp)
>  {
> @@ -628,7 +652,8 @@ mtdchar_write_ioctl(struct mtd_info *mtd, struct mtd_write_req __user *argp)
>  			return -ENOMEM;
>  	}
>  
> -	oobbuf_len = min_t(size_t, req.ooblen, mtd->erasesize);
> +	oobbuf_len = min_t(size_t, req.ooblen,
> +			   max(mtd->erasesize, mtd->oobsize));
>  	if (oobbuf_len > 0) {
>  		oobbuf = kvmalloc(oobbuf_len, GFP_KERNEL);
>  		if (!oobbuf) {
> @@ -648,13 +673,17 @@ mtdchar_write_ioctl(struct mtd_info *mtd, struct mtd_write_req __user *argp)
>  
>  		/*
>  		 * Shorten non-page-aligned, eraseblock-sized writes so that
> -		 * the write ends on an eraseblock boundary.  This is necessary
> +		 * the write ends on a page boundary.  This is necessary
>  		 * for adjust_oob_length() to properly handle non-page-aligned
>  		 * writes.
>  		 */
>  		if (ops.len == mtd->erasesize)
>  			ops.len -= mtd_mod_by_ws(req.start + ops.len, mtd);
>  
> +		ret = mtdchar_adjust_io_lengths(mtd, req.start, &ops, req.ooblen);
> +		if (ret)
> +			break;
> +
>  		/*
>  		 * For writes which are not OOB-only, adjust the amount of OOB
>  		 * data written according to the number of data pages written.
> @@ -674,7 +703,11 @@ mtdchar_write_ioctl(struct mtd_info *mtd, struct mtd_write_req __user *argp)
>  		if (ret)
>  			break;
>  
> -		req.start += ops.retlen;
> +		if (usr_data)
> +			req.start += ops.retlen;
> +		else
> +			req.start += (u64)(ops.oobretlen / mtd_oobavail(mtd, &ops)) *
> +				     mtd->writesize;
>  		req.len -= ops.retlen;
>  		usr_data += ops.retlen;
>  
> @@ -739,7 +772,8 @@ mtdchar_read_ioctl(struct mtd_info *mtd, struct mtd_read_req __user *argp)
>  		}
>  	}
>  
> -	oobbuf_len = min_t(size_t, req.ooblen, mtd->erasesize);
> +	oobbuf_len = min_t(size_t, req.ooblen,
> +			   max(mtd->erasesize, mtd->oobsize));
>  	if (oobbuf_len > 0) {
>  		oobbuf = kvmalloc(oobbuf_len, GFP_KERNEL);
>  		if (!oobbuf) {
> @@ -761,7 +795,7 @@ mtdchar_read_ioctl(struct mtd_info *mtd, struct mtd_read_req __user *argp)
>  
>  		/*
>  		 * Shorten non-page-aligned, eraseblock-sized reads so that the
> -		 * read ends on an eraseblock boundary.  This is necessary in
> +		 * read ends on a page boundary.  This is necessary in
>  		 * order to prevent OOB data for some pages from being
>  		 * duplicated in the output of non-page-aligned reads requiring
>  		 * multiple mtd_read_oob() calls to be completed.
> @@ -769,6 +803,9 @@ mtdchar_read_ioctl(struct mtd_info *mtd, struct mtd_read_req __user *argp)
>  		if (ops.len == mtd->erasesize)
>  			ops.len -= mtd_mod_by_ws(req.start + ops.len, mtd);
>  
> +		ret = mtdchar_adjust_io_lengths(mtd, req.start, &ops, req.ooblen);
> +		if (ret)
> +			break;
>  		if (ops.len < req.len)
>  			adjust_oob_length(mtd, req.start, &ops);
>  
> @@ -789,7 +826,11 @@ mtdchar_read_ioctl(struct mtd_info *mtd, struct mtd_read_req __user *argp)
>  			break;
>  		}
>  
> -		req.start += ops.retlen;
> +		if (usr_data)
> +			req.start += ops.retlen;
> +		else
> +			req.start += (u64)(ops.oobretlen / mtd_oobavail(mtd, &ops)) *
> +				     mtd->writesize;
>  		req.len -= ops.retlen;
>  		usr_data += ops.retlen;

[Severity: Medium]
Can a combined data and OOB request still exhaust req.len and exit the I/O
loop before all of req.ooblen has been transferred?

When req.ooblen exceeds oobbuf_len (so ops->ooblen < remaining_oob) and the
remaining data req.len fits within the pages covered by ops->ooblen,
min_t(u64, ops->len, len) in mtdchar_adjust_io_lengths() leaves ops.len equal
to req.len.  In mtdchar_read_ioctl(), adjust_oob_length() is then skipped
because ops.len == req.len, so mtd_read_oob() consumes all of req.len along
with ops.ooblen bytes of OOB across pages beyond ops.retlen.

Because usr_data is non-NULL, req.start only advances by ops.retlen instead
of the OOB pages visited, req.len drops to 0 while req.ooblen is still
non-zero, and the loop condition

  while (req.len > 0 || (!usr_data && req.ooblen > 0))

terminates the ioctl without transferring the remaining OOB chunks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-submit-mtdchar-page-chunking-v1-v1-0-2973a03531bc@gmail.com?part=3

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