tsan: make detecting the need for locking when using tsan easier #17479

paulidale · 2022-01-12T02:28:37Z

TSAN has a number of problems currently.

Eventually this might fix #17447

documentation is added or updated
tests are added or updated

paulidale · 2022-01-12T04:18:34Z

The files that reference TSAN_QUALIFIER are:

test/threadstest.c
crypto/core_namemap.c
crypto/mem.c
crypto/objects/obj_dat.c
crypto/lhash/lhash.c
crypto/lhash/lhash_local.h
crypto/x509/v3_purp.c
include/internal/tsan_assist.h
providers/implementations/rands/drbg_local.h
ssl/ssl_local.h
ssl/ssl_lib.c

bernd-edlinger

OK, whether or not you want to fix the white space issue.

crypto/mem.c

bernd-edlinger

ah sorry, overlooked the CI issue, which is relevant

bernd-edlinger · 2022-01-12T05:50:24Z

crypto/lhash/lhash.c:199:9: error: variable 'ret' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
    if (tsan_lock(lh)) {
        ^~~~~~~~~~~~~
crypto/lhash/lhash.c:210:12: note: uninitialized use occurs here
    return ret;
           ^~~
crypto/lhash/lhash.c:199:5: note: remove the 'if' if its condition is always true
    if (tsan_lock(lh)) {
    ^~~~~~~~~~~~~~~~~~~
crypto/lhash/lhash.c:192:14: note: initialize the variable 'ret' to silence this warning
    void *ret;
             ^
              = NULL
1 error generated.

crypto/lhash/lhash.c

bernd-edlinger

Nice.

bernd-edlinger · 2022-01-12T11:02:09Z

the test case is master only stuff.

Doing the tsan operations under lock would be difficult to arrange here (locks require memory allocation).

Most of the DRGB code is run under lock from the EVP layer. This is relied on to make the majority of TSAN operations safe. However, it is still necessary to enable locking for all DRBGs created.

Not all platforms support tsan operations, those that don't need to have an alternative locking path. Fixes openssl#17447

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Doing the tsan operations under lock would be difficult to arrange here (locks require memory allocation). Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Most of the DRGB code is run under lock from the EVP layer. This is relied on to make the majority of TSAN operations safe. However, it is still necessary to enable locking for all DRBGs created. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Not all platforms support tsan operations, those that don't need to have an alternative locking path. Fixes #17447 Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

paulidale · 2022-01-13T10:55:18Z

Merged to master. Manually cherry picked to 3.0 (needed to omit one test and the object change).
Thanks for the review.

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Doing the tsan operations under lock would be difficult to arrange here (locks require memory allocation). Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

Most of the DRGB code is run under lock from the EVP layer. This is relied on to make the majority of TSAN operations safe. However, it is still necessary to enable locking for all DRBGs created. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

paulidale added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Jan 12, 2022

paulidale self-assigned this Jan 12, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 12, 2022

bernd-edlinger approved these changes Jan 12, 2022

View reviewed changes

crypto/mem.c Outdated Show resolved Hide resolved

bernd-edlinger requested changes Jan 12, 2022

View reviewed changes

bernd-edlinger reviewed Jan 12, 2022

View reviewed changes

crypto/lhash/lhash.c Outdated Show resolved Hide resolved

paulidale marked this pull request as ready for review January 12, 2022 09:30

bernd-edlinger approved these changes Jan 12, 2022

View reviewed changes

bernd-edlinger added the approval: done This pull request has the required number of approvals label Jan 12, 2022

paulidale mentioned this pull request Jan 13, 2022

ssl: TSAN fixes when TSAN requires locking #17489

Closed

2 tasks

paulidale force-pushed the tsan branch from 5fac4fd to 296ebf6 Compare January 13, 2022 09:47

paulidale added 8 commits January 13, 2022 20:49

threadstest: add write check to lock checking

0e24e9e

tsan: make detecting the need for locking when using tsan easier

716e2e0

core namemap: use updated tsan lock detection capabilities

a247ce7

object: use updated tsan lock detection capabilities

1aa0561

mem: do not produce usage counts when tsan is unavailable.

ed4713d

Doing the tsan operations under lock would be difficult to arrange here (locks require memory allocation).

lhash: use lock when TSAN not available for statistics gathering

daefcfc

drbg: add handling for cases where TSAN isn't available

5112c33

Most of the DRGB code is run under lock from the EVP layer. This is relied on to make the majority of TSAN operations safe. However, it is still necessary to enable locking for all DRBGs created.

threadstest: use locking for tsan operations if required

1dcc416

Not all platforms support tsan operations, those that don't need to have an alternative locking path. Fixes openssl#17447

paulidale force-pushed the tsan branch from 296ebf6 to 1dcc416 Compare January 13, 2022 09:49

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

threadstest: add write check to lock checking

1fc9780

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

tsan: make detecting the need for locking when using tsan easier

e22cbe5

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

core namemap: use updated tsan lock detection capabilities

d8ed9e4

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

object: use updated tsan lock detection capabilities

e6b8f35

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

lhash: use lock when TSAN not available for statistics gathering

43f1327

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

paulidale closed this Jan 13, 2022

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

threadstest: add write check to lock checking

3517a3e

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

tsan: make detecting the need for locking when using tsan easier

d1ec059

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

core namemap: use updated tsan lock detection capabilities

cc05c3e

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

openssl-machine pushed a commit that referenced this pull request Jan 13, 2022

lhash: use lock when TSAN not available for statistics gathering

d1a488e

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #17479)

paulidale deleted the tsan branch May 9, 2022 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsan: make detecting the need for locking when using tsan easier #17479

tsan: make detecting the need for locking when using tsan easier #17479

paulidale commented Jan 12, 2022

paulidale commented Jan 12, 2022

bernd-edlinger left a comment

bernd-edlinger left a comment

bernd-edlinger commented Jan 12, 2022

bernd-edlinger left a comment

bernd-edlinger commented Jan 12, 2022

paulidale commented Jan 13, 2022

tsan: make detecting the need for locking when using tsan easier #17479

tsan: make detecting the need for locking when using tsan easier #17479

Conversation

paulidale commented Jan 12, 2022

paulidale commented Jan 12, 2022

bernd-edlinger left a comment

Choose a reason for hiding this comment

bernd-edlinger left a comment

Choose a reason for hiding this comment

bernd-edlinger commented Jan 12, 2022

bernd-edlinger left a comment

Choose a reason for hiding this comment

bernd-edlinger commented Jan 12, 2022

paulidale commented Jan 13, 2022