Page MenuHomeFreeBSD

Fix padding in struct tcp_info
ClosedPublic

Authored by asomers on Sep 17 2023, 2:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 1:52 PM
Unknown Object (File)
Mon, Nov 25, 1:52 PM
Unknown Object (File)
Mon, Nov 25, 1:52 PM
Unknown Object (File)
Mon, Nov 25, 1:28 PM
Unknown Object (File)
Thu, Nov 14, 7:41 AM
Unknown Object (File)
Thu, Nov 14, 4:54 AM
Unknown Object (File)
Thu, Nov 14, 4:42 AM
Unknown Object (File)
Tue, Nov 12, 8:17 AM

Details

Summary

This structure should have the same size across different FreeBSD
versions. It has since at least stable/7. But two recent changes
accidentally added fields without compensating by reducing padding:
945f9a7cc9dcc071bfcc702748fbbb11087ae773 and
22c81cc51636cfebe94e0979eb31556d87775938 . Reduce padding to
compensate.

PR: 273902
MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Note that I have no way to test this change.

This padding was discussed in the FreeBSD transport call. The result of the discussion was to not reduce the padding when adding fields in head. In the stable branches the padding should be reduced. I think glebius@ might have more insights.

glebius requested changes to this revision.EditedSep 19 2023, 5:07 PM

I accepted the revision, but I'd ask to commit it to stable/14 directly, bypassing main. Changing that to 'request changes', as there is no option just to withdraw approval.

Let me explain my vision that we previously discussed with the TCP group. There is no policy for structures to be stable across ALL FreeBSD versions. We don't want to carry stable/7 compatibility until 22nd century. However, there is policy to keep binary compatibility within a single stable branch. This is what padding is for. If we shrink padding in main, then what happens when we totally run out of padding? We'd be forced either to break compatibility suddenly or just to refuse to add new fields.

So, we have here two options for stable/14:

  1. Keep padding at 14 words. That will make stable/14 compatible with main until new structure field is introduced in main. May happen soon or later.
  2. Set padding to 10 words. That will make stable/14 compatible with stable/13 and immediately incompatible with main.

I'm fine with either option.

This revision now requires changes to proceed.Sep 19 2023, 5:07 PM

In other words. Padding is useful to allow merges of new features to a stable branch. Padding exists in the main branch for the only reason - not to forget to add it at a branch point.

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.

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.

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

I don't have a strong opinion how to handle padding across major revisions. I prefer to be consistent with some rule, which is communicated well.

In this particular case, I'm fine with committing the fix to stable/14 as long as it also goes into releng/14.0, since I think the size of struct tcp_info must not change between 14.0 and 14.1. I think glebius@ shares this view.

So are you considering to commit this change to stable/14 and releng/14.0 before 14.0 gets released?

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?

Rust uses FFI to link to C. For most libraries, the bindings are statically generated ahead of time. For example: https://github.com/rust-lang/libc/blob/6f31de33032bf37b5735357320feedb84bb44478/src/unix/bsd/freebsdlike/freebsd/mod.rs#L5178 . So at build time, nothing needs to parse C header files. Nothing like autotools needs to run. This makes cross-compiling a breeze! In fact, you can check whether most Rust programs will successfully compile on a different OS without installing _anything_ from that OS's environment (actually producing running executables does require the target's libraries, however). The problem with this approach is that Rust builds have only limited information about the target. They know "x86_64 FreeBSD", but not "x86_64 FreeBSD 13.1" and certainly not "x86_64 FreeBSD 13.1 with some custom changes". Since nothing parses header files at build time, Rust build just have to assume the lowest common denominator.

Currently, that means FreeBSD 11.0. If you look closely at that link I pasted, you'll see that it has a custom statfs@FBSD_1.0 link name. That means that it uses the FreeBSD 11 version of statfs regardless of where it actually runs. So too for stat, kqueue, etc. In fact, even today it's basically impossible to build a Rust program that uses 64-bit inodes. There have been various proposals to fix this problem, but to date they've gone nowhere. FreeBSD, OpenBSD, and GNU/Linux all want slightly different things. GNU/Linux, since it only adds APIs and never changes them, is the easiest to satisfy. And Rust's core developers are mostly Linux users, and mostly overworked, so they've been neglecting the problem.

Therefore, I would like to apply the padding changes at least to stable/14 if not to main, so that FreeBSD 14's struct tcp_info will continue to be usable by Rust programs even if Rust doesn't fix its symbol versioning problem.

@tuexen To answer your question yes. If you approve, I'll directly commit to stable/14 and MFC to releng/14.0.

I'm fine with committing to to stable/14 und the assumption that it is also merged to releng/14.0.

This revision was not accepted when it landed; it landed in state Needs Revision.Sep 22 2023, 5:25 PM
This revision was automatically updated to reflect the committed changes.

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