Planning to commit the minimal fix at D53767, is that OK for you?

Hadn't noticed this new revision (Phabricator mail sending seems to be delayed), so reposting the relevant part of the comment I added on D47878 in the meantime:

In D47878#1227393, @jhb wrote:

The memcpy size is wrong.

In D47878#1226923, @jhb wrote:

Actually, I found a bug in the compat32 setcred() as well. (...) I will work on some patches though I'll try to post for review today/tomorrow.

In D47878#1226543, @jhb wrote:

Blech, this conflicted quite a bit with CheriBSD downstream which has COMPAT_FREEBSD64 and used SV_CURPROC_FLAG to determine the ABI instead of a bool (that needs to not be a bool). At some point I will probably just have to upstream reworking this to remove all the bool stuff as you don't need it since you can just check the ABI here and do the fixup as needed. In CheriBSD we also fix the copying in of struct mac for exec which also needs to honor the ABI.

Seems fine. See also inline comment for a change that is superfluous.

In D53640#1224643, @jah wrote:

Do we have automated tests for rm(1) somewhere that I'll need to change?

In D53563#1223560, @kib wrote:

This is still a bad code. It allows two threads to race to cause rctl idea of the proc ucred to be different from the actual curproc->p_ucred.

Sorry, I'm running out of time and this (and also next revision, D53457) needs to land in 15.0, so please urgently tell me if you think the current version is fine functionality-wise. Unless it is not, I will commit it as is tomorrow, and can then amend it with more time later, as I agree with your comments and have prepared an update which unfortunately I haven't had much time to test. I also fear that modifying this relatively delicate code close to committing it could introduce some unseen problem, that's why I'd really prefer to avoid that.

On RCTL, guard the call to rctl_proc_ucred_changed() with cred_set instead of relying on error being 0 as this is more error-prone on potential future changes to setcred().

• Ensure the last block is executed only on no error.

Wow, sorry, I forgot to wrap the last block (doing rctl_proc_ucred_changed()) inside a if (error != 0).

Ping?

(With same remark as Alan.)

For the changes in unionfs_lock(), there is indeed a window after the interlock drop and before lock acquire and drop where the "root" reference unionfs holds (through unionfs_node; the one that allows to call vholdnz() above) could be dropped (it's the same window described in the big comment about checking whether the proper target vnode (lower or upper) was locked).

This change is an improvement without drawbacks, so should be committed.

We have indeed several problems with VOP_OPEN()/VOP_CLOSE(), e.g., they are not always called in pairs, and even if they are the passed td may not match between an open and close. These interfaces need serious revision, in particular determining which exact info the filesystems would like to use and if there are better ways to obtain them.

Please don't forget to exclude the changes in sys/netinet/ip_carp.c as they are completely unrelated.

In D52960#1212126, @rmacklem wrote:

Looks ok to me. I'll leave min vs MIN up to you.