Mailing List Archive

[xen stable-4.16] tools/ocaml/evtchn: Don't reference Custom objects with the GC lock released
commit e18faeb91e620624106b94c8821f8c9574eddb17
Author: Edwin Török <edwin.torok@cloud.com>
AuthorDate: Thu Jan 12 17:48:29 2023 +0000
Commit: Andrew Cooper <andrew.cooper3@citrix.com>
CommitDate: Thu Feb 9 15:58:51 2023 +0000

tools/ocaml/evtchn: Don't reference Custom objects with the GC lock released

The modification to the _H() macro for Ocaml 5 support introduced a subtle
bug. From the manual:

https://ocaml.org/manual/intfc.html#ss:parallel-execution-long-running-c-code

"After caml_release_runtime_system() was called and until
caml_acquire_runtime_system() is called, the C code must not access any OCaml
data, nor call any function of the run-time system, nor call back into OCaml
code."

Previously, the value was a naked C pointer, so dereferencing it wasn't
"accessing any Ocaml data", but the fix to avoid naked C pointers added a
layer of indirection through an Ocaml Custom object, meaning that the common
pattern of using _H() in a blocking section is unsafe.

In order to fix:

* Drop the _H() macro and replace it with a static inline xce_of_val().
* Opencode the assignment into Data_custom_val() in the two constructors.
* Rename "value xce" parameters to "value xce_val" so we can consistently
have "xenevtchn_handle *xce" on the stack, and obtain the pointer with the
GC lock still held.

Fixes: 22d5affdf0ce ("tools/ocaml/evtchn: OCaml 5 support, fix potential resource leak")
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
(cherry picked from commit 2636d8ff7a670c4d2485757dbe966e36c259a960)
---
tools/ocaml/libs/eventchn/xeneventchn_stubs.c | 60 ++++++++++++++++-----------
1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
index aa8a69cc1e..d7881ca95f 100644
--- a/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
+++ b/tools/ocaml/libs/eventchn/xeneventchn_stubs.c
@@ -33,11 +33,14 @@
#include <caml/fail.h>
#include <caml/signals.h>

-#define _H(__h) (*((xenevtchn_handle **)Data_custom_val(__h)))
+static inline xenevtchn_handle *xce_of_val(value v)
+{
+ return *(xenevtchn_handle **)Data_custom_val(v);
+}

static void stub_evtchn_finalize(value v)
{
- xenevtchn_close(_H(v));
+ xenevtchn_close(xce_of_val(v));
}

static struct custom_operations xenevtchn_ops = {
@@ -68,7 +71,7 @@ CAMLprim value stub_eventchn_init(value cloexec)
caml_failwith("open failed");

result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 0, 1);
- _H(result) = xce;
+ *(xenevtchn_handle **)Data_custom_val(result) = xce;

CAMLreturn(result);
}
@@ -87,18 +90,19 @@ CAMLprim value stub_eventchn_fdopen(value fdval)
caml_failwith("evtchn fdopen failed");

result = caml_alloc_custom(&xenevtchn_ops, sizeof(xce), 0, 1);
- _H(result) = xce;
+ *(xenevtchn_handle **)Data_custom_val(result) = xce;

CAMLreturn(result);
}

-CAMLprim value stub_eventchn_fd(value xce)
+CAMLprim value stub_eventchn_fd(value xce_val)
{
- CAMLparam1(xce);
+ CAMLparam1(xce_val);
CAMLlocal1(result);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
int fd;

- fd = xenevtchn_fd(_H(xce));
+ fd = xenevtchn_fd(xce);
if (fd == -1)
caml_failwith("evtchn fd failed");

@@ -107,13 +111,14 @@ CAMLprim value stub_eventchn_fd(value xce)
CAMLreturn(result);
}

-CAMLprim value stub_eventchn_notify(value xce, value port)
+CAMLprim value stub_eventchn_notify(value xce_val, value port)
{
- CAMLparam2(xce, port);
+ CAMLparam2(xce_val, port);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
int rc;

caml_enter_blocking_section();
- rc = xenevtchn_notify(_H(xce), Int_val(port));
+ rc = xenevtchn_notify(xce, Int_val(port));
caml_leave_blocking_section();

if (rc == -1)
@@ -122,15 +127,16 @@ CAMLprim value stub_eventchn_notify(value xce, value port)
CAMLreturn(Val_unit);
}

-CAMLprim value stub_eventchn_bind_interdomain(value xce, value domid,
+CAMLprim value stub_eventchn_bind_interdomain(value xce_val, value domid,
value remote_port)
{
- CAMLparam3(xce, domid, remote_port);
+ CAMLparam3(xce_val, domid, remote_port);
CAMLlocal1(port);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
xenevtchn_port_or_error_t rc;

caml_enter_blocking_section();
- rc = xenevtchn_bind_interdomain(_H(xce), Int_val(domid), Int_val(remote_port));
+ rc = xenevtchn_bind_interdomain(xce, Int_val(domid), Int_val(remote_port));
caml_leave_blocking_section();

if (rc == -1)
@@ -140,14 +146,15 @@ CAMLprim value stub_eventchn_bind_interdomain(value xce, value domid,
CAMLreturn(port);
}

-CAMLprim value stub_eventchn_bind_virq(value xce, value virq_type)
+CAMLprim value stub_eventchn_bind_virq(value xce_val, value virq_type)
{
- CAMLparam2(xce, virq_type);
+ CAMLparam2(xce_val, virq_type);
CAMLlocal1(port);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
xenevtchn_port_or_error_t rc;

caml_enter_blocking_section();
- rc = xenevtchn_bind_virq(_H(xce), Int_val(virq_type));
+ rc = xenevtchn_bind_virq(xce, Int_val(virq_type));
caml_leave_blocking_section();

if (rc == -1)
@@ -157,13 +164,14 @@ CAMLprim value stub_eventchn_bind_virq(value xce, value virq_type)
CAMLreturn(port);
}

-CAMLprim value stub_eventchn_unbind(value xce, value port)
+CAMLprim value stub_eventchn_unbind(value xce_val, value port)
{
- CAMLparam2(xce, port);
+ CAMLparam2(xce_val, port);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
int rc;

caml_enter_blocking_section();
- rc = xenevtchn_unbind(_H(xce), Int_val(port));
+ rc = xenevtchn_unbind(xce, Int_val(port));
caml_leave_blocking_section();

if (rc == -1)
@@ -172,14 +180,15 @@ CAMLprim value stub_eventchn_unbind(value xce, value port)
CAMLreturn(Val_unit);
}

-CAMLprim value stub_eventchn_pending(value xce)
+CAMLprim value stub_eventchn_pending(value xce_val)
{
- CAMLparam1(xce);
+ CAMLparam1(xce_val);
CAMLlocal1(result);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
xenevtchn_port_or_error_t port;

caml_enter_blocking_section();
- port = xenevtchn_pending(_H(xce));
+ port = xenevtchn_pending(xce);
caml_leave_blocking_section();

if (port == -1)
@@ -189,16 +198,17 @@ CAMLprim value stub_eventchn_pending(value xce)
CAMLreturn(result);
}

-CAMLprim value stub_eventchn_unmask(value xce, value _port)
+CAMLprim value stub_eventchn_unmask(value xce_val, value _port)
{
- CAMLparam2(xce, _port);
+ CAMLparam2(xce_val, _port);
+ xenevtchn_handle *xce = xce_of_val(xce_val);
evtchn_port_t port;
int rc;

port = Int_val(_port);

caml_enter_blocking_section();
- rc = xenevtchn_unmask(_H(xce), port);
+ rc = xenevtchn_unmask(xce, port);
caml_leave_blocking_section();

if (rc)
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.16