Skip to content

Commit 137160e

Browse files
authored
CA-400199: open /dev/urandom on first use and use an input channel to reduce number of syscalls (#6085)
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 +++ ```
2 parents 8435a4e + 2c99f2d commit 137160e

File tree

6 files changed

+33
-55
lines changed

6 files changed

+33
-55
lines changed

ocaml/forkexecd/test/fe_test.ml

+1-1
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ let slave = function
292292
(*
293293
Printf.fprintf stderr "%s %d\n" total_fds (List.length present - 1)
294294
*)
295-
if total_fds + 1 (* Uuid.dev_urandom *) <> List.length filtered then
295+
if total_fds <> List.length filtered then
296296
fail "Expected %d fds; /proc/self/fd has %d: %s" total_fds
297297
(List.length filtered) ls
298298

ocaml/libs/uuid/uuidx.ml

+31-40
Original file line numberDiff line numberDiff line change
@@ -116,48 +116,39 @@ let is_uuid str = match of_string str with None -> false | Some _ -> true
116116

117117
let dev_urandom = "/dev/urandom"
118118

119-
let dev_urandom_fd = Unix.openfile dev_urandom [Unix.O_RDONLY] 0o640
120-
(* we can't close this in at_exit, because Crowbar runs at_exit, and
121-
it'll fail because this FD will then be closed
122-
*)
123-
124-
let read_bytes dev n =
125-
let buf = Bytes.create n in
126-
let read = Unix.read dev buf 0 n in
127-
if read <> n then
128-
raise End_of_file
129-
else
130-
Bytes.to_string buf
131-
132-
let make_uuid_urnd () = of_bytes (read_bytes dev_urandom_fd 16) |> Option.get
133-
134-
(* State for random number generation. Random.State.t isn't thread safe, so
135-
only use this via with_non_csprng_state, which takes care of this.
136-
*)
137-
let rstate = Random.State.make_self_init ()
138-
139-
let rstate_m = Mutex.create ()
140-
141-
let with_non_csprng_state =
142-
(* On OCaml 5 we could use Random.State.split instead,
143-
and on OCaml 4 the mutex may not be strictly needed
144-
*)
145-
let finally () = Mutex.unlock rstate_m in
146-
fun f ->
147-
Mutex.lock rstate_m ;
148-
Fun.protect ~finally (f rstate)
149-
150-
(** Use non-CSPRNG by default, for CSPRNG see {!val:make_uuid_urnd} *)
151-
let make_uuid_fast () = with_non_csprng_state Uuidm.v4_gen
152-
153-
let make_default = ref make_uuid_urnd
154-
155-
let make () = !make_default ()
119+
let generate =
120+
let mutex = Mutex.create () in
121+
let dev_urandom_ic = ref None in
122+
let finally () = Mutex.unlock mutex in
123+
let with_mutex fn = Mutex.lock mutex ; Fun.protect ~finally fn in
124+
let close_ic () =
125+
with_mutex @@ fun () ->
126+
!dev_urandom_ic |> Option.iter close_in_noerr ;
127+
dev_urandom_ic := None
128+
in
129+
fun n ->
130+
with_mutex @@ fun () ->
131+
let ic =
132+
match !dev_urandom_ic with
133+
| None ->
134+
let ic = open_in_bin dev_urandom in
135+
at_exit close_ic ;
136+
dev_urandom_ic := Some ic ;
137+
ic
138+
| Some ic ->
139+
ic
140+
in
141+
really_input_string ic n
142+
143+
let make_uuid_urnd () = of_bytes (generate 16) |> Option.get
144+
145+
let make_uuid_fast = make_uuid_urnd
146+
147+
let make = make_uuid_urnd
156148

157149
let make_v7_uuid_from_parts time_ns rand_b = Uuidm.v7_ns ~time_ns ~rand_b
158150

159-
let rand64 () =
160-
with_non_csprng_state (fun rstate () -> Random.State.bits64 rstate)
151+
let rand64 () = String.get_int64_ne (generate 8) 0
161152

162153
let now_ns =
163154
let start = Mtime_clock.counter () in
@@ -174,7 +165,7 @@ let make_v7_uuid () = make_v7_uuid_from_parts (now_ns ()) (rand64 ())
174165
type cookie = string
175166

176167
let make_cookie () =
177-
read_bytes dev_urandom_fd 64
168+
generate 64
178169
|> String.to_seq
179170
|> Seq.map (fun c -> Printf.sprintf "%1x" (int_of_char c))
180171
|> List.of_seq

ocaml/libs/uuid/uuidx.mli

-5
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,3 @@ module Hash : sig
194194
(* UUID Version 5 derived from argument string and namespace UUID *)
195195
val string : string -> [< not_secret] t
196196
end
197-
198-
(**/**)
199-
200-
(* just for feature flag, to be removed *)
201-
val make_default : (unit -> [< not_secret] t) ref

ocaml/tests/bench/bench_uuid.ml

-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
open Bechamel
22

3-
let () = Uuidx.make_default := Uuidx.make_uuid_fast
4-
53
let benchmarks =
64
Test.make_grouped ~name:"uuidx creation"
75
[

ocaml/xapi/xapi_globs.ml

-6
Original file line numberDiff line numberDiff line change
@@ -1612,12 +1612,6 @@ let other_options =
16121612
, (fun () -> string_of_bool !disable_webserver)
16131613
, "Disable the host webserver"
16141614
)
1615-
; ( "use-prng-uuid-gen"
1616-
(* eventually this'll be the default, except for Sessions *)
1617-
, Arg.Unit (fun () -> Uuidx.make_default := Uuidx.make_uuid_fast)
1618-
, (fun () -> !Uuidx.make_default == Uuidx.make_uuid_fast |> string_of_bool)
1619-
, "Use PRNG based UUID generator instead of CSPRNG"
1620-
)
16211615
]
16221616

16231617
(* The options can be set with the variable xapiflags in /etc/sysconfig/xapi.

quality-gate.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ mli-files () {
4040
}
4141

4242
structural-equality () {
43-
N=10
43+
N=9
4444
EQ=$(git grep -r --count ' == ' -- '**/*.ml' ':!ocaml/sdk-gen/**/*.ml' | cut -d ':' -f 2 | paste -sd+ - | bc)
4545
if [ "$EQ" -eq "$N" ]; then
4646
echo "OK counted $EQ usages of ' == '"

0 commit comments

Comments
 (0)