Skip to content

fs/cromfs: Fix stale cache read in read() fast path - #19811

Merged
acassis merged 1 commit into
apache:masterfrom
alexcekay:fs_cromfs_cache_fix
Aug 12, 2026
Merged

fs/cromfs: Fix stale cache read in read() fast path#19811
acassis merged 1 commit into
apache:masterfrom
alexcekay:fs_cromfs_cache_fix

Conversation

@alexcekay

Copy link
Copy Markdown
Contributor

Summary

cromfs_read()'s fast path decompresses a block directly into the caller's buffer whenever a read reaches a block at its start and the caller has room for the whole decompressed block, bypassing the per-file decompression cache (ff_buffer). It nonetheless marked that block as cached by setting ff_offset, without ever writing ff_buffer itself.

A later read of the same block that fell onto the slow path trusted that false cache tag, skipped decompression, and copied from ff_buffer without it ever having been populated for that block. A repeated identical fast-path read of the same block hit the same false tag and skipped decompression entirely, leaving the caller's buffer untouched and returning whatever was already there.

Fixed by having the fast path only read the cache, never populate it: reuse ff_buffer when a prior slow-path read already cached the same block, otherwise decompress straight into the caller's buffer without touching ff_offset/ff_buffer.

Detection

  • We detected this in PX4 mavftp server which serves files from the CROMFS. It was only detected now as the radio link was quite lossy, which caused the ground station to request re-reads for the same file offset a second time due to loss on the link.
  • This caused a re-read on thus an access to the "poisoned" cache, which in-turn lead to the read not changing the user buffer at all, which caused the previous data delivered via mavftp to be delivered again.

Impact

  • This does not have a functional impact on existing users, CROMFS behavior is fixed with this change.
  • The fast-path cache will not be used anymore which may have a performance impact, but as this caching was not safe it's the better trade-off. As it was not reported yet I assume the cache path was never hit by other users.

Testing

  • Tested in the context of PX4 using a STM32H743VIH6
  • Test setup is a generated ROMFS (https://gist.github.com/alexcekay/03a240112dbdaf44e4ad374ffa12142b) which contains 2 compressed blocks (512 byte uncompressed each), 2 non-compressed blocks (512 byte uncompressed each) and one tail block (77 byte uncompresssed)
  • Test is a PX4 systemcmd that was executed (https://gist.github.com/alexcekay/404f42702edf5535bb563d24e49f2e0c).
  • Description of individual tests:
    • file_size: Use stat to check that CROMFS reports correct file size
    • full_read: Read the whole CROMFS file into one buffer and check that it contains the expected data
    • chunked_read: Read file with different chunk sizes and check the chunks contain the expected data
    • seek_and_reread: Seek to various offsets and do a chunked read. Check that chunks contain the expected data
    • regression_double_visit: Read using the fast path, afterwards using the slow path and afterwards using the fast path again. Checks that the fast path correctly uses the cache set by the slow path
    • regression_repeated_fast_path: Read the same block twice using the fast path. Ensure that this does not cause cache problems
    • eof_handling: Read and seek near EOF

Test result without fix:

nsh> cromfs_test
  [file_size] 		PASS
  [full_read] 		PASS
  [chunked_read] 		PASS
ERROR [cromfs_test] cromfs: mismatch at file offset 2560: got 0x61 expected 0x6f
ERROR [cromfs_test] cromfs: mismatch after seek to 2560
  [seek_and_reread] 		FAIL
ERROR [cromfs_test] cromfs: mismatch at file offset 2560: got 0xec expected 0x6f
  [regression_double_visit] 		FAIL
ERROR [cromfs_test] cromfs: mismatch at file offset 2560: got 0xa5 expected 0x6f
ERROR [cromfs_test] cromfs: REGRESSION - repeated fast-path read of the same block returned stale/unwritten data instead of de
  [regression_repeated_fast_path] 		FAIL
  [eof_handling] 		PASS
ERROR [cromfs_test] cromfs_test FAILED

Test result with fix:

nsh> cromfs_test
  [file_size] 		PASS
  [full_read] 		PASS
  [chunked_read] 		PASS
  [seek_and_reread] 		PASS
  [regression_double_visit] 		PASS
  [regression_repeated_fast_path] 		PASS
  [eof_handling] 		PASS
INFO  [cromfs_test] cromfs_test PASSED

cromfs_read()'s fast path decompresses a block directly into the
caller's buffer whenever a read reaches a block at its start and the
caller has room for the whole decompressed block, bypassing the
per-file decompression cache (ff_buffer). It nonetheless marked that
block as cached by setting ff_offset, without ever writing ff_buffer
itself.

A later read of the same block that fell onto the slow path trusted
that false cache tag, skipped decompression, and copied from
ff_buffer without it ever having been populated for that block. A
repeated identical fast-path read of the same block hit the same
false tag and skipped decompression entirely, leaving the caller's
buffer untouched and returning whatever was already there.

Fixed by having the fast path only read the cache, never populate it:
reuse ff_buffer when a prior slow-path read already cached the same
block, otherwise decompress straight into the caller's buffer without
touching ff_offset/ff_buffer.

Co-authored-by: Pavlo
Assisted-by: Claude Code:claude-sonnet-5
Signed-off-by: alexcekay <alexander@auterion.com>
@github-actions github-actions Bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Aug 12, 2026
@github-actions

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

@alexcekay alexcekay changed the title fs/cromfs: Fix stale cache read in read() fast path. fs/cromfs: Fix stale cache read in read() fast path Aug 12, 2026
@acassis

acassis commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@alexcekay could you please add this cromfs_test to apps/testing/ ?

@acassis
acassis merged commit a450392 into apache:master Aug 12, 2026
52 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: File System File System issues Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants