diff mbox series

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

Message ID 6e5fd9edfea379b69682fa538141298fc1bc3110.1659116941.git.edvin.torok@citrix.com (mailing list archive)
State New
Headers show
Series tools/ocaml code and build cleanups | expand

Commit Message

Edwin Torok July 29, 2022, 5:53 p.m. UTC
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@citrix.com>
---
 tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 29 +++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Christian Lindig Aug. 1, 2022, 8:20 a.m. UTC | #1
On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> 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

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
Andrew Cooper Aug. 5, 2022, 6:06 p.m. UTC | #2
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@citrix.com>

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.

> ---
>  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?

~Andrew
Edwin Torok Aug. 8, 2022, 8:28 a.m. UTC | #3
> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@citrix.com> 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@citrix.com>
> 
> 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
Andrew Cooper Aug. 8, 2022, 9:59 a.m. UTC | #4
On 08/08/2022 09:28, Edwin Torok wrote:
>> On 5 Aug 2022, at 19:06, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>
>> On 29/07/2022 18:53, Edwin Török wrote:
>>> +};
>>>
>>> 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?

Careful on terminology.  We're discussing an open /dev/xen/evtchn file
handle, upon which an arbitrary number of event channels are muliplexed.

There's no good reason to have more than one file handle open, and
there's no good reason to close it except during shutdown.

So 0 and 1 are probably the right values in this case.

~Andrew
diff mbox series

Patch

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 */
+};
 
 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);
+	_H(result) = xce;
 	CAMLreturn(result);
 }