Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16659 )
Change subject: KUDU-3210 Add lock before verifying signature ...................................................................... Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/16659/8/src/kudu/security/crypto.cc File src/kudu/security/crypto.cc: http://gerrit.cloudera.org:8080/#/c/16659/8/src/kudu/security/crypto.cc@145 PS8, Line 145: #if OPENSSL_VERSION_NUMBER < 0x10100000L : std::unique_lock<std::mutex> l(mutex); : #endif I'm not sure to be honest. I just did some perf testing and don't really see a difference. These tests were run on CentOS 7.3 and OpenSSL 1.0.1e-fips with FIPS mode disabled. "Before" is the key size increase patch, "after" is this patch. security-unknown-tsk-itest, all_types-itest and delete-table-itest have been my "canaries" during the FIPS testing work, these pretty consistently failed without the additional locking. SecurityUnknownTskTest.ErrorUnavailableDuringWorkload was additionally run in slow mode as it verifies signatures on multiple threads, token-test uses verify directly. Before (c9d01e8a1): Performance counter stats for './bin/security-unknown-tsk-itest' (5 runs): 11691.395220 task-clock (msec) # 0.784 CPUs utilized ( +- 4.30% ) 25,326 context-switches # 0.002 M/sec ( +- 2.25% ) 1,665 cpu-migrations # 0.142 K/sec ( +- 8.31% ) 29,932 page-faults # 0.003 M/sec ( +- 1.92% ) 17,773,601,853 cycles # 1.520 GHz ( +- 4.25% ) 28,342,927,657 instructions # 1.59 insn per cycle ( +- 2.29% ) 2,481,776,344 branches # 212.274 M/sec ( +- 4.26% ) 57,692,823 branch-misses # 2.32% of all branches ( +- 1.14% ) 14.912247426 seconds time elapsed ( +- 1.31% ) Performance counter stats for './bin/token-test' (5 runs): 3640.942265 task-clock (msec) # 0.233 CPUs utilized ( +- 4.47% ) 72 context-switches # 0.020 K/sec ( +- 0.68% ) 0 cpu-migrations # 0.000 K/sec ( +-100.00% ) 3,773 page-faults # 0.001 M/sec ( +- 2.53% ) 9,667,396,264 cycles # 2.655 GHz ( +- 4.07% ) 13,062,699,695 instructions # 1.35 insn per cycle ( +- 3.63% ) 497,520,881 branches # 136.646 M/sec ( +- 3.69% ) 2,997,835 branch-misses # 0.60% of all branches ( +- 1.31% ) 15.642611599 seconds time elapsed ( +- 1.04% ) Performance counter stats for './bin/security-unknown-tsk-itest --gtest_filter=SecurityUnknownTskTest.ErrorUnavailableDuringWorkload' (5 runs): 273538.239125 task-clock (msec) # 4.592 CPUs utilized ( +- 5.79% ) 209,412 context-switches # 0.766 K/sec ( +- 5.07% ) 11,997 cpu-migrations # 0.044 K/sec ( +- 5.45% ) 182,309 page-faults # 0.666 K/sec ( +- 8.52% ) 446,964,504,578 cycles # 1.634 GHz ( +- 6.06% ) 578,464,680,765 instructions # 1.29 insn per cycle ( +- 6.03% ) 93,909,196,404 branches # 343.313 M/sec ( +- 6.41% ) 1,056,063,188 branch-misses # 1.12% of all branches ( +- 5.60% ) 59.574074627 seconds time elapsed ( +- 3.77% ) Performance counter stats for './bin/all_types-itest' (5 runs): 70101.956008 task-clock (msec) # 1.273 CPUs utilized ( +- 0.96% ) 125,133 context-switches # 0.002 M/sec ( +- 0.26% ) 28,947 cpu-migrations # 0.413 K/sec ( +- 0.22% ) 1,677,875 page-faults # 0.024 M/sec ( +- 0.21% ) 134,147,072,319 cycles # 1.914 GHz ( +- 1.47% ) 161,856,596,607 instructions # 1.21 insn per cycle ( +- 1.52% ) 20,479,057,242 branches # 292.132 M/sec ( +- 0.91% ) 347,110,267 branch-misses # 1.69% of all branches ( +- 0.95% ) 55.072944706 seconds time elapsed ( +- 0.83% ) Performance counter stats for './bin/delete_table-itest' (5 runs): 115744.995670 task-clock (msec) # 1.584 CPUs utilized ( +- 0.96% ) 240,068 context-switches # 0.002 M/sec ( +- 0.36% ) 39,867 cpu-migrations # 0.344 K/sec ( +- 0.37% ) 2,252,100 page-faults # 0.019 M/sec ( +- 0.18% ) 224,610,715,212 cycles # 1.941 GHz ( +- 1.02% ) 226,900,191,280 instructions # 1.01 insn per cycle ( +- 1.09% ) 29,829,059,526 branches # 257.714 M/sec ( +- 0.93% ) 624,169,574 branch-misses # 2.09% of all branches ( +- 0.51% ) 73.050351263 seconds time elapsed ( +- 1.04% ) After (fd7cb7c53): Performance counter stats for './bin/security-unknown-tsk-itest' (5 runs): 11353.379747 task-clock (msec) # 0.753 CPUs utilized ( +- 2.61% ) 25,204 context-switches # 0.002 M/sec ( +- 1.46% ) 1,612 cpu-migrations # 0.142 K/sec ( +- 6.38% ) 29,595 page-faults # 0.003 M/sec ( +- 0.68% ) 16,811,249,774 cycles # 1.481 GHz ( +- 3.07% ) 27,103,399,703 instructions # 1.61 insn per cycle ( +- 1.91% ) 2,361,932,643 branches # 208.038 M/sec ( +- 3.20% ) 56,890,752 branch-misses # 2.41% of all branches ( +- 1.10% ) 15.081628058 seconds time elapsed ( +- 0.64% ) Performance counter stats for './bin/token-test' (5 runs): 3011.291660 task-clock (msec) # 0.201 CPUs utilized ( +- 7.73% ) 81 context-switches # 0.027 K/sec ( +- 5.09% ) 0 cpu-migrations # 0.000 K/sec ( +- 61.24% ) 3,976 page-faults # 0.001 M/sec ( +- 2.42% ) 7,761,301,820 cycles # 2.577 GHz ( +- 6.70% ) 10,651,976,922 instructions # 1.37 insn per cycle ( +- 6.45% ) 411,642,877 branches # 136.700 M/sec ( +- 6.25% ) 2,521,605 branch-misses # 0.61% of all branches ( +- 5.81% ) 15.013104478 seconds time elapsed ( +- 1.55% ) Performance counter stats for './bin/security-unknown-tsk-itest --gtest_filter=SecurityUnknownTskTest.ErrorUnavailableDuringWorkload' (5 runs): 175115.356193 task-clock (msec) # 3.036 CPUs utilized ( +- 3.91% ) 167,088 context-switches # 0.954 K/sec ( +- 3.20% ) 9,012 cpu-migrations # 0.051 K/sec ( +- 4.64% ) 102,040 page-faults # 0.583 K/sec ( +- 5.94% ) 273,941,630,023 cycles # 1.564 GHz ( +- 4.12% ) 346,251,669,147 instructions # 1.26 insn per cycle ( +- 4.20% ) 53,977,706,057 branches # 308.241 M/sec ( +- 4.76% ) 701,071,352 branch-misses # 1.30% of all branches ( +- 3.63% ) 57.674038243 seconds time elapsed ( +- 5.35% ) Performance counter stats for './bin/all_types-itest' (5 runs): 70566.308078 task-clock (msec) # 1.276 CPUs utilized ( +- 0.64% ) 125,382 context-switches # 0.002 M/sec ( +- 0.44% ) 28,820 cpu-migrations # 0.408 K/sec ( +- 0.42% ) 1,667,782 page-faults # 0.024 M/sec ( +- 0.08% ) 134,668,824,022 cycles # 1.908 GHz ( +- 0.89% ) 161,832,686,469 instructions # 1.20 insn per cycle ( +- 1.21% ) 20,324,235,692 branches # 288.016 M/sec ( +- 0.65% ) 344,727,471 branch-misses # 1.70% of all branches ( +- 0.62% ) 55.312051723 seconds time elapsed ( +- 1.52% ) Performance counter stats for './bin/delete_table-itest' (5 runs): 117756.620111 task-clock (msec) # 1.578 CPUs utilized ( +- 1.06% ) 241,456 context-switches # 0.002 M/sec ( +- 0.39% ) 40,255 cpu-migrations # 0.342 K/sec ( +- 0.46% ) 2,258,647 page-faults # 0.019 M/sec ( +- 0.21% ) 227,969,750,561 cycles # 1.936 GHz ( +- 0.95% ) 230,782,693,669 instructions # 1.01 insn per cycle ( +- 1.01% ) 30,536,932,250 branches # 259.322 M/sec ( +- 0.93% ) 634,297,040 branch-misses # 2.08% of all branches ( +- 0.64% ) 74.645221815 seconds time elapsed ( +- 0.67% ) > If I'm not mistaken, we haven't seen the race manifesting itself in other but > FIPS-enabled OpenSSL environments yet, and it's been few years of using this > code. I don't think we have, but as clients retry, we can't be sure this never happened in the wild. We only know that this has never been reported and we didn't run into this in our testing, or at least not enough to notice. OpenSSL 1.0.2, unfortunately, doesn't guarantee thread safety ("Note that OpenSSL is not completely thread-safe, and unfortunately not all global resources have the necessary locks."[1]), so it's possible this is a problem on other versions too. In the versions where this works correctly, I assume there is a lock on signature verification, which is missing in some other versions. As it seems this locking doesn't hurt performance (in the thread-safe versions there's probably a lock there anyway), I'd rather not add custom FIPS-mode-only locking here, but I'm not 100% sure about this. I'm definitely open to hearing other perspectives. [1] https://www.openssl.org/docs/man1.0.2/man3/threads.html -- To view, visit http://gerrit.cloudera.org:8080/16659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifafc7dcf91db910123276b657515e410bb7f6fcd Gerrit-Change-Number: 16659 Gerrit-PatchSet: 12 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Mon, 02 Nov 2020 17:02:54 +0000 Gerrit-HasComments: Yes
