cred: 'struct ucred': Move 'cr_ngroups' out of the bcopied area

[olce]

Summary

crcopy() has always called crsetgroups() after copying fields between
'cr_startcopy' and 'cr_endcopy', which sets 'cr_ngroups' properly.
There is thus no need to copy the previous number of groups.

Consequently, move 'cr_ngroups' out of the copied area, and close to
'cr_groups' and 'cr_agroups', these fields being accessed together.
This creates two 32-bit holes, which have been placed in such way to
avoid splitting related fields. To avoid growing the structure's size,
one spare pointer has been removed to compensate.

With this change, pre-setting 'cr_ngroups' to avoid tripping over
crextend()'s assertion becomes unnecessary.

MFC after: Never

[mhorne]

It seems okay (correct). I am not convinced the ABI break is really warranted. This seems more like a blemish in the existing the code than a true problem that needs fixing.

[olce]

In D46916#1079291, @mhorne wrote:

It seems okay (correct). I am not convinced the ABI break is really warranted. This seems more like a blemish in the existing the code than a true problem that needs fixing.

Yeah, it's a code blemish. We could lift it by not changing the ABI at the price of complicating the generic field copying code, or simply live with it (i.e., have cr_ngroups assigned to twice, possibly elided by the compiler in the produced machine code).

However, is changing the ABI really a problem, as the changed header will trigger recompilation of outside kernel modules and applications (provided they have a decent toolchain)? We don't provide any guarantee that some module built for a given version of -CURRENT works on a later version, at least once __FreeBSD_version has been bumped, do we? Additionally, AFAIK, applications are not even supposed to use struct ucred structures directly but rather grab them through, e.g., sysctl() or some apps/libs in the base system.

[mhorne]

In D46916#1079351, @olce wrote:

In D46916#1079291, @mhorne wrote:

It seems okay (correct). I am not convinced the ABI break is really warranted. This seems more like a blemish in the existing the code than a true problem that needs fixing.

Yeah, it's a code blemish. We could lift it by not changing the ABI at the price of complicating the generic field copying code, or simply live with it (i.e., have cr_ngroups assigned to twice, possibly elided by the compiler in the produced machine code).

However, is changing the ABI really a problem, as the changed header will trigger recompilation of outside kernel modules and applications (provided they have a decent toolchain)? We don't provide any guarantee that some module built for a given version of -CURRENT works on a later version, at least once __FreeBSD_version has been bumped, do we? Additionally, AFAIK, applications are not even supposed to use struct ucred structures directly but rather grab them through, e.g., sysctl() or some apps/libs in the base system.

The offending consumers are in libkvm and libprocstat, which both #define WANT_UCRED and access the cr_ngroups field. I believe this change of layout will affect any application that calls e.g. kvm_open(), and there are several. The use-case that will be broken is attempting to read from an old kernel coredump, such as pgrep -M kcore. AFAIK libkvm does not guarantee backwards compatibility across major versions, you would need to open the 14-STABLE kernel core in a 14-STABLE chroot/jail to be sure, but I think it is wise and desirable to avoid obvious breakage here for this key structure.

I personally think the redundant copying of cr_ngroups is, in the end, fine as-is.

[olce]

I agree with you, so giving up on this revision, and updating some comments in D46915.