Skip to content

FreeBSD: Support kernel-offloaded TLS TCP #4482

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Jun 26, 2025
Merged

Conversation

kostikbel
Copy link
Contributor

Description

The PR adds definitions for kernel-offloaded TLS TCP session information sysctls. It is used by the utility to list kTLS connections https://github.com/kostikbel/ktcplist

Sources

https://github.com/freebsd/freebsd-src/blob/4078e0d1d6f8189bff44fcad04df256220035f76/sys/sys/ktls.h
freebsd/freebsd-src@c9e9a0f

@rustbot label +stable-nominated

@rustbot rustbot added O-unix S-waiting-on-review stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Jun 10, 2025
@kostikbel kostikbel force-pushed the ktls branch 5 times, most recently from b60cffa to 7e42c78 Compare June 10, 2025 08:10
@kostikbel
Copy link
Contributor Author

For nightly freebsd-15 ci task to success, you would need to update the ci freebsd env with the new headers.

I have no idea why freebsd-13 is failing.

@tgross35
Copy link
Contributor

@asomers would you mind reviewing this since it's FreeBSD?

For nightly freebsd-15 ci task to success, you would need to update the ci freebsd env with the new headers.

If there is a version bump available, feel freeto submit a PR updating it.

I'm not sure about freebsd-13 either, ctest seems extremely flaky on that target.

@kostikbel
Copy link
Contributor Author

kostikbel commented Jun 16, 2025

For nightly freebsd-15 ci task to success, you would need to update the ci freebsd env with the new headers.

If there is a version bump available, feel freeto submit a PR updating it.

Is there an existing example of the PR with such update?

@tgross35
Copy link
Contributor

You just want to update a minor freebsd version? I don't know one off the top of my head but it shouldn't be too hard, the FreeBSD CI config is in https://github.com/rust-lang/libc/blob/9c8cd59b2367ce9cd5acd6247c3764cdf670a9f8/.cirrus.yml

@asomers
Copy link
Contributor

asomers commented Jun 16, 2025

If there is a version bump available, feel freeto submit a PR updating it.

Our CI config uses the latest FreeBSD 15 snapshot on GCE. So we don't need to do anything. If we just wait, GCE will have the latest release soon enough.

Copy link
Contributor

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool project, kib! I look forward to trying it.

Comment on lines 1730 to 1733
pub _xig_spare32: u32,
pub xig_gen: inp_gen_t,
pub xig_sogen: so_gen_t,
pub _xig_spare64: [u64; 4],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention used elsewhere in libc is to make spare and padding fields private. The forces consumers to initialize with mem::zeroed, but it also makes it easier for future versions of libc to replace those spare fields with no backwards-compatibility problems.

Suggested change
pub _xig_spare32: u32,
pub xig_gen: inp_gen_t,
pub xig_sogen: so_gen_t,
pub _xig_spare64: [u64; 4],
_xig_spare32: u32,
pub xig_gen: inp_gen_t,
pub xig_sogen: so_gen_t,
_xig_spare64: [u64; 4],

}

pub struct in_addr_4in6 {
pub ia46_pad32: [u32; 3],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub ia46_pad32: [u32; 3],
ia46_pad32: [u32; 3],

pub inp_gencnt: u64,
pub so_pcb: kvaddr_t,
pub coninf: crate::in_conninfo,
pub rx_vlan_id: c_short,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub rx_vlan_id: c_short,
pub rx_vlan_id: c_ushort,

@kostikbel
Copy link
Contributor Author

Could you please push this?

@tgross35
Copy link
Contributor

I rebased this to pick up a new set of CI runs since one thing was fixed. The FreeBSD-15 and FreeBSD-13 x86-64 jobs are spurious, but the FreeBSD-13 i686 "bad xinpgen align: rust: 4 (0x4) != c 8 (0x8)" message looks legit. Is one of these fields actually a different size on 32-bit?

@@ -1722,6 +1725,72 @@ s_no_extra_traits! {
pub uc_flags: c_int,
__spare__: [c_int; 4],
}

pub struct xinpgen {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will fix the latest test failure on i386.

Suggested change
pub struct xinpgen {
#[repr(align(8)]
pub struct xinpgen {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, can I still update the branch, or should the change be a fixup?

@kostikbel
Copy link
Contributor Author

kostikbel commented Jun 25, 2025

I rebased this to pick up a new set of CI runs since one thing was fixed. The FreeBSD-15 and FreeBSD-13 x86-64 jobs are spurious, but the FreeBSD-13 i686 "bad xinpgen align: rust: 4 (0x4) != c 8 (0x8)" message looks legit. Is one of these fields actually a different size on 32-bit?

Hm, no. The reason for the message is the attribute of the C structure, declaring 8-bytes alignment.
See https://github.com/freebsd/freebsd-src/blob/cd0169c9379c400ec75b77e87ca770e37f964276/sys/netinet/in_pcb.h#L296

Would it be enough to add
#[repr(C, align(8))]
before the structure?

@tgross35
Copy link
Contributor

You don't need to include C since that's added by default for everything in the macro, but #[repr(align(8))] should work!

@kostikbel
Copy link
Contributor Author

You don't need to include C since that's added by default for everything in the macro, but #[repr(align(8))] should work!

So I added a commit with the align. If you want me to squash, please say so.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No need to squash everything to one but if you combine the align fix with whatever added the struct, that would make a bit more clean cherry pick.

@kostikbel
Copy link
Contributor Author

Thanks! No need to squash everything to one but if you combine the align fix with whatever added the struct, that would make a bit more clean cherry pick.

Squashed just the fix.

@tgross35 tgross35 changed the title Ktls FreeBSD: Support kernel-offloaded TLS TCP Jun 26, 2025
@tgross35
Copy link
Contributor

Thanks!

@tgross35 tgross35 added this pull request to the merge queue Jun 26, 2025
Merged via the queue into rust-lang:main with commit ec44bd2 Jun 26, 2025
50 of 51 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
O-unix S-waiting-on-review stable-nominated This PR should be considered for cherry-pick to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants