From c927979fbf20b975752185febd8238cb86b335e0 Mon Sep 17 00:00:00 2001 From: Guillaume Date: Tue, 8 Apr 2025 18:22:21 +0200 Subject: [PATCH] Check that there are no changes during SR.scan Currently, we are only checking that no VDIs have been removed during the SR scan performed by the SM plugin. However, there are situations where a VDI has been added, and if this VDI is not present in the list obtained from SR.scan, it will be forgotten. The checks only prevent this in the case where the VDI was added during the scan. There is still a TOCTOU situation if the issue happens after the scan, and there is room for that. Signed-off-by: Guillaume --- ocaml/xapi/xapi_sr.ml | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/ocaml/xapi/xapi_sr.ml b/ocaml/xapi/xapi_sr.ml index d09490b9521..4a0684147af 100644 --- a/ocaml/xapi/xapi_sr.ml +++ b/ocaml/xapi/xapi_sr.ml @@ -757,6 +757,11 @@ let update_vdis ~__context ~sr db_vdis vdi_infos = (* Perform a scan of this locally-attached SR *) let scan ~__context ~sr = + let module RefSet = Set.Make (struct + type t = [`VDI] Ref.t + + let compare = Ref.compare + end) in let open Storage_access in let task = Context.get_task_id __context in let module C = Storage_interface.StorageAPI (Idl.Exn.GenClient (struct @@ -781,9 +786,21 @@ let scan ~__context ~sr = (* It is sufficient to just compare the refs in two db_vdis, as this is what update_vdis uses to determine what to delete *) let vdis_ref_equal db_vdi1 db_vdi2 = - Listext.List.set_difference (List.map fst db_vdi1) - (List.map fst db_vdi2) - = [] + let refs1 = RefSet.of_list (List.map fst db_vdi1) in + let refs2 = RefSet.of_list (List.map fst db_vdi2) in + if RefSet.equal refs1 refs2 then + true + else + let log_diff label a b = + RefSet.diff a b + |> RefSet.elements + |> List.map Ref.string_of + |> String.concat " " + |> debug "%s: VDIs %s during scan: %s" __FUNCTION__ label + in + log_diff "removed" refs1 refs2 ; + log_diff "added" refs2 refs1 ; + false in let db_vdis_before = find_vdis () in let vs, sr_info = @@ -793,21 +810,13 @@ let scan ~__context ~sr = let db_vdis_after = find_vdis () in if limit > 0 && not (vdis_ref_equal db_vdis_before db_vdis_after) then ( - debug - "%s detected db change while scanning, before scan vdis %s, \ - after scan vdis %s, retry limit left %d" - __FUNCTION__ - (List.map (fun (_, v) -> v.vDI_uuid) db_vdis_before - |> String.concat "," - ) - (List.map (fun (_, v) -> v.vDI_uuid) db_vdis_after - |> String.concat "," - ) - limit ; + debug "%s detected db change while scanning, retry limit left %d" + __FUNCTION__ limit ; (scan_rec [@tailcall]) (limit - 1) ) else if limit = 0 then Helpers.internal_error "SR.scan retry limit exceeded" else ( + debug "%s no change detected, updating VDIs" __FUNCTION__ ; update_vdis ~__context ~sr db_vdis_after vs ; let virtual_allocation = List.fold_left