Page MenuHomeFreeBSD

vfs: Simplify and harden vn_dir_next_dirent()
ClosedPublic

Authored by olce on Apr 22 2023, 9:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 29, 4:13 PM
Unknown Object (File)
Oct 8 2024, 5:06 AM
Unknown Object (File)
Oct 5 2024, 11:50 PM
Unknown Object (File)
Sep 26 2024, 2:46 PM
Unknown Object (File)
Sep 23 2024, 11:07 PM
Unknown Object (File)
Sep 23 2024, 9:51 PM
Unknown Object (File)
Sep 23 2024, 7:20 PM
Unknown Object (File)
Sep 23 2024, 1:19 AM

Details

Summary

Simplify the old interface (one less argument, simpler termination test) and
add documentation about it. Add more sanity checks (mostly under INVARIANTS,
but also in the general case to prevent infinite loops). Drop the explicit test
on minimum directory entry size (without INVARIANTS).

Deal with the impacts in callers (dirent_exists() and vop_stdvptocnp()).
dirent_exists() has been simplified a bit but preserves the exact same
semantics, even if the return code has been reversed (0 now means the entry
exists, ENOENT that it doesn't and other values are genuine errors).

Export new _GENERIC_MINDIRSIZ and _GENERIC_MAXDIRSIZ on __BSD_VISIBLE, and
GENERIC_MINDIRSIZ and GENERIC_MAXDIRSIZ on _KERNEL.

Test Plan

vn_dir_next_dirent() has been tested by a 'make -j4 buildkernel' with a
temporary modification to the VFS cache causing vn_vptocnp() to always call
VOP_VPTOCNP() and finally vop_stdvptocnp() (observed with temporary debug
counters).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

olce requested review of this revision.Apr 22 2023, 9:28 PM
olce created this revision.
olce retitled this revision from vfs: Fix "emptydir" mount option to vfs: Simplify and harden vn_dir_next_dirent().
olce edited the summary of this revision. (Show Details)
olce edited the test plan for this revision. (Show Details)

Now just contains the changes to vn_dir_next_dirent().

sys/kern/vfs_default.c
294

This is bug. VOP_ISLOCKED() return value is not boolean. Please change this to ASSERT_VOP_LOCKED() while there.

304

The cast is redundant.

752–754
790

Is this line still needed?

sys/kern/vfs_vnops.c
3740–3746

See my note about multi-line comment in another review.

3791–3825

This should be ASSERT_VOP_LOCKED() as well.

3793

I think that pointer arithmetic overflow is UB, so practically this assert does not check for anything, with modern compilers. I do not believe that this situation (overflow) can actually occur, but if you want to assert it, perhaps you need to cast dirbuf to uintptr_t.

3808

Better to add {} around this branch as well.

3876

I wonder if these asserts need to be proper runtime checks to provide some resiliency against misbehaving fses or even damaged storage. Old reclen < DIRECT_MINSIZE was the runtime check for this reason, I believe.

olce marked 9 inline comments as done.

Comments addressed.

Hardened the function a bit more by ensuring that '*dpp' is reset to NULL in
case of error or at end of directory.

sys/kern/vfs_default.c
790

Yes (see the break after if (len==0)).

I refrained from simplifying this function more to minimize the diff and because I intend to change it more substantially in another commit.

sys/kern/vfs_vnops.c
3793

I had initially put (uintptr_t) casts, but removed them because I thought they were not necessary. But indeed, I have the same reading of the standard, so I've put them back.

3876

I was wondering the same. That's why there was a leftover mentioning EINTEGRITY in the documentation.

I agree that the prudent approach is to leave them enable in production, returning EINVAL and logging the error.

But then there are other callers of VOP_READDIR(), in particular kern_getdirentries(), which would benefit from this kind of protection (for this one, there is no risk of kernel corruption, but ultimately the kernel would return an invalid result to userland). I think ultimately it would be great that we ensure that FSes always produce valid results. They are the best positioned to figure out what to do on integrity errors, and they must go over every directory entry they finally put in VOP_READDIR()'s result buffer. I'll think about doing something like that.

olce marked an inline comment as not done.Apr 25 2023, 10:07 AM
olce marked an inline comment as not done.
sys/kern/vfs_default.c
790

Removing the line and changing 'break' into assigning to error and goto out is needed, otherwise having this special case is simply confusing.

sys/kern/vfs_vnops.c
3875–3876
3876

No redundant braces according to style(9).

olce marked 3 inline comments as done.Apr 26 2023, 1:40 PM
kib added inline comments.
sys/kern/vfs_default.c
739

Perhaps remove this cast as well.

This revision is now accepted and ready to land.Apr 27 2023, 1:33 AM
This revision was automatically updated to reflect the committed changes.