Fixed single unchecked bit during commit verification

This bug was exposed by the bad-block tests due to changes to block
allocation, but could have been hit before these changes.

In flash, when blocks fail, they don't fail in a predictable manner. To
account for this, the bad-block tests check a number of failure
behaviors. The interesting one here is "LFS_TESTBD_BADBLOCK_ERASENOOP",
in which bad blocks can not be erased or programmed, and are stuck with
the data written at the time the blocks go bad.

This is actually a pretty realistic failure behavior, since flash needs a
large voltage to force the electrons of the floating gates. Though
realistically, such a failure would like corrupt the data a bit, not leave the
underlying data perfectly intact.

LFS_TESTBD_BADBLOCK_ERASENOOP is rather interesting to test for because it
means bad blocks can end up with perfectly valid CRCs after a failed write,
confusing littlefs.

---

In this case, we had the perfect series of operations such that a test
was repeatedly writing the same sequence of metadata commits to the same
block, which eventually goes bad, leaving the block stuck with metadata
that occurs later in the sequence.

What this means is that after the first commit, the metadata block
contained both the first and second commits, even though the loop in the
test hadn't reached that point yet.

expected       actual
.----------.  .----------.
| commit 1 |  | commit 1 |
| crc 1    |  | crc 1    |
|          |  | commit 2 <-- (from previous iteration)
|          |  | crc 2    |
'----------'  '----------'

To protect against this, littlefs normally compares the written CRC
against the expected CRC, but because this was the exact same data that
it was going to write, this CRCs end up the same.

Ah! But doesn't littlefs also encode the state of the next page to keep
track of if the next page has been erased or not? Wouldn't that change
between iterations?

It does! In a single bit in the CRC-tag. But thanks to some incorrect
logic attempting to avoid an extra condition in the loop for writing out
padding commits, the CRC that littlefs checked against was the CRC
immediately before we include the "is-next-page-erased" bit.

Changing the verification check to use the same CRC as what is used to
verify commits on fetch solves this problem.

Makefile

@@ -26,8 +26,6 @@ endif
 override CFLAGS += -I.
 override CFLAGS += -std=c99 -Wall -pedantic
 override CFLAGS += -Wextra -Wshadow -Wjump-misses-init -Wundef
-# Remove missing-field-initializers because of GCC bug
-override CFLAGS += -Wno-missing-field-initializers
 
 ifdef VERBOSE
 override TFLAGS += -v

lfs.c

@@ -452,14 +452,16 @@ static int lfs_alloc_lookahead(void *p, lfs_block_t block) {
     return 0;
 }
 
+// indicate allocated blocks have been committed into the filesystem, this
+// is to prevent blocks from being garbage collected in the middle of a
+// commit operation
 static void lfs_alloc_ack(lfs_t *lfs) {
     lfs->free.ack = lfs->cfg->block_count;
 }
 
-// Invalidate the lookahead buffer. This is done during mounting and
+// drop the lookahead buffer, this is done during mounting and failed
-// failed traversals
+// traversals in order to avoid invalid lookahead state
-static void lfs_alloc_reset(lfs_t *lfs) {
+static void lfs_alloc_drop(lfs_t *lfs) {
-    lfs->free.off = lfs->seed % lfs->cfg->block_size;
     lfs->free.size = 0;
     lfs->free.i = 0;
     lfs_alloc_ack(lfs);
@@ -505,7 +507,7 @@ static int lfs_alloc(lfs_t *lfs, lfs_block_t *block) {
         memset(lfs->free.buffer, 0, lfs->cfg->lookahead_size);
         int err = lfs_fs_traverseraw(lfs, lfs_alloc_lookahead, lfs, true);
         if (err) {
-            lfs_alloc_reset(lfs);
+            lfs_alloc_drop(lfs);
             return err;
         }
     }
@@ -870,8 +872,10 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                 ptag ^= (lfs_tag_t)(lfs_tag_chunk(tag) & 1U) << 31;
 
                 // toss our crc into the filesystem seed for
-                // pseudorandom numbers
+                // pseudorandom numbers, note we use another crc here
-                lfs->seed ^= crc;
+                // as a collection function because it is sufficiently
+                // random and convenient
+                lfs->seed = lfs_crc(lfs->seed, &crc, sizeof(crc));
 
                 // update with what's found so far
                 besttag = tempbesttag;
@@ -1261,12 +1265,13 @@ static int lfs_dir_commitattr(lfs_t *lfs, struct lfs_commit *commit,
 }
 
 static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
-    const lfs_off_t off1 = commit->off;
-    const uint32_t crc1 = commit->crc;
     // align to program units
-    const lfs_off_t end = lfs_alignup(off1 + 2*sizeof(uint32_t),
+    const lfs_off_t end = lfs_alignup(commit->off + 2*sizeof(uint32_t),
             lfs->cfg->prog_size);
 
+    lfs_off_t off1 = 0;
+    uint32_t crc1 = 0;
+
     // create crc tags to fill up remainder of commit, note that
     // padding is not crced, which lets fetches skip padding but
     // makes committing a bit more complicated
@@ -1302,6 +1307,12 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
             return err;
         }
 
+        // keep track of non-padding checksum to verify
+        if (off1 == 0) {
+            off1 = commit->off + sizeof(uint32_t);
+            crc1 = commit->crc;
+        }
+
         commit->off += sizeof(tag)+lfs_tag_size(tag);
         commit->ptag = tag ^ ((lfs_tag_t)reset << 31);
         commit->crc = 0xffffffff; // reset crc for next "commit"
@@ -1315,7 +1326,7 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
 
     // successful commit, check checksums to make sure
     lfs_off_t off = commit->begin;
-    lfs_off_t noff = off1 + sizeof(uint32_t);
+    lfs_off_t noff = off1;
     while (off < end) {
         uint32_t crc = 0xffffffff;
         for (lfs_off_t i = off; i < noff+sizeof(uint32_t); i++) {
@@ -2432,9 +2443,9 @@ int lfs_file_opencfg(lfs_t *lfs, lfs_file_t *file,
 
         // get next slot and create entry to remember name
         err = lfs_dir_commit(lfs, &file->m, LFS_MKATTRS(
-                {LFS_MKTAG(LFS_TYPE_CREATE, file->id, 0)},
+                {LFS_MKTAG(LFS_TYPE_CREATE, file->id, 0), NULL},
                 {LFS_MKTAG(LFS_TYPE_REG, file->id, nlen), path},
-                {LFS_MKTAG(LFS_TYPE_INLINESTRUCT, file->id, 0)}));
+                {LFS_MKTAG(LFS_TYPE_INLINESTRUCT, file->id, 0), NULL}));
         if (err) {
             err = LFS_ERR_NAMETOOLONG;
             goto cleanup;
@@ -3200,7 +3211,7 @@ int lfs_remove(lfs_t *lfs, const char *path) {
 
     // delete the entry
     err = lfs_dir_commit(lfs, &cwd, LFS_MKATTRS(
-            {LFS_MKTAG(LFS_TYPE_DELETE, lfs_tag_id(tag), 0)}));
+            {LFS_MKTAG(LFS_TYPE_DELETE, lfs_tag_id(tag), 0), NULL}));
     if (err) {
         lfs->mlist = dir.next;
         LFS_TRACE("lfs_remove -> %d", err);
@@ -3326,12 +3337,12 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
     // move over all attributes
     err = lfs_dir_commit(lfs, &newcwd, LFS_MKATTRS(
             {LFS_MKTAG_IF(prevtag != LFS_ERR_NOENT,
-                LFS_TYPE_DELETE, newid, 0)},
+                LFS_TYPE_DELETE, newid, 0), NULL},
-            {LFS_MKTAG(LFS_TYPE_CREATE, newid, 0)},
+            {LFS_MKTAG(LFS_TYPE_CREATE, newid, 0), NULL},
             {LFS_MKTAG(lfs_tag_type3(oldtag), newid, strlen(newpath)), newpath},
             {LFS_MKTAG(LFS_FROM_MOVE, newid, lfs_tag_id(oldtag)), &oldcwd},
             {LFS_MKTAG_IF(samepair,
-                LFS_TYPE_DELETE, newoldid, 0)}));
+                LFS_TYPE_DELETE, newoldid, 0), NULL}));
     if (err) {
         lfs->mlist = prevdir.next;
         LFS_TRACE("lfs_rename -> %d", err);
@@ -3344,7 +3355,7 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
         // prep gstate and delete move id
         lfs_fs_prepmove(lfs, 0x3ff, NULL);
         err = lfs_dir_commit(lfs, &oldcwd, LFS_MKATTRS(
-                {LFS_MKTAG(LFS_TYPE_DELETE, lfs_tag_id(oldtag), 0)}));
+                {LFS_MKTAG(LFS_TYPE_DELETE, lfs_tag_id(oldtag), 0), NULL}));
         if (err) {
             lfs->mlist = prevdir.next;
             LFS_TRACE("lfs_rename -> %d", err);
@@ -3636,7 +3647,7 @@ int lfs_format(lfs_t *lfs, const struct lfs_config *cfg) {
 
         lfs_superblock_tole32(&superblock);
         err = lfs_dir_commit(lfs, &root, LFS_MKATTRS(
-                {LFS_MKTAG(LFS_TYPE_CREATE, 0, 0)},
+                {LFS_MKTAG(LFS_TYPE_CREATE, 0, 0), NULL},
                 {LFS_MKTAG(LFS_TYPE_SUPERBLOCK, 0, 8), "littlefs"},
                 {LFS_MKTAG(LFS_TYPE_INLINESTRUCT, 0, sizeof(superblock)),
                     &superblock}));
@@ -3797,8 +3808,10 @@ int lfs_mount(lfs_t *lfs, const struct lfs_config *cfg) {
     lfs->gstate.tag += !lfs_tag_isvalid(lfs->gstate.tag);
     lfs->gdisk = lfs->gstate;
 
-    // setup free lookahead
+    // setup free lookahead, to distribute allocations uniformly across
-    lfs_alloc_reset(lfs);
+    // boots, we start the allocator at a random location
+    lfs->free.off = lfs->seed % lfs->cfg->block_count;
+    lfs_alloc_drop(lfs);
 
     LFS_TRACE("lfs_mount -> %d", 0);
     return 0;
@@ -4050,7 +4063,7 @@ static int lfs_fs_relocate(lfs_t *lfs,
         lfs_pair_tole32(newpair);
         int err = lfs_dir_commit(lfs, &parent, LFS_MKATTRS(
                 {LFS_MKTAG_IF(moveid != 0x3ff,
-                    LFS_TYPE_DELETE, moveid, 0)},
+                    LFS_TYPE_DELETE, moveid, 0), NULL},
                 {tag, newpair}));
         lfs_pair_fromle32(newpair);
         if (err) {
@@ -4084,7 +4097,7 @@ static int lfs_fs_relocate(lfs_t *lfs,
         lfs_pair_tole32(newpair);
         err = lfs_dir_commit(lfs, &parent, LFS_MKATTRS(
                 {LFS_MKTAG_IF(moveid != 0x3ff,
-                    LFS_TYPE_DELETE, moveid, 0)},
+                    LFS_TYPE_DELETE, moveid, 0), NULL},
                 {LFS_MKTAG(LFS_TYPE_TAIL + parent.split, 0x3ff, 8), newpair}));
         lfs_pair_fromle32(newpair);
         if (err) {
@@ -4132,7 +4145,7 @@ static int lfs_fs_demove(lfs_t *lfs) {
     uint16_t moveid = lfs_tag_id(lfs->gdisk.tag);
     lfs_fs_prepmove(lfs, 0x3ff, NULL);
     err = lfs_dir_commit(lfs, &movedir, LFS_MKATTRS(
-            {LFS_MKTAG(LFS_TYPE_DELETE, moveid, 0)}));
+            {LFS_MKTAG(LFS_TYPE_DELETE, moveid, 0), NULL}));
     if (err) {
         return err;
     }

scripts/readtree.py

@@ -106,7 +106,7 @@ def main(args):
             struct.unpack('<HH', superblock[1].data[0:4].ljust(4, b'\xff'))))
     print("%-47s%s" % ("littlefs v%s.%s" % version,
         "data (truncated, if it fits)"
-        if not any([args.no_truncate, args.tags, args.log, args.all]) else ""))
+        if not any([args.no_truncate, args.log, args.all]) else ""))
 
     # print gstate
     print("gstate 0x%s" % ''.join('%02x' % c for c in gstate))