Github/littlefs
1
Fork 0
Code Actions Activity

Switched to clobbering rcache for prog checking

Browse Source 
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%)
This commit is contained in:
Christopher Haster
2024-05-29 23:24:08 -05:00
parent 89f7f98fba
commit 8e77a5eebc
3 changed files with 39 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+37 -34
View File
@@ -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);
lfs.h
+1 -1
View File
@@ -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
tests/test_badblocks.toml
+1 -2
View File
@@ -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'