From ba09513e7d46775487a0d917640d9091b652f1f8 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 15 Aug 2024 11:54:24 -0500 Subject: [PATCH] Fixed mdir-relocate-pcache corruption, test_ck_spam_* bitflips Ckprogs does not suffer from rollback issues! I was too quick to assume this was the case in test_ck_spam_* (I blame ckfetches), but it just turned out that the more aggressive bit flip tests found an actual bug! The bug in question is caused by bit-errors being introduced in multiple blocks during mdir relocation. When relocating, we make the false assumption that if lfsr_mdir_compact__ returns success, the intermediary compaction has successfully been written to disk. But this is not true until we write the rest of the commit and flush the pcache. If the remaining commit fails due to a bit-error, the pcache can end up corrupt and the intermediary compaction lost. But why do we care about the intermediary compaction at all after corruption? Why do we keep updating the mdir every attempted relocation? We already mark all relevant mdirs as unerased (eoff=-1) in the top-level lfsr_mdir_commit, so as far as I can tell the only reason for updating the mdir on error is to propagate mdir.rbyd.weight=0 when the mdir is empty (LFS_ERR_NOENT). But this is a bit stupid. Relying on mdir state across function boundaries on error is incredibly fragile. If instead we consider the mdir clobbered on any error and move all the implicit mdir.rbyd.weight=0 stuff up into lfsr_mdir_commit, this whole category of problems goes away. So yeah, that's what we do now: - lfsr_mdir_commit__ failed => mdir clobbered - lfsr_mdir_compact__ failed => mdir clobbered - lfsr_mdir_commit_ failed => mdir preserved, marked unerased - lfsr_mdir_commit failed => mdir preserved, marked unerased --- Curiously, all of these changes ended up with a net-zero cost: code stack before: 36432 2672 after: 36432 (+0.0%) 2672 (+0.0%) --- lfs.c | 18 +++++++++++------- tests/test_ck.toml | 20 +++++--------------- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/lfs.c b/lfs.c index ea66e228..2b431ed7 100644 --- a/lfs.c +++ b/lfs.c @@ -7743,9 +7743,8 @@ static int lfsr_mdir_commit__(lfs_t *lfs, lfsr_mdir_t *mdir, if (rbyd_.weight == 0 // unless we are an mroot && !(mdir->mid == -1 || lfsr_mdir_cmp(mdir, &lfs->mroot) == 0)) { - // mark weight as zero, but note! we can no longer read from this mdir - // as our pcache may be clobbered - mdir->rbyd.weight = 0; + // note we can no longer read from this mdir as our pcache may + // be clobbered return LFS_ERR_NOENT; } @@ -8143,14 +8142,11 @@ relocate:; return err; } - // update mdir, we need to propagate mdir changes if commit fails - *mdir = mdir_; - // now try to commit again // // upper layers should make sure this can't fail by limiting the // maximum commit size - err = lfsr_mdir_commit__(lfs, mdir, start_rid, end_rid, + err = lfsr_mdir_commit__(lfs, &mdir_, start_rid, end_rid, mid, attrs, attr_count); if (err) { LFS_ASSERT(err != LFS_ERR_RANGE); @@ -8161,6 +8157,8 @@ relocate:; return err; } + // update mdir + *mdir = mdir_; return 0; } @@ -8349,6 +8347,10 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, } goto failed; } + // empty? set weight to zero + if (err == LFS_ERR_NOENT) { + mdir_[i^left].rbyd.weight = 0; + } } // adjust our sibling's mid after committing attrs @@ -8467,6 +8469,8 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, "0x{%"PRIx32",%"PRIx32"}", mdir->mid >> lfs->mdir_bits, mdir->rbyd.blocks[0], mdir->rbyd.blocks[1]); + // set weight to zero + mdir_[0].rbyd.weight = 0; // consume gstate so we don't lose any info err = lfsr_fs_consumegdelta(lfs, mdir); diff --git a/tests/test_ck.toml b/tests/test_ck.toml index 3d093be3..243e38da 100644 --- a/tests/test_ck.toml +++ b/tests/test_ck.toml @@ -1493,9 +1493,7 @@ code = ''' defines.METHOD = [0] defines.PERIOD = 10 defines.PROTECTED_MROOTANCHOR = [false, true] -# TODO rollback is even a problem for ckprogs, change this -# to PROGFLIP when rollback protection is in place -defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP' +defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP' defines.CKPROGS = 'METHOD == 0' defines.CKREADS = 'METHOD == 1' defines.CKFETCHES = 'METHOD == 2' @@ -1736,9 +1734,7 @@ corrupt_mounted:; defines.METHOD = [0] defines.PERIOD = 10 defines.PROTECTED_MROOTANCHOR = [false, true] -# TODO rollback is even a problem for ckprogs, change this -# to PROGFLIP when rollback protection is in place -defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP' +defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP' defines.CKPROGS = 'METHOD == 0' defines.CKREADS = 'METHOD == 1' defines.CKFETCHES = 'METHOD == 2' @@ -2051,9 +2047,7 @@ corrupt_mounted:; defines.METHOD = [0] defines.PERIOD = 10 defines.PROTECTED_MROOTANCHOR = [false, true] -# TODO rollback is even a problem for ckprogs, change this -# to PROGFLIP when rollback protection is in place -defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP' +defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP' defines.CKPROGS = 'METHOD == 0' defines.CKREADS = 'METHOD == 1' defines.CKFETCHES = 'METHOD == 2' @@ -2281,9 +2275,7 @@ defines.BADBLOCK = -1 defines.METHOD = [0] defines.PERIOD = 10 defines.PROTECTED_MROOTANCHOR = [false, true] -# TODO rollback is even a problem for ckprogs, change this -# to PROGFLIP when rollback protection is in place -defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP' +defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP' defines.CKPROGS = 'METHOD == 0' defines.CKREADS = 'METHOD == 1' defines.CKFETCHES = 'METHOD == 2' @@ -2717,9 +2709,7 @@ corrupt_mounted:; defines.METHOD = [0] defines.PERIOD = 10 defines.PROTECTED_MROOTANCHOR = [false, true] -# TODO rollback is even a problem for ckprogs, change this -# to PROGFLIP when rollback protection is in place -defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGNOOP' +defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP' defines.CKPROGS = 'METHOD == 0' defines.CKREADS = 'METHOD == 1' defines.CKFETCHES = 'METHOD == 2'