Refactor CPUID code by levitte · Pull Request #11311 · openssl/openssl

levitte · 2020-03-11T16:43:01Z

We were using CPUID coded in several modules, but it was unclear how
it actually got there, and could fail randomly.

To remedy that, this change separates the CPUID C code from the rest
of cryptlib.c, and ensures the right modules get both that and the
assembler sources explicitly.

Fixes #11281

paulidale

CI failures are relevant.

paulidale · 2020-03-12T03:06:30Z

I do wonder if propagating the CPUID is the way to go, would having a series of utility functions which query specific features be more readable? It won't change what needs to be done here but it would avoid magic constants throughout the code.

levitte · 2020-03-12T03:52:39Z

Do we want to make the CPUID functions public? Considering that legacy.so links with libcrypto.so, that's what it'll take. Or did you mean utility functions like replacing (OPENSSL_ia32cap_P[2] & (1 << 29)) == 0 with OPENSSL_ia32cap_has_SHAEXT()? I would straight up implement those as macros.

Anyway, that sounds like an effort for another PR

paulidale · 2020-03-12T04:05:20Z

The latter. Agreed, it is an enhancement.

xkqian

This fix works on my local machine.

mattcaswell · 2020-03-12T09:28:58Z

Travis red cross is relevant

levitte · 2020-03-12T18:51:42Z

I figured out one of the failures... it's possible that the other one is fixed as well

openssl-machine · 2020-04-12T00:08:12Z

This issue is in a state where it requires action by @openssl/committers but the last update was 30 days ago

t8m · 2020-04-14T16:33:47Z

I suppose the travis failure is still relevant, also needs rebase.

levitte · 2020-04-14T16:58:58Z

Rebased

slontis · 2020-04-15T12:57:25Z

@levitte - travis is running now - it has some issues in liblegacy.

t8m · 2020-04-15T13:18:08Z

Hmm, looking at the ppccap.c it seems to me it would make sense to rather refactor the algorithm-specific code out than to add everything to the legacy provider.

t8m · 2020-04-15T13:26:17Z

Another option - more ugly but less invasive - #ifdef all the code not needed in the legacy provider so when the .c is built for the legacy provider this code is not compiled.

openssl-machine · 2020-05-16T00:08:13Z

This issue is in a state where it requires action by @openssl/committers but the last update was 30 days ago

t8m · 2020-05-19T14:14:30Z

This is rather WIP - there are conflicts and issues in Travis jobs in legacy provider.

levitte · 2020-05-29T06:47:29Z

This is rather WIP - there are conflicts and issues in Travis jobs in legacy provider.

Yeah... this has turned out to contain a building problem that I've been avoiding for some time. I'll have to bite that bullet...

levitte · 2020-09-16T19:59:56Z

I changed this back to milestone 3.0.0. This is essentially to clear a building failure that appears under some conditions and some platforms. I don't see that we must rush this before beta1

We were using CPUID coded in several modules, but it was unclear how it actually got there, and could fail randomly. To remedy that, this change separates the CPUID C code from the rest of cryptlib.c, and ensures the right modules get both that and the assembler sources explicitly. Fixes openssl#11281

This will allow us to see what goes wrong on that platform

It turns out that some CPUID code requires the presence of some BN assembler code, so we make sure it's included in the same manner as the CPUID code itself.

levitte · 2020-12-17T20:05:41Z

Rebased on a fresher master. Let's see what can be done with this

The approval is stale and was made too early

levitte · 2021-01-21T18:46:27Z

I've done several tests lately, that were failing previously, but that now build and run correctly, given a fresher master.
With that in mind, I'm wondering if this fix is still necessary. If someone still gets clashes when linking, and can give me a way to reproduce it on Linux (I run Debian Sid), then this may be worth another look. If not, I'm considering closing this, to possibly pick up on another rainy day.

t8m · 2021-01-22T09:43:03Z

I remember the problems were mostly on non x86 architectures. PPC was the most problematic IMO.

xkqian · 2021-01-22T10:07:13Z

I remember the problems were mostly on non x86 architectures. PPC was the most problematic IMO.

Seems it happened on x86 32bit platform. I remember that Richard had one fix before, I verified it locally and the issue had gone. But that fix seems cause Travis red cross. I will verify it locally to see whether the issue has gone now.

levitte · 2021-01-23T07:14:59Z

Seems it happened on x86 32bit platform.

Yes, and I could reproduce that... then.

I will verify it locally to see whether the issue has gone now.

Please do.

xkqian · 2021-01-25T03:34:28Z

Seems it happened on x86 32bit platform.

Yes, and I could reproduce that... then.

I will verify it locally to see whether the issue has gone now.

Please do.

Done, the issue has gone with the openssl master branch code, as least the commit: daa86f9

paulidale

One question but approved nonetheless.

paulidale · 2021-01-25T03:46:39Z

.travis.yml

          arch: s390x
          compiler: gcc
          env: CONFIG_OPTS="--strict-warnings"
+        - os: linux-ppc64le


I'm wondering why?

It's quite possible that this platform demonstrated the issue at the time.
Since we don't do Travis any more, that's a bit hard to test for the moment... I'm not sure if it's even possible to specify specific hardware with GitHub Actions

paulidale · 2021-01-25T03:47:44Z

I also removed the [WIP] from the subject here.

levitte · 2021-01-25T11:38:20Z

I also removed the [WIP] from the subject here.

... because?

openssl-machine · 2021-01-26T04:00:18Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Approval dismissed, because there's currently nothing that demonstrates that this fixes the problem at this point in time

levitte · 2021-01-26T10:08:39Z

Done, the issue has gone with the openssl master branch code, as least the commit: daa86f9

After a conversation with the rest of the team, it's been decided to close this for now. If the issue turns out to still be present, we can pick it up then.

levitte · 2021-03-31T10:10:51Z

Re-opening this, it's time to get it done, now that there's a workflow to test against (#14753)

levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Mar 11, 2020

levitte added this to the 3.0.0 milestone Mar 11, 2020

levitte mentioned this pull request Mar 11, 2020

Build OpenSSL on ubuntu, compile failed in x86_32(providers/legacy.so) #11281

Closed

paulidale reviewed Mar 12, 2020

View reviewed changes

xkqian approved these changes Mar 12, 2020

View reviewed changes

levitte force-pushed the fix-11281 branch from 28cc0f0 to 49fc9a9 Compare April 14, 2020 16:59

levitte mentioned this pull request Apr 15, 2020

Add dh keygen to provider #11332

Closed

2 tasks

levitte mentioned this pull request Apr 24, 2020

3.0.0-alpha1: duplicate symbols with no-shared on macOS #11620

Closed

bernd-edlinger previously approved these changes May 16, 2020

View reviewed changes

levitte changed the title ~~Refactor CPUID code~~ [WIP] Refactor CPUID code May 29, 2020

kroeckx modified the milestones: 3.0.0, 3.0.0 beta1 Sep 16, 2020

levitte modified the milestones: 3.0.0 beta1, 3.0.0 Sep 16, 2020

t8m mentioned this pull request Dec 15, 2020

symbol collision with libcrypto.a and providers/liblegacy.a with "no-shared" #13627

Closed

levitte added 6 commits December 17, 2020 17:24

fixup! Refactor CPUID code

317e3cd

fixup! Refactor CPUID code

b9aec59

TEMPORARY: Add a BUILDONLY Travis build for ppc64le

89b299d

This will allow us to see what goes wrong on that platform

Include BN assembler alongside CPUID code

ce606dd

It turns out that some CPUID code requires the presence of some BN assembler code, so we make sure it's included in the same manner as the CPUID code itself.

fixup! Include BN assembler alongside CPUID code

6a1b9b5

levitte force-pushed the fix-11281 branch from 21b4783 to 6a1b9b5 Compare December 17, 2020 20:05

fixup! Refactor CPUID code

f598813

paulidale previously approved these changes Jan 25, 2021

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 25, 2021

paulidale changed the title ~~[WIP] Refactor CPUID code~~ Refactor CPUID code Jan 25, 2021

levitte removed the approval: done This pull request has the required number of approvals label Jan 26, 2021

levitte closed this Jan 26, 2021

levitte mentioned this pull request Mar 31, 2021

Github workflows: re-implement a no-shared build #14753

Closed

mattcaswell mentioned this pull request Apr 6, 2021

3.0.0-alpha13: duplicate symbols with no-shared on macOS 11.2 #14537

Closed

Uh oh!

Comments

Conversation

levitte commented Mar 11, 2020

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

paulidale commented Mar 12, 2020

Uh oh!

levitte commented Mar 12, 2020

Uh oh!

paulidale commented Mar 12, 2020

Uh oh!

xkqian left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Mar 12, 2020

Uh oh!

levitte commented Mar 12, 2020

Uh oh!

openssl-machine commented Apr 12, 2020

Uh oh!

t8m commented Apr 14, 2020

Uh oh!

levitte commented Apr 14, 2020

Uh oh!

slontis commented Apr 15, 2020

Uh oh!

t8m commented Apr 15, 2020

Uh oh!

t8m commented Apr 15, 2020

Uh oh!

openssl-machine commented May 16, 2020

Uh oh!

t8m commented May 19, 2020

Uh oh!

levitte commented May 29, 2020

Uh oh!

levitte commented Sep 16, 2020

Uh oh!

levitte commented Dec 17, 2020

Uh oh!

levitte commented Jan 21, 2021

Uh oh!

t8m commented Jan 22, 2021

Uh oh!

xkqian commented Jan 22, 2021

Uh oh!

levitte commented Jan 23, 2021

Uh oh!

xkqian commented Jan 25, 2021

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

paulidale Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

levitte Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

paulidale commented Jan 25, 2021

Uh oh!

levitte commented Jan 25, 2021

Uh oh!

openssl-machine commented Jan 26, 2021

Uh oh!

levitte commented Jan 26, 2021

Uh oh!

levitte commented Mar 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants