Page MenuHomeFreeBSD

Improve portability of structs xinpcb and friends.
ClosedPublic

Authored by brooks on May 10 2018, 11:49 PM.
Tags
None
Referenced Files
F103117100: D15386.id42605.diff
Thu, Nov 21, 6:06 AM
Unknown Object (File)
Wed, Nov 20, 12:34 AM
Unknown Object (File)
Tue, Nov 19, 11:15 PM
Unknown Object (File)
Tue, Nov 19, 8:43 PM
Unknown Object (File)
Tue, Nov 19, 5:38 PM
Unknown Object (File)
Fri, Nov 15, 3:13 PM
Unknown Object (File)
Mon, Nov 11, 8:59 AM
Unknown Object (File)
Mon, Nov 11, 8:58 AM

Details

Summary

Replace size_t members with uint64_t and pointer members (never used as
pointers, but instead as unique idenitifiers) with int64_t. This makes
the structs identical between 32-bit and 64-bit systems.

On 64-bit bit systems, the ABI is maintained. On 32-bit systems, this
is an ABI breaking change. The ABI was previously broken in r315662.
This also imposes a small API change on userspace consumers who must
handle kernel pointers becoming virtual addresses.

PR: 228301 (exp-run by antoine)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • Make struct xfile platform independant.
  • Fix struct xinpgen size.

With this set of changes (plus D15438) i386 netstat and sockstat binaries work on amd64.

And what is the scope of the ABI breakage ? I suspect that user programs like squid are affected.

In D15386#325723, @kib wrote:

And what is the scope of the ABI breakage ? I suspect that user programs like squid are affected.

I don't have a way to determine what all is broken. It's worth noting that we've already broken these ABIs in 12 with rS315662 and follow on commits. I probably wouldn't have proposed this change otherwise, but one more break and this fairly small change eliminates the need for a compatibility shim for freebsd32 (and also CHERI).

Then, add a padding while you add more ABI breakage, perhaps ?

  • Add an explict pad to struct xinpcb and add more padding while breaking the ABI.
  • Add padding to struct xfile.
  • Bump __FreeBSD_version.
  • Add a note indicating the need for netstat/sockstat and the kernel to be
  • Rebase
  • Update proposed commit date and __FreeBSD_version bump.

Anyone on transport care about this?

Yes, people in the transport group probably do care. (At least, they usually care about this sort of thing. :-) )

I'll add it to the agenda for our bi-weekly meeting tomorrow. Or, if we don't get the right people tomorrow, you can probably get a good answer at the Transport Working Group at the BSDCan Dev Summit next week.

  • Fix typo.
  • Update UPDATING date and __FreeBSD_version

I'm fine with the SCTP related changes.

Rather than using raw integer types, I wonder if we should introduce new FreeBSD types -- e.g., ksize_t, kptr_t -- that wrap them, instead? They would still all map through to int64_t/uint64_t as required, but if we needed to change this in the future, we could do so more easily.

(This would also allow us to more easily grep for kernel sizes and pointers exposed to userspace..)

Rather than using raw integer types, I wonder if we should introduce new FreeBSD types -- e.g., ksize_t, kptr_t -- that wrap them, instead? They would still all map through to int64_t/uint64_t as required, but if we needed to change this in the future, we could do so more easily.

Yes please! It also makes it clearer what type you're trying to get and makes errors more obvious if have the wrong one.

  • Use new ksize_t and kptr_t instead of (u)int64_t.
  • Adjust some padding.
  • Use new ksize_t and kptr_t instead of (u)int64_t.
  • Adjust some padding.

Hmm. This is the problem with thinking overnight on things. Do you think it should be kvaddr_t rather than kptr_t?

  • Use new ksize_t and kptr_t instead of (u)int64_t.
  • Adjust some padding.

Hmm. This is the problem with thinking overnight on things. Do you think it should be kvaddr_t rather than kptr_t?

Yeah, I almost change to kaddr_t half way through. I'll switch to kvaddr_t.

  • Fix a botched kptr_t change.
  • Make kvaddr_t unsigned to match the defintion in kvm.h and remove the
This revision was not accepted when it landed; it landed in state Needs Review.Jul 5 2018, 1:14 PM
This revision was automatically updated to reflect the committed changes.