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.

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

CA-400199: open /dev/urandom on first use and use an input channel to reduce number of syscalls #6085

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

edwintorok
Copy link
Contributor

This is an alternative to #6077

2 recent optimizations have changed the Uuidx module to open
/dev/urandom once on startup, instead of every time a value was
requested.

However 'networkd_db' runs in the installer environment, inside a chroot where /dev/urandom is not available.

Open /dev/urandom on first use instead.

Simplify the code and use a single implementation for both fast and secure urandom generation:

  • use a mutex to protect accesses to global urandom state
  • use an input channel, rather than a Unix file descriptor, this allows us to read many bytes in one go, and then generate multiple random numbers without having to make syscalls that often

(syscalls are slow in this case because they require releasing the runtime mutex, which gives another thread the opportunity to run for 50ms).

Fixes: a0176da ("CP-49135: open /dev/urandom just once")
Fixes: a2d9fbe ("IH-577 Implement v7 UUID generation")
Fixes: 6635a00 ("CP-49136: Introduce PRNG for generating non-secret UUIDs")

This is slightly slower than before, but still fast enough:

│  uuidx creation/Uuidx.make            │             0.0004 mjw/run│            16.0001 mnw/run│            105.8801 ns/run│
│  uuidx creation/Uuidx.make_uuid_urnd  │             0.0004 mjw/run│            16.0001 mnw/run│            105.1474 ns/run│

Previously this used to take ~88ns, so in fact the difference is barely noticable.

Also remove the feature flag: the previous change was feature flagged too, but broke master anyway (I wouldn't have though anything doesn't have /dev/urandom available, and didn't feature flag that part, because in general it is not possible to feature flag startup code without races).

networkd_db now doesn't try to open this anymore:

strace -e openat -e open networkd_db
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libpthread.so.0", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/librt.so.1", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libm.so.6", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libdl.so.2", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
open("/var/lib/xcp/networkd.db", O_RDONLY) = 3
+++ exited with 0 +++

@edwintorok edwintorok changed the title CA-400199: open /dev/urandom on first use CA-400199: open /dev/urandom on first use and use an input channel to reduce number of syscalls Oct 25, 2024
2 recent optimizations have changed the Uuidx module to open
  /dev/urandom once on startup, instead of every time a value was
  requested.

However 'networkd_db' runs in the installer environment, inside a chroot
where /dev/urandom is not available.

Open /dev/urandom on first use instead.

Simplify the code and use a single implementation for both fast and
secure urandom generation:
* use a mutex to protect accesses to global urandom state
* use an input channel, rather than a Unix file descriptor, this allows
  us to read many bytes in one go, and then generate multiple random
  numbers without having to make syscalls that often

(syscalls are slow in this case because they require releasing the
runtime mutex, which gives another thread the opportunity to run for
50ms).

Fixes: a0176da ("CP-49135: open /dev/urandom just once")
Fixes: a2d9fbe ("IH-577 Implement v7 UUID generation")
Fixes: 6635a00 ("CP-49136: Introduce PRNG for generating non-secret UUIDs")

This is slightly slower than before, but still fast enough:
```
│  uuidx creation/Uuidx.make            │             0.0004 mjw/run│            16.0001 mnw/run│            105.8801 ns/run│
│  uuidx creation/Uuidx.make_uuid_urnd  │             0.0004 mjw/run│            16.0001 mnw/run│            105.1474 ns/run│
```

Previously this used to take ~88ns, so in fact the difference is barely
noticable.

Also remove the feature flag: the previous change was feature flagged
too, but broke master anyway (I wouldn't have though anything *doesn't*
have /dev/urandom available, and didn't feature flag that part, because
in general it is not possible to feature flag startup code without races)

Signed-off-by: Edwin Török <[email protected]>

let make_uuid_urnd () = of_bytes (generate 16) |> Option.get

let make_uuid_fast = make_uuid_urnd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit pointless now, as there is only one "make" function. Can we just have

let make () = of_bytes (generate 16) |> Option.get

?

Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any problems with this, apart from the small simplification mentioned above. In general, I think it is better to not open files when a module is loaded in a library, as it can lead to unintended consequences (as we have seen), so this is an improvement.

@lindig
Copy link
Contributor

lindig commented Oct 28, 2024

Another problem with side effects in module initialisation is the somewhat unknown order in which modules are initialised.

@lindig lindig marked this pull request as ready for review October 28, 2024 14:45
@lindig lindig added this pull request to the merge queue Oct 29, 2024
Merged via the queue into xapi-project:master with commit 137160e Oct 29, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants