From d0d690c89dee8cd53b468670a6006d1296674009 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= Date: Wed, 11 Oct 2023 17:14:50 +0100 Subject: [PATCH] Pam: avoid sleep(1) call when multithreaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This enables PAM to be used in multithreaded mode (currently XAPI has a global lock around auth). Using an off-cpu flamegraph I identified that concurrent PAM calls are slow due to a call to `sleep(1)`. `pam_authenticate` calls `crypt_r` which calls `NSSLOW_Init` which on first use will try to initialize the just `dlopen`-ed library. If it encounters a race condition it does a `sleep(1)`. This race condition can be quite reliably reproduced when performing a lot of PAM authentications from multiple threads in parallel. GDB can also be used to confirm this by putting a breakpoint on `sleep`: ``` #0 __sleep (seconds=seconds@entry=1) at ../sysdeps/unix/sysv/linux/sleep.c:42 #1 0x00007ffff1548e22 in freebl_RunLoaderOnce () at lowhash_vector.c:122 #2 0x00007ffff1548f31 in freebl_InitVector () at lowhash_vector.c:131 #3 NSSLOW_Init () at lowhash_vector.c:148 #4 0x00007ffff1b8f09a in __sha512_crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=0x7ffff31e17b8 "dIJbsXKc0", #5 0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=, #6 0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=, nullok=nullok@entry=0) at passverify.c:111 #7 0x00007ffff1dc9139 in _unix_verify_password (pamh=pamh@entry=0x7fffd8002910, name=0x7fffd8002ab0 "pamtest-edvint", p=0x7fffd8005a60 "pamtest-edvint", ctrl=ctrl@entry=8389156) at support.c:777 #8 0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=, argc=, argv=) at pam_unix_auth.c:178 #9 0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=, resumed=, h=, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110 #10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426 #11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34 #12 0x00000000005ae567 in XA_mh_authorize (username=username@entry=0x7fffd80028d0 "pamtest-edvint", password=password@entry=0x7fffd80028f0 "pamtest-edvint", error=error@entry=0x7ffff31e1be8) at xa_auth.c:83 #13 0x00000000005adf20 in stub_XA_mh_authorize (username=, password=) at xa_auth_stubs.c:42 ``` `pam_start` and `pam_end` doesn't help here, because on `pam_end` the library is `dlclose`-ed, so on next `pam_authenticate` it will have to go through the initialization code again. (This initialization code would've belonged into `pam_start`, not `pam_authenticate`, but there are several layers here including a call to `crypt_r`). Upstream has fixed this problem >5 years ago by switching to libxcrypt instead. Signed-off-by: Edwin Török --- ocaml/auth/dune | 2 +- ocaml/auth/xa_auth_stubs.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/ocaml/auth/dune b/ocaml/auth/dune index 8d6774ab817..f963fbb591b 100644 --- a/ocaml/auth/dune +++ b/ocaml/auth/dune @@ -4,7 +4,7 @@ (names xa_auth xa_auth_stubs) ) (name pam) - (c_library_flags -lpam) + (c_library_flags -lpam -lcrypt) (wrapped false) ) diff --git a/ocaml/auth/xa_auth_stubs.c b/ocaml/auth/xa_auth_stubs.c index 78dfb131eaf..6c6c7a5b915 100644 --- a/ocaml/auth/xa_auth_stubs.c +++ b/ocaml/auth/xa_auth_stubs.c @@ -11,8 +11,9 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU Lesser General Public License for more details. */ -/* - */ + +/* must be at the beginning, it affects defines in other headers that cannot be reenabled later */ +#define _GNU_SOURCE #include #include @@ -70,6 +71,30 @@ CAMLprim value stub_XA_mh_chpasswd(value username, value new_password){ CAMLreturn(ret); } +#include +/* 'constructor' attribute will ensure this function gets call early during program startup. */ +void __attribute__((constructor)) stub_XA_workaround(void) +{ + struct crypt_data data; + memset(&data, 0, sizeof(data)); + + /* Initialize and load crypt library used for password hashing. + This library is loaded and initialized at [pam_authenticate] time and not at [pam_start]. + If it detects a race condition (multiple threads with their own PAM contexts trying to call 'crypt_r'), + then it does [sleep 1] as can be seen in this call trace: + [pam_authenticate -> crypt_r -> __sha512_crypt_r -> freebl_InitVector -> freebl_RunLoaderOnce -> sleep]. + + As a workaround link with 'libcrypt' and call 'crypt_r' with a setting that will make it take tha sha512 route + to ensure that the library gets initialized while we're still single-threaded and stays loaded and initialized. + + '$6$' is the setting for sha512 according to crypt(5). + + Upstream has switched to using libxcrypt instead which doesn't have these problems, when we switch then + this workaround can be dropped. + */ + crypt_r("", "$6$", &data); +} + /* * Local variables: * mode: C