From 8e77a5eebc97cb9c13a6d863181abb2c7626fd2a Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 29 May 2024 23:24:08 -0500 Subject: [PATCH] Switched to clobbering rcache for prog checking While exploring the test_badblocks ERASENOOP failure more, I realized the problem is that we are nesting crc32cs. To be clear, using crc32cs to validate progs in general is not an issue, that is perfectly fine on paper. The issue is that we were using crc32cs to validate progs _that contain crc32cs_. Looking at the collision, we can see the fully expanded lleb128s we use for our cksum tags: 00 00 00 ff b0 02 00 87 80 80 00 3e c0 7f 7e => bdfa9b10 ab 77 de c2 b0 03 00 87 80 80 00 3e 38 d5 22 => bdfa9b10 '-.-' ^ '----.----' '----.----' '----|------|-----------|-- cksum tag '------|-----------|-- cksum weight (0) '-----------|-- cksum size + padding '-- cksum crc32c So we ended up perfectly aligning the cksum's crc32c with our cache line. Lucky us. Unfortunately funny math makes it so that whenever a crc32c contains a crc32c, the inner crc32c sort of cancels itself out from the outer crc32c. So these two messages end up mathematically equivalent, even though they contain different data: crc(m) = m(x) x^|P|-1 mod P crc(m ++ crc(m)) = (m(x) x^|P|-1 + (m(x) x^|P|-1 mod P)) x^|P|-1 mod P crc(m ++ crc(m)) = (m(x) x^|P|-1 + m(x) x^|P|-1) x^|P|-1 mod P crc(m ++ crc(m)) = 0 x^|P|-1 mod P crc(m ++ crc(m)) = 0 So using a crc32c to check progs is not fit for purpose. This leaves us with a couple options: 1. Use a different checksum, or do something like rearranging bytes to avoid this cancelling out issue. Unfortunately this gets tricky since crc32cs are linear, simply using an xor mask won't work... 2. Don't check progs at such a low-level, but at a high-level using the rbyd/data block crc32cs. Since this would mean only one crc32c, this would avoid nesting issues. Unfortunately this would probably come with quite a high code cost to try to keep track of both the before+after rbyd cksums everywhere... 3. Just read back the data into the rcache to compare at the byte-level, which would mean clobbering our rcache when prog checking is enabled. This commit goes with option 3., which is probably the simplest. It also removes any question of crc32c collision, which could be a real nuisance when debugging low-level block device operations, a use case where prog checking will hopefully be quite valuable. Clobbering the rcache also has the advantage of reverting the prog >= read requirement, which is nice for flexibility. Though this needs to be tested. --- There was a bit of a hiccup, and that was how prog checking interacts with lfsr_bd_cpy. lfsr_bd_cpy used the rcache to hold data being copied to/from disk, but this data needs to be checked, and prog checking would clobber the rcache. Problems! I guess this is one footgun of the internal lfsr_bd_readnext API... The solution is to instead turn this around and use the pcache to hold any copied data, since this would not be clobbered when prog checking. This has some other knock-on effects, mainly that we can't take advantage of read hints in lfsr_bd_cpy, but has the added advantage of potentially not clobbering the rcache at all when no checking progs. Code changes were fairly minimal: code stack before: 33718 2608 after: 33690 (-0.1%) 2608 (+0.0%) --- lfs.c | 71 ++++++++++++++++++++------------------- lfs.h | 2 +- tests/test_badblocks.toml | 3 +- 3 files changed, 39 insertions(+), 37 deletions(-) diff --git a/lfs.c b/lfs.c index fb9184e1..aca7d41a 100644 --- a/lfs.c +++ b/lfs.c @@ -165,6 +165,9 @@ static int lfsr_bd_readnext(lfs_t *lfs, lfsr_bd_droprcache(lfs); // load into rcache, above conditions can no longer fail + // + // note it's ok if we overlap the pcache a bit, pcache always + // takes priority until flush, which updates the rcache lfs_size_t off__ = lfs_aligndown(off, lfs->cfg->read_size); lfs_size_t size__ = lfs_alignup( lfs_min( @@ -289,6 +292,11 @@ static int lfsr_bd_read(lfs_t *lfs, return 0; } +// needed in lfsr_bd_prog_ for prog validation +static lfs_scmp_t lfsr_bd_cmp(lfs_t *lfs, + lfs_block_t block, lfs_size_t off, lfs_size_t hint, + const void *buffer, lfs_size_t size); + // low-level prog stuff static int lfsr_bd_prog_(lfs_t *lfs, lfs_block_t block, lfs_size_t off, const void *buffer, lfs_size_t size, @@ -301,34 +309,21 @@ static int lfsr_bd_prog_(lfs_t *lfs, lfs_block_t block, lfs_size_t off, // check progs? if (lfs->cfg->check_progs) { - // we want to reuse our buffer, so use a checksum to compare - uint32_t pcksum = lfs_crc32c(0, buffer, size); - // pcache should have been dropped at this point LFS_ASSERT(lfs->pcache.size == 0); - // read back our prog - uint32_t pcksum_ = 0; - lfs_size_t off_ = off; - while (off_ < off + size) { - lfs_size_t size_ = lfs_min( - size - (off_-off), - lfs->cfg->pcache_size); - err = lfsr_bd_read__(lfs, block, off_, - lfs->pcache.buffer, size_); - if (err) { - return err; - } + // invalidate rcache, we're going to clobber it anyways + lfsr_bd_droprcache(lfs); - pcksum_ = lfs_crc32c(pcksum_, lfs->pcache.buffer, size_); - off_ += size_; + lfs_scmp_t cmp = lfsr_bd_cmp(lfs, block, off, 0, + buffer, size); + if (cmp < 0) { + return cmp; } - if (pcksum != pcksum_) { - LFS_DEBUG("Bad prog 0x%"PRIx32".%"PRIx32" %"PRIu32" " - "(%08"PRIx32" != %08"PRIx32")", - block, off, size, - pcksum, pcksum_); + if (cmp != LFS_CMP_EQ) { + LFS_DEBUG("Bad prog 0x%"PRIx32".%"PRIx32" %"PRIu32" (checked)", + block, off, size); return LFS_ERR_CORRUPT; } } @@ -648,6 +643,9 @@ static int lfsr_bd_cpy(lfs_t *lfs, lfs_block_t src_block, lfs_size_t src_off, lfs_size_t hint, lfs_size_t size, uint32_t *cksum_) { + // we don't really use hint here because we go through our pcache + (void)hint; + // check for in-bounds LFS_ASSERT(dst_block < lfs->cfg->block_count); if (dst_off+size > lfs->cfg->block_size) { @@ -660,26 +658,33 @@ static int lfsr_bd_cpy(lfs_t *lfs, lfs_size_t dst_off_ = dst_off; lfs_size_t src_off_ = src_off; - lfs_size_t hint_ = lfs_max(hint, size); // make sure hint >= size lfs_size_t size_ = size; while (size_ > 0) { - const uint8_t *buffer__; + // prefer the pcache here to avoid rcache conflicts with prog + // validation, if we're lucky we might even be able to avoid + // clobbering the rcache at all + uint8_t *buffer__; lfs_size_t size__; - int err = lfsr_bd_readnext(lfs, src_block, src_off_, hint_, size_, - &buffer__, &size__); - if (err) { - return err; - } - - err = lfsr_bd_prog(lfs, dst_block, dst_off_, buffer__, size__, + int err = lfsr_bd_prognext(lfs, dst_block, dst_off_, size_, + &buffer__, &size__, cksum_); if (err) { return err; } + err = lfsr_bd_read(lfs, src_block, src_off_, 0, + buffer__, size__); + if (err) { + return err; + } + + // optional checksum + if (cksum_) { + *cksum_ = lfs_crc32c(*cksum_, buffer__, size__); + } + dst_off_ += size__; src_off_ += size__; - hint_ -= size__; size_ -= size__; } @@ -15495,8 +15500,6 @@ static int lfs_init(lfs_t *lfs, const struct lfs_config *cfg) { LFS_ASSERT(lfs->cfg->rcache_size % lfs->cfg->read_size == 0); LFS_ASSERT(lfs->cfg->pcache_size % lfs->cfg->prog_size == 0); - // prog_size must be a multiple of read_size - LFS_ASSERT(lfs->cfg->prog_size % lfs->cfg->read_size == 0); // block_size must be a multiple of both prog/read size LFS_ASSERT(lfs->cfg->block_size % lfs->cfg->read_size == 0); LFS_ASSERT(lfs->cfg->block_size % lfs->cfg->prog_size == 0); diff --git a/lfs.h b/lfs.h index 783b8f0f..6468ca51 100644 --- a/lfs.h +++ b/lfs.h @@ -195,7 +195,7 @@ struct lfs_config { lfs_size_t read_size; // Minimum size of a program in bytes. All program operations will be a - // multiple of this value. Must be a multiple of the read size. + // multiple of this value. lfs_size_t prog_size; // Size of an erasable block in bytes. This does not impact ram consumption diff --git a/tests/test_badblocks.toml b/tests/test_badblocks.toml index a7799fa4..fe159760 100644 --- a/tests/test_badblocks.toml +++ b/tests/test_badblocks.toml @@ -332,8 +332,7 @@ defines.BADBLOCK_BEHAVIOR = [ 'LFS_EMUBD_BADBLOCK_ERASEERROR', 'LFS_EMUBD_BADBLOCK_READERROR', 'LFS_EMUBD_BADBLOCK_PROGNOOP', -# TODO, this finds a crc32c collision -# 'LFS_EMUBD_BADBLOCK_ERASENOOP', + 'LFS_EMUBD_BADBLOCK_ERASENOOP', ] # we need prog checking to detect read errors defines.CHECK_PROGS = 'BADBLOCK_BEHAVIOR >= LFS_EMUBD_BADBLOCK_READERROR'