Skip to content

Revert the DEVRANDOM_WAIT feature - #9084

Closed
mspncp wants to merge 1 commit into
openssl:masterfrom
mspncp:pr-revert-devrandom-wait
Closed

mspncp wants to merge 1 commit into
openssl:masterfrom
mspncp:pr-revert-devrandom-wait

Conversation

@mspncp

@mspncp mspncp commented Jun 5, 2019 •

Copy link
Copy Markdown
Contributor

The DEVRANDOM_WAIT feature added a select() call to wait for the
/dev/random device to become readable before reading from the
/dev/urandom device. It was introduced in commit 38023b8
in order to mitigate the fact that the /dev/urandom device
does not block until the initial seeding of the kernel CSPRNG
has completed, contrary to the behaviour of the getrandom()
system call.

It turned out that this change had negative side effects on the
performance which were not acceptable. After some discussion it
was decided to revert this feature and leave it up to the OS
resp. the platform maintainer to ensure a proper initialization
during early boot time.

Fixes #9078

This partially reverts commit 38023b8.

Checklist
  • documentation is added or updated
  • tests are added or updated

@mspncp
mspncp force-pushed the pr-revert-devrandom-wait branch from 873e0c9 to de3fe09 Compare June 5, 2019 09:47
@t-j-h

t-j-h commented Jun 5, 2019

Copy link
Copy Markdown
Member

+1 on the patch - but explicitly not actually approving it - leaving it open for other comments

@mspncp

mspncp commented Jun 5, 2019

Copy link
Copy Markdown
Contributor Author

This pull request inherits the '-1' added by @paulidale to issue #9078.

@mspncp mspncp added branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) hold branch: master Applies to master branch labels Jun 5, 2019
@mspncp

mspncp commented Jun 5, 2019 •

Copy link
Copy Markdown
Contributor Author

I added the [hold] label until the OMC vote is decided. My suggestion would be to go through a regular review process and as soon as it has two OMC approvals, one of them could initiate a vote on it. Is that ok?

@t-j-h

t-j-h commented Jun 5, 2019

Copy link
Copy Markdown
Member

The process is covered - basically anyone on the OMC can place a -1 on an item and that requires a vote to clear the -1 (or the person placing the -1 can themselves remove it). Anyone on the OMC can call for a vote to decide the issue. There is no time frame specified for when the vote is called or a requirement to ever call a vote (nor should there be).

@mspncp

mspncp commented Jun 5, 2019

Copy link
Copy Markdown
Contributor Author

Ok, thanks for the clarification.

@paulidale

Copy link
Copy Markdown
Contributor

Thanks @t-j-h for the clarification.

@kroeckx

kroeckx commented Jun 5, 2019 •

Copy link
Copy Markdown
Member

-1 (Just to make it clear this needs a vote)

The DEVRANDOM_WAIT feature added a select() call to wait for the
`/dev/random` device to become readable before reading from the
`/dev/urandom` device. It was introduced in commit 38023b8
in order to mitigate the fact that the `/dev/urandom` device
does not block until the initial seeding of the kernel CSPRNG
has completed, contrary to the behaviour of the `getrandom()`
system call.

It turned out that this change had negative side effects on the
performance which were not acceptable. After some discussion it
was decided to revert this feature and leave it up to the OS
resp. the platform maintainer to ensure a proper initialization
during early boot time.

Fixes openssl#9078

This partially reverts commit 38023b8.
@mspncp
mspncp force-pushed the pr-revert-devrandom-wait branch from de3fe09 to c19c5a6 Compare June 6, 2019 09:12
@mspncp

mspncp commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

Rebased without changes in order to pick up the doc-nit fix (b1f6925).

@mspncp

mspncp commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

So is anyone of the @openssl/omc members willing to support this reversal of the DEVRANDOM_WAIT feature by calling for an OMC vote?

@mattcaswell

Copy link
Copy Markdown
Member

So is anyone of the @openssl/omc members willing to support this reversal of the DEVRANDOM_WAIT feature by calling for an OMC vote?

I haven't yet decided how I will vote. I will leave the calling of the vote to someone else.

@t-j-h

t-j-h commented Jun 6, 2019

Copy link
Copy Markdown
Member

I have just called for the OMC vote on approving this PR.
I will update the PR once the vote completes (it may take up to 14 days).

@mspncp

mspncp commented Jun 6, 2019

Copy link
Copy Markdown
Contributor Author

Thank you Tim. I will add a separate commit in the next days which adds CHANGES entries for the reversal c19c5a6 and the original commit 38023b8 to the respective sections (1.1.1d resp. 1.1.1c).

@paulidale

Copy link
Copy Markdown
Contributor

Preempting @t-j-h the vote has passed and this PR is good to merge after appropriate review.

@vdukhovni vdukhovni left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks. Looks fine.

@vdukhovni vdukhovni removed the hold label Jun 8, 2019
@mspncp

mspncp commented Jun 8, 2019

Copy link
Copy Markdown
Contributor Author

Thank you, your decision was made faster than I expected. I'd still like to add the two CHANGES entries, but I am travelling today. Hope I will be able to do it late this evening or tomorrow.

@mspncp

mspncp commented Jun 9, 2019

Copy link
Copy Markdown
Contributor Author

It turned out to be simpler to create a separate pull request for 1.1.1 which contains the cherry-pick and adds the CHANGES entries. @t-j-h @vdukhovni please take a look at #9118.

@mspncp

mspncp commented Jun 9, 2019

Copy link
Copy Markdown
Contributor Author

I'll merge this pr together with #9118, as soon as both are approved.

levitte pushed a commit that referenced this pull request Jun 9, 2019
The DEVRANDOM_WAIT feature added a select() call to wait for the
`/dev/random` device to become readable before reading from the
`/dev/urandom` device. It was introduced in commit 38023b8
in order to mitigate the fact that the `/dev/urandom` device
does not block until the initial seeding of the kernel CSPRNG
has completed, contrary to the behaviour of the `getrandom()`
system call.

It turned out that this change had negative side effects on
performance which were not acceptable. After some discussion it
was decided to revert this feature and leave it up to the OS
resp. the platform maintainer to ensure a proper initialization
during early boot time.

Fixes #9078

This partially reverts commit 38023b8.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from #9084)
@mspncp

mspncp commented Jun 9, 2019

Copy link
Copy Markdown
Contributor Author

Merged, thank you!

master
a08714e Revert the DEVRANDOM_WAIT feature

1.1.1 (#9118)
247b8a0 Add CHANGES entries for the DEVRANDOM_WAIT feature and its removal
ad416c8 Revert the DEVRANDOM_WAIT feature

@mspncp mspncp closed this Jun 9, 2019
@mspncp mspncp removed the branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) label Jun 9, 2019
@mspncp
mspncp deleted the pr-revert-devrandom-wait branch June 27, 2019 15:05
ofrobots added a commit to ofrobots/node that referenced this pull request Aug 5, 2019
Original commit message:
    Revert the DEVRANDOM_WAIT feature

    The DEVRANDOM_WAIT feature added a select() call to wait for the
    `/dev/random` device to become readable before reading from the
    `/dev/urandom` device. It was introduced in commit 38023b8
    in order to mitigate the fact that the `/dev/urandom` device
    does not block until the initial seeding of the kernel CSPRNG
    has completed, contrary to the behaviour of the `getrandom()`
    system call.

    It turned out that this change had negative side effects on the
    performance which were not acceptable. After some discussion it
    was decided to revert this feature and leave it up to the OS
    resp. the platform maintainer to ensure a proper initialization
    during early boot time.

    Fixes 9078

    This partially reverts commit 38023b8.

Refs: openssl/openssl#9084
Fixes: nodejs#28932
BethGriggs pushed a commit to nodejs/node that referenced this pull request Aug 6, 2019
Original commit message:
    Revert the DEVRANDOM_WAIT feature

    The DEVRANDOM_WAIT feature added a select() call to wait for the
    `/dev/random` device to become readable before reading from the
    `/dev/urandom` device. It was introduced in commit 38023b8
    in order to mitigate the fact that the `/dev/urandom` device
    does not block until the initial seeding of the kernel CSPRNG
    has completed, contrary to the behaviour of the `getrandom()`
    system call.

    It turned out that this change had negative side effects on the
    performance which were not acceptable. After some discussion it
    was decided to revert this feature and leave it up to the OS
    resp. the platform maintainer to ensure a proper initialization
    during early boot time.

    Fixes 9078

    This partially reverts commit 38023b8.

Refs: openssl/openssl#9084
Fixes: #28932

PR-URL: #28983
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
arcadia-devtools pushed a commit to catboost/catboost that referenced this pull request Aug 29, 2019
It was deleted in openssl/openssl#9084
It corrupts the stack: openssl/openssl#9686

Note: mandatory check (NEED_CHECK) was skipped
ref:23dc8df65263efcb9cc1e95187a6597a15c60e49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance issues with OpenSSL 1.1.1c caused by DEVRANDOM_WAIT

6 participants