vfs: Simplify and harden vn_dir_next_dirent()

[olce]

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).

[olce]

Now just contains the changes to vn_dir_next_dirent().

[kib]

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]

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.

[olce]

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.

[kib]

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]

Comments addressed.

[kib]

sys/kern/vfs_default.c

739

Perhaps remove this cast as well.