Re: [PATCH] tools/ocaml/xc: Drop the GC lock for all hypercalls

[Edwin Torok <edwin.torok@xxxxxxxxx>]

On Mon, Sep 2, 2024 at 5:38 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 02/09/2024 9:10 am, Edwin Torok wrote:
> > On Fri, Aug 30, 2024 at 6:53 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> > wrote:
> >> We should be doing this unilaterally.
> > Agreed, but we should do it safely, since last time I did this I
> > learned about a few more instances of behaviours I previously thought
> > to be safe, but that are undefined behaviour.
> > Which probably means we have a bunch of other code to fixup (I should
> > really finish my static analyzer project, and update it with the newly
> > learned rules to catch all these...).
> > See below for comments.
> >
> > Although there is one bug here we've previously known to avoid:
> > String_val cannot be dereferenced with the lock released, that one is
> > an OCaml value and will cause actual problems,
> > so we need to caml_copy_string that one.
> >
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
> >> CC: David Scott <dave@xxxxxxxxxx>
> >> CC: Edwin Török <edwin.torok@xxxxxxxxx>
> >> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx>
> >> CC: Andrii Sultanov <andrii.sultanov@xxxxxxxxx>
> >>
> >> Also pulled out of a larger cleanup series.
> >> ---
> >>  tools/ocaml/libs/xc/xenctrl_stubs.c | 63 +++++++++++++++++++++++++++--
> >>  1 file changed, 60 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> >> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> >> index c78191f95abc..20487b21008f 100644
> >> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> >> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> >> @@ -312,7 +312,10 @@ CAMLprim value stub_xc_domain_max_vcpus(value 
> >> xch_val, value domid,
> >>         xc_interface *xch = xch_of_val(xch_val);
> >>         int r;
> >>
> >> +       caml_enter_blocking_section();
> >>         r = xc_domain_max_vcpus(xch, Int_val(domid), Int_val(max_vcpus));
> > We need to move the Int_val macros out of here, domid is registered as
> > a GC root, so the GC *will* write to it (it'll write the same value).
>
> How?
>
> As value's, both domid and max_vcpu are integers living in GPRs.

domid and max_vcpus are registered with CAMLparam macro, so their
address is registered as a GC root,
which the GC will see and go and update.
The compiler may have cached this in a GPR, but we can't rely on that.

In particular if you run the thread sanitizer in OCaml 5 it'll warn
all over the place about this (I haven't gotten as far as having a
full build with Xen and OCaml 5, I only have a XAPI build so far).
See here for a discussion https://github.com/ocaml/ocaml/pull/13188.
The GC could avoid the potential race here by checking whether the
value is an integer prior to updating it, but that might have a
(small) performance impact, so it is not done.

>
> These expressions are not dereferencing  into the Ocaml Heap.

No, but they are reachable from a C pointer that is registered as a GC
root in the OCaml GC.
(the CAMLparam macro registers all its parameters as GC roots).

Another option is to not register integers with CAMLparam, but that
breaks symmetry, and it'd then be very easy to forget to register an
OCaml parameter.

Best regards,
--Edwin

>
> ~Andrew