Optimize session cache flushing by tmshort · Pull Request #8687 · openssl/openssl

tmshort · 2019-04-05T18:20:05Z

Sort SSL_SESSION structures by timeout in the linked list.
Iterate over the linked list for timeout, stopping when no more
session can be flushed.
Do SSL_SESSION_free() outside of SSL_CTX lock
Update timeout upon use

Checklist

tests are added or updated

tmshort · 2019-04-05T18:20:50Z

This does update the timeout upon use; which is a behavior change. This could instead be an option on the SSL_CTX to which it has been added (so, new API)

FdaSilvaYY · 2019-04-06T10:54:59Z

typo in commit heading message and PR Title ;)

tmshort · 2019-04-08T17:08:41Z

Added unit tests

tmshort · 2019-04-08T18:40:57Z

Rebase, fix comment, fix CI (I hope)

tmshort · 2019-04-10T17:13:07Z

Travis failure seems to be a build timeout?

tmshort · 2019-04-10T17:55:55Z

Added option to update the time upon add.

kaduk · 2019-05-03T16:56:09Z

ssl/ssl_sess.c

I know this is inviting controversy to propose, but we may want to assert (in some form) that next != NULL after this line.

See above as to what the proposed assert should check.

ssl/ssl_sess.c

ssl/ssl_locl.h

ssl/ssl_sess.c

tmshort · 2019-10-03T00:52:25Z

Thanks @paulidale, we actually backed this out of our repo because it caused a problem... or exposed a problem... we're not exactly sure, but we had some weird behavior with this.

paulidale · 2019-10-03T00:56:45Z

Should this be closed then?

tmshort · 2019-10-03T01:02:19Z

No, we need to test it more. If you see anything obvious; great. But I think it exposed a problem we had, and we actually need to clean up the other code (which is PR #3761).

tmshort · 2020-07-15T19:01:52Z

This exposed a problem in some of our other changes @paulidale, so it's actually good.

openssl-machine · 2020-10-01T00:07:52Z

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

openssl-machine · 2020-11-01T00:07:53Z

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

openssl-machine · 2020-12-02T00:07:47Z

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

openssl-machine · 2021-01-02T00:07:43Z

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

openssl-machine · 2021-02-02T00:07:29Z

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

openssl-machine · 2021-03-05T00:07:12Z

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

tmshort · 2021-05-20T17:50:54Z

ping @mattcaswell ?

FdaSilvaYY · 2021-05-31T14:47:19Z

Ping @openssl : this PR is failing in the limbo.

ssl/ssl_local.h

ssl/ssl_sess.c

mattcaswell · 2021-06-01T12:02:05Z

ssl/ssl_sess.c

This reads s->time with no lock, and then subsequently writes to it under a write lock. Shouldn't there be a read lock here?

Not necessarily. It's checking to see if it's different; the actual value doesn't matter. This is an optimization to see if it should be updated. Previously it would just slam the time into the session without any locking at all. Any manipulation is done under lock. But note that the lock only happens if the session is in a SSL_CTX cache; the SSL_SESSION structure does not have a lock.

But if s->owner is non-NULL then we know that the session has already been placed in the cache and therefore taking a read lock might be appropriate. It definitely seems odd to control writes with a write lock, but not reads.

I would agree 100% if the value read were used for something meaningful. In this case, it's really not. It's read to see if there needs to be a change. If so, do the update; the prior value doesn't matter.
Because of that, locking for read here is pointless; if we're always going to lock, then we might as well always update, and remove the (albeit minor) optimization. I can make that change (unless my argument is convincing 😄 ).

I would prefer to see it modified as you suggest.

mattcaswell · 2021-06-01T12:04:15Z

ssl/ssl_sess.c

Reads s->timeout not under lock, and then subsequently writes to it under lock. This seems wrong.

Same as before.

ssl/statem/statem_clnt.c

tmshort · 2021-06-01T13:05:18Z

@mattcaswell lots (most?) of the code was originally written w/o read locks; this doesn't really change that.

Sort SSL_SESSION structures by timeout in the linked list. Iterate over the linked list for timeout, stopping when no more session can be flushed. Do SSL_SESSION_free() outside of SSL_CTX lock Update timeout upon use

mattcaswell · 2021-06-02T10:14:46Z

@mattcaswell lots (most?) of the code was originally written w/o read locks; this doesn't really change that.

I'm satisfied with most of the answers - although I've left 2 comments unresolved which I'm still not convinced about.

ssl/ssl_sess.c

tmshort · 2021-06-08T19:29:08Z

@mattcaswell I think I made the requested changes.

mattcaswell · 2021-06-09T07:33:51Z

@paulidale - please can you reconfirm your earlier approval?

openssl-machine · 2021-06-10T08:00:23Z

This pull request is ready to merge

paulidale · 2021-06-10T08:32:44Z

Merged, thanks for this.

Sort SSL_SESSION structures by timeout in the linked list. Iterate over the linked list for timeout, stopping when no more session can be flushed. Do SSL_SESSION_free() outside of SSL_CTX lock Update timeout upon use Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #8687)

Sort SSL_SESSION structures by timeout in the linked list. Iterate over the linked list for timeout, stopping when no more session can be flushed. Do SSL_SESSION_free() outside of SSL_CTX lock Update timeout upon use Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#8687)

tmshort changed the title ~~Optimize sesion cache flushing~~ Optimize session cache flushing Apr 8, 2019

tmshort force-pushed the master-flush branch from 97a3af6 to 1a1360a Compare April 8, 2019 18:36

kaduk reviewed May 3, 2019

View reviewed changes

tmshort commented Jul 10, 2019

View reviewed changes

ssl/ssl_sess.c Outdated Show resolved Hide resolved

paulidale reviewed Oct 3, 2019

View reviewed changes

ssl/ssl_locl.h Outdated Show resolved Hide resolved

ssl/ssl_sess.c Outdated Show resolved Hide resolved

tmshort force-pushed the master-flush branch from 17154d8 to 9f8e6fa Compare October 3, 2019 14:08

tmshort force-pushed the master-flush branch from 9f8e6fa to d48cc7c Compare July 15, 2020 19:00

tmshort force-pushed the master-flush branch from d48cc7c to 294e2d7 Compare July 15, 2020 19:35

tmshort force-pushed the master-flush branch from 294e2d7 to 094ff53 Compare August 17, 2020 22:42

tmshort force-pushed the master-flush branch 2 times, most recently from ca21130 to 394a332 Compare August 31, 2020 22:01

tmshort requested review from kaduk and paulidale August 31, 2020 22:02

tmshort force-pushed the master-flush branch from 394a332 to 3d630a9 Compare December 11, 2020 20:55

tmshort force-pushed the master-flush branch from 7e1f7fd to 1b50136 Compare May 12, 2021 12:21

mattcaswell requested changes Jun 1, 2021

View reviewed changes

Optimize session cache flushing

2a23a7d

Sort SSL_SESSION structures by timeout in the linked list. Iterate over the linked list for timeout, stopping when no more session can be flushed. Do SSL_SESSION_free() outside of SSL_CTX lock Update timeout upon use

tmshort force-pushed the master-flush branch from 1b50136 to 2a23a7d Compare June 1, 2021 13:55

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jun 1, 2021

paulidale reviewed Jun 3, 2021

View reviewed changes

ssl/ssl_sess.c Show resolved Hide resolved

fixup! Optimize session cache flushing

bc5b42c

github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Jun 4, 2021

mattcaswell approved these changes Jun 9, 2021

View reviewed changes

paulidale approved these changes Jun 9, 2021

View reviewed changes

paulidale added the approval: done This pull request has the required number of approvals label Jun 9, 2021

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jun 10, 2021

paulidale closed this Jun 10, 2021

tmshort deleted the master-flush branch June 11, 2021 21:00

FdaSilvaYY mentioned this pull request Oct 4, 2021

Y2038 overflow problem in API functions taking long values when compiling for Windows #16726

Closed

This was referenced Feb 28, 2022

Fix time_t arithmetic after 2038 #17701

Closed

Improve post-2038 compatibility of time_t usage #17778

Closed

vdukhovni mentioned this pull request Jul 6, 2022

Fatal - Unexpected Message returned with OpenSSL 3.0 #18690

Closed

wbl mentioned this pull request Apr 27, 2023

Address session ownership #20848

Closed

2 tasks

Uh oh!

Conversation

tmshort commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

tmshort commented Apr 5, 2019

Uh oh!

FdaSilvaYY commented Apr 6, 2019

Uh oh!

tmshort commented Apr 8, 2019

Uh oh!

tmshort commented Apr 8, 2019

Uh oh!

tmshort commented Apr 10, 2019

Uh oh!

tmshort commented Apr 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tmshort commented Oct 3, 2019

Uh oh!

paulidale commented Oct 3, 2019

Uh oh!

tmshort commented Oct 3, 2019

Uh oh!

tmshort commented Jul 15, 2020

Uh oh!

openssl-machine commented Oct 1, 2020

Uh oh!

openssl-machine commented Nov 1, 2020

Uh oh!

openssl-machine commented Dec 2, 2020

Uh oh!

openssl-machine commented Jan 2, 2021

Uh oh!

openssl-machine commented Feb 2, 2021

Uh oh!

openssl-machine commented Mar 5, 2021

Uh oh!

tmshort commented May 20, 2021

Uh oh!

FdaSilvaYY commented May 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmshort commented Jun 1, 2021

Uh oh!

mattcaswell commented Jun 2, 2021

Uh oh!

Uh oh!

tmshort commented Jun 8, 2021

Uh oh!

mattcaswell commented Jun 9, 2021

Uh oh!

openssl-machine commented Jun 10, 2021

Uh oh!

tmshort commented Apr 5, 2019 •

edited

Loading

tmshort Jun 1, 2021 •

edited

Loading