Re: [PATCH v1 7/7] tools/ocaml/libs/eventchn: do not leak event channels and OCaml 5.0 compat

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 29/07/2022 18:53, Edwin Török wrote:
>> Add a finalizer on the event channel value, so that it calls
>> `xenevtchn_close` when the value would be GCed.
>> 
>> In practice oxenstored seems to be the only user of this,
>> and it creates a single global event channel only,
>> but freeing this could still be useful when run with OCAMLRUNPARAM=c
>> 
>> The code was previously casting a C pointer to an OCaml value,
>> which should be avoided: OCaml 5.0 won't support it.
>> (all "naked" C pointers must be wrapped inside an OCaml value,
>> either an Abstract tag, or Nativeint, see the manual
>> https://ocaml.org/manual/intfc.html#ss:c-outside-head)
>> 
>> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
> 
> So while this looks like a good improvement, don't we need to do it for
> all libraries, not just evtchn?
> 
> It doesn't look as if Ocaml 5.0 is very far away.

There are 2 ways to make it safe: use a block with Abstract_tag, or a block 
with Custom_tag:
https://v2.ocaml.org/manual/intfc.html#ss:c-outside-head

(technically it only ever was safe to use naked pointers for word-aligned 
pointers previously, although malloc usually
 ensures some minimal alignment).

There is a mode where the runtime can warn on stderr whenever naked pointers 
are used (there is an opam switch for it,
or one can specify a flag during the compiler build time), but it only does so 
when that codepath is hit,
not at build/link time.

A quick look at the libs:
* libs/mmap: uses Abstract_tag
* eventchn: fixed here to use Custom_tag
* libs/xc: needs fixing, stub_xc_interface_open
* libs/xl: uses Custom_tag (although it has other known issues (it should never 
use caml_callbackN directly, but use caml_callbackN_exn, see 
https://v2.ocaml.org/manual/intfc.html#ss:c-callbacks)
* libs/xentoollog: uses Custom_tag

So we need a patch for libs/xc.

> 
>> ---
>> tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
>> 1 file changed, 27 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c 
>> b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
>> index f889a7a2e4..c0d57e2954 100644
>> --- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
>> +++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
>> @@ -33,7 +33,30 @@
>> #include <caml/fail.h>
>> #include <caml/signals.h>
>> 
>> -#define _H(__h) ((xenevtchn_handle *)(__h))
>> +/* We want to close the event channel when it is no longer in use,
>> +   which can only be done safely with a finalizer.
>> +   Event channels are typically long lived, so we don't need tighter 
>> control over resource deallocation.
>> +   Use a custom block
>> +*/
>> +
>> +/* Access the xenevtchn_t* part of the OCaml custom block */
>> +#define _H(__h) (*((xenevtchn_handle**)Data_custom_val(__h)))
>> +
>> +static void stub_evtchn_finalize(value v)
>> +{
>> +    /* docs say to not use any CAMLparam* macros here */
>> +    xenevtchn_close(_H(v));
>> +}
>> +
>> +static struct custom_operations xenevtchn_ops = {
>> +    "xenevtchn",
>> +    stub_evtchn_finalize,
>> +    custom_compare_default, /* raises Failure, cannot compare */
>> +    custom_hash_default, /* ignored */
>> +    custom_serialize_default, /* raises Failure, can't serialize */
>> +    custom_deserialize_default, /* raises Failure, can't deserialize */
>> +    custom_compare_ext_default /* raises Failure */
> 
> This wants to gain a trailing comma.
> 
>> +};
>> 
>> CAMLprim value stub_eventchn_init(void)
>> {
>> @@ -48,7 +71,9 @@ CAMLprim value stub_eventchn_init(void)
>>      if (xce == NULL)
>>              caml_failwith("open failed");
>> 
>> -    result = (value)xce;
>> +    /* contains file descriptors, trigger full GC at least every 128 
>> allocations */
>> +    result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 1, 128);
> 
> The memory allocated for xce is tiny (48 bytes) and invariant for the
> lifetime of the evtchn object, which we've already established tends to
> operate as a singleton anyway.
> 
> Don't we want to use the recommended 0 and 1 here?

It is not just about the memory itself, but also about the file descriptors: 
those are a limited resource,
and if we use the 0 and 1 it means that this will be garbage collected very 
infrequently since the allocation itself is very small,
and you could potentially run out of file descriptors if you keep opening new 
event channels.
Notice there is no API for the user to close the event channel, so it has to 
rely on the GC, which is not ideal.

The mirage version of the eventchn lib does provide a close function: 
https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.mli,
although its implementation just leaks it (to avoid use-after-free): 
https://github.com/mirage/ocaml-evtchn/blob/main/lib/eventchn.ml#L42

Are event channel always expected to be singletons, is there a valid use case 
where you'd want more than 1 event channel/process?

Best regards,
--Edwin