In D42825#976834, @imp wrote:

I'd have liked to see these configurable thresholds for people that want to more or less aggressively fault things.
But as is, it's fine, and it's a nice to have not something absolutely required.

ping @jrtc27 does the latest version look better to you?

LGTM. But under what circumstances can mp be NULL? We should probably add a regression test for that.

• style fixup to "libc/libc/rpc: refactor some global variables"
• fixup to "rpc: Replace per-fd condvars with mutexes in clnt_{vc,dg}.c"

• style change to dg_fd_find and vc_fd_find
• rpc: Replace per-fd condvars with mutexes in clnt_{vc,dg}.c

• Respond to kib's comments.

This latest version fixes the script with UFS, and makes the bootconfig script less ZFS-specific.

• Add ESP to all ZFS boot disks, pretty edition

• Restore the original behavior in vfs_lookup

Approved, but please note in the commit message that this condition can only occur due to FPU rounding error.

In D42081#960676, @markj wrote:

Sorry, but I still don't like it. :)

fuse_vnop_readlink() should be sanitizing its inputs to the VFS rather than expecting the VFS to handle this situation in some arbitrary way. With this patch, we are now hiding what could be bugs in all VOP_READLINK implementations. If it were ZFS returning corrupted symlink targets, then I'd want the system to panic (at least on INVARIANTS systems). If this situation can legitimately arise in FUSE filesystems, then fuse_vnop_readlink() needs to decide how to handle it; in this case it seems like you want to simply return the string up to the first nul terminator.

In D42081#960674, @kib wrote:

BTW is there a check in fusefs that lookup never returns vp == dvp (of non-dot cnp, but this should never reach VOP)? IMO it is even more fundamental invariant.

@markj I've moved the check into namei_follow_link . Do you like this version better? I could also have it print some kind of warning to dmesg, subject to INVARIANTS, if you would prefer. Or I could remove it entirely and restore the panic, if you prefer. The problem with doing the check in namei_follow_link is that it causes an extra scan through the string, rather than piggy backing on the scan that already takes place in vfs_lookup.

If we were computing with real numbers, then it should be impossible for vari to ever be negative. And in fact, all of the failing tests use -t 1, which means that n will be 1, so vari should always be exactly zero. So the fact that you're seeing these errors must mean that we're hitting some kind of floating point round-off error. Your patch will work, but it begs the question: why print min/avg/max/stddev when there is only one datapoint? I think that we should only print those things when n > 1. In fact, the standard deviation isn't really meaningful when n == 2, but it probably wouldn't hurt to print it, if that makes the code more readable.

• Move the nul check into namei_follow_link

In D42081#960578, @markj wrote:

In D42081#960550, @asomers wrote:

You could add something to namei_follow_link(), where VOP_READLINK is actually called during name resolution. But I also want to ask, why is not sufficient to modify fuse alone? How would other filesystems allow this situation to occur, barring corruption of on-disk data?

Exactly, corruption of on-disk data. From inspection, I think that ext2fs is vulnerable. I haven't checked others. But I think we should be resilient against corrupt disks.

But your approach to resiliency should depend on the filesystem. FUSE explicitly needs to guard against this particular possibility, so it should check for nul bytes in symlink targets. OpenZFS probably does not, assuming that it always writes valid symlinks to disk and can catch corruption using checksums. UFS and ext2 provide no mechanism to detect data corruption, so we cannot be resilient to corruption in any general sense. Certainly they could explicitly check for this case and panic, but then we should presumably also make sure that directory entries don't contain nul bytes (ext2_readdir() and ufs_readdir() don't appear to verify this), and the rabbit hole doesn't end there.

You could add something to namei_follow_link(), where VOP_READLINK is actually called during name resolution. But I also want to ask, why is not sufficient to modify fuse alone? How would other filesystems allow this situation to occur, barring corruption of on-disk data?

In D42081#960121, @markj wrote:

Why do other filesystems need to be modified if we check the invariant in a vop_readlink_post()?

In D42081#959985, @mjg wrote:

i don't think the patch is legitimate in that length indicated by VOP_READLINK does not line up with the real length -- it is that discrepancy which needs to be avoided instead. i would make it an invariant that VOP_READLINK returns correct size or an error like in the case above. then a debug_pre func for VOP_READLINK could validate it. i also note the proposed patch pessimizes all lookups (not only those which encounter a symlink) by adding a func call and no longer avoiding an extra branch per character (currently achieved by slapping / at the end)

Sorry, @mjg . Good luck with your next project.

• Restrict the new fusefs warning to INVARIANTS

Let me see if I understand this dlopen business correctly. Your plan is that if you create a separate .c file containing "test_strcmp" and link it to t_strcmp.o, then the test will operate on that function. Otherwise, it will operate on the standard strcmp?

In its current state, the "base" test doesn't pass. It seems like the expected_value variable isn't getting correctly expanded. Its value is just the name of the variable. Could you please fix that?

Ok, I've commited to stable/14 now.

In D41894#955899, @cc wrote:

I am learning Rust if you meant the Rust programming language. Would you please elaborate why is Rust involved here?

If the caller provides insufficient space for the entire structure, will the kernel fail the ioctl or copy as much as it can? It looks to me like it will do the latter. That also provides backwards compatibility, as long as we don't move or delete fields. @tuexen do you accept @glebius's suggestion to directly commit to stable/14? I would like to do that because it improves the Rust story. The Rust community still has not accepted that some APIs change between versions, like 64-bit inodes. Pushing the tcp_info compatibility break to FreeBSD 15 instead of 14 will give Rust some more time to come to terms.

Looks like this review got away from me. Oops. I'll rebase, test, and hopefully commit today.

Note that I have no way to test this change.