[PATCH 00/22] dm-verity: more FEC fixes and cleanups

[PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Eric Biggers]

This series applies to linux-dm/for-next.  It can also be retrieved from:

    git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git dm-verity-fec-2026-02-05

Patches 1-5 fix bugs in dm-verity's forward error correction (FEC):

- FEC and hash devices that are too small were not rejected.

- Corrected blocks could be multiple-counted in statistics.

- The erasures array was significantly oversized for its use case.

- An out-of-bounds read could occur when decoding an RS codeword whose
  parity bytes span a block boundary.

Patches 6-22 clean up the FEC implementation to be easier to understand
and improve documentation and log messages.

Eric Biggers (22):
  dm-verity-fec: correctly reject too-small FEC devices
  dm-verity-fec: correctly reject too-small hash devices
  dm-verity-fec: fix corrected block count stat
  dm-verity-fec: fix the size of dm_verity_fec_io::erasures
  dm-verity-fec: fix reading parity bytes split across blocks (take 3)
  dm-verity: rename dm_verity::hash_blocks to dm_verity::hash_end
  dm-verity-fec: improve documentation for Forward Error Correction
  dm-verity-fec: replace {MAX,MIN}_RSN with {MIN,MAX}_ROOTS
  dm-verity-fec: use standard names for Reed-Solomon parameters
  dm-verity-fec: rename "RS block" to "RS codeword"
  dm-verity-fec: replace io_size with block_size
  dm-verity-fec: rename rounds to region_blocks
  dm-verity-fec: simplify computation of rsb
  dm-verity-fec: simplify computation of ileaved
  dm-verity-fec: simplify deinterleaving
  dm-verity-fec: rename block_offset to out_pos
  dm-verity-fec: move computation of offset and rsb down a level
  dm-verity-fec: compute target region directly
  dm-verity-fec: pass down index_in_region instead of rsb
  dm-verity-fec: make fec_decode_bufs() just return 0 or error
  dm-verity-fec: log target_block instead of index_in_region
  dm-verity-fec: improve comments for fec_read_bufs()

 .../admin-guide/device-mapper/verity.rst      | 122 +++++-
 drivers/md/dm-verity-fec.c                    | 380 ++++++++----------
 drivers/md/dm-verity-fec.h                    |  28 +-
 drivers/md/dm-verity-target.c                 |   8 +-
 drivers/md/dm-verity.h                        |   4 +-
 5 files changed, 294 insertions(+), 248 deletions(-)


base-commit: 218b16992a37ea97b9e09b7659a25a864fb9976f
-- 
2.52.0

[PATCH 01/22] dm-verity-fec: correctly reject too-small FEC devices

[Eric Biggers]

Fix verity_fec_ctr() to reject too-small FEC devices by correctly
computing the number of parity blocks as 'f->rounds * f->roots'.
Previously it incorrectly used 'div64_u64(f->rounds * f->roots,
v->fec->roots << SECTOR_SHIFT)' which is a much smaller value.

Note that the units of 'rounds' are blocks, not bytes.  This matches the
units of the value returned by dm_bufio_get_device_size(), which are
also blocks.  A later commit will give 'rounds' a clearer name.

Fixes: a739ff3f543a ("dm verity: add support for forward error correction")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 7583607a8aa62..5c276d0fc20c0 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -623,11 +623,11 @@ int verity_fec_ctr_alloc(struct dm_verity *v)
  */
 int verity_fec_ctr(struct dm_verity *v)
 {
 	struct dm_verity_fec *f = v->fec;
 	struct dm_target *ti = v->ti;
-	u64 hash_blocks, fec_blocks;
+	u64 hash_blocks;
 	int ret;
 
 	if (!verity_fec_is_enabled(v)) {
 		verity_fec_dtr(v);
 		return 0;
@@ -704,12 +704,11 @@ int verity_fec_ctr(struct dm_verity *v)
 		return PTR_ERR(f->bufio);
 	}
 
 	dm_bufio_set_sector_offset(f->bufio, f->start << (v->data_dev_block_bits - SECTOR_SHIFT));
 
-	fec_blocks = div64_u64(f->rounds * f->roots, v->fec->roots << SECTOR_SHIFT);
-	if (dm_bufio_get_device_size(f->bufio) < fec_blocks) {
+	if (dm_bufio_get_device_size(f->bufio) < f->rounds * f->roots) {
 		ti->error = "FEC device is too small";
 		return -E2BIG;
 	}
 
 	f->data_bufio = dm_bufio_client_create(v->data_dev->bdev,
-- 
2.52.0

[PATCH 02/22] dm-verity-fec: correctly reject too-small hash devices

[Eric Biggers]

Fix verity_fec_ctr() to reject too-small hash devices by correctly
taking hash_start into account.

Note that this is necessary because dm-verity doesn't call
dm_bufio_set_sector_offset() on the hash device's bufio client
(v->bufio).  Thus, dm_bufio_get_device_size(v->bufio) returns a size
relative to 0 rather than hash_start.  An alternative fix would be to
call dm_bufio_set_sector_offset() on v->bufio, but then all the code
that reads from the hash device would have to be adjusted accordingly.

Fixes: a739ff3f543a ("dm verity: add support for forward error correction")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 5c276d0fc20c0..9f06bd66bae31 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -686,11 +686,12 @@ int verity_fec_ctr(struct dm_verity *v)
 	/*
 	 * Metadata is accessed through the hash device, so we require
 	 * it to be large enough.
 	 */
 	f->hash_blocks = f->blocks - v->data_blocks;
-	if (dm_bufio_get_device_size(v->bufio) < f->hash_blocks) {
+	if (dm_bufio_get_device_size(v->bufio) <
+	    v->hash_start + f->hash_blocks) {
 		ti->error = "Hash device is too small for "
 			DM_VERITY_OPT_FEC_BLOCKS;
 		return -E2BIG;
 	}
 
-- 
2.52.0

[PATCH 03/22] dm-verity-fec: fix corrected block count stat

[Eric Biggers]

dm_verity_fec::corrected seems to have been intended to count the number
of corrected blocks.  However, it actually counted the number of calls
to fec_decode_bufs() that corrected at least one error.  That's not the
same thing.  For example, in low-memory situations correcting a single
block can require many calls to fec_decode_bufs().

Fix it to count corrected blocks instead.

Fixes: ae97648e14f7 ("dm verity fec: Expose corrected block count via status")
Cc: Shubhankar Mishra <shubhankarm@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 9f06bd66bae31..d4a9367a2fee6 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -161,15 +161,13 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	dm_bufio_release(buf);
 
 	if (r < 0 && neras)
 		DMERR_LIMIT("%s: FEC %llu: failed to correct: %d",
 			    v->data_dev->name, (unsigned long long)rsb, r);
-	else if (r > 0) {
+	else if (r > 0)
 		DMWARN_LIMIT("%s: FEC %llu: corrected %d errors",
 			     v->data_dev->name, (unsigned long long)rsb, r);
-		atomic64_inc(&v->fec->corrected);
-	}
 
 	return r;
 }
 
 /*
@@ -437,10 +435,11 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		if (r < 0)
 			goto done;
 	}
 
 	memcpy(dest, fio->output, 1 << v->data_dev_block_bits);
+	atomic64_inc(&v->fec->corrected);
 
 done:
 	fio->level--;
 	return r;
 }
-- 
2.52.0

[PATCH 04/22] dm-verity-fec: fix the size of dm_verity_fec_io::erasures

[Eric Biggers]

At most 25 entries in dm_verity_fec_io::erasures are used: the maximum
number of FEC roots plus one.  Therefore, set the array size
accordingly.  This reduces the size of dm_verity_fec_io by 912 bytes.

Note: a later commit introduces a constant DM_VERITY_FEC_MAX_ROOTS,
which allows the size to be more clearly expressed as
DM_VERITY_FEC_MAX_ROOTS + 1.  This commit just fixes the size first.

Fixes: a739ff3f543a ("dm verity: add support for forward error correction")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 35d28d9f8a9b0..32ca2bfee1db7 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -45,11 +45,12 @@ struct dm_verity_fec {
 };
 
 /* per-bio data */
 struct dm_verity_fec_io {
 	struct rs_control *rs;	/* Reed-Solomon state */
-	int erasures[DM_VERITY_FEC_MAX_RSN];	/* erasures for decode_rs8 */
+	/* erasures for decode_rs8 */
+	int erasures[DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN + 1];
 	u8 *output;		/* buffer for corrected output */
 	unsigned int level;		/* recursion level */
 	unsigned int nbufs;		/* number of buffers allocated */
 	/*
 	 * Buffers for deinterleaving RS blocks.  Each buffer has space for
-- 
2.52.0

[PATCH 05/22] dm-verity-fec: fix reading parity bytes split across blocks (take 3)

[Eric Biggers]

fec_decode_bufs() assumes that the parity bytes of the first RS codeword
it decodes are never split across parity blocks.

This assumption is false.  Consider v->fec->block_size == 4096 &&
v->fec->roots == 17 && fio->nbufs == 1, for example.  In that case, each
call to fec_decode_bufs() consumes v->fec->roots * (fio->nbufs <<
DM_VERITY_FEC_BUF_RS_BITS) = 272 parity bytes.

Considering that the parity data for each message block starts on a
block boundary, the byte alignment in the parity data will iterate
through 272*i mod 4096 until the 3 parity blocks have been consumed.  On
the 16th call (i=15), the alignment will be 4080 bytes into the first
block.  Only 16 bytes remain in that block, but 17 parity bytes will be
needed.  The code reads out-of-bounds from the parity block buffer.

Fortunately this doesn't normally happen, since it can occur only for
certain non-default values of fec_roots *and* when the maximum number of
buffers couldn't be allocated due to low memory.  For example with
block_size=4096 only the following cases are affected:

    fec_roots=17: nbufs in [1, 3, 5, 15]
    fec_roots=19: nbufs in [1, 229]
    fec_roots=21: nbufs in [1, 3, 5, 13, 15, 39, 65, 195]
    fec_roots=23: nbufs in [1, 89]

Regardless, fix it by refactoring how the parity blocks are read.

Fixes: 6df90c02bae4 ("dm-verity FEC: Fix RS FEC repair for roots unaligned to block size (take 2)")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 100 ++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 56 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index d4a9367a2fee6..fcfacaec2989a 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -31,40 +31,10 @@ static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
 
 	mod = do_div(offset, v->fec->rsn);
 	return offset + mod * (v->fec->rounds << v->data_dev_block_bits);
 }
 
-/*
- * Read error-correcting codes for the requested RS block. Returns a pointer
- * to the data block. Caller is responsible for releasing buf.
- */
-static u8 *fec_read_parity(struct dm_verity *v, u64 rsb, int index,
-			   unsigned int *offset, unsigned int par_buf_offset,
-			   struct dm_buffer **buf, unsigned short ioprio)
-{
-	u64 position, block, rem;
-	u8 *res;
-
-	/* We have already part of parity bytes read, skip to the next block */
-	if (par_buf_offset)
-		index++;
-
-	position = (index + rsb) * v->fec->roots;
-	block = div64_u64_rem(position, v->fec->io_size, &rem);
-	*offset = par_buf_offset ? 0 : (unsigned int)rem;
-
-	res = dm_bufio_read_with_ioprio(v->fec->bufio, block, buf, ioprio);
-	if (IS_ERR(res)) {
-		DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
-		      v->data_dev->name, (unsigned long long)rsb,
-		      (unsigned long long)block, PTR_ERR(res));
-		*buf = NULL;
-	}
-
-	return res;
-}
-
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
 	for (__i = 0; __i < (io)->nbufs; __i++)
 
 /* Loop over each RS block in each allocated buffer. */
@@ -100,28 +70,66 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			   struct dm_verity_fec_io *fio, u64 rsb, int byte_index,
 			   unsigned int block_offset, int neras)
 {
 	int r, corrected = 0, res;
 	struct dm_buffer *buf;
-	unsigned int n, i, j, offset, par_buf_offset = 0;
+	unsigned int n, i, j, parity_pos, to_copy;
 	uint16_t par_buf[DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN];
 	u8 *par, *block;
+	u64 parity_block;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
-	par = fec_read_parity(v, rsb, block_offset, &offset,
-			      par_buf_offset, &buf, bio->bi_ioprio);
-	if (IS_ERR(par))
+	/*
+	 * Compute the index of the first parity block that will be needed and
+	 * the starting position in that block.  Then read that block.
+	 *
+	 * io_size is always a power of 2, but roots might not be.  Note that
+	 * when it's not, a codeword's parity bytes can span a block boundary.
+	 */
+	parity_block = (rsb + block_offset) * v->fec->roots;
+	parity_pos = parity_block & (v->fec->io_size - 1);
+	parity_block >>= v->data_dev_block_bits;
+	par = dm_bufio_read_with_ioprio(v->fec->bufio, parity_block, &buf,
+					bio->bi_ioprio);
+	if (IS_ERR(par)) {
+		DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
+		      v->data_dev->name, rsb, parity_block, PTR_ERR(par));
 		return PTR_ERR(par);
+	}
 
 	/*
 	 * Decode the RS blocks we have in bufs. Each RS block results in
 	 * one corrected target byte and consumes fec->roots parity bytes.
 	 */
 	fec_for_each_buffer_rs_block(fio, n, i) {
 		block = fec_buffer_rs_block(v, fio, n, i);
-		for (j = 0; j < v->fec->roots - par_buf_offset; j++)
-			par_buf[par_buf_offset + j] = par[offset + j];
+
+		/*
+		 * Copy the next 'roots' parity bytes to 'par_buf', reading
+		 * another parity block if needed.
+		 */
+		to_copy = min(v->fec->io_size - parity_pos, v->fec->roots);
+		for (j = 0; j < to_copy; j++)
+			par_buf[j] = par[parity_pos++];
+		if (to_copy < v->fec->roots) {
+			parity_block++;
+			parity_pos = 0;
+
+			dm_bufio_release(buf);
+			par = dm_bufio_read_with_ioprio(v->fec->bufio,
+							parity_block, &buf,
+							bio->bi_ioprio);
+			if (IS_ERR(par)) {
+				DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
+				      v->data_dev->name, rsb, parity_block,
+				      PTR_ERR(par));
+				return PTR_ERR(par);
+			}
+			for (; j < v->fec->roots; j++)
+				par_buf[j] = par[parity_pos++];
+		}
+
 		/* Decode an RS block using Reed-Solomon */
 		res = decode_rs8(fio->rs, block, par_buf, v->fec->rsn,
 				 NULL, neras, fio->erasures, 0, NULL);
 		if (res < 0) {
 			r = res;
@@ -132,30 +140,10 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		fio->output[block_offset] = block[byte_index];
 
 		block_offset++;
 		if (block_offset >= 1 << v->data_dev_block_bits)
 			goto done;
-
-		/* Read the next block when we run out of parity bytes */
-		offset += (v->fec->roots - par_buf_offset);
-		/* Check if parity bytes are split between blocks */
-		if (offset < v->fec->io_size && (offset + v->fec->roots) > v->fec->io_size) {
-			par_buf_offset = v->fec->io_size - offset;
-			for (j = 0; j < par_buf_offset; j++)
-				par_buf[j] = par[offset + j];
-			offset += par_buf_offset;
-		} else
-			par_buf_offset = 0;
-
-		if (offset >= v->fec->io_size) {
-			dm_bufio_release(buf);
-
-			par = fec_read_parity(v, rsb, block_offset, &offset,
-					      par_buf_offset, &buf, bio->bi_ioprio);
-			if (IS_ERR(par))
-				return PTR_ERR(par);
-		}
 	}
 done:
 	r = corrected;
 error:
 	dm_bufio_release(buf);
-- 
2.52.0

[PATCH 06/22] dm-verity: rename dm_verity::hash_blocks to dm_verity::hash_end

[Eric Biggers]

Rename hash_blocks to hash_end to reflect what it actually is.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c    | 2 +-
 drivers/md/dm-verity-target.c | 8 ++++----
 drivers/md/dm-verity.h        | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index fcfacaec2989a..c3f2516f9c2a9 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -633,11 +633,11 @@ int verity_fec_ctr(struct dm_verity *v)
 	 *
 	 * If metadata is included, we require it to be available on the
 	 * hash device after the hash blocks.
 	 */
 
-	hash_blocks = v->hash_blocks - v->hash_start;
+	hash_blocks = v->hash_end - v->hash_start;
 
 	/*
 	 * Require matching block sizes for data and hash devices for
 	 * simplicity.
 	 */
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 631ccc6a2bb7e..ad9a86e8e44bb 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -730,12 +730,12 @@ static void verity_prefetch_io(struct work_struct *work)
 			if (unlikely(cluster & (cluster - 1)))
 				cluster = 1 << __fls(cluster);
 
 			hash_block_start &= ~(sector_t)(cluster - 1);
 			hash_block_end |= cluster - 1;
-			if (unlikely(hash_block_end >= v->hash_blocks))
-				hash_block_end = v->hash_blocks - 1;
+			if (unlikely(hash_block_end >= v->hash_end))
+				hash_block_end = v->hash_end - 1;
 		}
 no_prefetch_cluster:
 		dm_bufio_prefetch_with_ioprio(v->bufio, hash_block_start,
 					hash_block_end - hash_block_start + 1,
 					pw->ioprio);
@@ -1604,11 +1604,11 @@ static int verity_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 			r = -E2BIG;
 			goto bad;
 		}
 		hash_position += s;
 	}
-	v->hash_blocks = hash_position;
+	v->hash_end = hash_position;
 
 	r = mempool_init_page_pool(&v->recheck_pool, 1, 0);
 	if (unlikely(r)) {
 		ti->error = "Cannot allocate mempool";
 		goto bad;
@@ -1631,11 +1631,11 @@ static int verity_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		r = PTR_ERR(v->bufio);
 		v->bufio = NULL;
 		goto bad;
 	}
 
-	if (dm_bufio_get_device_size(v->bufio) < v->hash_blocks) {
+	if (dm_bufio_get_device_size(v->bufio) < v->hash_end) {
 		ti->error = "Hash device is too small";
 		r = -E2BIG;
 		goto bad;
 	}
 
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index d6bfabb27113b..2922263501f68 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -51,13 +51,13 @@ struct dm_verity {
 #ifdef CONFIG_SECURITY
 	u8 *root_digest_sig;	/* signature of the root digest */
 	unsigned int sig_size;	/* root digest signature size */
 #endif /* CONFIG_SECURITY */
 	unsigned int salt_size;
-	sector_t hash_start;	/* hash start in blocks */
+	sector_t hash_start;	/* index of first hash block on hash_dev */
+	sector_t hash_end;	/* 1 + index of last hash block on hash dev */
 	sector_t data_blocks;	/* the number of data blocks */
-	sector_t hash_blocks;	/* the number of hash blocks */
 	unsigned char data_dev_block_bits;	/* log2(data blocksize) */
 	unsigned char hash_dev_block_bits;	/* log2(hash blocksize) */
 	unsigned char hash_per_block_bits;	/* log2(hashes in hash block) */
 	unsigned char levels;	/* the number of tree levels */
 	unsigned char version;
-- 
2.52.0

[PATCH 07/22] dm-verity-fec: improve documentation for Forward Error Correction

[Eric Biggers]

Update verity.rst to add a dedicated section about FEC and improve the
documentation for the FEC-related parameters.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 .../admin-guide/device-mapper/verity.rst      | 122 +++++++++++++++---
 1 file changed, 102 insertions(+), 20 deletions(-)

diff --git a/Documentation/admin-guide/device-mapper/verity.rst b/Documentation/admin-guide/device-mapper/verity.rst
index 3ecab1cff9c64..eb9475d7e1965 100644
--- a/Documentation/admin-guide/device-mapper/verity.rst
+++ b/Documentation/admin-guide/device-mapper/verity.rst
@@ -100,33 +100,46 @@ ignore_zero_blocks
     Do not verify blocks that are expected to contain zeroes and always return
     zeroes instead. This may be useful if the partition contains unused blocks
     that are not guaranteed to contain zeroes.
 
 use_fec_from_device <fec_dev>
-    Use forward error correction (FEC) to recover from corruption if hash
-    verification fails. Use encoding data from the specified device. This
-    may be the same device where data and hash blocks reside, in which case
-    fec_start must be outside data and hash areas.
+    Use forward error correction (FEC) parity data from the specified device to
+    try to automatically recover from corruption and I/O errors.
 
-    If the encoding data covers additional metadata, it must be accessible
-    on the hash device after the hash blocks.
+    If this option is given, then <fec_roots> and <fec_blocks> must also be
+    given.  <hash_block_size> must also be equal to <data_block_size>.
 
-    Note: block sizes for data and hash devices must match. Also, if the
-    verity <dev> is encrypted the <fec_dev> should be too.
+    <fec_dev> can be the same as <dev>, in which case <fec_start> must be
+    outside the data area.  It can also be the same as <hash_dev>, in which case
+    <fec_start> must be outside the hash and optional additional metadata areas.
+
+    If the data <dev> is encrypted, the <fec_dev> should be too.
+
+    For more information, see `Forward error correction`_.
 
 fec_roots <num>
-    Number of generator roots. This equals to the number of parity bytes in
-    the encoding data. For example, in RS(M, N) encoding, the number of roots
-    is M-N.
+    The number of parity bytes in each 255-byte Reed-Solomon codeword.  The
+    Reed-Solomon code used will be an RS(255, k) code where k = 255 - fec_roots.
+
+    The supported values are 2 through 24 inclusive.  Higher values provide
+    stronger error correction.  However, the minimum value of 2 already provides
+    strong error correction due to the use of interleaving, so 2 is the
+    recommended value for most users.  fec_roots=2 corresponds to an
+    RS(255, 253) code, which has a space overhead of about 0.8%.
 
 fec_blocks <num>
-    The number of encoding data blocks on the FEC device. The block size for
-    the FEC device is <data_block_size>.
+    The total number of <data_block_size> blocks that are error-checked using
+    FEC.  This must be at least the sum of <num_data_blocks> and the number of
+    blocks needed by the hash tree.  It can include additional metadata blocks,
+    which are assumed to be accessible on <hash_dev> following the hash blocks.
+
+    Note that this is *not* the number of parity blocks.  The number of parity
+    blocks is inferred from <fec_blocks>, <fec_roots>, and <data_block_size>.
 
 fec_start <offset>
-    This is the offset, in <data_block_size> blocks, from the start of the
-    FEC device to the beginning of the encoding data.
+    This is the offset, in <data_block_size> blocks, from the start of <fec_dev>
+    to the beginning of the parity data.
 
 check_at_most_once
     Verify data blocks only the first time they are read from the data device,
     rather than every time.  This reduces the overhead of dm-verity so that it
     can be used on systems that are memory and/or CPU constrained.  However, it
@@ -178,15 +191,10 @@ tampering with any data on the device and the hash data.
 Cryptographic hashes are used to assert the integrity of the device on a
 per-block basis. This allows for a lightweight hash computation on first read
 into the page cache. Block hashes are stored linearly, aligned to the nearest
 block size.
 
-If forward error correction (FEC) support is enabled any recovery of
-corrupted data will be verified using the cryptographic hash of the
-corresponding data. This is why combining error correction with
-integrity checking is essential.
-
 Hash Tree
 ---------
 
 Each node in the tree is a cryptographic hash.  If it is a leaf node, the hash
 of some data block on disk is calculated. If it is an intermediary node,
@@ -210,10 +218,84 @@ The tree looks something like:
                     /  . . .  \                 . . .   \
          [entry_0_0]   . . .  [entry_0_127]    . . . .  [entry_1_127]
            / ... \             /   . . .  \             /           \
      blk_0 ... blk_127  blk_16256   blk_16383      blk_32640 . . . blk_32767
 
+Forward error correction
+------------------------
+
+dm-verity's optional forward error correction (FEC) support adds strong error
+correction capabilities to dm-verity.  It allows systems that would be rendered
+inoperable by errors to continue operating, albeit with reduced performance.
+
+FEC uses Reed-Solomon (RS) codes that are interleaved across the entire
+device(s), allowing long bursts of corrupt or unreadable blocks to be recovered.
+
+dm-verity validates any FEC-corrected block against the wanted hash before using
+it.  Therefore, FEC doesn't affect the security properties of dm-verity.
+
+The integration of FEC with dm-verity provides significant benefits over a
+separate error correction layer:
+
+- dm-verity invokes FEC only when a block's hash doesn't match the wanted hash
+  or the block cannot be read at all.  As a result, FEC doesn't add overhead to
+  the common case where no error occurs.
+
+- dm-verity hashes are also used to identify erasure locations for RS decoding.
+  This allows correcting twice as many errors.
+
+FEC uses an RS(255, k) code where k = 255 - fec_roots.  fec_roots is usually 2.
+This means that each k (usually 253) message bytes have fec_roots (usually 2)
+bytes of parity data added to get a 255-byte codeword.  (Many external sources
+call RS codewords "blocks".  Since dm-verity already uses the term "block" to
+mean something else, we'll use the clearer term "RS codeword".)
+
+FEC checks fec_blocks blocks of message data in total, consisting of:
+
+1. The data blocks from the data device
+2. The hash blocks from the hash device
+3. Optional additional metadata that follows the hash blocks on the hash device
+
+dm-verity assumes that the FEC parity data was computed as if the following
+procedure were followed:
+
+1. Concatenate the message data from the above sources.
+2. Zero-pad to the next multiple of k blocks.  Let msg be the resulting byte
+   array, and msglen its length in bytes.
+3. For 0 <= i < msglen / k (for each RS codeword):
+     a. Select msg[i + j * msglen / k] for 0 <= j < k.
+        Consider these to be the 'k' message bytes of an RS codeword.
+     b. Compute the corresponding 'fec_roots' parity bytes of the RS codeword,
+        and concatenate them to the FEC parity data.
+
+Step 3a interleaves the RS codewords across the entire device using an
+interleaving degree of data_block_size * ceil(fec_blocks / k).  This is the
+maximal interleaving, such that the message data consists of a region containing
+byte 0 of all the RS codewords, then a region containing byte 1 of all the RS
+codewords, and so on up to the region for byte 'k - 1'.  Note that the number of
+codewords is set to a multiple of data_block_size; thus, the regions are
+block-aligned, and there is an implicit zero padding of up to 'k - 1' blocks.
+
+This interleaving allows long bursts of errors to be corrected.  It provides
+much stronger error correction than storage devices typically provide, while
+keeping the space overhead low.
+
+The cost is slow decoding: correcting a single block usually requires reading
+254 extra blocks spread evenly across the device(s).  However, that is
+acceptable because dm-verity uses FEC only when there is actually an error.
+
+The list below contains additional details about the RS codes used by
+dm-verity's FEC.  Userspace programs that generate the parity data need to use
+these parameters for the parity data to match exactly:
+
+- Field used is GF(256)
+- Bytes are mapped to/from GF(256) elements in the natural way, where bits 0
+  through 7 (low-order to high-order) map to the coefficients of x^0 through x^7
+- Field generator polynomial is x^8 + x^4 + x^3 + x^2 + 1
+- The codes used are systematic, BCH-view codes
+- Primitive element alpha is 'x'
+- First consecutive root of code generator polynomial is 'x^0'
 
 On-disk format
 ==============
 
 The verity kernel code does not read the verity metadata on-disk header.
-- 
2.52.0

[PATCH 08/22] dm-verity-fec: replace {MAX,MIN}_RSN with {MIN,MAX}_ROOTS

[Eric Biggers]

Every time DM_VERITY_FEC_{MAX,MIN}_RSN are used, they are subtracted
from DM_VERITY_FEC_RSM to get the bounds on the number of roots.
Therefore, replace these with {MIN,MAX}_ROOTS constants which are more
directly useful.  (Note the inversion, where MAX_RSN maps to MIN_ROOTS
and MIN_RSN maps to MAX_ROOTS.)  No functional change.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 6 +++---
 drivers/md/dm-verity-fec.h | 7 +++----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index c3f2516f9c2a9..c5f97a49e5b79 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -71,11 +71,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			   unsigned int block_offset, int neras)
 {
 	int r, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
-	uint16_t par_buf[DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN];
+	uint16_t par_buf[DM_VERITY_FEC_MAX_ROOTS];
 	u8 *par, *block;
 	u64 parity_block;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	/*
@@ -570,12 +570,12 @@ int verity_fec_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 		}
 		v->fec->start = num_ll;
 
 	} else if (!strcasecmp(arg_name, DM_VERITY_OPT_FEC_ROOTS)) {
 		if (sscanf(arg_value, "%hhu%c", &num_c, &dummy) != 1 || !num_c ||
-		    num_c < (DM_VERITY_FEC_RSM - DM_VERITY_FEC_MAX_RSN) ||
-		    num_c > (DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN)) {
+		    num_c < DM_VERITY_FEC_MIN_ROOTS ||
+		    num_c > DM_VERITY_FEC_MAX_ROOTS) {
 			ti->error = "Invalid " DM_VERITY_OPT_FEC_ROOTS;
 			return -EINVAL;
 		}
 		v->fec->roots = num_c;
 
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 32ca2bfee1db7..d8d0e81da2701 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -11,12 +11,12 @@
 #include "dm-verity.h"
 #include <linux/rslib.h>
 
 /* Reed-Solomon(M, N) parameters */
 #define DM_VERITY_FEC_RSM		255
-#define DM_VERITY_FEC_MAX_RSN		253
-#define DM_VERITY_FEC_MIN_RSN		231	/* ~10% space overhead */
+#define DM_VERITY_FEC_MIN_ROOTS	2	/* RS(255, 253): ~0.8% space overhead */
+#define DM_VERITY_FEC_MAX_ROOTS	24	/* RS(255, 231): ~10% space overhead */
 
 /* buffers for deinterleaving and decoding */
 #define DM_VERITY_FEC_BUF_RS_BITS	4	/* 1 << RS blocks per buffer */
 
 #define DM_VERITY_OPT_FEC_DEV		"use_fec_from_device"
@@ -45,12 +45,11 @@ struct dm_verity_fec {
 };
 
 /* per-bio data */
 struct dm_verity_fec_io {
 	struct rs_control *rs;	/* Reed-Solomon state */
-	/* erasures for decode_rs8 */
-	int erasures[DM_VERITY_FEC_RSM - DM_VERITY_FEC_MIN_RSN + 1];
+	int erasures[DM_VERITY_FEC_MAX_ROOTS + 1]; /* erasures for decode_rs8 */
 	u8 *output;		/* buffer for corrected output */
 	unsigned int level;		/* recursion level */
 	unsigned int nbufs;		/* number of buffers allocated */
 	/*
 	 * Buffers for deinterleaving RS blocks.  Each buffer has space for
-- 
2.52.0

[PATCH 09/22] dm-verity-fec: use standard names for Reed-Solomon parameters

[Eric Biggers]

"RS(n, k)" is by far the most common and standard notation for
describing Reed-Solomon codes.  Each RS codeword consists of 'n'
symbols, divided into 'k' message symbols and 'n - k' parity symbols.
'n - k' is also the number of roots of the generator polynomial.

dm-verity uses "RS(M, N)" instead.  I haven't been able to find any
other source that uses this convention.  This quirk makes the code
harder to understand than necessary, especially due to dm-verity's 'N'
meaning something different from the standard 'n'.

Therefore, update dm-verity-fec.c and dm-verity-fec.h to use the
standard parameter names.  No functional changes.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 30 +++++++++++++++---------------
 drivers/md/dm-verity-fec.h |  8 ++++----
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index c5f97a49e5b79..8e9482a6df4a1 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -27,11 +27,11 @@ static inline unsigned int fec_max_nbufs(struct dm_verity *v)
  */
 static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
 {
 	u32 mod;
 
-	mod = do_div(offset, v->fec->rsn);
+	mod = do_div(offset, v->fec->rs_k);
 	return offset + mod * (v->fec->rounds << v->data_dev_block_bits);
 }
 
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
@@ -48,11 +48,11 @@ static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
  */
 static inline u8 *fec_buffer_rs_block(struct dm_verity *v,
 				      struct dm_verity_fec_io *fio,
 				      unsigned int i, unsigned int j)
 {
-	return &fio->bufs[i][j * v->fec->rsn];
+	return &fio->bufs[i][j * v->fec->rs_k];
 }
 
 /*
  * Return an index to the current RS block when called inside
  * fec_for_each_buffer_rs_block.
@@ -127,11 +127,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			for (; j < v->fec->roots; j++)
 				par_buf[j] = par[parity_pos++];
 		}
 
 		/* Decode an RS block using Reed-Solomon */
-		res = decode_rs8(fio->rs, block, par_buf, v->fec->rsn,
+		res = decode_rs8(fio->rs, block, par_buf, v->fec->rs_k,
 				 NULL, neras, fio->erasures, 0, NULL);
 		if (res < 0) {
 			r = res;
 			goto error;
 		}
@@ -195,19 +195,19 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 	if (WARN_ON(v->digest_size > sizeof(want_digest)))
 		return -EINVAL;
 
 	/*
-	 * read each of the rsn data blocks that are part of the RS block, and
+	 * read each of the rs_k data blocks that are part of the RS block, and
 	 * interleave contents to available bufs
 	 */
-	for (i = 0; i < v->fec->rsn; i++) {
-		ileaved = fec_interleave(v, rsb * v->fec->rsn + i);
+	for (i = 0; i < v->fec->rs_k; i++) {
+		ileaved = fec_interleave(v, rsb * v->fec->rs_k + i);
 
 		/*
 		 * target is the data block we want to correct, target_index is
-		 * the index of this block within the rsn RS blocks
+		 * the index of this block within the rs_k RS blocks
 		 */
 		if (ileaved == target)
 			target_index = i;
 
 		block = ileaved >> v->data_dev_block_bits;
@@ -320,11 +320,11 @@ static struct dm_verity_fec_io *fec_alloc_and_init_io(struct dm_verity *v)
 static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 {
 	unsigned int n;
 
 	fec_for_each_buffer(fio, n)
-		memset(fio->bufs[n], 0, v->fec->rsn << DM_VERITY_FEC_BUF_RS_BITS);
+		memset(fio->bufs[n], 0, v->fec->rs_k << DM_VERITY_FEC_BUF_RS_BITS);
 
 	memset(fio->erasures, 0, sizeof(fio->erasures));
 }
 
 /*
@@ -392,16 +392,16 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 
 	if (type == DM_VERITY_BLOCK_TYPE_METADATA)
 		block = block - v->hash_start + v->data_blocks;
 
 	/*
-	 * For RS(M, N), the continuous FEC data is divided into blocks of N
-	 * bytes. Since block size may not be divisible by N, the last block
+	 * For RS(n, k), the continuous FEC data is divided into blocks of k
+	 * bytes. Since block size may not be divisible by k, the last block
 	 * is zero padded when decoding.
 	 *
-	 * Each byte of the block is covered by a different RS(M, N) code,
-	 * and each code is interleaved over N blocks to make it less likely
+	 * Each byte of the block is covered by a different RS(n, k) code,
+	 * and each code is interleaved over k blocks to make it less likely
 	 * that bursty corruption will leave us in unrecoverable state.
 	 */
 
 	offset = block << v->data_dev_block_bits;
 	res = div64_u64(offset, v->fec->rounds << v->data_dev_block_bits);
@@ -648,19 +648,19 @@ int verity_fec_ctr(struct dm_verity *v)
 
 	if (!f->roots) {
 		ti->error = "Missing " DM_VERITY_OPT_FEC_ROOTS;
 		return -EINVAL;
 	}
-	f->rsn = DM_VERITY_FEC_RSM - f->roots;
+	f->rs_k = DM_VERITY_FEC_RS_N - f->roots;
 
 	if (!f->blocks) {
 		ti->error = "Missing " DM_VERITY_OPT_FEC_BLOCKS;
 		return -EINVAL;
 	}
 
 	f->rounds = f->blocks;
-	if (sector_div(f->rounds, f->rsn))
+	if (sector_div(f->rounds, f->rs_k))
 		f->rounds++;
 
 	/*
 	 * Due to optional metadata, f->blocks can be larger than
 	 * data_blocks and hash_blocks combined.
@@ -728,11 +728,11 @@ int verity_fec_ctr(struct dm_verity *v)
 		ti->error = "Cannot allocate RS pool";
 		return ret;
 	}
 
 	f->cache = kmem_cache_create("dm_verity_fec_buffers",
-				     f->rsn << DM_VERITY_FEC_BUF_RS_BITS,
+				     f->rs_k << DM_VERITY_FEC_BUF_RS_BITS,
 				     0, 0, NULL);
 	if (!f->cache) {
 		ti->error = "Cannot create FEC buffer cache";
 		return -ENOMEM;
 	}
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index d8d0e81da2701..5afa93f2f1fc7 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -9,12 +9,12 @@
 #define DM_VERITY_FEC_H
 
 #include "dm-verity.h"
 #include <linux/rslib.h>
 
-/* Reed-Solomon(M, N) parameters */
-#define DM_VERITY_FEC_RSM		255
+/* Reed-Solomon(n, k) parameters */
+#define DM_VERITY_FEC_RS_N	255
 #define DM_VERITY_FEC_MIN_ROOTS	2	/* RS(255, 253): ~0.8% space overhead */
 #define DM_VERITY_FEC_MAX_ROOTS	24	/* RS(255, 231): ~10% space overhead */
 
 /* buffers for deinterleaving and decoding */
 #define DM_VERITY_FEC_BUF_RS_BITS	4	/* 1 << RS blocks per buffer */
@@ -32,12 +32,12 @@ struct dm_verity_fec {
 	size_t io_size;		/* IO size for roots */
 	sector_t start;		/* parity data start in blocks */
 	sector_t blocks;	/* number of blocks covered */
 	sector_t rounds;	/* number of interleaving rounds */
 	sector_t hash_blocks;	/* blocks covered after v->hash_start */
-	unsigned char roots;	/* number of parity bytes, M-N of RS(M, N) */
-	unsigned char rsn;	/* N of RS(M, N) */
+	unsigned char roots;	/* parity bytes per RS codeword, n-k of RS(n, k) */
+	unsigned char rs_k;	/* message bytes per RS codeword, k of RS(n, k) */
 	mempool_t fio_pool;	/* mempool for dm_verity_fec_io */
 	mempool_t rs_pool;	/* mempool for fio->rs */
 	mempool_t prealloc_pool;	/* mempool for preallocated buffers */
 	mempool_t output_pool;	/* mempool for output */
 	struct kmem_cache *cache;	/* cache for buffers */
-- 
2.52.0

[PATCH 10/22] dm-verity-fec: rename "RS block" to "RS codeword"

[Eric Biggers]

The literature refers to the unit of a Reed-Solomon (RS) code as either
a "block" or a "codeword".

dm-verity's source code uses "RS block".  Unfortunately, that's really
confusing because "block" already means something else in dm-verity.
Especially problematic is the fact that dm-verity sometimes uses "RS
block" to mean an RS codeword and sometimes to mean some dm-verity block
that's related to the RS decoding process, for example one of the blocks
that shares its RS codewords with the target block.

Let's use "RS codeword" instead, or "RS message" when referring to just
the message part of the codeword.  Update some comments, function names,
macro names, and variable names accordingly.  No functional change.

There are still some remaining comments where "RS block" refers to a
dm-verity block.  Later commits will handle these cases.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 58 ++++++++++++++++++++------------------
 drivers/md/dm-verity-fec.h | 10 +++----
 2 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 8e9482a6df4a1..619645edaf509 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -9,15 +9,16 @@
 #include <linux/math64.h>
 
 #define DM_MSG_PREFIX	"verity-fec"
 
 /*
- * When correcting a data block, the FEC code performs optimally when it can
- * collect all the associated RS blocks at the same time.  As each byte is part
- * of a different RS block, there are '1 << data_dev_block_bits' RS blocks.
- * There are '1 << DM_VERITY_FEC_BUF_RS_BITS' RS blocks per buffer, so that
- * gives '1 << (data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS)' buffers.
+ * When correcting a block, the FEC implementation performs optimally when it
+ * can collect all the associated RS codewords at the same time.  As each byte
+ * is part of a different codeword, there are '1 << data_dev_block_bits'
+ * codewords.  Each buffer has space for the message bytes for
+ * '1 << DM_VERITY_FEC_BUF_RS_BITS' codewords, so that gives
+ * '1 << (data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS)' buffers.
  */
 static inline unsigned int fec_max_nbufs(struct dm_verity *v)
 {
 	return 1 << (v->data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS);
 }
@@ -35,48 +36,49 @@ static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
 
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
 	for (__i = 0; __i < (io)->nbufs; __i++)
 
-/* Loop over each RS block in each allocated buffer. */
-#define fec_for_each_buffer_rs_block(io, __i, __j) \
+/* Loop over each RS message in each allocated buffer. */
+/* To stop early, use 'goto', not 'break' (since this uses nested loops). */
+#define fec_for_each_buffer_rs_message(io, __i, __j) \
 	fec_for_each_buffer(io, __i) \
 		for (__j = 0; __j < 1 << DM_VERITY_FEC_BUF_RS_BITS; __j++)
 
 /*
- * Return a pointer to the current RS block when called inside
- * fec_for_each_buffer_rs_block.
+ * Return a pointer to the current RS message when called inside
+ * fec_for_each_buffer_rs_message.
  */
-static inline u8 *fec_buffer_rs_block(struct dm_verity *v,
-				      struct dm_verity_fec_io *fio,
-				      unsigned int i, unsigned int j)
+static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
+					struct dm_verity_fec_io *fio,
+					unsigned int i, unsigned int j)
 {
 	return &fio->bufs[i][j * v->fec->rs_k];
 }
 
 /*
- * Return an index to the current RS block when called inside
- * fec_for_each_buffer_rs_block.
+ * Return the index of the current RS message when called inside
+ * fec_for_each_buffer_rs_message.
  */
 static inline unsigned int fec_buffer_rs_index(unsigned int i, unsigned int j)
 {
 	return (i << DM_VERITY_FEC_BUF_RS_BITS) + j;
 }
 
 /*
- * Decode all RS blocks from buffers and copy corrected bytes into fio->output
- * starting from block_offset.
+ * Decode all RS codewords whose message bytes were loaded into fio->bufs.  Copy
+ * the corrected bytes into fio->output starting from block_offset.
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			   struct dm_verity_fec_io *fio, u64 rsb, int byte_index,
 			   unsigned int block_offset, int neras)
 {
 	int r, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
 	uint16_t par_buf[DM_VERITY_FEC_MAX_ROOTS];
-	u8 *par, *block;
+	u8 *par, *msg_buf;
 	u64 parity_block;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	/*
 	 * Compute the index of the first parity block that will be needed and
@@ -95,15 +97,16 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		      v->data_dev->name, rsb, parity_block, PTR_ERR(par));
 		return PTR_ERR(par);
 	}
 
 	/*
-	 * Decode the RS blocks we have in bufs. Each RS block results in
-	 * one corrected target byte and consumes fec->roots parity bytes.
+	 * Decode the RS codewords whose message bytes are in bufs. Each RS
+	 * codeword results in one corrected target byte and consumes fec->roots
+	 * parity bytes.
 	 */
-	fec_for_each_buffer_rs_block(fio, n, i) {
-		block = fec_buffer_rs_block(v, fio, n, i);
+	fec_for_each_buffer_rs_message(fio, n, i) {
+		msg_buf = fec_buffer_rs_message(v, fio, n, i);
 
 		/*
 		 * Copy the next 'roots' parity bytes to 'par_buf', reading
 		 * another parity block if needed.
 		 */
@@ -126,20 +129,20 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			}
 			for (; j < v->fec->roots; j++)
 				par_buf[j] = par[parity_pos++];
 		}
 
-		/* Decode an RS block using Reed-Solomon */
-		res = decode_rs8(fio->rs, block, par_buf, v->fec->rs_k,
+		/* Decode an RS codeword using the Reed-Solomon library. */
+		res = decode_rs8(fio->rs, msg_buf, par_buf, v->fec->rs_k,
 				 NULL, neras, fio->erasures, 0, NULL);
 		if (res < 0) {
 			r = res;
 			goto error;
 		}
 
 		corrected += res;
-		fio->output[block_offset] = block[byte_index];
+		fio->output[block_offset] = msg_buf[byte_index];
 
 		block_offset++;
 		if (block_offset >= 1 << v->data_dev_block_bits)
 			goto done;
 	}
@@ -183,11 +186,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	int i, j, target_index = -1;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
 	struct dm_verity_fec_io *fio = io->fec_io;
 	u64 block, ileaved;
-	u8 *bbuf, *rs_block;
+	u8 *bbuf;
 	u8 want_digest[HASH_MAX_DIGESTSIZE];
 	unsigned int n, k;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	if (neras)
@@ -260,18 +263,17 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 		/*
 		 * deinterleave and copy the bytes that fit into bufs,
 		 * starting from block_offset
 		 */
-		fec_for_each_buffer_rs_block(fio, n, j) {
+		fec_for_each_buffer_rs_message(fio, n, j) {
 			k = fec_buffer_rs_index(n, j) + block_offset;
 
 			if (k >= 1 << v->data_dev_block_bits)
 				goto done;
 
-			rs_block = fec_buffer_rs_block(v, fio, n, j);
-			rs_block[i] = bbuf[k];
+			fec_buffer_rs_message(v, fio, n, j)[i] = bbuf[k];
 		}
 done:
 		dm_bufio_release(buf);
 	}
 
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 5afa93f2f1fc7..257a609274c7c 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -15,11 +15,11 @@
 #define DM_VERITY_FEC_RS_N	255
 #define DM_VERITY_FEC_MIN_ROOTS	2	/* RS(255, 253): ~0.8% space overhead */
 #define DM_VERITY_FEC_MAX_ROOTS	24	/* RS(255, 231): ~10% space overhead */
 
 /* buffers for deinterleaving and decoding */
-#define DM_VERITY_FEC_BUF_RS_BITS	4	/* 1 << RS blocks per buffer */
+#define DM_VERITY_FEC_BUF_RS_BITS	4 /* log2(RS messages per buffer) */
 
 #define DM_VERITY_OPT_FEC_DEV		"use_fec_from_device"
 #define DM_VERITY_OPT_FEC_BLOCKS	"fec_blocks"
 #define DM_VERITY_OPT_FEC_START		"fec_start"
 #define DM_VERITY_OPT_FEC_ROOTS		"fec_roots"
@@ -50,14 +50,14 @@ struct dm_verity_fec_io {
 	int erasures[DM_VERITY_FEC_MAX_ROOTS + 1]; /* erasures for decode_rs8 */
 	u8 *output;		/* buffer for corrected output */
 	unsigned int level;		/* recursion level */
 	unsigned int nbufs;		/* number of buffers allocated */
 	/*
-	 * Buffers for deinterleaving RS blocks.  Each buffer has space for
-	 * the data bytes of (1 << DM_VERITY_FEC_BUF_RS_BITS) RS blocks.  The
-	 * array length is fec_max_nbufs(v), and we try to allocate that many
-	 * buffers.  However, in low-memory situations we may be unable to
+	 * Buffers for deinterleaving RS codewords.  Each buffer has space for
+	 * the message bytes of (1 << DM_VERITY_FEC_BUF_RS_BITS) RS codewords.
+	 * The array length is fec_max_nbufs(v), and we try to allocate that
+	 * many buffers.  However, in low-memory situations we may be unable to
 	 * allocate all buffers.  'nbufs' holds the number actually allocated.
 	 */
 	u8 *bufs[];
 };
 
-- 
2.52.0

[PATCH 11/22] dm-verity-fec: replace io_size with block_size

[Eric Biggers]

dm-verity's FEC implementation assumes that data_block_size ==
hash_block_size, and it accesses the FEC device in units of the same
size.  Many places in the code want that size and compute it on-demand
as '1 << v->data_dev_block_bits'.  However, it's actually already
available in v->fec->io_size.  Rename that field to block_size,
initialize it a bit earlier, and use it in the appropriate places.

Note that while these sizes could in principle be different, that case
is not supported.  So there's no need to complicate the code for it.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 30 +++++++++++++-----------------
 drivers/md/dm-verity-fec.h |  2 +-
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 619645edaf509..6ba9a1e039be3 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -82,15 +82,15 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 	/*
 	 * Compute the index of the first parity block that will be needed and
 	 * the starting position in that block.  Then read that block.
 	 *
-	 * io_size is always a power of 2, but roots might not be.  Note that
+	 * block_size is always a power of 2, but roots might not be.  Note that
 	 * when it's not, a codeword's parity bytes can span a block boundary.
 	 */
 	parity_block = (rsb + block_offset) * v->fec->roots;
-	parity_pos = parity_block & (v->fec->io_size - 1);
+	parity_pos = parity_block & (v->fec->block_size - 1);
 	parity_block >>= v->data_dev_block_bits;
 	par = dm_bufio_read_with_ioprio(v->fec->bufio, parity_block, &buf,
 					bio->bi_ioprio);
 	if (IS_ERR(par)) {
 		DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
@@ -108,11 +108,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 		/*
 		 * Copy the next 'roots' parity bytes to 'par_buf', reading
 		 * another parity block if needed.
 		 */
-		to_copy = min(v->fec->io_size - parity_pos, v->fec->roots);
+		to_copy = min(v->fec->block_size - parity_pos, v->fec->roots);
 		for (j = 0; j < to_copy; j++)
 			par_buf[j] = par[parity_pos++];
 		if (to_copy < v->fec->roots) {
 			parity_block++;
 			parity_pos = 0;
@@ -141,11 +141,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 		corrected += res;
 		fio->output[block_offset] = msg_buf[byte_index];
 
 		block_offset++;
-		if (block_offset >= 1 << v->data_dev_block_bits)
+		if (block_offset >= v->fec->block_size)
 			goto done;
 	}
 done:
 	r = corrected;
 error:
@@ -165,11 +165,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
  * Locate data block erasures using verity hashes.
  */
 static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 			  const u8 *want_digest, const u8 *data)
 {
-	if (unlikely(verity_hash(v, io, data, 1 << v->data_dev_block_bits,
+	if (unlikely(verity_hash(v, io, data, v->fec->block_size,
 				 io->tmp_digest)))
 		return 0;
 
 	return memcmp(io->tmp_digest, want_digest, v->digest_size) != 0;
 }
@@ -266,11 +266,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		 * starting from block_offset
 		 */
 		fec_for_each_buffer_rs_message(fio, n, j) {
 			k = fec_buffer_rs_index(n, j) + block_offset;
 
-			if (k >= 1 << v->data_dev_block_bits)
+			if (k >= v->fec->block_size)
 				goto done;
 
 			fec_buffer_rs_message(v, fio, n, j)[i] = bbuf[k];
 		}
 done:
@@ -339,11 +339,11 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 			  const u8 *want_digest, bool use_erasures)
 {
 	int r, neras = 0;
 	unsigned int pos;
 
-	for (pos = 0; pos < 1 << v->data_dev_block_bits; ) {
+	for (pos = 0; pos < v->fec->block_size;) {
 		fec_init_bufs(v, fio);
 
 		r = fec_read_bufs(v, io, rsb, offset, pos,
 				  use_erasures ? &neras : NULL);
 		if (unlikely(r < 0))
@@ -355,12 +355,11 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 
 		pos += fio->nbufs << DM_VERITY_FEC_BUF_RS_BITS;
 	}
 
 	/* Always re-validate the corrected block against the expected hash */
-	r = verity_hash(v, io, fio->output, 1 << v->data_dev_block_bits,
-			io->tmp_digest);
+	r = verity_hash(v, io, fio->output, v->fec->block_size, io->tmp_digest);
 	if (unlikely(r < 0))
 		return r;
 
 	if (memcmp(io->tmp_digest, want_digest, v->digest_size)) {
 		DMERR_LIMIT("%s: FEC %llu: failed to correct (%d erasures)",
@@ -424,11 +423,11 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		r = fec_decode_rsb(v, io, fio, rsb, offset, want_digest, true);
 		if (r < 0)
 			goto done;
 	}
 
-	memcpy(dest, fio->output, 1 << v->data_dev_block_bits);
+	memcpy(dest, fio->output, v->fec->block_size);
 	atomic64_inc(&v->fec->corrected);
 
 done:
 	fio->level--;
 	return r;
@@ -645,10 +644,11 @@ int verity_fec_ctr(struct dm_verity *v)
 	 */
 	if (v->data_dev_block_bits != v->hash_dev_block_bits) {
 		ti->error = "Block sizes must match to use FEC";
 		return -EINVAL;
 	}
+	f->block_size = 1 << v->data_dev_block_bits;
 
 	if (!f->roots) {
 		ti->error = "Missing " DM_VERITY_OPT_FEC_ROOTS;
 		return -EINVAL;
 	}
@@ -682,14 +682,11 @@ int verity_fec_ctr(struct dm_verity *v)
 		ti->error = "Hash device is too small for "
 			DM_VERITY_OPT_FEC_BLOCKS;
 		return -E2BIG;
 	}
 
-	f->io_size = 1 << v->data_dev_block_bits;
-
-	f->bufio = dm_bufio_client_create(f->dev->bdev,
-					  f->io_size,
+	f->bufio = dm_bufio_client_create(f->dev->bdev, f->block_size,
 					  1, 0, NULL, NULL, 0);
 	if (IS_ERR(f->bufio)) {
 		ti->error = "Cannot initialize FEC bufio client";
 		return PTR_ERR(f->bufio);
 	}
@@ -699,12 +696,11 @@ int verity_fec_ctr(struct dm_verity *v)
 	if (dm_bufio_get_device_size(f->bufio) < f->rounds * f->roots) {
 		ti->error = "FEC device is too small";
 		return -E2BIG;
 	}
 
-	f->data_bufio = dm_bufio_client_create(v->data_dev->bdev,
-					       1 << v->data_dev_block_bits,
+	f->data_bufio = dm_bufio_client_create(v->data_dev->bdev, f->block_size,
 					       1, 0, NULL, NULL, 0);
 	if (IS_ERR(f->data_bufio)) {
 		ti->error = "Cannot initialize FEC data bufio client";
 		return PTR_ERR(f->data_bufio);
 	}
@@ -747,11 +743,11 @@ int verity_fec_ctr(struct dm_verity *v)
 		return ret;
 	}
 
 	/* Preallocate an output buffer for each thread */
 	ret = mempool_init_kmalloc_pool(&f->output_pool, num_online_cpus(),
-					1 << v->data_dev_block_bits);
+					f->block_size);
 	if (ret) {
 		ti->error = "Cannot allocate FEC output pool";
 		return ret;
 	}
 
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 257a609274c7c..49d43894ea746 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -27,11 +27,11 @@
 /* configuration */
 struct dm_verity_fec {
 	struct dm_dev *dev;	/* parity data device */
 	struct dm_bufio_client *data_bufio;	/* for data dev access */
 	struct dm_bufio_client *bufio;		/* for parity data access */
-	size_t io_size;		/* IO size for roots */
+	size_t block_size;	/* size of data, hash, and parity blocks in bytes */
 	sector_t start;		/* parity data start in blocks */
 	sector_t blocks;	/* number of blocks covered */
 	sector_t rounds;	/* number of interleaving rounds */
 	sector_t hash_blocks;	/* blocks covered after v->hash_start */
 	unsigned char roots;	/* parity bytes per RS codeword, n-k of RS(n, k) */
-- 
2.52.0

[PATCH 12/22] dm-verity-fec: rename rounds to region_blocks

[Eric Biggers]

It's hard to reconcile the value stored in dm_verity_fec::rounds with
its name and documentation.  Most likely "rounds" is being used as an
alias for what is more commonly called the interleaving degree or
"number of ways".  But the interleaving is done at the byte level,
whereas the units of "rounds" are blocks.  So it's not really that.

In practice, the reason the code needs this value is that it expresses
the number of blocks in each "region" of the message data, where each
region contains the bytes from a particular index in the RS codewords.

Rename it to region_blocks to make the code a bit more understandable.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 16 ++++++++--------
 drivers/md/dm-verity-fec.h |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 6ba9a1e039be3..28b47497c3d3f 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -29,11 +29,11 @@ static inline unsigned int fec_max_nbufs(struct dm_verity *v)
 static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
 {
 	u32 mod;
 
 	mod = do_div(offset, v->fec->rs_k);
-	return offset + mod * (v->fec->rounds << v->data_dev_block_bits);
+	return offset + mod * (v->fec->region_blocks << v->data_dev_block_bits);
 }
 
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
 	for (__i = 0; __i < (io)->nbufs; __i++)
@@ -403,17 +403,17 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 	 * and each code is interleaved over k blocks to make it less likely
 	 * that bursty corruption will leave us in unrecoverable state.
 	 */
 
 	offset = block << v->data_dev_block_bits;
-	res = div64_u64(offset, v->fec->rounds << v->data_dev_block_bits);
+	res = div64_u64(offset, v->fec->region_blocks << v->data_dev_block_bits);
 
 	/*
 	 * The base RS block we can feed to the interleaver to find out all
 	 * blocks required for decoding.
 	 */
-	rsb = offset - res * (v->fec->rounds << v->data_dev_block_bits);
+	rsb = offset - res * (v->fec->region_blocks << v->data_dev_block_bits);
 
 	/*
 	 * Locating erasures is slow, so attempt to recover the block without
 	 * them first. Do a second attempt with erasures if the corruption is
 	 * bad enough.
@@ -657,19 +657,19 @@ int verity_fec_ctr(struct dm_verity *v)
 	if (!f->blocks) {
 		ti->error = "Missing " DM_VERITY_OPT_FEC_BLOCKS;
 		return -EINVAL;
 	}
 
-	f->rounds = f->blocks;
-	if (sector_div(f->rounds, f->rs_k))
-		f->rounds++;
+	f->region_blocks = f->blocks;
+	if (sector_div(f->region_blocks, f->rs_k))
+		f->region_blocks++;
 
 	/*
 	 * Due to optional metadata, f->blocks can be larger than
 	 * data_blocks and hash_blocks combined.
 	 */
-	if (f->blocks < v->data_blocks + hash_blocks || !f->rounds) {
+	if (f->blocks < v->data_blocks + hash_blocks || !f->region_blocks) {
 		ti->error = "Invalid " DM_VERITY_OPT_FEC_BLOCKS;
 		return -EINVAL;
 	}
 
 	/*
@@ -691,11 +691,11 @@ int verity_fec_ctr(struct dm_verity *v)
 		return PTR_ERR(f->bufio);
 	}
 
 	dm_bufio_set_sector_offset(f->bufio, f->start << (v->data_dev_block_bits - SECTOR_SHIFT));
 
-	if (dm_bufio_get_device_size(f->bufio) < f->rounds * f->roots) {
+	if (dm_bufio_get_device_size(f->bufio) < f->region_blocks * f->roots) {
 		ti->error = "FEC device is too small";
 		return -E2BIG;
 	}
 
 	f->data_bufio = dm_bufio_client_create(v->data_dev->bdev, f->block_size,
diff --git a/drivers/md/dm-verity-fec.h b/drivers/md/dm-verity-fec.h
index 49d43894ea746..50b5e187d5cc1 100644
--- a/drivers/md/dm-verity-fec.h
+++ b/drivers/md/dm-verity-fec.h
@@ -30,11 +30,11 @@ struct dm_verity_fec {
 	struct dm_bufio_client *data_bufio;	/* for data dev access */
 	struct dm_bufio_client *bufio;		/* for parity data access */
 	size_t block_size;	/* size of data, hash, and parity blocks in bytes */
 	sector_t start;		/* parity data start in blocks */
 	sector_t blocks;	/* number of blocks covered */
-	sector_t rounds;	/* number of interleaving rounds */
+	sector_t region_blocks; /* blocks per region: ceil(blocks / rs_k) */
 	sector_t hash_blocks;	/* blocks covered after v->hash_start */
 	unsigned char roots;	/* parity bytes per RS codeword, n-k of RS(n, k) */
 	unsigned char rs_k;	/* message bytes per RS codeword, k of RS(n, k) */
 	mempool_t fio_pool;	/* mempool for dm_verity_fec_io */
 	mempool_t rs_pool;	/* mempool for fio->rs */
-- 
2.52.0

[PATCH 13/22] dm-verity-fec: simplify computation of rsb

[Eric Biggers]

To compute 'rsb', verity_fec_decode() divides 'offset' by
'v->fec->region_blocks << v->data_dev_block_bits', then subtracts the
quotient times that divisor.  That's simply the long way to do a modulo
operation, i.e. a - b * floor(a / b) instead of just a % b.  Use
div64_u64_rem() to get the remainder more concisely.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 28b47497c3d3f..63eeef26a3999 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -375,11 +375,11 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		      enum verity_block_type type, const u8 *want_digest,
 		      sector_t block, u8 *dest)
 {
 	int r;
 	struct dm_verity_fec_io *fio;
-	u64 offset, res, rsb;
+	u64 offset, rsb;
 
 	if (!verity_fec_is_enabled(v))
 		return -EOPNOTSUPP;
 
 	fio = io->fec_io;
@@ -403,17 +403,17 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 	 * and each code is interleaved over k blocks to make it less likely
 	 * that bursty corruption will leave us in unrecoverable state.
 	 */
 
 	offset = block << v->data_dev_block_bits;
-	res = div64_u64(offset, v->fec->region_blocks << v->data_dev_block_bits);
 
 	/*
 	 * The base RS block we can feed to the interleaver to find out all
 	 * blocks required for decoding.
 	 */
-	rsb = offset - res * (v->fec->region_blocks << v->data_dev_block_bits);
+	div64_u64_rem(offset, v->fec->region_blocks << v->data_dev_block_bits,
+		      &rsb);
 
 	/*
 	 * Locating erasures is slow, so attempt to recover the block without
 	 * them first. Do a second attempt with erasures if the corruption is
 	 * bad enough.
-- 
2.52.0

[PATCH 14/22] dm-verity-fec: simplify computation of ileaved

[Eric Biggers]

fec_read_bufs() just iterates over a sequence of message blocks with
step size region_blocks.  At each step, 'ileaved' is just the offset (in
bytes) to one of these blocks.  Compute it in the straightforward way,
eliminating fec_interleave().

In more detail, previously the code computed
'ileaved = (n / k) + (n % k) * (region_blocks * block_size)'
where n = rsb * k + i and 0 <= i < k.  Substituting 'n' gives:

    ileaved = ((rsb * k + i) / k) + ((rsb * k + i) % k) * region_blocks * block_size
            = rsb + (i * region_blocks * block_size)

The result is more efficient and easier to understand.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 63eeef26a3999..3122017569718 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -21,21 +21,10 @@
 static inline unsigned int fec_max_nbufs(struct dm_verity *v)
 {
 	return 1 << (v->data_dev_block_bits - DM_VERITY_FEC_BUF_RS_BITS);
 }
 
-/*
- * Return an interleaved offset for a byte in RS block.
- */
-static inline u64 fec_interleave(struct dm_verity *v, u64 offset)
-{
-	u32 mod;
-
-	mod = do_div(offset, v->fec->rs_k);
-	return offset + mod * (v->fec->region_blocks << v->data_dev_block_bits);
-}
-
 /* Loop over each allocated buffer. */
 #define fec_for_each_buffer(io, __i) \
 	for (__i = 0; __i < (io)->nbufs; __i++)
 
 /* Loop over each RS message in each allocated buffer. */
@@ -202,11 +191,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	/*
 	 * read each of the rs_k data blocks that are part of the RS block, and
 	 * interleave contents to available bufs
 	 */
 	for (i = 0; i < v->fec->rs_k; i++) {
-		ileaved = fec_interleave(v, rsb * v->fec->rs_k + i);
+		ileaved = rsb + i * (v->fec->region_blocks << v->data_dev_block_bits);
 
 		/*
 		 * target is the data block we want to correct, target_index is
 		 * the index of this block within the rs_k RS blocks
 		 */
-- 
2.52.0

[PATCH 15/22] dm-verity-fec: simplify deinterleaving

[Eric Biggers]

Since fec_read_bufs() deinterleaves the bytes from 'bbuf' sequentially
starting from 'block_offset', it can just do simple increments instead
of the more complex fec_buffer_rs_index() computation.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 3122017569718..a49c43ca07763 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -42,19 +42,10 @@ static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
 					unsigned int i, unsigned int j)
 {
 	return &fio->bufs[i][j * v->fec->rs_k];
 }
 
-/*
- * Return the index of the current RS message when called inside
- * fec_for_each_buffer_rs_message.
- */
-static inline unsigned int fec_buffer_rs_index(unsigned int i, unsigned int j)
-{
-	return (i << DM_VERITY_FEC_BUF_RS_BITS) + j;
-}
-
 /*
  * Decode all RS codewords whose message bytes were loaded into fio->bufs.  Copy
  * the corrected bytes into fio->output starting from block_offset.
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
@@ -177,11 +168,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	struct dm_bufio_client *bufio;
 	struct dm_verity_fec_io *fio = io->fec_io;
 	u64 block, ileaved;
 	u8 *bbuf;
 	u8 want_digest[HASH_MAX_DIGESTSIZE];
-	unsigned int n, k;
+	unsigned int n, src_pos;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
 	if (neras)
 		*neras = 0;
 
@@ -252,17 +243,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 
 		/*
 		 * deinterleave and copy the bytes that fit into bufs,
 		 * starting from block_offset
 		 */
+		src_pos = block_offset;
 		fec_for_each_buffer_rs_message(fio, n, j) {
-			k = fec_buffer_rs_index(n, j) + block_offset;
-
-			if (k >= v->fec->block_size)
+			if (src_pos >= v->fec->block_size)
 				goto done;
-
-			fec_buffer_rs_message(v, fio, n, j)[i] = bbuf[k];
+			fec_buffer_rs_message(v, fio, n, j)[i] = bbuf[src_pos++];
 		}
 done:
 		dm_bufio_release(buf);
 	}
 
-- 
2.52.0

[PATCH 16/22] dm-verity-fec: rename block_offset to out_pos

[Eric Biggers]

The current position in the output block buffer is called 'pos' in
fec_decode_rsb(), and 'block_offset' in fec_read_bufs() and
fec_decode_bufs().  These names aren't very clear, especially
'block_offset' which is easily confused with the offset of a message or
parity block or the position in the current parity block.

Rename it to 'out_pos'.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index a49c43ca07763..51263f2be1350 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -44,15 +44,15 @@ static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
 	return &fio->bufs[i][j * v->fec->rs_k];
 }
 
 /*
  * Decode all RS codewords whose message bytes were loaded into fio->bufs.  Copy
- * the corrected bytes into fio->output starting from block_offset.
+ * the corrected bytes into fio->output starting from out_pos.
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			   struct dm_verity_fec_io *fio, u64 rsb, int byte_index,
-			   unsigned int block_offset, int neras)
+			   unsigned int out_pos, int neras)
 {
 	int r, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
 	uint16_t par_buf[DM_VERITY_FEC_MAX_ROOTS];
@@ -65,11 +65,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	 * the starting position in that block.  Then read that block.
 	 *
 	 * block_size is always a power of 2, but roots might not be.  Note that
 	 * when it's not, a codeword's parity bytes can span a block boundary.
 	 */
-	parity_block = (rsb + block_offset) * v->fec->roots;
+	parity_block = (rsb + out_pos) * v->fec->roots;
 	parity_pos = parity_block & (v->fec->block_size - 1);
 	parity_block >>= v->data_dev_block_bits;
 	par = dm_bufio_read_with_ioprio(v->fec->bufio, parity_block, &buf,
 					bio->bi_ioprio);
 	if (IS_ERR(par)) {
@@ -118,14 +118,13 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			r = res;
 			goto error;
 		}
 
 		corrected += res;
-		fio->output[block_offset] = msg_buf[byte_index];
+		fio->output[out_pos++] = msg_buf[byte_index];
 
-		block_offset++;
-		if (block_offset >= v->fec->block_size)
+		if (out_pos >= v->fec->block_size)
 			goto done;
 	}
 done:
 	r = corrected;
 error:
@@ -157,12 +156,11 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 /*
  * Read data blocks that are part of the RS block and deinterleave as much as
  * fits into buffers. Check for erasure locations if @neras is non-NULL.
  */
 static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			 u64 rsb, u64 target, unsigned int block_offset,
-			 int *neras)
+			 u64 rsb, u64 target, unsigned int out_pos, int *neras)
 {
 	bool is_zero;
 	int i, j, target_index = -1;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
@@ -241,13 +239,13 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 				fio->erasures[(*neras)++] = i;
 		}
 
 		/*
 		 * deinterleave and copy the bytes that fit into bufs,
-		 * starting from block_offset
+		 * starting from out_pos
 		 */
-		src_pos = block_offset;
+		src_pos = out_pos;
 		fec_for_each_buffer_rs_message(fio, n, j) {
 			if (src_pos >= v->fec->block_size)
 				goto done;
 			fec_buffer_rs_message(v, fio, n, j)[i] = bbuf[src_pos++];
 		}
@@ -315,25 +313,25 @@ static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
 			  struct dm_verity_fec_io *fio, u64 rsb, u64 offset,
 			  const u8 *want_digest, bool use_erasures)
 {
 	int r, neras = 0;
-	unsigned int pos;
+	unsigned int out_pos;
 
-	for (pos = 0; pos < v->fec->block_size;) {
+	for (out_pos = 0; out_pos < v->fec->block_size;) {
 		fec_init_bufs(v, fio);
 
-		r = fec_read_bufs(v, io, rsb, offset, pos,
+		r = fec_read_bufs(v, io, rsb, offset, out_pos,
 				  use_erasures ? &neras : NULL);
 		if (unlikely(r < 0))
 			return r;
 
-		r = fec_decode_bufs(v, io, fio, rsb, r, pos, neras);
+		r = fec_decode_bufs(v, io, fio, rsb, r, out_pos, neras);
 		if (r < 0)
 			return r;
 
-		pos += fio->nbufs << DM_VERITY_FEC_BUF_RS_BITS;
+		out_pos += fio->nbufs << DM_VERITY_FEC_BUF_RS_BITS;
 	}
 
 	/* Always re-validate the corrected block against the expected hash */
 	r = verity_hash(v, io, fio->output, v->fec->block_size, io->tmp_digest);
 	if (unlikely(r < 0))
-- 
2.52.0

[PATCH 17/22] dm-verity-fec: move computation of offset and rsb down a level

[Eric Biggers]

verity_fec_decode() computes (offset, rsb) from the target block index
and calls fec_decode_rsb() with these parameters.  Move this computation
into fec_decode_rsb(), and rename fec_decode_rsb() to fec_decode().

This ends up being simpler and enables further refactoring, specifically
making use of the quotient from the division more easily.  The function
renaming also eliminates a reference to the ambiguous term "rsb".

This change does mean the same div64_u64_rem() can now be executed twice
per block, since verity_fec_decode() calls fec_decode() up to twice per
block.  However, this cost is negligible compared to the rest of FEC.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 46 +++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 51263f2be1350..91670f7d0ea16 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -304,20 +304,30 @@ static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 
 	memset(fio->erasures, 0, sizeof(fio->erasures));
 }
 
 /*
- * Decode all RS blocks in a single data block and return the target block
- * (indicated by @offset) in fio->output. If @use_erasures is non-zero, uses
- * hashes to locate erasures.
+ * Try to correct the message (data or hash) block at index @target_block.
+ *
+ * If @use_erasures is true, use verity hashes to locate erasures.  This makes
+ * the error correction slower but up to twice as capable.
+ *
+ * On success, return 0 and write the corrected block to @fio->output.  0 is
+ * returned only if the digest of the corrected block matches @want_digest; this
+ * is critical to ensure that FEC can't cause dm-verity to return bad data.
  */
-static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io,
-			  struct dm_verity_fec_io *fio, u64 rsb, u64 offset,
-			  const u8 *want_digest, bool use_erasures)
+static int fec_decode(struct dm_verity *v, struct dm_verity_io *io,
+		      struct dm_verity_fec_io *fio, u64 target_block,
+		      const u8 *want_digest, bool use_erasures)
 {
 	int r, neras = 0;
 	unsigned int out_pos;
+	u64 offset = target_block << v->data_dev_block_bits;
+	u64 rsb;
+
+	div64_u64_rem(offset, v->fec->region_blocks << v->data_dev_block_bits,
+		      &rsb);
 
 	for (out_pos = 0; out_pos < v->fec->block_size;) {
 		fec_init_bufs(v, fio);
 
 		r = fec_read_bufs(v, io, rsb, offset, out_pos,
@@ -351,11 +361,10 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		      enum verity_block_type type, const u8 *want_digest,
 		      sector_t block, u8 *dest)
 {
 	int r;
 	struct dm_verity_fec_io *fio;
-	u64 offset, rsb;
 
 	if (!verity_fec_is_enabled(v))
 		return -EOPNOTSUPP;
 
 	fio = io->fec_io;
@@ -368,37 +377,18 @@ int verity_fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 	fio->level++;
 
 	if (type == DM_VERITY_BLOCK_TYPE_METADATA)
 		block = block - v->hash_start + v->data_blocks;
 
-	/*
-	 * For RS(n, k), the continuous FEC data is divided into blocks of k
-	 * bytes. Since block size may not be divisible by k, the last block
-	 * is zero padded when decoding.
-	 *
-	 * Each byte of the block is covered by a different RS(n, k) code,
-	 * and each code is interleaved over k blocks to make it less likely
-	 * that bursty corruption will leave us in unrecoverable state.
-	 */
-
-	offset = block << v->data_dev_block_bits;
-
-	/*
-	 * The base RS block we can feed to the interleaver to find out all
-	 * blocks required for decoding.
-	 */
-	div64_u64_rem(offset, v->fec->region_blocks << v->data_dev_block_bits,
-		      &rsb);
-
 	/*
 	 * Locating erasures is slow, so attempt to recover the block without
 	 * them first. Do a second attempt with erasures if the corruption is
 	 * bad enough.
 	 */
-	r = fec_decode_rsb(v, io, fio, rsb, offset, want_digest, false);
+	r = fec_decode(v, io, fio, block, want_digest, false);
 	if (r < 0) {
-		r = fec_decode_rsb(v, io, fio, rsb, offset, want_digest, true);
+		r = fec_decode(v, io, fio, block, want_digest, true);
 		if (r < 0)
 			goto done;
 	}
 
 	memcpy(dest, fio->output, v->fec->block_size);
-- 
2.52.0

[PATCH 18/22] dm-verity-fec: compute target region directly

[Eric Biggers]

Instead of determining the target block's region by checking which block
of the k blocks being iterated over in fec_read_bufs() is equal to the
target block, instead just directly use the quotient of the division of
target_block by region_blocks.

This is the same value, just derived in a more straightforward way.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 91670f7d0ea16..1b5052ba4f5a5 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -47,12 +47,12 @@ static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
 /*
  * Decode all RS codewords whose message bytes were loaded into fio->bufs.  Copy
  * the corrected bytes into fio->output starting from out_pos.
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			   struct dm_verity_fec_io *fio, u64 rsb, int byte_index,
-			   unsigned int out_pos, int neras)
+			   struct dm_verity_fec_io *fio, u64 rsb,
+			   int target_region, unsigned int out_pos, int neras)
 {
 	int r, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
 	uint16_t par_buf[DM_VERITY_FEC_MAX_ROOTS];
@@ -118,11 +118,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			r = res;
 			goto error;
 		}
 
 		corrected += res;
-		fio->output[out_pos++] = msg_buf[byte_index];
+		fio->output[out_pos++] = msg_buf[target_region];
 
 		if (out_pos >= v->fec->block_size)
 			goto done;
 	}
 done:
@@ -156,14 +156,14 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 /*
  * Read data blocks that are part of the RS block and deinterleave as much as
  * fits into buffers. Check for erasure locations if @neras is non-NULL.
  */
 static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			 u64 rsb, u64 target, unsigned int out_pos, int *neras)
+			 u64 rsb, unsigned int out_pos, int *neras)
 {
 	bool is_zero;
-	int i, j, target_index = -1;
+	int i, j;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
 	struct dm_verity_fec_io *fio = io->fec_io;
 	u64 block, ileaved;
 	u8 *bbuf;
@@ -181,18 +181,10 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	 * read each of the rs_k data blocks that are part of the RS block, and
 	 * interleave contents to available bufs
 	 */
 	for (i = 0; i < v->fec->rs_k; i++) {
 		ileaved = rsb + i * (v->fec->region_blocks << v->data_dev_block_bits);
-
-		/*
-		 * target is the data block we want to correct, target_index is
-		 * the index of this block within the rs_k RS blocks
-		 */
-		if (ileaved == target)
-			target_index = i;
-
 		block = ileaved >> v->data_dev_block_bits;
 		bufio = v->fec->data_bufio;
 
 		if (block >= v->data_blocks) {
 			block -= v->data_blocks;
@@ -250,12 +242,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			fec_buffer_rs_message(v, fio, n, j)[i] = bbuf[src_pos++];
 		}
 done:
 		dm_bufio_release(buf);
 	}
-
-	return target_index;
+	return 0;
 }
 
 /*
  * Allocate and initialize a struct dm_verity_fec_io to use for FEC for a bio.
  * This runs the first time a block needs to be corrected for a bio.  In the
@@ -318,26 +309,30 @@ static void fec_init_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio)
 static int fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		      struct dm_verity_fec_io *fio, u64 target_block,
 		      const u8 *want_digest, bool use_erasures)
 {
 	int r, neras = 0;
-	unsigned int out_pos;
-	u64 offset = target_block << v->data_dev_block_bits;
+	unsigned int target_region, out_pos;
 	u64 rsb;
 
-	div64_u64_rem(offset, v->fec->region_blocks << v->data_dev_block_bits,
-		      &rsb);
+	target_region = div64_u64_rem(
+		target_block << v->data_dev_block_bits,
+		v->fec->region_blocks << v->data_dev_block_bits, &rsb);
+	if (WARN_ON_ONCE(target_region >= v->fec->rs_k))
+		/* target_block is out-of-bounds.  Should never happen. */
+		return -EIO;
 
 	for (out_pos = 0; out_pos < v->fec->block_size;) {
 		fec_init_bufs(v, fio);
 
-		r = fec_read_bufs(v, io, rsb, offset, out_pos,
+		r = fec_read_bufs(v, io, rsb, out_pos,
 				  use_erasures ? &neras : NULL);
 		if (unlikely(r < 0))
 			return r;
 
-		r = fec_decode_bufs(v, io, fio, rsb, r, out_pos, neras);
+		r = fec_decode_bufs(v, io, fio, rsb, target_region,
+				    out_pos, neras);
 		if (r < 0)
 			return r;
 
 		out_pos += fio->nbufs << DM_VERITY_FEC_BUF_RS_BITS;
 	}
-- 
2.52.0

[PATCH 19/22] dm-verity-fec: pass down index_in_region instead of rsb

[Eric Biggers]

Replace 'rsb', which is a byte index, with 'index_in_region' which is a
block index.  The block index is slightly easier to compute, it matches
what fec_read_bufs() wants, and it avoids the mismatch between the name
and the units of the variable.  ('rsb' stood for "Reed-Solomon block",
but its units were bytes, not blocks.)

fec_decode_bufs() does want it as a byte index when computing
parity_block, but that's easily handled locally.

As long as the parameters to the log messages are being adjusted, also
eliminate the unnecessary casts to 'unsigned long long'.  %llu is the
correct way to print a u64 in the Linux kernel, as documented in
printk-formats.rst.  There's no PRIu64 macro like there is in userspace.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 47 +++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 1b5052ba4f5a5..37c4eb6a11dee 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -47,11 +47,11 @@ static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
 /*
  * Decode all RS codewords whose message bytes were loaded into fio->bufs.  Copy
  * the corrected bytes into fio->output starting from out_pos.
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			   struct dm_verity_fec_io *fio, u64 rsb,
+			   struct dm_verity_fec_io *fio, u64 index_in_region,
 			   int target_region, unsigned int out_pos, int neras)
 {
 	int r, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
@@ -65,18 +65,20 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	 * the starting position in that block.  Then read that block.
 	 *
 	 * block_size is always a power of 2, but roots might not be.  Note that
 	 * when it's not, a codeword's parity bytes can span a block boundary.
 	 */
-	parity_block = (rsb + out_pos) * v->fec->roots;
+	parity_block = ((index_in_region << v->data_dev_block_bits) + out_pos) *
+		       v->fec->roots;
 	parity_pos = parity_block & (v->fec->block_size - 1);
 	parity_block >>= v->data_dev_block_bits;
 	par = dm_bufio_read_with_ioprio(v->fec->bufio, parity_block, &buf,
 					bio->bi_ioprio);
 	if (IS_ERR(par)) {
 		DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
-		      v->data_dev->name, rsb, parity_block, PTR_ERR(par));
+		      v->data_dev->name, index_in_region, parity_block,
+		      PTR_ERR(par));
 		return PTR_ERR(par);
 	}
 
 	/*
 	 * Decode the RS codewords whose message bytes are in bufs. Each RS
@@ -101,12 +103,12 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			par = dm_bufio_read_with_ioprio(v->fec->bufio,
 							parity_block, &buf,
 							bio->bi_ioprio);
 			if (IS_ERR(par)) {
 				DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
-				      v->data_dev->name, rsb, parity_block,
-				      PTR_ERR(par));
+				      v->data_dev->name, index_in_region,
+				      parity_block, PTR_ERR(par));
 				return PTR_ERR(par);
 			}
 			for (; j < v->fec->roots; j++)
 				par_buf[j] = par[parity_pos++];
 		}
@@ -130,14 +132,14 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 error:
 	dm_bufio_release(buf);
 
 	if (r < 0 && neras)
 		DMERR_LIMIT("%s: FEC %llu: failed to correct: %d",
-			    v->data_dev->name, (unsigned long long)rsb, r);
+			    v->data_dev->name, index_in_region, r);
 	else if (r > 0)
 		DMWARN_LIMIT("%s: FEC %llu: corrected %d errors",
-			     v->data_dev->name, (unsigned long long)rsb, r);
+			     v->data_dev->name, index_in_region, r);
 
 	return r;
 }
 
 /*
@@ -156,18 +158,18 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 /*
  * Read data blocks that are part of the RS block and deinterleave as much as
  * fits into buffers. Check for erasure locations if @neras is non-NULL.
  */
 static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			 u64 rsb, unsigned int out_pos, int *neras)
+			 u64 index_in_region, unsigned int out_pos, int *neras)
 {
 	bool is_zero;
 	int i, j;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
 	struct dm_verity_fec_io *fio = io->fec_io;
-	u64 block, ileaved;
+	u64 block;
 	u8 *bbuf;
 	u8 want_digest[HASH_MAX_DIGESTSIZE];
 	unsigned int n, src_pos;
 	struct bio *bio = dm_bio_from_per_bio_data(io, v->ti->per_io_data_size);
 
@@ -180,12 +182,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	/*
 	 * read each of the rs_k data blocks that are part of the RS block, and
 	 * interleave contents to available bufs
 	 */
 	for (i = 0; i < v->fec->rs_k; i++) {
-		ileaved = rsb + i * (v->fec->region_blocks << v->data_dev_block_bits);
-		block = ileaved >> v->data_dev_block_bits;
+		block = i * v->fec->region_blocks + index_in_region;
 		bufio = v->fec->data_bufio;
 
 		if (block >= v->data_blocks) {
 			block -= v->data_blocks;
 
@@ -201,13 +202,12 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		}
 
 		bbuf = dm_bufio_read_with_ioprio(bufio, block, &buf, bio->bi_ioprio);
 		if (IS_ERR(bbuf)) {
 			DMWARN_LIMIT("%s: FEC %llu: read failed (%llu): %ld",
-				     v->data_dev->name,
-				     (unsigned long long)rsb,
-				     (unsigned long long)block, PTR_ERR(bbuf));
+				     v->data_dev->name, index_in_region, block,
+				     PTR_ERR(bbuf));
 
 			/* assume the block is corrupted */
 			if (neras && *neras <= v->fec->roots)
 				fio->erasures[(*neras)++] = i;
 
@@ -310,28 +310,33 @@ static int fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		      struct dm_verity_fec_io *fio, u64 target_block,
 		      const u8 *want_digest, bool use_erasures)
 {
 	int r, neras = 0;
 	unsigned int target_region, out_pos;
-	u64 rsb;
+	u64 index_in_region;
 
-	target_region = div64_u64_rem(
-		target_block << v->data_dev_block_bits,
-		v->fec->region_blocks << v->data_dev_block_bits, &rsb);
+	/*
+	 * Compute 'target_region', the index of the region the target block is
+	 * in; and 'index_in_region', the index of the target block within its
+	 * region.  The latter value is also the index within its region of each
+	 * message block that shares its RS codewords with the target block.
+	 */
+	target_region = div64_u64_rem(target_block, v->fec->region_blocks,
+				      &index_in_region);
 	if (WARN_ON_ONCE(target_region >= v->fec->rs_k))
 		/* target_block is out-of-bounds.  Should never happen. */
 		return -EIO;
 
 	for (out_pos = 0; out_pos < v->fec->block_size;) {
 		fec_init_bufs(v, fio);
 
-		r = fec_read_bufs(v, io, rsb, out_pos,
+		r = fec_read_bufs(v, io, index_in_region, out_pos,
 				  use_erasures ? &neras : NULL);
 		if (unlikely(r < 0))
 			return r;
 
-		r = fec_decode_bufs(v, io, fio, rsb, target_region,
+		r = fec_decode_bufs(v, io, fio, index_in_region, target_region,
 				    out_pos, neras);
 		if (r < 0)
 			return r;
 
 		out_pos += fio->nbufs << DM_VERITY_FEC_BUF_RS_BITS;
@@ -342,11 +347,11 @@ static int fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 	if (unlikely(r < 0))
 		return r;
 
 	if (memcmp(io->tmp_digest, want_digest, v->digest_size)) {
 		DMERR_LIMIT("%s: FEC %llu: failed to correct (%d erasures)",
-			    v->data_dev->name, (unsigned long long)rsb, neras);
+			    v->data_dev->name, index_in_region, neras);
 		return -EILSEQ;
 	}
 
 	return 0;
 }
-- 
2.52.0

[PATCH 20/22] dm-verity-fec: make fec_decode_bufs() just return 0 or error

[Eric Biggers]

fec_decode_bufs() returns the number of errors corrected or a negative
errno value.  However, the caller just checks for an errno value and
doesn't do anything with the number of errors corrected.  Simplify the
code by just returning 0 instead of the number of errors corrected.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 37c4eb6a11dee..59ff3d58f49df 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -50,11 +50,11 @@ static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			   struct dm_verity_fec_io *fio, u64 index_in_region,
 			   int target_region, unsigned int out_pos, int neras)
 {
-	int r, corrected = 0, res;
+	int r = 0, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
 	uint16_t par_buf[DM_VERITY_FEC_MAX_ROOTS];
 	u8 *par, *msg_buf;
 	u64 parity_block;
@@ -116,30 +116,27 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		/* Decode an RS codeword using the Reed-Solomon library. */
 		res = decode_rs8(fio->rs, msg_buf, par_buf, v->fec->rs_k,
 				 NULL, neras, fio->erasures, 0, NULL);
 		if (res < 0) {
 			r = res;
-			goto error;
+			goto done;
 		}
-
 		corrected += res;
 		fio->output[out_pos++] = msg_buf[target_region];
 
 		if (out_pos >= v->fec->block_size)
 			goto done;
 	}
 done:
-	r = corrected;
-error:
 	dm_bufio_release(buf);
 
 	if (r < 0 && neras)
 		DMERR_LIMIT("%s: FEC %llu: failed to correct: %d",
 			    v->data_dev->name, index_in_region, r);
-	else if (r > 0)
+	else if (r == 0 && corrected > 0)
 		DMWARN_LIMIT("%s: FEC %llu: corrected %d errors",
-			     v->data_dev->name, index_in_region, r);
+			     v->data_dev->name, index_in_region, corrected);
 
 	return r;
 }
 
 /*
-- 
2.52.0

[PATCH 21/22] dm-verity-fec: log target_block instead of index_in_region

[Eric Biggers]

The log message for a FEC error or correction includes the data device
name and index_in_region as the context.  Although the result of FEC
(for a particular dm-verity instance) is expected to be the same for a
given index_in_region, index_in_region does not uniquely identify the
actual target block that is being corrected.  Since that value
(target_block) is likely more useful, log it instead.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index 59ff3d58f49df..fbe01f809aca3 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -47,12 +47,13 @@ static inline u8 *fec_buffer_rs_message(struct dm_verity *v,
 /*
  * Decode all RS codewords whose message bytes were loaded into fio->bufs.  Copy
  * the corrected bytes into fio->output starting from out_pos.
  */
 static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			   struct dm_verity_fec_io *fio, u64 index_in_region,
-			   int target_region, unsigned int out_pos, int neras)
+			   struct dm_verity_fec_io *fio, u64 target_block,
+			   unsigned int target_region, u64 index_in_region,
+			   unsigned int out_pos, int neras)
 {
 	int r = 0, corrected = 0, res;
 	struct dm_buffer *buf;
 	unsigned int n, i, j, parity_pos, to_copy;
 	uint16_t par_buf[DM_VERITY_FEC_MAX_ROOTS];
@@ -73,11 +74,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 	parity_block >>= v->data_dev_block_bits;
 	par = dm_bufio_read_with_ioprio(v->fec->bufio, parity_block, &buf,
 					bio->bi_ioprio);
 	if (IS_ERR(par)) {
 		DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
-		      v->data_dev->name, index_in_region, parity_block,
+		      v->data_dev->name, target_block, parity_block,
 		      PTR_ERR(par));
 		return PTR_ERR(par);
 	}
 
 	/*
@@ -103,11 +104,11 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			par = dm_bufio_read_with_ioprio(v->fec->bufio,
 							parity_block, &buf,
 							bio->bi_ioprio);
 			if (IS_ERR(par)) {
 				DMERR("%s: FEC %llu: parity read failed (block %llu): %ld",
-				      v->data_dev->name, index_in_region,
+				      v->data_dev->name, target_block,
 				      parity_block, PTR_ERR(par));
 				return PTR_ERR(par);
 			}
 			for (; j < v->fec->roots; j++)
 				par_buf[j] = par[parity_pos++];
@@ -129,14 +130,14 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_io *io,
 done:
 	dm_bufio_release(buf);
 
 	if (r < 0 && neras)
 		DMERR_LIMIT("%s: FEC %llu: failed to correct: %d",
-			    v->data_dev->name, index_in_region, r);
+			    v->data_dev->name, target_block, r);
 	else if (r == 0 && corrected > 0)
 		DMWARN_LIMIT("%s: FEC %llu: corrected %d errors",
-			     v->data_dev->name, index_in_region, corrected);
+			     v->data_dev->name, target_block, corrected);
 
 	return r;
 }
 
 /*
@@ -155,11 +156,12 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 /*
  * Read data blocks that are part of the RS block and deinterleave as much as
  * fits into buffers. Check for erasure locations if @neras is non-NULL.
  */
 static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
-			 u64 index_in_region, unsigned int out_pos, int *neras)
+			 u64 target_block, u64 index_in_region,
+			 unsigned int out_pos, int *neras)
 {
 	bool is_zero;
 	int i, j;
 	struct dm_buffer *buf;
 	struct dm_bufio_client *bufio;
@@ -199,11 +201,11 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		}
 
 		bbuf = dm_bufio_read_with_ioprio(bufio, block, &buf, bio->bi_ioprio);
 		if (IS_ERR(bbuf)) {
 			DMWARN_LIMIT("%s: FEC %llu: read failed (%llu): %ld",
-				     v->data_dev->name, index_in_region, block,
+				     v->data_dev->name, target_block, block,
 				     PTR_ERR(bbuf));
 
 			/* assume the block is corrupted */
 			if (neras && *neras <= v->fec->roots)
 				fio->erasures[(*neras)++] = i;
@@ -324,17 +326,17 @@ static int fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 		return -EIO;
 
 	for (out_pos = 0; out_pos < v->fec->block_size;) {
 		fec_init_bufs(v, fio);
 
-		r = fec_read_bufs(v, io, index_in_region, out_pos,
+		r = fec_read_bufs(v, io, target_block, index_in_region, out_pos,
 				  use_erasures ? &neras : NULL);
 		if (unlikely(r < 0))
 			return r;
 
-		r = fec_decode_bufs(v, io, fio, index_in_region, target_region,
-				    out_pos, neras);
+		r = fec_decode_bufs(v, io, fio, target_block, target_region,
+				    index_in_region, out_pos, neras);
 		if (r < 0)
 			return r;
 
 		out_pos += fio->nbufs << DM_VERITY_FEC_BUF_RS_BITS;
 	}
@@ -344,11 +346,11 @@ static int fec_decode(struct dm_verity *v, struct dm_verity_io *io,
 	if (unlikely(r < 0))
 		return r;
 
 	if (memcmp(io->tmp_digest, want_digest, v->digest_size)) {
 		DMERR_LIMIT("%s: FEC %llu: failed to correct (%d erasures)",
-			    v->data_dev->name, index_in_region, neras);
+			    v->data_dev->name, target_block, neras);
 		return -EILSEQ;
 	}
 
 	return 0;
 }
-- 
2.52.0

[PATCH 22/22] dm-verity-fec: improve comments for fec_read_bufs()

[Eric Biggers]

Update the comments in and above fec_read_bufs() to more clearly
describe what it does.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 drivers/md/dm-verity-fec.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c
index fbe01f809aca3..70aa4a20891bc 100644
--- a/drivers/md/dm-verity-fec.c
+++ b/drivers/md/dm-verity-fec.c
@@ -152,12 +152,25 @@ static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io,
 
 	return memcmp(io->tmp_digest, want_digest, v->digest_size) != 0;
 }
 
 /*
- * Read data blocks that are part of the RS block and deinterleave as much as
- * fits into buffers. Check for erasure locations if @neras is non-NULL.
+ * Read the message block at index @index_in_region within each of the
+ * @v->fec->rs_k regions and deinterleave their contents into @io->fec_io->bufs.
+ *
+ * @target_block gives the index of specific block within this sequence that is
+ * being corrected, relative to the start of all the FEC message blocks.
+ *
+ * @out_pos gives the current output position, i.e. the position in (each) block
+ * from which to start the deinterleaving.  Deinterleaving continues until
+ * either end-of-block is reached or there's no more buffer space.
+ *
+ * If @neras is non-NULL, then also use verity hashes and the presence/absence
+ * of I/O errors to determine which of the message blocks in the sequence are
+ * likely to be incorrect.  Write the number of such blocks to *@neras and the
+ * indices of the corresponding RS message bytes in [0, k - 1] to
+ * @io->fec_io->erasures, up to a limit of @v->fec->roots + 1 such blocks.
  */
 static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			 u64 target_block, u64 index_in_region,
 			 unsigned int out_pos, int *neras)
 {
@@ -176,15 +189,15 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 		*neras = 0;
 
 	if (WARN_ON(v->digest_size > sizeof(want_digest)))
 		return -EINVAL;
 
-	/*
-	 * read each of the rs_k data blocks that are part of the RS block, and
-	 * interleave contents to available bufs
-	 */
 	for (i = 0; i < v->fec->rs_k; i++) {
+		/*
+		 * Read the block from region i.  It contains the i'th message
+		 * byte of the target block's RS codewords.
+		 */
 		block = i * v->fec->region_blocks + index_in_region;
 		bufio = v->fec->data_bufio;
 
 		if (block >= v->data_blocks) {
 			block -= v->data_blocks;
@@ -229,12 +242,13 @@ static int fec_read_bufs(struct dm_verity *v, struct dm_verity_io *io,
 			    fec_is_erasure(v, io, want_digest, bbuf))
 				fio->erasures[(*neras)++] = i;
 		}
 
 		/*
-		 * deinterleave and copy the bytes that fit into bufs,
-		 * starting from out_pos
+		 * Deinterleave the bytes of the block, starting from 'out_pos',
+		 * into the i'th byte of the RS message buffers.  Stop when
+		 * end-of-block is reached or there are no more buffers.
 		 */
 		src_pos = out_pos;
 		fec_for_each_buffer_rs_message(fio, n, j) {
 			if (src_pos >= v->fec->block_size)
 				goto done;
-- 
2.52.0

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Sami Tolvanen]

Hi Eric,

On Thu, Feb 5, 2026 at 9:01 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> This series applies to linux-dm/for-next.  It can also be retrieved from:
>
>     git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git dm-verity-fec-2026-02-05
>
> Patches 1-5 fix bugs in dm-verity's forward error correction (FEC):
>
> - FEC and hash devices that are too small were not rejected.
>
> - Corrected blocks could be multiple-counted in statistics.
>
> - The erasures array was significantly oversized for its use case.
>
> - An out-of-bounds read could occur when decoding an RS codeword whose
>   parity bytes span a block boundary.

Thanks for the fixes, these look correct to me. It would be nice to
have tests for the edge cases though. Perhaps in the
verity-compat-test script that's included in the cryptsetup repo?

> Patches 6-22 clean up the FEC implementation to be easier to understand
> and improve documentation and log messages.

The clean-ups also look reasonable. For the series:

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Eric Biggers]

On Wed, Feb 11, 2026 at 02:29:28PM -0800, Sami Tolvanen wrote:
> Hi Eric,
> 
> On Thu, Feb 5, 2026 at 9:01 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > This series applies to linux-dm/for-next.  It can also be retrieved from:
> >
> >     git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git dm-verity-fec-2026-02-05
> >
> > Patches 1-5 fix bugs in dm-verity's forward error correction (FEC):
> >
> > - FEC and hash devices that are too small were not rejected.
> >
> > - Corrected blocks could be multiple-counted in statistics.
> >
> > - The erasures array was significantly oversized for its use case.
> >
> > - An out-of-bounds read could occur when decoding an RS codeword whose
> >   parity bytes span a block boundary.
> 
> Thanks for the fixes, these look correct to me. It would be nice to
> have tests for the edge cases though. Perhaps in the
> verity-compat-test script that's included in the cryptsetup repo?
> 
> > Patches 6-22 clean up the FEC implementation to be easier to understand
> > and improve documentation and log messages.
> 
> The clean-ups also look reasonable. For the series:
> 
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> 
> Sami

"FEC and hash devices that are too small were not rejected" should be
fairly straightforward to test.

Testing "Corrected blocks could be multiple-counted in statistics" and
"An out-of-bounds read could occur when decoding an RS codeword whose
parity bytes span a block boundary" would require injecting kmalloc
failures.  It might be possible using the fault injection framework.

"The erasures array was significantly oversized for its use case" just
changes the size of an internal structure.  Not testable from userspace.

Unfortunately the verity-compat-test script isn't in very good shape.  I
opened a pull request to clean it up:
https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/890

After that I'll add test cases for at least the "FEC and hash devices
that are too small were not rejected" bug.

I noticed verity-compat-test also never tests fec_roots != 2, even
though it contains code that seems to have been intended to.  So I'll
plan to fix that too, though unless we're able to add fault injection
too it won't specifically cover the bug fixed by this series.

Anyway, point is, I'm indeed working on improving the test script.  I
think these patches can be applied for 7.1 either way though.

- Eric

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Milan Broz]

On 3/3/26 9:16 PM, Eric Biggers wrote:
> Unfortunately the verity-compat-test script isn't in very good shape.  I
> opened a pull request to clean it up:
> https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/890

The testing suite was written mostly as a volunteer effort for cryptsetup
to cover userspace interoperability, so it uses mostly parameters that
worked properly with many ancient kernels (root=2, etc.).

The testsuite was not intended to test the kernel in the first place,
similar to the dm-crypt one. Despite that, it has found many kernel
bugs in the past, apparently because the kernel itself lacks proper tests.

As I am the primary author of this test and have not been paid for working
on cryptsetup for years now, I have really enough of this "your code is shit,
I know better" approach without even cc'ing us.

We try not to touch tests much (e.g., refactoring) unless really necessary,
to prevent accidentally introducing regressions.
If there is a QA team validating it, it would be much simpler, but each
corporation cares only about its own corporate sh^W products.

Your approach is going exactly in the opposite direction, rewriting only
parts you are interested in, according to your taste. So now we have multiple
approaches in different scripts. I am really tempted to reject this, as it will
add us much more effort in the future.

That said, I want to add new tests there, but please at least try to think
about it from this point of view.

Thanks,
Milan

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Eric Biggers]

On Wed, Mar 04, 2026 at 09:25:02AM +0100, Milan Broz wrote:
> On 3/3/26 9:16 PM, Eric Biggers wrote:
> > Unfortunately the verity-compat-test script isn't in very good shape.  I
> > opened a pull request to clean it up:
> > https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/890
> 
> The testing suite was written mostly as a volunteer effort for cryptsetup
> to cover userspace interoperability, so it uses mostly parameters that
> worked properly with many ancient kernels (root=2, etc.).
> 
> The testsuite was not intended to test the kernel in the first place,
> similar to the dm-crypt one. Despite that, it has found many kernel
> bugs in the past, apparently because the kernel itself lacks proper tests.
> 
> As I am the primary author of this test and have not been paid for working
> on cryptsetup for years now, I have really enough of this "your code is shit,
> I know better" approach without even cc'ing us.
> 
> We try not to touch tests much (e.g., refactoring) unless really necessary,
> to prevent accidentally introducing regressions.
> If there is a QA team validating it, it would be much simpler, but each
> corporation cares only about its own corporate sh^W products.
> 
> Your approach is going exactly in the opposite direction, rewriting only
> parts you are interested in, according to your taste. So now we have multiple
> approaches in different scripts. I am really tempted to reject this, as it will
> add us much more effort in the future.
> 
> That said, I want to add new tests there, but please at least try to think
> about it from this point of view.

Okay, it sounds like the verity-compat-test script isn't open for
contributions then.  Would have been nice to know earlier, but thanks
for letting me know now.  I'll plan to start some new tests for
kselftests.

- Eric

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Milan Broz]

On 3/4/26 10:00 AM, Eric Biggers wrote:
> On Wed, Mar 04, 2026 at 09:25:02AM +0100, Milan Broz wrote:

> Okay, it sounds like the verity-compat-test script isn't open for
> contributions then.  Would have been nice to know earlier, but thanks
> for letting me know now.  I'll plan to start some new tests for
> kselftests.

That's not what I said, just I would prefer adding new test without
the need to completely reformatting the test.

But IMO, kernel self tests _should_ cover it. It can do a lot of
more things there and can catch regressions earlier, as we usually
catch them once it appears in Fedora rawhide (as this is in our CI).

Milan

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Eric Biggers]

On Wed, Mar 04, 2026 at 10:34:37AM +0100, Milan Broz wrote:
> On 3/4/26 10:00 AM, Eric Biggers wrote:
> > On Wed, Mar 04, 2026 at 09:25:02AM +0100, Milan Broz wrote:
> 
> > Okay, it sounds like the verity-compat-test script isn't open for
> > contributions then.  Would have been nice to know earlier, but thanks
> > for letting me know now.  I'll plan to start some new tests for
> > kselftests.
> 
> That's not what I said, just I would prefer adding new test without
> the need to completely reformatting the test.

Adding new test cases to that script naturally requires understanding a
lot of the existing code so the new cases can be properly integrated.
For example new test cases logically should use the existing helper
functions, common global variables, and failure reporting mechanism; and
they shouldn't duplicate or interfere with the existing test cases.  And
in some cases a new test case may be much more easily handled as an
extension or fix to an existing one; for example the easiest way to test
fec_roots != 2 probably would be to just fix check_fec() to honor its
existing fec_roots parameter as seems to have been intended.

But with the existing script not following modern shell-scripting
practices like naming parameters, using local variables, and passing
'shellcheck', I found that to be a challenge for understanding the
existing code and adding new code.  Especially when these practices are
causing bugs, like the ones my merge request would have fixed.

So that's why I thought a broader cleanup of this test script would be
really useful first.  And I was glad to do that, which I did.  None of
this was meant of a criticism of you; I'm just trying to help.

But it's clear I wasted my time and should focus my efforts on the
kernel, kselftests, and other userspace projects.  So I'll continue to
do that and leave cryptsetup alone.  Sorry for trying to contribute.

- Eric

Re: [PATCH 00/22] dm-verity: more FEC fixes and cleanups

[Milan Broz]

On 3/4/26 6:45 PM, Eric Biggers wrote:
> On Wed, Mar 04, 2026 at 10:34:37AM +0100, Milan Broz wrote:
>> On 3/4/26 10:00 AM, Eric Biggers wrote:
>>> On Wed, Mar 04, 2026 at 09:25:02AM +0100, Milan Broz wrote:
>>
>>> Okay, it sounds like the verity-compat-test script isn't open for
>>> contributions then.  Would have been nice to know earlier, but thanks
>>> for letting me know now.  I'll plan to start some new tests for
>>> kselftests.
>>
>> That's not what I said, just I would prefer adding new test without
>> the need to completely reformatting the test.
> 
> Adding new test cases to that script naturally requires understanding a
> lot of the existing code so the new cases can be properly integrated.
> For example new test cases logically should use the existing helper
> functions, common global variables, and failure reporting mechanism; and
> they shouldn't duplicate or interfere with the existing test cases.  And
> in some cases a new test case may be much more easily handled as an
> extension or fix to an existing one; for example the easiest way to test
> fec_roots != 2 probably would be to just fix check_fec() to honor its
> existing fec_roots parameter as seems to have been intended.
> 
> But with the existing script not following modern shell-scripting
> practices like naming parameters, using local variables, and passing
> 'shellcheck', I found that to be a challenge for understanding the
> existing code and adding new code.  Especially when these practices are
> causing bugs, like the ones my merge request would have fixed.
> 
> So that's why I thought a broader cleanup of this test script would be
> really useful first.  And I was glad to do that, which I did.  None of
> this was meant of a criticism of you; I'm just trying to help.
> 
> But it's clear I wasted my time and should focus my efforts on the
> kernel, kselftests, and other userspace projects.  So I'll continue to
> do that and leave cryptsetup alone.  Sorry for trying to contribute.

Hi Eric,

I actually agree with you on almost everything above.
But this is not the only test script we have.

If we decide to do cleanup, things like ">/dev/null 2>&1" to "&>/dev/null",
or wrap long lines (as in your series), I would like to do this across
the entire testsuite. And actually, I can do that myself; I just need to plan it.
These changes are pure cosmetics.

Then there are some real cleanups, like local variable naming,
that make sense to apply now. If you submit these with real test additions,
I already merged them to the main branch.

This is the diff I am talking about
  https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/890/diffs

It's common for maintainers to have requirements for code changes.
I think we both know that.

I would like to see that both the kernel and the cryptsetup project have good
code coverage for dm-verity. Kernel has none currently.

As we already have a testsuite, it would be nice and probably much easier to add
new corner cases to test your recent dm-verity improvements.
But please also try to respect what I said above.

Thank you,
Milan