Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add more definitions from linux/tls.h #3422

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Add more definitions from linux/tls.h #3422

merged 1 commit into from
Nov 9, 2023

Conversation

Arnavion
Copy link
Contributor

@Arnavion Arnavion commented Nov 3, 2023

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Nov 3, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Comment on lines +3273 to +3278
pub const TLS_1_2_VERSION: ::__u16 =
((TLS_1_2_VERSION_MAJOR as ::__u16) << 8) | (TLS_1_2_VERSION_MINOR as ::__u16);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual header uses a macro for this and TLS_1_3_VERSION:

#define TLS_VERSION_NUMBER(id)  ((((id##_VERSION_MAJOR) & 0xFF) << 8) | \
                                 ((id##_VERSION_MINOR) & 0xFF))

I've inlined the calculation here rather than bind it to a fn because I figured there's no reason a user would want to use it.

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2023

📌 Commit df3e305 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 3, 2023

⌛ Testing commit df3e305 with merge 1c06f6c...

bors added a commit that referenced this pull request Nov 3, 2023
Add more definitions from linux/tls.h
@bors
Copy link
Contributor

bors commented Nov 3, 2023

💔 Test failed - checks-actions

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 3, 2023

What does the CI failure mean? That the mips builder's version of the header doesn't define the CHACHA20-POLY1305-related items? The definitions in the header aren't arch-specific so I guess the builder just has old headers (added in v5.11). What should be done?

@devnexen
Copy link
Contributor

devnexen commented Nov 5, 2023

e.g. tls12_crypto_info_chacha20_poly1305 is available only from 5.11, you might need to add a bunch of exceptions in libc-test/build.rs appropriately.

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 5, 2023

Okay, I'll look into that. I tried updating the OpenWRT SDK like 7015ee9 had done to solve a similar problem in the past, which does pull in new kernel headers (5.15) and fixes the problem, but it also ends up pulling in musl 1.2 which is broken without #1848

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 5, 2023

Can I get a bors rerun please?

@@ -3593,6 +3593,10 @@ fn test_linux(target: &str) {
// FIXME: The size of `iv` has been changed since Linux v6.0
// https://github.com/torvalds/linux/commit/94dfc73e7cf4a31da66b8843f0b9283ddd6b8381
"af_alg_iv" => true,

// FIXME: Requires >= 5.11 kernel headers
"tls12_crypto_info_chacha20_poly1305" if (mips || mips64) && musl => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check there is really no more to add than this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I ran ci/run-docker.sh for the mips target locally. The issue is only with Chacha20-Poly1305 definitions, so this one struct and the six consts.

@JohnTitor
Copy link
Member

Could you squash commits into one?

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 6, 2023

Sure. I was hoping to have the CI run first just in case there are other architectures that are also broken, since the previous CI run cancelled them when mips failed.

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 6, 2023

@JohnTitor Can you re-run bors please?

@JohnTitor
Copy link
Member

👍 @bors r+

@bors
Copy link
Contributor

bors commented Nov 8, 2023

📌 Commit ce582a1 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 8, 2023

⌛ Testing commit ce582a1 with merge d2d81c8...

bors added a commit that referenced this pull request Nov 8, 2023
Add more definitions from linux/tls.h
@bors
Copy link
Contributor

bors commented Nov 8, 2023

💔 Test failed - checks-actions

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 8, 2023

Sigh, i686 failed this time. And again because it cancelled all the other jobs I don't know what else is broken. I'm going to have to run them all locally...

@Arnavion
Copy link
Contributor Author

Arnavion commented Nov 8, 2023

Every target that uses install-musl.sh also pulls in ancient headers without TLS 1.3 or AES-256-GCM support, so I've updated build.rs to catch those too.

I ran all the targets that CI tests locally and they all pass now. So bors should work too. @JohnTitor Can you rerun it please?

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2023

📌 Commit fbcacd0 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 9, 2023

⌛ Testing commit fbcacd0 with merge 79d195b...

@bors
Copy link
Contributor

bors commented Nov 9, 2023

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 79d195b to main...

@bors bors merged commit 79d195b into rust-lang:main Nov 9, 2023
11 checks passed
@Arnavion Arnavion deleted the ktls branch November 9, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants