[PATCH 0/3] mtdchar: keep data and OOB aligned across ioctl chunks

[PATCH 0/3] mtdchar: keep data and OOB aligned across ioctl chunks

[James Hilliard]

MEMREAD and MEMWRITE split requests into bounded data and OOB buffers.
Their chunk lengths and flash-address updates must keep both streams
associated with the same pages.

An intermediate read can consume OOB from pages beyond its data chunk.
OOB-only requests do not advance the flash address between chunks, while
a buffer ending within a page's OOB can leave the next operation starting
at the wrong page. When the OOB buffer is the limiting factor, the data
chunk must stop at the same page boundary.

The OOB-limited case was found while testing a proposed BootROM-format
MTD view with 1024 data bytes and 1152 OOB bytes per page. A 64-page
request has 65536 data bytes and 73728 OOB bytes, but the original
65536-byte buffer limit lets the first operation consume all the data
while transferring only part of the OOB.

First simplify the page-capacity calculation, then bound OOB in
intermediate reads to their data pages. Finally keep non-final OOB chunks
page-aligned, limit their accompanying data and advance OOB-only requests
by the pages transferred.

Assisted-by: Codex:gpt-6-astra
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>

---
James Hilliard (3):
      mtdchar: calculate OOB capacity from the starting column
      mtdchar: bound OOB reads to the data in intermediate chunks
      mtdchar: keep OOB chunks aligned with their data pages

 drivers/mtd/mtdchar.c | 65 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 54 insertions(+), 11 deletions(-)
---
base-commit: 1c1a342aceec79528b3ba51f376eca2be5928fe7
change-id: 20261106-submit-mtdchar-page-chunking-v1-668636b0becd

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>


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

[PATCH 1/3] mtdchar: calculate OOB capacity from the starting column

[James Hilliard]

adjust_oob_length() needs the number of pages covered by an operation,
not their absolute indices. Calculate that count from the initial page
column and the data length instead of subtracting absolute page numbers.

Use a u64 product for the resulting OOB capacity before limiting the
requested OOB length. This keeps the calculation independent of the
absolute flash offset and avoids narrowing the capacity before the
comparison.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/mtd/mtdchar.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index bf01e6ac7293..19d33dacb62d 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -576,18 +576,17 @@ static int mtdchar_blkpg_ioctl(struct mtd_info *mtd,
 static void adjust_oob_length(struct mtd_info *mtd, uint64_t start,
 			      struct mtd_oob_ops *ops)
 {
-	uint32_t start_page, end_page;
+	u64 pages;
 	u32 oob_per_page;
 
 	if (ops->len == 0 || ops->ooblen == 0)
 		return;
 
-	start_page = mtd_div_by_ws(start, mtd);
-	end_page = mtd_div_by_ws(start + ops->len - 1, mtd);
+	pages = mtd_div_by_ws(mtd_mod_by_ws(start, mtd) + (u64)ops->len - 1,
+			      mtd) + 1;
 	oob_per_page = mtd_oobavail(mtd, ops);
 
-	ops->ooblen = min_t(size_t, ops->ooblen,
-			    (end_page - start_page + 1) * oob_per_page);
+	ops->ooblen = min_t(u64, ops->ooblen, pages * oob_per_page);
 }
 
 static noinline_for_stack int

-- 
2.53.0


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

[PATCH 2/3] mtdchar: bound OOB reads to the data in intermediate chunks

[James Hilliard]

MEMREAD limits the data and OOB lengths independently for each call to
mtd_read_oob(). A backend such as SPI-NAND continues reading until both
streams are exhausted, so an intermediate call can read OOB from pages
beyond the end of its data chunk.

The next call advances by the data bytes transferred, not by the pages
visited for OOB. If OOB remains, it starts again at pages already read
and returns their OOB a second time.

Limit OOB in intermediate reads to the pages covered by their data,
as MEMWRITE already does. Leave the final call unchanged so SPI-NAND
can still return additional OOB pages when no data chunk follows.

Fixes: 095bb6e44eb1 ("mtdchar: add MEMREAD ioctl")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/mtd/mtdchar.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 19d33dacb62d..189e462a9df0 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -769,6 +769,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);
 
+		if (ops.len < req.len)
+			adjust_oob_length(mtd, req.start, &ops);
+
 		ret = mtd_read_oob(mtd, (loff_t)req.start, &ops);
 
 		req.ecc_stats.uncorrectable_errors +=

-- 
2.53.0


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

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

[James Hilliard]

MEMREAD and MEMWRITE advance the flash address by the data bytes
transferred. OOB-only operations transfer no data, so another buffer-sized
chunk starts at the same address and repeats pages.

Advance OOB-only requests by the number of pages transferred. Round
non-final OOB chunks down to whole pages so the next call starts at the
next page, rather than repeating a partially consumed page. Allow the
buffer to hold at least one page of OOB and reject requests when the
selected mode has no OOB capacity.

When OOB is the limiting buffer, also cap the data chunk to those same
pages. Otherwise a combined request can exhaust its data before all OOB
has been transferred.

Fixes: 6420ac0af95d ("mtdchar: prevent unbounded allocation in MEMWRITE ioctl")
Fixes: 095bb6e44eb1 ("mtdchar: add MEMREAD ioctl")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/mtd/mtdchar.c | 53 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 189e462a9df0..85e98905f732 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;
 

-- 
2.53.0


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

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

[sashiko-bot]

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/