mirror of
				https://github.com/eledio-devices/thirdparty-littlefs.git
				synced 2025-10-31 16:14:16 +01:00 
			
		
		
		
	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.
This commit is contained in:
		
							
								
								
									
										15
									
								
								lfs.c
									
									
									
									
									
								
							
							
						
						
									
										15
									
								
								lfs.c
									
									
									
									
									
								
							| @@ -1265,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) { | 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 |     // 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->cfg->prog_size); | ||||||
|  |  | ||||||
|  |     lfs_off_t off1 = 0; | ||||||
|  |     uint32_t crc1 = 0; | ||||||
|  |  | ||||||
|     // create crc tags to fill up remainder of commit, note that |     // create crc tags to fill up remainder of commit, note that | ||||||
|     // padding is not crced, which lets fetches skip padding but |     // padding is not crced, which lets fetches skip padding but | ||||||
|     // makes committing a bit more complicated |     // makes committing a bit more complicated | ||||||
| @@ -1306,6 +1307,12 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { | |||||||
|             return err; |             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->off += sizeof(tag)+lfs_tag_size(tag); | ||||||
|         commit->ptag = tag ^ ((lfs_tag_t)reset << 31); |         commit->ptag = tag ^ ((lfs_tag_t)reset << 31); | ||||||
|         commit->crc = 0xffffffff; // reset crc for next "commit" |         commit->crc = 0xffffffff; // reset crc for next "commit" | ||||||
| @@ -1319,7 +1326,7 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) { | |||||||
|  |  | ||||||
|     // successful commit, check checksums to make sure |     // successful commit, check checksums to make sure | ||||||
|     lfs_off_t off = commit->begin; |     lfs_off_t off = commit->begin; | ||||||
|     lfs_off_t noff = off1 + sizeof(uint32_t); |     lfs_off_t noff = off1; | ||||||
|     while (off < end) { |     while (off < end) { | ||||||
|         uint32_t crc = 0xffffffff; |         uint32_t crc = 0xffffffff; | ||||||
|         for (lfs_off_t i = off; i < noff+sizeof(uint32_t); i++) { |         for (lfs_off_t i = off; i < noff+sizeof(uint32_t); i++) { | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user