dm-verity: more FEC fixes and cleanups
Subsystems (3)
cover dm-verity: more FEC fixes and cleanups
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
Signature · 1 line
2.52.0
PATCH 1/22 dm-verity-fec: correctly reject too-small FEC devices
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.
--- drivers/md/dm-verity-fec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/md/dm-verity-fec.c +2 -3
@@ -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,
Signature · 1 line
2.52.0
PATCH 2/22 dm-verity-fec: correctly reject too-small hash devices
In reply to Eric BiggersFix 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.
--- drivers/md/dm-verity-fec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/md/dm-verity-fec.c +2 -1
@@ -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 ; }
Signature · 1 line
2.52.0
PATCH 3/22 dm-verity-fec: fix corrected block count stat
In reply to Eric Biggersdm_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.
--- drivers/md/dm-verity-fec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/md/dm-verity-fec.c +2 -3
@@ -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; }
Signature · 1 line
2.52.0
PATCH 4/22 dm-verity-fec: fix the size of dm_verity_fec_io::erasures
In reply to Eric BiggersAt 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.
--- drivers/md/dm-verity-fec.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/md/dm-verity-fec.h +2 -1
@@ -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
Signature · 1 line
2.52.0
PATCH 5/22 dm-verity-fec: fix reading parity bytes split across blocks (take 3)
In reply to Eric Biggersfec_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.
--- drivers/md/dm-verity-fec.c | 100 ++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 56 deletions(-)
drivers/md/dm-verity-fec.c +44 -56
@@ -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);
Signature · 1 line
2.52.0
PATCH 6/22 dm-verity: rename dm_verity::hash_blocks to dm_verity::hash_end
In reply to Eric BiggersRename hash_blocks to hash_end to reflect what it actually is.
--- 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(-)
drivers/md/dm-verity-fec.c +1 -1
@@ -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. */
drivers/md/dm-verity-target.c +4 -4
@@ -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; }
drivers/md/dm-verity.h +2 -2
@@ -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;
Signature · 1 line
2.52.0
PATCH 7/22 dm-verity-fec: improve documentation for Forward Error Correction
In reply to Eric BiggersUpdate verity.rst to add a dedicated section about FEC and improve the documentation for the FEC-related parameters.
--- .../admin-guide/device-mapper/verity.rst | 122 +++++++++++++++--- 1 file changed, 102 insertions(+), 20 deletions(-)
Documentation/admin-guide/device-mapper/verity.rst +102 -20
@@ -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.
Signature · 1 line
2.52.0
PATCH 8/22 dm-verity-fec: replace {MAX,MIN}_RSN with {MIN,MAX}_ROOTS
In reply to Eric BiggersEvery 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.
--- drivers/md/dm-verity-fec.c | 6 +++--- drivers/md/dm-verity-fec.h | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-)
drivers/md/dm-verity-fec.c +3 -3
@@ -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;
drivers/md/dm-verity-fec.h +3 -4
@@ -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
Signature · 1 line
2.52.0
PATCH 9/22 dm-verity-fec: use standard names for Reed-Solomon parameters
In reply to 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.
--- drivers/md/dm-verity-fec.c | 30 +++++++++++++++--------------- drivers/md/dm-verity-fec.h | 8 ++++---- 2 files changed, 19 insertions(+), 19 deletions(-)
drivers/md/dm-verity-fec.c +15 -15
@@ -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 ; }
drivers/md/dm-verity-fec.h +4 -4
@@ -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 */
Signature · 1 line
2.52.0
PATCH 10/22 dm-verity-fec: rename "RS block" to "RS codeword"
In reply to Eric BiggersThe 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.
--- drivers/md/dm-verity-fec.c | 58 ++++++++++++++++++++------------------ drivers/md/dm-verity-fec.h | 10 +++---- 2 files changed, 35 insertions(+), 33 deletions(-)
drivers/md/dm-verity-fec.c +30 -28
@@ -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); }
drivers/md/dm-verity-fec.h +5 -5
@@ -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[]; };
Signature · 1 line
2.52.0
PATCH 11/22 dm-verity-fec: replace io_size with block_size
In reply to Eric Biggersdm-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.
--- drivers/md/dm-verity-fec.c | 30 +++++++++++++----------------- drivers/md/dm-verity-fec.h | 2 +- 2 files changed, 14 insertions(+), 18 deletions(-)
drivers/md/dm-verity-fec.c +13 -17
@@ -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; }
drivers/md/dm-verity-fec.h +1 -1
@@ -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) */
Signature · 1 line
2.52.0
PATCH 12/22 dm-verity-fec: rename rounds to region_blocks
In reply to Eric BiggersIt'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.
--- drivers/md/dm-verity-fec.c | 16 ++++++++-------- drivers/md/dm-verity-fec.h | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-)
drivers/md/dm-verity-fec.c +8 -8
@@ -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,
drivers/md/dm-verity-fec.h +1 -1
@@ -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 */
Signature · 1 line
2.52.0
PATCH 13/22 dm-verity-fec: simplify computation of rsb
In reply to Eric BiggersTo 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.
--- drivers/md/dm-verity-fec.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/md/dm-verity-fec.c +3 -3
@@ -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.
Signature · 1 line
2.52.0
PATCH 14/22 dm-verity-fec: simplify computation of ileaved
In reply to Eric Biggersfec_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.
--- drivers/md/dm-verity-fec.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)
drivers/md/dm-verity-fec.c +1 -12
@@ -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 */
Signature · 1 line
2.52.0
PATCH 15/22 dm-verity-fec: simplify deinterleaving
In reply to Eric BiggersSince 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.
--- drivers/md/dm-verity-fec.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)
drivers/md/dm-verity-fec.c +4 -15
@@ -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); }
Signature · 1 line
2.52.0
PATCH 16/22 dm-verity-fec: rename block_offset to out_pos
In reply to Eric BiggersThe 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'.
--- drivers/md/dm-verity-fec.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)
drivers/md/dm-verity-fec.c +13 -15
@@ -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 ))
Signature · 1 line
2.52.0
PATCH 17/22 dm-verity-fec: move computation of offset and rsb down a level
In reply to Eric Biggersverity_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.
--- drivers/md/dm-verity-fec.c | 46 +++++++++++++++----------------------- 1 file changed, 18 insertions(+), 28 deletions(-)
drivers/md/dm-verity-fec.c +18 -28
@@ -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);
Signature · 1 line
2.52.0
PATCH 18/22 dm-verity-fec: compute target region directly
In reply to Eric BiggersInstead 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.
--- drivers/md/dm-verity-fec.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
drivers/md/dm-verity-fec.c +16 -21
@@ -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 ; }
Signature · 1 line
2.52.0
PATCH 19/22 dm-verity-fec: pass down index_in_region instead of rsb
In reply to Eric BiggersReplace '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.
--- drivers/md/dm-verity-fec.c | 47 +++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 21 deletions(-)
drivers/md/dm-verity-fec.c +26 -21
@@ -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 ; }
Signature · 1 line
2.52.0
PATCH 20/22 dm-verity-fec: make fec_decode_bufs() just return 0 or error
In reply to Eric Biggersfec_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.
--- drivers/md/dm-verity-fec.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
drivers/md/dm-verity-fec.c +4 -7
@@ -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; }/*
Signature · 1 line
2.52.0
PATCH 21/22 dm-verity-fec: log target_block instead of index_in_region
In reply to Eric BiggersThe 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.
--- drivers/md/dm-verity-fec.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
drivers/md/dm-verity-fec.c +14 -12
@@ -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 ; }
Signature · 1 line
2.52.0
PATCH 22/22 dm-verity-fec: improve comments for fec_read_bufs()
In reply to Eric BiggersUpdate the comments in and above fec_read_bufs() to more clearly describe what it does.
--- drivers/md/dm-verity-fec.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)
drivers/md/dm-verity-fec.c +22 -8
@@ -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;
Signature · 1 line
2.52.0
Hi Eric,
Quoted 7 lines · 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-05Patches 1-5 fix bugs in dm-verity's forward error correction (FEC):
Quoted 8 lines
- 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?
Quoted 2 lines
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:
Sami
Quoted 23 lines · Sami Tolvanen
Hi Eric,
Quoted 7 lines · 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-05Patches 1-5 fix bugs in dm-verity's forward error correction (FEC):
Quoted 8 lines
- 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?
Quoted 8 lines
Quoted 2 lines
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 <[email protected]>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
Quoted 3 lines · Eric Biggers
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
Quoted 22 lines · Milan Broz
Quoted 3 lines · Eric Biggers
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/890The 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.
Quoted 7 lines
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
Quoted 6 lines · Eric Biggers
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
Quoted 10 lines · Milan Broz
Quoted 6 lines · Eric Biggers
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
Quoted 28 lines · Eric Biggers
Quoted 10 lines · Milan Broz
Quoted 6 lines · Eric Biggers
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 fixcheck_fec()to honor its existingfec_rootsparameter 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.
Quoted 7 lines
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