CRC8 in FCB

[JPHutchins]

I'm concerned about the use of CRC8 CCITT-8 in FCB records.

zephyr/subsys/fs/fcb/fcb_elem_info.c

Lines 15 to 67 in e646b7f

	/*
	* Given offset in flash sector, fill in rest of the fcb_entry, and crc8 over
	* the data.
	*/
	static int
	fcb_elem_crc8(struct fcb *_fcb, struct fcb_entry *loc, uint8_t *c8p)
	{
	uint8_t tmp_str[FCB_TMP_BUF_SZ];
	int cnt;
	int blk_sz;
	uint8_t crc8;
	uint16_t len;
	uint32_t off;
	uint32_t end;
	int rc;
	if (loc->fe_elem_off + 2 > loc->fe_sector->fs_size) {
	return -ENOTSUP;
	}
	rc = fcb_flash_read(_fcb, loc->fe_sector, loc->fe_elem_off, tmp_str, 2);
	if (rc) {
	return -EIO;
	}
	cnt = fcb_get_len(_fcb, tmp_str, &len);
	if (cnt < 0) {
	return cnt;
	}
	loc->fe_data_off = loc->fe_elem_off + fcb_len_in_flash(_fcb, cnt);
	loc->fe_data_len = len;
	crc8 = CRC8_CCITT_INITIAL_VALUE;
	crc8 = crc8_ccitt(crc8, tmp_str, cnt);
	off = loc->fe_data_off;
	end = loc->fe_data_off + len;
	for (; off < end; off += blk_sz) {
	blk_sz = end - off;
	if (blk_sz > sizeof(tmp_str)) {
	blk_sz = sizeof(tmp_str);
	}
	rc = fcb_flash_read(_fcb, loc->fe_sector, off, tmp_str, blk_sz);
	if (rc) {
	return -EIO;
	}
	crc8 = crc8_ccitt(crc8, tmp_str, blk_sz);
	}
	*c8p = crc8;
	return 0;
	}

CCITT-8 has Hamming Distance = 3 up to 247 bits. That's HD=3 for only ~30 bytes of data.

https://users.ece.cmu.edu/~koopman/crc/crc8.html

I'd propose that the addition of this CRC is optional and off by default. Use of a CRC8 provides a false sense of integrity for entries of any significant length.

Additionally, I am concerned about the overhead of this implementation:

	for (; off < end; off += blk_sz) {
		blk_sz = end - off;
		if (blk_sz > sizeof(tmp_str)) {
			blk_sz = sizeof(tmp_str);
		}

		rc = fcb_flash_read(_fcb, loc->fe_sector, off, tmp_str, blk_sz);
		if (rc) {
			return -EIO;
		}
		crc8 = crc8_ccitt(crc8, tmp_str, blk_sz);
	}

The operation reads 32 bytes at a time from (possibly slow, external, pm runtime managed) flash to the function call stack (the tmp_str). I am suggesting that this is unnecessary in all use cases of the API. The reason is that this CRC is being calculated at write time or at read time.

• At write time, the user has already provided a buffer of the data to be written, in RAM, there is no reason to read back from flash. In fact, if I am reading this correctly, the CRC that's written is calculated AFTER the data has been written to flash, meaning that it will not detect any write errors, instead only read errors!
• At read time, the user has already provided a buffer for the data to be read into.

TBH I'm not sure I even understand the intention of this CRC8.

Removing the CRC would alleviate these concerns and allow the user to add the CRC that is correct for their data. In my present use case, the records are 1624 bytes (though I'll probably shrunk them to align with sectors better - truly, I think that FCB should write records across sector boundaries!) I am already adding a reasonable CRC32 for the data - one that has good HD for the number of bits I intend to check.

FCB seems to be a promising API for a "flash circular buffer". Perhaps I should abandon it for NVS or other, but at first glance it seems an elegant solution for "storing N latest records".

[JPHutchins]

OK, looks like this is already possible:

#if IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER)
/* Given the offset in flash sector, calculate the FCB entry data offset and size, and set
 * the fixed endmarker.
 */
static int
fcb_elem_endmarker_fixed(struct fcb *_fcb, struct fcb_entry *loc, uint8_t *em)
{
	uint8_t tmp_str[2];
	int cnt;
	uint16_t len;
	int rc;

	if (loc->fe_elem_off + 2 > loc->fe_sector->fs_size) {
		return -ENOTSUP;
	}

	rc = fcb_flash_read(_fcb, loc->fe_sector, loc->fe_elem_off, tmp_str, 2);
	if (rc) {
		return -EIO;
	}

	cnt = fcb_get_len(_fcb, tmp_str, &len);
	if (cnt < 0) {
		return cnt;
	}
	loc->fe_data_off = loc->fe_elem_off + fcb_len_in_flash(_fcb, cnt);
	loc->fe_data_len = len;

	*em = FCB_FIXED_ENDMARKER;
	return 0;
}
#endif /* IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER) */

/* Given the offset in flash sector, calculate the FCB entry data offset and size, and calculate
 * the expected endmarker.
 */
int
fcb_elem_endmarker(struct fcb *_fcb, struct fcb_entry *loc, uint8_t *em)
{
#if IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER)
	if (_fcb->f_flags & FCB_FLAGS_CRC_DISABLED) {
		return fcb_elem_endmarker_fixed(_fcb, loc, em);
	}
#endif /* IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER) */

	return fcb_elem_crc8(_fcb, loc, em);
}

But I still think that the CRC8 usage and implementation should be investigated.

[JPHutchins]

Also needed to set in the config:

static struct fcb fcb = {
    .f_magic = FCB_MAGIC,
    .f_version = ADC_DATA_FCB_VERSION_0,
    .f_sector_cnt = ARRAY_SIZE(sectors),
    .f_sectors = sectors,
    .f_flags = FCB_FLAGS_CRC_DISABLED,
};

The reads are WAY faster cause it's not doing 32 byte at a time CRC over a serial bus anymore.

[JPHutchins]

Summary:

• Why wouldn't the CRC be calculated from RAM before the write?
• Why wouldn't the CRC be calculated from RAM after the read?

[Laczen]

Because the crc is only written when the item write is finished (a write of an item can be split-up in steps). The same is valid for the read, before a read is performed fcb will verify the crc8, after that the read can be split-up. In both cases there will never be a ram buffer that can be used to validate the crc.

[JPHutchins]

Because the crc is only written when the item write is finished (a write of an item can be split-up in steps). The same is valid for the read, before a read is performed fcb will verify the crc8, after that the read can be split-up. In both cases there will never be a ram buffer that can be used to validate the crc.

Yes, but just like the flash can be written incrementally, the CRC can be calculated incrementally, from the RAM buffer.

[Laczen]

True, but this would need storage of the crc in the entry and a dedicated fcb_flash_area_write() to update the crc. Not that it can't be done, but the fcb developers (the fcb origin is in mynewt) have chosen not to do that and to calculate the crc from the flash data on closing the entry.

[Laczen]

As an additional remark: FCB allows rewriting data to flash (if supported by the device), using this feature would make it impossible to calculate the crc directly from the writes performed.

[JPHutchins]

I see that the FCB API has no way of incrementally calculating CRC8 because it simply exposes some chunk of address to write to. It can't know what data has been written other than by reading back.

[JPHutchins]

If I understand correctly, the CRC8 is calculated

• after the data has been transported over a serial interface (I2C, SPI, etc.)
• and after the transported data has been written to flash

If the above is correct, then the CRC8 is not valid to detect errors in:

• the transmission of the data
• the writing of the data

Seeking feedback.

[Laczen]

Correct, the crc8 in FCB is not used to detect errors, it is more like a "finished writing" indication. The reason for a crc8 selection is not documented, but from the 1-byte size it can be concluded that it is not meant to be used for data integrity.

[JPHutchins]

Agreed. The CRC8 is used in fcb_elem_info() which is in turn used by fcb_getnext_in_sector() to verify that an FCB record is valid. And we agree that there is no difference between using CRC8 and the fixed marker for determining "validity". I will move on to PR recommending that the fixed marker is default, or at least update the docstrings indicating why the CRC8 should not be used.

/* Given the offset in flash sector, calculate the FCB entry data offset and size, and verify that
 * the FCB entry endmarker is correct.
 */
int fcb_elem_info(struct fcb *_fcb, struct fcb_entry *loc)
{
	int rc;
	uint8_t em;
	uint8_t fl_em;
	off_t off;

	rc = fcb_elem_endmarker(_fcb, loc, &em);
	if (rc) {
		return rc;
	}
	off = loc->fe_data_off + fcb_len_in_flash(_fcb, loc->fe_data_len);

	rc = fcb_flash_read(_fcb, loc->fe_sector, off, &fl_em, sizeof(fl_em));
	if (rc) {
		return -EIO;
	}

	if (IS_ENABLED(CONFIG_FCB_ALLOW_FIXED_ENDMARKER) && (fl_em != em)) {
		rc = fcb_elem_crc8(_fcb, loc, &em);
		if (rc) {
			return rc;
		}
	}

	if (fl_em != em) {
		return -EBADMSG;
	}
	return 0;
}

int
fcb_getnext_in_sector(struct fcb *fcb, struct fcb_entry *loc)
{
	int rc;

	rc = fcb_elem_info(fcb, loc);
	if (rc == 0 || rc == -EBADMSG) {
		do {
			loc->fe_elem_off = loc->fe_data_off +
			  fcb_len_in_flash(fcb, loc->fe_data_len) +
			  fcb_len_in_flash(fcb, FCB_CRC_SZ);
			rc = fcb_elem_info(fcb, loc);
			if (rc != -EBADMSG) {
				break;
			}
		} while (rc == -EBADMSG);
	}
	return rc;
}