[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH 6/7] tools/ocaml/xc: Don't reference Abstract_Tag objects with the GC lock released


  • To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 31 Jan 2023 21:29:12 +0000
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Christian Lindig <christian.lindig@xxxxxxxxxx>, David Scott <dave@xxxxxxxxxx>, Edwin Török <edwin.torok@xxxxxxxxx>, Rob Hoes <Rob.Hoes@xxxxxxxxxx>
  • Delivery-date: Tue, 31 Jan 2023 21:29:40 +0000
  • Ironport-data: A9a23:Gk3+MazYHRxfxxsTyDp6t+c2xirEfRIJ4+MujC+fZmUNrF6WrkUGz 2MYWjuBPKmIYTH8L4onOYvjo0gE75SDydNhSAc5rSAxQypGp/SeCIXCJC8cHc8wwu7rFxs7s ppEOrEsCOhuExcwcz/0auCJQUFUjP3OHfykTbaeYUidfCc8IA85kxVvhuUltYBhhNm9Emult Mj75sbSIzdJ4RYtWo4vw//F+UwHUMja4mtC5QRnPqkT5zcyqlFOZH4hDfDpR5fHatE88t6SH 47r0Ly/92XFyBYhYvvNfmHTKxBirhb6ZGBiu1IOM0SQqkEqSh8ai87XAME0e0ZP4whlqvgqo Dl7WT5cfi9yVkHEsLx1vxC1iEiSN4UekFPMCSDXXcB+UyQq2pYjqhljJBheAGEWxgp4KUhW0 KQqbxITVwmKiu6Sm+6SeNt9v+12eaEHPKtH0p1h5TTQDPJgSpHfWaTao9Rf2V/chOgXQ6yYP ZBAL2MyMlKZOUYn1lQ/UfrSmM+BgHXlfiIeg1WSvactuEDYzRBr0airO93QEjCPbZQIwhfJ/ zKal4j/Ki8lNMbA6AurzmKDueXSkg/ZXZwxEYTto5aGh3XMnzdOWXX6T2CTsfS/z0KzRd9bA 0gV4TY167g/8lSxSdvwVAH+p2SL1jYQUsRdO/c34waMzuzT+QnxO4QfZmcfMpp87pZwHGF0k AbTxLsFGACDrpW8UVfFxPC2swqrMCUZCTReTB02XDIstoyLTJ4IsjrDSdNqEaiQh9LzGC3tz z3ikBXSl4n/nuZQifzloAmvbyaE48GQE1Vrvlm/sneNtFsRWWKzW2C/BbE3B95kJZ3RcFSOt WNsdyO2vLFXVsHleMBgrYww8FCVCxStamW0bb1HRcNJG9GRF5mLI+htDMlWfhsBDyr9UWaBj LXvkQ1Q/oRPG3ChcLV6ZYm8Y+xzk/e9TIW9DqiJNIARCnSUSONg1Hg+DXN8Iki3yBR8+U3BE cjznTmQ4YYyVv08kWveqxY12r433CEurV4/triipylLJYG2PSbPIZ9caQvmUwzMxP/cyOkj2 4oFZpTiJtQ2eLGWXxQ7BqZIdAxUdidmWcqmwyGVH8baSjdb9KgaI6e56dscl0ZNxsy5Ss+gE qmBZ3Jl
  • Ironport-hdrordr: A9a23:nQbdZ6p2m/YJiPBfwBbHvWUaV5syLNV00zEX/kB9WHVpm5Oj+v xGzc5w6farsl0ssSkb6Ki90KnpexPhHO1OkPIs1NaZLUDbUQSTXeVfBOfZrQEIXheOj9K1tp 0QO5SWaueAamSS5PySiGXWLz9j+qjgzEnCv5a8854Zd3AOV0gW1XYaNu/0KCxLbTgDIaB8OI uX58JBqTblUXMLbv6jDn1Ac/nfq8bNnJfGZwdDIxI88gGBgR6h9ba/SnGjr10jegIK5Y1n3X nOkgT/6Knmm/anyiXE32uWw4VKlMDnwt5jAtXJrsQOMD3jhiuheYwkcbyfuzIepv2p9T8R4Z LxiiZlG/42x2Laf2mzrxeo8RLnyiwS53jrzkLdqWf/oOTiLQhKR/ZptMZ8SF/0+kAgtNZz3O ZgxGSCradaChvGgWDU+8XIbRd3jUC5yEBS3tL7zkYvH7f2WoUh7bD3z3klU6vo2xiKqrzPJd MeTf00IswmNG9yIUqp+lWHi+bcJEjbVi32P3Tq/PblngS+1UoJs3cw1YgRmGwN+4k6TIQB7+ PYMr5wnLULVcMOa7lhbd1xNfdfJ1a9My4kCljiVGjPBeUCITbAupT36LI66KWjf4EJ1oI7nN DEXElDvWA/dkryAYnWtac7hCzlUSG4R3Dg28te7592tvn1Q6fqKzSKTBQrn9G7q/sSD8XHU7 K4OY5QAfXkMWzycLw5qDHWSt1XMz0TQccVstE0VxaHpd/KMJTjsqjBfPPaNNPWYEUZs6PEcw s+tRTIVbR9BxqQKwDFaTDqKg3QRnA=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

The intf->{addr,len} references in the xc_map_foreign_range() call are unsafe.
>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."

More than what the manual says, the intf pointer is (potentially) invalidated
by caml_enter_blocking_section() if another thread happens to perform garbage
collection at just the right (wrong) moment.

Rewrite the logic.  There's no need to stash data in the Ocaml object until
the success path at the very end.

Fixes: 8b7ce06a2d34 ("ocaml: Add XC bindings.")
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>

Note: the mmap stub has a similar pattern when constructing a mmap_interface,
but, but it's not actually unsafe because it doesn't drop the GC lock.

_H() is buggy too, but this patch needs backporting further than that fix.
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 291663bb278a..e5277f6f19a2 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -1028,26 +1028,25 @@ CAMLprim value stub_map_foreign_range(value xch, value 
dom,
        CAMLparam4(xch, dom, size, mfn);
        CAMLlocal1(result);
        struct mmap_interface *intf;
-       uint32_t c_dom;
-       unsigned long c_mfn;
+       unsigned long c_mfn = Nativeint_val(mfn);
+       int len = Int_val(size);
+       void *ptr;
 
        BUILD_BUG_ON((sizeof(struct mmap_interface) % sizeof(value)) != 0);
        result = caml_alloc(Wsize_bsize(sizeof(struct mmap_interface)),
                            Abstract_tag);
 
-       intf = (struct mmap_interface *) result;
-
-       intf->len = Int_val(size);
-
-       c_dom = _D(dom);
-       c_mfn = Nativeint_val(mfn);
        caml_enter_blocking_section();
-       intf->addr = xc_map_foreign_range(_H(xch), c_dom,
-                                         intf->len, PROT_READ|PROT_WRITE,
-                                         c_mfn);
+       ptr = xc_map_foreign_range(_H(xch), _D(dom), len,
+                                  PROT_READ|PROT_WRITE, c_mfn);
        caml_leave_blocking_section();
-       if (!intf->addr)
+
+       if (!ptr)
                caml_failwith("xc_map_foreign_range error");
+
+       intf = Data_abstract_val(result);
+       *intf = (struct mmap_interface){ ptr, len };
+
        CAMLreturn(result);
 }
 
-- 
2.11.0




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.