On 24/11/2022 14:03, Edwin Torok wrote: > >> On 24 Nov 2022, at 13:43, Andrew Cooper <[email protected]> wrote: >> >> On 24/11/2022 09:03, Edwin Torok wrote: >>>> On 23 Nov 2022, at 22:25, Andrew Cooper <[email protected]> wrote: >>>> >>>> The binding for xc_interface_close() free the underlying handle while >>>> leaving >>>> the Ocaml object still in scope and usable. This would make it easy to >>>> suffer >>>> a use-after-free, if it weren't for the fact that the typical usage is as a >>>> singleton that lives for the lifetime of the program. >>>> >>>> Ocaml 5 no longer permits storing a naked C pointer in an Ocaml value. >>>> >>>> Therefore, use a Custom block. This allows us to use the finaliser >>>> callback >>>> to call xc_interface_close(), if the Ocaml object goes out of scope. >>>> >>>> Signed-off-by: Andrew Cooper <[email protected]> >>>> --- >>>> CC: Christian Lindig <[email protected]> >>>> CC: David Scott <[email protected]> >>>> CC: Edwin Torok <[email protected]> >>>> CC: Rob Hoes <[email protected]> >>>> >>>> I've confirmed that Xenctrl.close_handle does cause the finaliser to be >>>> called, simply by dropping the handle reference. >>> Thanks, a good way to test this is with OCAMLRUNPARAM=c, possible under >>> valgrind, which causes all finalisers to be called on exit >>> (normally they are not because the program is exiting anyway) >> I do that anyway, but it's not relevant here. >> >> What matters is checking that calling close_handle releases the object >> (albeit with a forced GC sweep) before the program ends. >> >>>> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c >>>> b/tools/ocaml/libs/xc/xenctrl_stubs.c >>>> index f37848ae0bb3..4e1204085422 100644 >>>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c >>>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c >>>> @@ -37,13 +37,28 @@ >>>> >>>> #include "mmap_stubs.h" >>>> >>>> -#define _H(__h) ((xc_interface *)(__h)) >>>> +#define _H(__h) (*((xc_interface **)Data_custom_val(__h))) >>>> #define _D(__d) ((uint32_t)Int_val(__d)) >>> I think this requires an update in xenopsd too to match, otherwise it'll >>> crash: >>> https://github.com/xapi-project/xenopsd/blob/master/c_stubs/xenctrlext_stubs.c#L32 >> Urgh. I'll take a note to do that when bringing in the change. >> >>> This wasn't an issue with the original patch which used Data_abstract_val >>> here, because >>> that (currently) happens to boil down to just a cast (with some GC metadata >>> *before* it), >>> so the old way of just casting OCaml value to C pointer still worked. >>> >>> However Data_custom_val boils down to accessing a value at +sizeof(value) >>> offset, >>> so xenopsd would now read the wrong pointer. >>> Perhaps it would've been better to have this _H defined in some header, >>> otherwise extending Xenctrl the way xenopsd does it is quite brittle. >> Exporting _H won't help because everything is statically built. > > As long as you don't rebuilt xenopsd you will keep using the old C stubs that > xenopsd got compiled with, the change in Xen will have no effect until > xenopsd is recompiled, > at which point it could pick up the new _H if available, but I agree with > your point below. > > >> It's >> brittle because xenopsd has got a local piece of C playing around with >> the internals of someone else's library. This violates more rules than >> I care to list. >> >> We (XenServer) should definitely work to improve things, but this is >> entirely a xenopsd problem, not an upstream Xen problem. > > It is a lot easier to add new xenctrl bindings and test them out in xenopsd > than it is to add them to Xen. > We should try to either upstream all xenopsd xenctrl bindings to Xen, and > make it easier to add them to Xen going forward; > or move all the Xenctrl bindings to xenopsd, then at least you only need to > rebuild the one piece you are changing. > > But to do the latter we first need to get everything that relies on xenctrl > to move to more stable interfaces (including oxenstored). > There are some Mirage libraries as well that use xenctrl, when a more > suitable stable interface exists in newer versions of Xen that they should > use instead. > > Perhaps a compromise between the 2 extremes would be for xenopsd to open and > have its own xenctrl handle, even if that leads to some code duplication, it > would at least not rely on an undocumented and unstable internal detail of an > already unstable ABI. And that would still allow xenopsd to extend xenctrl > with bindings that are not (yet) present in Xen. > What do you think?
Many of these problems will disappear with a stable tools interface. But yes, in the short term, xcext opening its own handle would definitely improve things by keeping the two sets of bindings separate. ~Andrew
