Skip to content

Commit

Permalink
Fixed synthetic move underflows in lfs_dir_get
Browse files Browse the repository at this point in the history
By "luck" the previous code somehow managed to not be broken, though it
was possible to traverse the same file twice in lfs_fs_traverse/size
(which is not an error).

The problem was an underlying assumption in lfs_dir_get that it would
never be called when the requested id is pending removal because of a
powerloss. The assumption was either:

1. lfs_dir_find would need to be called first to find the id, and it
   would correctly toss out pending-rms with LFS_ERR_NOENT.

2. lfs_fs_mkconsistent would be implicitly called before any filesystem
   traversals, cleaning up any pending-rms. This is at least true for
   allocator scans.

But, as noted by andriyndev, both lfs_fs_traverse and lfs_fs_size can
call lfs_fs_get with a pending-rm id if called in a readonly context.

---

By "luck" this somehow manages to not break anything:

1. If the pending-rm id is >0, the id is decremented by 1 in lfs_fs_get,
   returning the previous file entry during traversal. Worst case, this
   reports any blocks owned by the previous file entry twice.

   Note this is not an error, lfs_fs_traverse/size may return the same
   block multiple times due to underlying copy-on-write structures.

2. More concerning, if the pending-rm id is 0, the id is decremented by
   1 in lfs_fs_get and underflows. This underflow propagates into the
   type field of the tag we are searching for, decrementing it from
   0x200 (LFS_TYPE_STRUCT) to 0x1ff (LFS_TYPE_INTERNAL(UNUSED)).

   Fortunately, since this happens to underflow to the INTERNAL tag
   type, the type intended to never exist on disk, we should never find
   a matching tag during our lfs_fs_get search. The result? lfs_dir_get
   returns LFS_ERR_NOENT, which is actually what we want.

Also note that LFS_ERR_NOENT does not terminate the mdir traversal
early. If it did we would have missed files instead of duplicating
files, which is a slightly worse situation.

---

The fix is to add an explicit check for pending-rms in lfs_dir_get, just
like in lfs_dir_find. This avoids relying on unintended underflow
propagation, and should make the internal API behavior more consistent.

This is especially important for potential future gc extensions.

Found by andriyndev
  • Loading branch information
geky committed Feb 4, 2024
1 parent f53a0cc commit ddbfcaa
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions lfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -710,11 +710,14 @@ static lfs_stag_t lfs_dir_getslice(lfs_t *lfs, const lfs_mdir_t *dir,
lfs_tag_t ntag = dir->etag;
lfs_stag_t gdiff = 0;

// synthetic moves
if (lfs_gstate_hasmovehere(&lfs->gdisk, dir->pair) &&
lfs_tag_id(gmask) != 0 &&
lfs_tag_id(lfs->gdisk.tag) <= lfs_tag_id(gtag)) {
// synthetic moves
gdiff -= LFS_MKTAG(0, 1, 0);
lfs_tag_id(gmask) != 0) {
if (lfs_tag_id(lfs->gdisk.tag) == lfs_tag_id(gtag)) {
return LFS_ERR_NOENT;
} else if (lfs_tag_id(lfs->gdisk.tag) < lfs_tag_id(gtag)) {
gdiff -= LFS_MKTAG(0, 1, 0);
}
}

// iterate over dir block backwards (for faster lookups)
Expand Down

0 comments on commit ddbfcaa

Please sign in to comment.