Page MenuHomeFreeBSD

Improve portability of structs xinpcb and friends.
ClosedPublic

Authored by brooks on May 10 2018, 11:49 PM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.May 10 2018, 11:49 PM
brooks updated this revision to Diff 42553.May 14 2018, 10:17 PM
  • Make struct xfile platform independant.
  • Fix struct xinpgen size.

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

kib added a comment.May 15 2018, 9:19 AM

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

brooks edited the summary of this revision. (Show Details)May 15 2018, 4:20 PM
kib accepted this revision.May 15 2018, 4:41 PM

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

brooks updated this revision to Diff 42604.May 15 2018, 9:53 PM
  • Add an explict pad to struct xinpcb and add more padding while breaking the ABI.
  • Add padding to struct xfile.
brooks updated this revision to Diff 42605.May 15 2018, 10:02 PM
  • Bump __FreeBSD_version.
  • Add a note indicating the need for netstat/sockstat and the kernel to be
swills added a subscriber: swills.May 27 2018, 8:46 PM
brooks edited the summary of this revision. (Show Details)May 30 2018, 8:46 PM
brooks updated this revision to Diff 43174.May 30 2018, 8:52 PM
  • Rebase
  • Update proposed commit date and __FreeBSD_version bump.
brooks updated this revision to Diff 43175.May 30 2018, 8:52 PM
  • Fix typo.

Anyone on transport care about this?

jtl added a subscriber: jtl.May 31 2018, 1:26 AM

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.

thj added a subscriber: thj.May 31 2018, 4:14 PM
brooks updated this revision to Diff 43786.Jun 14 2018, 9:32 PM
  • Fix typo.
  • Update UPDATING date and __FreeBSD_version
tuexen added a subscriber: tuexen.Jun 14 2018, 9:45 PM

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

eadler added a subscriber: eadler.Jun 16 2018, 7:32 PM

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.

brooks updated this revision to Diff 43960.Jun 17 2018, 6:58 PM
  • 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.

brooks updated this revision to Diff 44006.Jun 18 2018, 3:18 PM
  • s/kptr_t/kvaddr_t/
brooks updated this revision to Diff 44664.Jun 29 2018, 8:52 PM
  • 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.