Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SARIF test #2

Open
wants to merge 75 commits into
base: master
Choose a base branch
from
Open

SARIF test #2

wants to merge 75 commits into from

Conversation

edwintorok
Copy link
Owner

No description provided.

psafont and others added 22 commits January 5, 2023 14:49
@psafont
xenopsd: use uuid instead of deprecated uuidm functions
2acdc29 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
message-switch: conform to new APIs in jst libs
e833182 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
xapi-storage-script: conform to new APIs in jst libs
80ee829 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
xen-api-client: conform to new APIs in jst libs
2ee0819 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
stream_vdi, import: conform to new APIs in tar
0604f31 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
gencert: conform to new APIs in x509
2ce7645 
Now keys using elliptical curve cryptography need to be taken into
account. Keep the same behaviour as previously by rejecting them

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
xapi-guard, xen-api-client: conform to new APIs in conduit
8b4d026 
The context is now is lazy-loaded, so force it's loading.

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
message-switch, vhd-tool: drop io-page-unix
bb1e9ec 
Now io-page is compiled using dune and it's not split into two.

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
nbd: change ocaml-nbd usage
661d682 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont
session_check: add action name in the error returned
efe5654 
Makes it easier to spot which call failed this check when triaging
related issues

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont @edwintorok
xenctrlext_stubs: fix xfm_open parameter mismatch
d30d352 
The parameter open_flags was missing from the definition. Since it's
actually unused by the underlying implementation its meaning is
undefined. Remove the parameter and pass 0 to it.

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
@edwintorok
vhd-tool/direct_copy_stubs: fix setting of O_DIRECT flag
c61d2ad 
If a file descriptor is open in the normal way then its flags will be 0,
and the code here that attempts to enable O_DIRECT will do nothing.

This appears to have been an incorrect way of inverting an error condition
`ret < 0`, the negation of which is `ret >= 0`, and not `ret > 0`.

We open the file with O_DIRECT elsewhere though (in vhd-format), but fix this
code to avoid confusion.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
Makefile: add a rule to write out a compile_flags.txt
e72966b 
compile_flags.txt (or compile_commands.json) can be used by C language servers,
like 'clangd' to provide in-editor feedback about C code compile
warnings/errors (similar to ocaml-lsp).

By default it wouldn't know where to find the OCaml includes and show a lot of
warnings/errors when editing C stubs, and with this file it works.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
direct_copy_stubs.c: uerror is available in caml/unixsupport.h
b49a2b4 
There is no need to declare it by hand (it also has a slightly different
prototype taking a 'const char*' as argument).

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
add .editorconfig: use spaces instead of tabs in C files
4390fab 
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
tuntap_stubs.c: raise Unix.error instead of failwith
d8b7031 
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
xenctrlext_stubs.c: xc_get_last_error is not thread safe, use just er…
4a7eec6 
…rno which is

It is usually set to XC_INTERNAL_ERROR, except in xenguest, but we have no
bindings for that.

This API might get deleted in upstream Xen too: the error code is stored in the
xenctrl handle, but we made that global per process, and shared among all
threads. Which means that xenctrl calls from different threads can overwrite
each-others' error code.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
xa_auth_stubs.c: move free inside the blocking section
7987d71 
This is a C call, and might benefit from a bit more parallelism if it inside
the blocking section.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
xenctrlext_stubs.c: add missing enter/leave blocking section
1e02ea2 
Most of these calls pass only integers or pointers allocated in C,
so it is safe to release the OCaml runtime lock.

Replace direct passing of Xfm_val/Caml_ba_data_val with storing in a temporary
local variable (the result of Xfm_val or Caml_ba_data_val won't move since they
are C pointers, but the OCaml value passed in as params to these macros might!)

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
unixpwd_stubs.c: factor out common code and use enter/leave blocking …
ed21d97 
…section where possible

unshadow is not thread safe, but the other ones should be.

TODO: double check all C API calls in unixpwd.c with the MT-safe portion of the manpage

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
C stubs: add and use .clang-format
57c6d2b 
We may intend to upstream the xenctrlext stubs to Xen, so it should follow
the Xen CODING_STYLE.
the .clang-format here is based on the version I sent upstream that tries to
follow that coding style.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok
dbgring: switch from Xenmmap to Gnt + Io_page
10ff86e 
Xenmmap.read is being removed from Xen, use upstream mirage libs to access this
instead.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
edwintorok and others added 7 commits January 10, 2023 15:17
@edwintorok
xe-toolstack-restart: stop and start all services at once
f6eceeb 
Starting all services at once is what happens on boot, so this should
work.

We currently stop services one at a time, and if they get stuck (e.g.
due to waiting on a network filesystem that is stuck) then each has to
hit the 1m30s timeout in systemd before they get killed, which is a
minimum of ~32m before the toolstack is stopped.

Which is a lot higher than the default 2 minute HA timeout, and even if
the XAPI watcher would attempt recovery by restarting XAPI (or the
toolstack), it would be stuck waiting for this 30m process to complete
first.

Stop all services at once and then start all needed ones at once.
(Don't attempt to restart all in a single command since that might
result in some services restarting and immediately seeing another
service being restarted, interrupting communication and potentially
leading to errors).

Systemd can then use existing service dependencies to order the
restarts.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@psafont @edwintorok
schematest: add better instructions
03788e3 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@edwintorok
WIP: PVH
a51711a 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@edwintorok
WiP
d5794f7
@edwintorok
wip
611956e
@edwintorok
wip
764ab12
@edwintorok
wip
918ab89
edwintorok added a commit that referenced this pull request Oct 10, 2023
@edwintorok
Workaround: avoid sleep(1) call by crypt_r
c916e7d 
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",
 xapi-project#5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
 xapi-project#6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
 xapi-project#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
 xapi-project#8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
 xapi-project#9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
 xapi-project#10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
 xapi-project#11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
 xapi-project#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
 xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) at xa_auth_stubs.c:42
 xapi-project#14 0x00000000004a0a6a in camlDune__exe__Bench_pam__pam_authenticate$27_320 () at ocaml/tests/bench/pam/bench_pam.ml:63
 xapi-project#15 0x00000000004a1113 in camlEzbechamel_concurrent__worker_loop_359 () at ocaml/tests/bench/lib/concurrent/ezbechamel_concurrent.ml:36
 xapi-project#16 0x00000000005935b9 in camlStdlib__Fun__protect_317 ()
 xapi-project#17 0x00000000004a1955 in camlThread__fun_850 ()
 xapi-project#18 0x00000000005d6401 in caml_start_program ()
 xapi-project#19 0x00000000005cd0fd in caml_callback_exn ()
 xapi-project#20 0x00000000005af810 in caml_thread_start ()
 xapi-project#21 0x00007ffff79b7e25 in start_thread (arg=0x7ffff31e2700) at pthread_create.c:308
 xapi-project#22 0x00007ffff71dbbad in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:113
```

`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`).

To avoid this link with `libcrypt` and call `crypt_r` once ourselves (and ensure it loads `libfreeblpriv3` by using the sha512 prefix).
That way the library will stay loaded (we'll hold a reference count on it), and the `dlclose` done by PAM won't unload it.

Confirmed that there are no `sleep` calls now, and the results are also visible when running the benchmark targeted to the with and without fix code:
```
╭─────────────────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮
│name                                             │  major-allocated          │  minor-allocated          │  monotonic-clock          │
├─────────────────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤
│  concurrent authenticate (sleep fix, actual):8  │             0.0000 mjw/run│            50.0000 mnw/run│       27043467.0000 ns/run│
╰─────────────────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯

╭────────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮
│name                                    │  major-allocated          │  minor-allocated          │  monotonic-clock          │
├────────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤
│  concurrent authenticate (no reuse):8  │             0.0000 mjw/run│            50.0000 mnw/run│     1029831372.0000 ns/run│
╰────────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯
```

Without this fix using 2 threads to perform PAM authentication would result in a 38x slowdown compared to using no threads at all (which is what XAPI currently does).

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
edwintorok added a commit that referenced this pull request Oct 11, 2023
@edwintorok
Pam: avoid sleep(1) call when multithreaded
d0d690c 
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",
  xapi-project#5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
  xapi-project#6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
  xapi-project#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
  xapi-project#8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
  xapi-project#9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
  xapi-project#10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
  xapi-project#11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
  xapi-project#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
  xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) 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 <edwin.torok@cloud.com>
edwintorok added a commit that referenced this pull request Oct 11, 2023
@edwintorok
CP-43755: Pam: avoid sleep(1) call when multithreaded
ba49ad0 
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",
  xapi-project#5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
  xapi-project#6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
  xapi-project#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
  xapi-project#8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
  xapi-project#9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
  xapi-project#10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
  xapi-project#11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
  xapi-project#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
  xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) 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 <edwin.torok@cloud.com>
edwintorok added a commit that referenced this pull request Oct 12, 2023
@edwintorok
CP-43755: Pam: avoid sleep(1) call when multithreaded
3b52b72 
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",
  xapi-project#5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
  xapi-project#6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
  xapi-project#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
  xapi-project#8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
  xapi-project#9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
  xapi-project#10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
  xapi-project#11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
  xapi-project#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
  xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) 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 <edwin.torok@cloud.com>
@edwintorok edwintorok force-pushed the master branch 10 times, most recently from 9efab52 to aeeade1 Compare August 5, 2024 14:24
edwintorok added a commit that referenced this pull request Aug 28, 2024
@edwintorok @lindig
CP-43755: Pam: avoid sleep(1) call when multithreaded
8f355ea 
Backport of 3b52b72

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",
  xapi-project#5  0x00007ffff1b8d070 in __crypt_r (key=key@entry=0x7fffd8005a60 "pamtest-edvint", salt=<optimized out>,
  xapi-project#6  0x00007ffff1dc9abc in verify_pwd_hash (p=p@entry=0x7fffd8005a60 "pamtest-edvint", hash=<optimized out>, nullok=nullok@entry=0) at passverify.c:111
  xapi-project#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
  xapi-project#8  0x00007ffff1dc6556 in pam_sm_authenticate (pamh=0x7fffd8002910, flags=<optimized out>, argc=<optimized out>, argv=<optimized out>) at pam_unix_auth.c:178
  xapi-project#9  0x00007ffff7bcef1a in _pam_dispatch_aux (use_cached_chain=<optimized out>, resumed=<optimized out>, h=<optimized out>, flags=1, pamh=0x7fffd8002910) at pam_dispatch.c:110
  xapi-project#10 _pam_dispatch (pamh=pamh@entry=0x7fffd8002910, flags=1, choice=choice@entry=1) at pam_dispatch.c:426
  xapi-project#11 0x00007ffff7bce7e0 in pam_authenticate (pamh=0x7fffd8002910, flags=flags@entry=1) at pam_auth.c:34
  xapi-project#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
  xapi-project#13 0x00000000005adf20 in stub_XA_mh_authorize (username=<optimized out>, password=<optimized out>) 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 <edwin.torok@cloud.com>
Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@edwintorok @psafont