From ab12662499593bdfa7f5f4a4295823a8767a7d3e Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Wed, 22 Jan 2025 13:25:29 +0000 Subject: [PATCH 1/2] gnt: Avoid double unmapping on domain cleanup Oxenstored itself, when cleaning up domains and connections, can call Gnt.unmap transitively several times. Under certain conditions, this can later result in a segfault bringing oxenstored down when it tries to use a freed grant. Set the mmap_interface pointer to NULL after the first attempt so that it can be detected on the subsequent ones. Since we are already at it, bring out accesses to OCaml fields out of the critical section. Signed-off-by: Andrii Sultanov --- gnt/gnttab_stubs.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/gnt/gnttab_stubs.c b/gnt/gnttab_stubs.c index 387b59a..972add8 100644 --- a/gnt/gnttab_stubs.c +++ b/gnt/gnttab_stubs.c @@ -73,16 +73,26 @@ stub_gnttab_unmap (value xgh, value array) { CAMLparam2 (xgh, array); int result; - - caml_enter_blocking_section (); - result = xengnttab_unmap (_G (xgh), _M (array)->addr, - _M (array)->len >> XEN_PAGE_SHIFT); - caml_leave_blocking_section (); - - if (result != 0) - { - caml_failwith ("Failed to unmap grant"); - } + xengnttab_handle* xgt = _G(xgh); + void* start_address = _M(array)->addr; + uint32_t count = _M(array)->len >> XEN_PAGE_SHIFT; + char s[64]; + + /* Check if this grant hasn't already been unmapped before */ + if (start_address) { + caml_enter_blocking_section (); + result = xengnttab_unmap (xgt, start_address, count); + caml_leave_blocking_section (); + /* Avoid double unmapping by NULL-ing the pointer */ + _M(array)->addr = NULL; + _M(array)->len = 0; + + if (result != 0) + { + int r = snprintf(s, 64, "Failed to unmap grant (errno %d, rc %d)", errno, result); + caml_failwith(s); + } + } CAMLreturn (Val_unit); } From 24b70bc0e6c2be8333b3c6bff48b50ca7bd5c389 Mon Sep 17 00:00:00 2001 From: Andrii Sultanov Date: Thu, 23 Jan 2025 08:04:15 +0000 Subject: [PATCH 2/2] gnt: Bring accesses to OCaml fields outside of the critical section for stub_gnttab_map_fresh Signed-off-by: Andrii Sultanov --- gnt/gnttab_stubs.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gnt/gnttab_stubs.c b/gnt/gnttab_stubs.c index 972add8..e41fd12 100644 --- a/gnt/gnttab_stubs.c +++ b/gnt/gnttab_stubs.c @@ -104,12 +104,13 @@ stub_gnttab_map_fresh (value xgh, CAMLparam4 (xgh, reference, domid, writable); CAMLlocal1 (contents); void *map; + xengnttab_handle* xgt = _G(xgh); + uint32_t domid_c = Int_val(domid); + uint32_t ref = Int_val(reference); + int prot = Bool_val (writable) ? PROT_READ | PROT_WRITE : PROT_READ; caml_enter_blocking_section (); - map = xengnttab_map_grant_ref (_G (xgh), Int_val (domid), - Int_val (reference), - Bool_val (writable) ? PROT_READ | - PROT_WRITE : PROT_READ); + map = xengnttab_map_grant_ref (xgt, domid_c, ref, prot); caml_leave_blocking_section (); if (map == NULL)