| Message ID | 20221130165455.31125-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More Oxenstored live update fixes | expand |
> On 30 Nov 2022, at 16:54, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > Inter-domain event channels are always a pair of local and remote ports. > Right now the handling is asymmetric, caused by the fact that the evtchn is > bound after the associated Domain object is constructed. > > First, move binding of the event channel into the Domain.make() constructor. > This means the local port no longer needs to be an option. It also removes > the final callers of Domain.bind_interdomain. > > Next, introduce a new port_pair type to encapsulate the fact that these two > should be updated together, and replace the previous port and remote_port > fields. This refactoring also changes the Domain.get_port interface (removing > an option) so take the opportunity to name it get_local_port instead. > > Also, this fixes a use-after-free risk with Domain.close. Once the evtchn has > been unbound, the same local port number can be reused for a different > purpose, so explicitly invalidate the ports to prevent their accidental misuse > in the future. > > This also cleans up some of the debugging, to always print a port pair. Reviewed in-person, I've suggested to use explicit labeled arguments for the case where multiple integers with very close semantic meaning are passed as arguments, e.g. local vs remote port, it'd be quite easy to accidentally swap them in the caller, leading to bugs. Reviewed-by: Edwin Török <edvin.torok@citrix.com> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Christian Lindig <christian.lindig@citrix.com> > CC: David Scott <dave@recoil.org> > CC: Edwin Torok <edvin.torok@citrix.com> > CC: Rob Hoes <Rob.Hoes@citrix.com> > > v2: > * New > --- > tools/ocaml/xenstored/connections.ml | 9 +---- > tools/ocaml/xenstored/domain.ml | 75 ++++++++++++++++++------------------ > tools/ocaml/xenstored/domains.ml | 2 - > 3 files changed, 39 insertions(+), 47 deletions(-) > > diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml > index 7d68c583b43a..a80ae0bed2ce 100644 > --- a/tools/ocaml/xenstored/connections.ml > +++ b/tools/ocaml/xenstored/connections.ml > @@ -48,9 +48,7 @@ let add_domain cons dom = > let xbcon = Xenbus.Xb.open_mmap ~capacity (Domain.get_interface dom) (fun () -> Domain.notify dom) in > let con = Connection.create xbcon (Some dom) in > Hashtbl.add cons.domains (Domain.get_id dom) con; > - match Domain.get_port dom with > - | Some p -> Hashtbl.add cons.ports p con; > - | None -> () > + Hashtbl.add cons.ports (Domain.get_local_port dom) con > > let select ?(only_if = (fun _ -> true)) cons = > Hashtbl.fold (fun _ con (ins, outs) -> > @@ -97,10 +95,7 @@ let del_domain cons id = > let con = find_domain cons id in > Hashtbl.remove cons.domains id; > (match Connection.get_domain con with > - | Some d -> > - (match Domain.get_port d with > - | Some p -> Hashtbl.remove cons.ports p > - | None -> ()) > + | Some d -> Hashtbl.remove cons.ports (Domain.get_local_port d) > | None -> ()); > del_watches cons con; > Connection.close con > diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml > index d59a9401e211..ecdd65f3209a 100644 > --- a/tools/ocaml/xenstored/domain.ml > +++ b/tools/ocaml/xenstored/domain.ml > @@ -19,14 +19,31 @@ open Printf > let debug fmt = Logging.debug "domain" fmt > let warn fmt = Logging.warn "domain" fmt > > +(* An event channel port pair. The remote port, and the local port it is > + bound to. *) > +type port_pair = > +{ > + local: Xeneventchn.t; > + remote: int; > +} > + > +(* Sentinal port_pair with both set to EVTCHN_INVALID *) > +let invalid_ports = > +{ > + local = Xeneventchn.of_int 0; > + remote = 0 > +} > + > +let string_of_port_pair p = > + sprintf "(l %d, r %d)" (Xeneventchn.to_int p.local) p.remote > + > type t = > { > id: Xenctrl.domid; > mfn: nativeint; > interface: Xenmmap.mmap_interface; > eventchn: Event.t; > - mutable remote_port: int; > - mutable port: Xeneventchn.t option; > + mutable ports: port_pair; > mutable bad_client: bool; > mutable io_credit: int; (* the rounds of ring process left to do, default is 0, > usually set to 1 when there is work detected, could > @@ -41,8 +58,8 @@ let is_dom0 d = d.id = 0 > let get_id domain = domain.id > let get_interface d = d.interface > let get_mfn d = d.mfn > -let get_remote_port d = d.remote_port > -let get_port d = d.port > +let get_remote_port d = d.ports.remote > +let get_local_port d = d.ports.local > > let is_bad_domain domain = domain.bad_client > let mark_as_bad domain = domain.bad_client <- true > @@ -56,54 +73,36 @@ let is_paused_for_conflict dom = dom.conflict_credit <= 0.0 > > let is_free_to_conflict = is_dom0 > > -let string_of_port = function > - | None -> "None" > - | Some x -> string_of_int (Xeneventchn.to_int x) > - > let dump d chan = > - fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.remote_port > + fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.ports.remote > > let rebind_evtchn d remote_port = > - begin match d.port with > - | None -> () > - | Some p -> Event.unbind d.eventchn p > - end; > + Event.unbind d.eventchn d.ports.local; > let local = Event.bind_interdomain d.eventchn d.id remote_port in > - debug "domain %d rebind (l %s, r %d) => (l %d, r %d)" > - d.id (string_of_port d.port) d.remote_port > - (Xeneventchn.to_int local) remote_port; > - d.remote_port <- remote_port; > - d.port <- Some (local) > + let ports = { local; remote = remote_port } in > + debug "domain %d rebind %s => %s" > + d.id (string_of_port_pair d.ports) (string_of_port_pair ports); > + d.ports <- ports > > let notify dom = > - match dom.port with > - | None -> warn "domain %d: attempt to notify on unknown port" dom.id > - | Some port -> Event.notify dom.eventchn port > - > -let bind_interdomain dom = > - begin match dom.port with > - | None -> () > - | Some port -> Event.unbind dom.eventchn port > - end; > - dom.port <- Some (Event.bind_interdomain dom.eventchn dom.id dom.remote_port); > - debug "bound domain %d remote port %d to local port %s" dom.id dom.remote_port (string_of_port dom.port) > - > + Event.notify dom.eventchn dom.ports.local > > let close dom = > - debug "domain %d unbound port %s" dom.id (string_of_port dom.port); > - begin match dom.port with > - | None -> () > - | Some port -> Event.unbind dom.eventchn port > - end; > + debug "domain %d unbind %s" dom.id (string_of_port_pair dom.ports); > + Event.unbind dom.eventchn dom.ports.local; > + dom.ports <- invalid_ports; > Xenmmap.unmap dom.interface > > -let make id mfn remote_port interface eventchn = { > +let make id mfn remote_port interface eventchn = > + let local = Event.bind_interdomain eventchn id remote_port in > + let ports = { local; remote = remote_port } in > + debug "domain %d bind %s" id (string_of_port_pair ports); > +{ > id = id; > mfn = mfn; > - remote_port = remote_port; > + ports; > interface = interface; > eventchn = eventchn; > - port = None; > bad_client = false; > io_credit = 0; > conflict_credit = !Define.conflict_burst_limit; > diff --git a/tools/ocaml/xenstored/domains.ml b/tools/ocaml/xenstored/domains.ml > index 26018ac0dd3d..2ab0c5f4d8d0 100644 > --- a/tools/ocaml/xenstored/domains.ml > +++ b/tools/ocaml/xenstored/domains.ml > @@ -126,7 +126,6 @@ let create doms domid mfn remote_port = > let interface = Xenctrl.map_foreign_range xc domid (Xenmmap.getpagesize()) mfn in > let dom = Domain.make domid mfn remote_port interface doms.eventchn in > Hashtbl.add doms.table domid dom; > - Domain.bind_interdomain dom; > dom > > let xenstored_kva = ref "" > @@ -144,7 +143,6 @@ let create0 doms = > > let dom = Domain.make 0 Nativeint.zero remote_port interface doms.eventchn in > Hashtbl.add doms.table 0 dom; > - Domain.bind_interdomain dom; > Domain.notify dom; > dom > > -- > 2.11.0 >
> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > Inter-domain event channels are always a pair of local and remote ports. > Right now the handling is asymmetric, caused by the fact that the evtchn is > bound after the associated Domain object is constructed. > > First, move binding of the event channel into the Domain.make() constructor. > This means the local port no longer needs to be an option. It also removes > the final callers of Domain.bind_interdomain. > > Next, introduce a new port_pair type to encapsulate the fact that these two > should be updated together, and replace the previous port and remote_port > fields. This refactoring also changes the Domain.get_port interface (removing > an option) so take the opportunity to name it get_local_port instead. > > Also, this fixes a use-after-free risk with Domain.close. Once the evtchn has > been unbound, the same local port number can be reused for a different > purpose, so explicitly invalidate the ports to prevent their accidental misuse > in the future. > > This also cleans up some of the debugging, to always print a port pair. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Christian Lindig <christian.lindig@citrix.com> > CC: David Scott <dave@recoil.org> > CC: Edwin Torok <edvin.torok@citrix.com> > CC: Rob Hoes <Rob.Hoes@citrix.com> Acked-by: Christian Lindig <christian.lindig@citrix.com> > > v2: > * New > --- > tools/ocaml/xenstored/connections.ml | 9 +---- > tools/ocaml/xenstored/domain.ml | 75 ++++++++++++++++++------------------ > tools/ocaml/xenstored/domains.ml | 2 - > 3 files changed, 39 insertions(+), 47 deletions(-) > > diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml > index 7d68c583b43a..a80ae0bed2ce 100644 > --- a/tools/ocaml/xenstored/connections.ml > +++ b/tools/ocaml/xenstored/connections.ml > @@ -48,9 +48,7 @@ let add_domain cons dom = > let xbcon = Xenbus.Xb.open_mmap ~capacity (Domain.get_interface dom) (fun () -> Domain.notify dom) in > let con = Connection.create xbcon (Some dom) in > Hashtbl.add cons.domains (Domain.get_id dom) con; > - match Domain.get_port dom with > - | Some p -> Hashtbl.add cons.ports p con; > - | None -> () > + Hashtbl.add cons.ports (Domain.get_local_port dom) con I would prefer Hashtbl.replace. Hashtbl.add shadows an existing binding which becomes visible again after Hashtabl.remove. When we are sure that we only have one binding per key, we should use replace instead of add. > > let select ?(only_if = (fun _ -> true)) cons = > Hashtbl.fold (fun _ con (ins, outs) -> > @@ -97,10 +95,7 @@ let del_domain cons id = > let con = find_domain cons id in > Hashtbl.remove cons.domains id; > (match Connection.get_domain con with > - | Some d -> > - (match Domain.get_port d with > - | Some p -> Hashtbl.remove cons.ports p > - | None -> ()) > + | Some d -> Hashtbl.remove cons.ports (Domain.get_local_port d) > | None -> ()); > del_watches cons con; > Connection.close con > diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml > index d59a9401e211..ecdd65f3209a 100644 > --- a/tools/ocaml/xenstored/domain.ml > +++ b/tools/ocaml/xenstored/domain.ml > @@ -19,14 +19,31 @@ open Printf > let debug fmt = Logging.debug "domain" fmt > let warn fmt = Logging.warn "domain" fmt > > +(* An event channel port pair. The remote port, and the local port it is > + bound to. *) > +type port_pair = > +{ > + local: Xeneventchn.t; > + remote: int; > +} > + > +(* Sentinal port_pair with both set to EVTCHN_INVALID *) > +let invalid_ports = > +{ > + local = Xeneventchn.of_int 0; > + remote = 0 > +} > + > +let string_of_port_pair p = > + sprintf "(l %d, r %d)" (Xeneventchn.to_int p.local) p.remote > + > type t = > { > id: Xenctrl.domid; > mfn: nativeint; > interface: Xenmmap.mmap_interface; > eventchn: Event.t; > - mutable remote_port: int; > - mutable port: Xeneventchn.t option; > + mutable ports: port_pair; > mutable bad_client: bool; > mutable io_credit: int; (* the rounds of ring process left to do, default is 0, > usually set to 1 when there is work detected, could > @@ -41,8 +58,8 @@ let is_dom0 d = d.id = 0 > let get_id domain = domain.id > let get_interface d = d.interface > let get_mfn d = d.mfn > -let get_remote_port d = d.remote_port > -let get_port d = d.port > +let get_remote_port d = d.ports.remote > +let get_local_port d = d.ports.local > > let is_bad_domain domain = domain.bad_client > let mark_as_bad domain = domain.bad_client <- true > @@ -56,54 +73,36 @@ let is_paused_for_conflict dom = dom.conflict_credit <= 0.0 > > let is_free_to_conflict = is_dom0 > > -let string_of_port = function > - | None -> "None" > - | Some x -> string_of_int (Xeneventchn.to_int x) > - > let dump d chan = > - fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.remote_port > + fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.ports.remote > > let rebind_evtchn d remote_port = > - begin match d.port with > - | None -> () > - | Some p -> Event.unbind d.eventchn p > - end; > + Event.unbind d.eventchn d.ports.local; > let local = Event.bind_interdomain d.eventchn d.id remote_port in > - debug "domain %d rebind (l %s, r %d) => (l %d, r %d)" > - d.id (string_of_port d.port) d.remote_port > - (Xeneventchn.to_int local) remote_port; > - d.remote_port <- remote_port; > - d.port <- Some (local) > + let ports = { local; remote = remote_port } in > + debug "domain %d rebind %s => %s" > + d.id (string_of_port_pair d.ports) (string_of_port_pair ports); > + d.ports <- ports > > let notify dom = > - match dom.port with > - | None -> warn "domain %d: attempt to notify on unknown port" dom.id > - | Some port -> Event.notify dom.eventchn port > - > -let bind_interdomain dom = > - begin match dom.port with > - | None -> () > - | Some port -> Event.unbind dom.eventchn port > - end; > - dom.port <- Some (Event.bind_interdomain dom.eventchn dom.id dom.remote_port); > - debug "bound domain %d remote port %d to local port %s" dom.id dom.remote_port (string_of_port dom.port) > - > + Event.notify dom.eventchn dom.ports.local > > let close dom = > - debug "domain %d unbound port %s" dom.id (string_of_port dom.port); > - begin match dom.port with > - | None -> () > - | Some port -> Event.unbind dom.eventchn port > - end; > + debug "domain %d unbind %s" dom.id (string_of_port_pair dom.ports); > + Event.unbind dom.eventchn dom.ports.local; > + dom.ports <- invalid_ports; > Xenmmap.unmap dom.interface > > -let make id mfn remote_port interface eventchn = { > +let make id mfn remote_port interface eventchn = > + let local = Event.bind_interdomain eventchn id remote_port in > + let ports = { local; remote = remote_port } in > + debug "domain %d bind %s" id (string_of_port_pair ports); > +{ > id = id; > mfn = mfn; > - remote_port = remote_port; > + ports; > interface = interface; > eventchn = eventchn; > - port = None; > bad_client = false; > io_credit = 0; > conflict_credit = !Define.conflict_burst_limit; > diff --git a/tools/ocaml/xenstored/domains.ml b/tools/ocaml/xenstored/domains.ml > index 26018ac0dd3d..2ab0c5f4d8d0 100644 > --- a/tools/ocaml/xenstored/domains.ml > +++ b/tools/ocaml/xenstored/domains.ml > @@ -126,7 +126,6 @@ let create doms domid mfn remote_port = > let interface = Xenctrl.map_foreign_range xc domid (Xenmmap.getpagesize()) mfn in > let dom = Domain.make domid mfn remote_port interface doms.eventchn in > Hashtbl.add doms.table domid dom; > - Domain.bind_interdomain dom; > dom > > let xenstored_kva = ref "" > @@ -144,7 +143,6 @@ let create0 doms = > > let dom = Domain.make 0 Nativeint.zero remote_port interface doms.eventchn in > Hashtbl.add doms.table 0 dom; > - Domain.bind_interdomain dom; > Domain.notify dom; > dom > > -- > 2.11.0 >
On 01/12/2022 11:59, Christian Lindig wrote: >> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >> >> Inter-domain event channels are always a pair of local and remote ports. >> Right now the handling is asymmetric, caused by the fact that the evtchn is >> bound after the associated Domain object is constructed. >> >> First, move binding of the event channel into the Domain.make() constructor. >> This means the local port no longer needs to be an option. It also removes >> the final callers of Domain.bind_interdomain. >> >> Next, introduce a new port_pair type to encapsulate the fact that these two >> should be updated together, and replace the previous port and remote_port >> fields. This refactoring also changes the Domain.get_port interface (removing >> an option) so take the opportunity to name it get_local_port instead. >> >> Also, this fixes a use-after-free risk with Domain.close. Once the evtchn has >> been unbound, the same local port number can be reused for a different >> purpose, so explicitly invalidate the ports to prevent their accidental misuse >> in the future. >> >> This also cleans up some of the debugging, to always print a port pair. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Christian Lindig <christian.lindig@citrix.com> >> CC: David Scott <dave@recoil.org> >> CC: Edwin Torok <edvin.torok@citrix.com> >> CC: Rob Hoes <Rob.Hoes@citrix.com> > Acked-by: Christian Lindig <christian.lindig@citrix.com> Thanks. > >> v2: >> * New >> --- >> tools/ocaml/xenstored/connections.ml | 9 +---- >> tools/ocaml/xenstored/domain.ml | 75 ++++++++++++++++++------------------ >> tools/ocaml/xenstored/domains.ml | 2 - >> 3 files changed, 39 insertions(+), 47 deletions(-) >> >> diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml >> index 7d68c583b43a..a80ae0bed2ce 100644 >> --- a/tools/ocaml/xenstored/connections.ml >> +++ b/tools/ocaml/xenstored/connections.ml >> @@ -48,9 +48,7 @@ let add_domain cons dom = >> let xbcon = Xenbus.Xb.open_mmap ~capacity (Domain.get_interface dom) (fun () -> Domain.notify dom) in >> let con = Connection.create xbcon (Some dom) in >> Hashtbl.add cons.domains (Domain.get_id dom) con; >> - match Domain.get_port dom with >> - | Some p -> Hashtbl.add cons.ports p con; >> - | None -> () >> + Hashtbl.add cons.ports (Domain.get_local_port dom) con > I would prefer Hashtbl.replace. Hashtbl.add shadows an existing binding which becomes visible again after Hashtabl.remove. When we are sure that we only have one binding per key, we should use replace instead of add. That's surprising behaviour. Presumably the add->replace suggestion applies the other hashtable here (cons.domains)? And possibly elsewhere too. ~Andrew
> On 1 Dec 2022, at 14:22, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 01/12/2022 11:59, Christian Lindig wrote: >>> On 30 Nov 2022, at 16:54, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: >>> >>> Inter-domain event channels are always a pair of local and remote ports. >>> Right now the handling is asymmetric, caused by the fact that the evtchn is >>> bound after the associated Domain object is constructed. >>> >>> First, move binding of the event channel into the Domain.make() constructor. >>> This means the local port no longer needs to be an option. It also removes >>> the final callers of Domain.bind_interdomain. >>> >>> Next, introduce a new port_pair type to encapsulate the fact that these two >>> should be updated together, and replace the previous port and remote_port >>> fields. This refactoring also changes the Domain.get_port interface (removing >>> an option) so take the opportunity to name it get_local_port instead. >>> >>> Also, this fixes a use-after-free risk with Domain.close. Once the evtchn has >>> been unbound, the same local port number can be reused for a different >>> purpose, so explicitly invalidate the ports to prevent their accidental misuse >>> in the future. >>> >>> This also cleans up some of the debugging, to always print a port pair. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Christian Lindig <christian.lindig@citrix.com> >>> CC: David Scott <dave@recoil.org> >>> CC: Edwin Torok <edvin.torok@citrix.com> >>> CC: Rob Hoes <Rob.Hoes@citrix.com> >> Acked-by: Christian Lindig <christian.lindig@citrix.com> > > Thanks. > >> >>> v2: >>> * New >>> --- >>> tools/ocaml/xenstored/connections.ml | 9 +---- >>> tools/ocaml/xenstored/domain.ml | 75 ++++++++++++++++++------------------ >>> tools/ocaml/xenstored/domains.ml | 2 - >>> 3 files changed, 39 insertions(+), 47 deletions(-) >>> >>> diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml >>> index 7d68c583b43a..a80ae0bed2ce 100644 >>> --- a/tools/ocaml/xenstored/connections.ml >>> +++ b/tools/ocaml/xenstored/connections.ml >>> @@ -48,9 +48,7 @@ let add_domain cons dom = >>> let xbcon = Xenbus.Xb.open_mmap ~capacity (Domain.get_interface dom) (fun () -> Domain.notify dom) in >>> let con = Connection.create xbcon (Some dom) in >>> Hashtbl.add cons.domains (Domain.get_id dom) con; >>> - match Domain.get_port dom with >>> - | Some p -> Hashtbl.add cons.ports p con; >>> - | None -> () >>> + Hashtbl.add cons.ports (Domain.get_local_port dom) con >> I would prefer Hashtbl.replace. Hashtbl.add shadows an existing binding which becomes visible again after Hashtabl.remove. When we are sure that we only have one binding per key, we should use replace instead of add. > > That's surprising behaviour. Presumably the add->replace suggestion > applies the other hashtable here (cons.domains)? And possibly elsewhere > too. Yes: * Hashtbl.add -> Hashtbl.replace * Hashtbl.clear -> Hashtbl.reset Using anything on the left is almost always an indication of a subtle bug (e.g. Hashtbl.clear won't release the memory used by the buckets, and the only time that is useful is if you'd immediately fill the hashtable with lots of elements again, really code should always use Hashtbl.reset but that only got introduced in OCaml 4.0.0, so older code won't have it). And the use of Hashtbl.add can lead to "space leaks" (eventually OOM) unless one really knows what they are doing (i.e. there are only a finite number of add calls ever). In XAPI we have a "quality gate" that counts the number of problematic functions/etc, and makes it a hard build time failure if any new usages are introduced (and we strive to reduce that to 0). I don't think these 2 Hashtbl calls are there yet, but they probably should be. Best regards, --Edwin
diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml index 7d68c583b43a..a80ae0bed2ce 100644 --- a/tools/ocaml/xenstored/connections.ml +++ b/tools/ocaml/xenstored/connections.ml @@ -48,9 +48,7 @@ let add_domain cons dom = let xbcon = Xenbus.Xb.open_mmap ~capacity (Domain.get_interface dom) (fun () -> Domain.notify dom) in let con = Connection.create xbcon (Some dom) in Hashtbl.add cons.domains (Domain.get_id dom) con; - match Domain.get_port dom with - | Some p -> Hashtbl.add cons.ports p con; - | None -> () + Hashtbl.add cons.ports (Domain.get_local_port dom) con let select ?(only_if = (fun _ -> true)) cons = Hashtbl.fold (fun _ con (ins, outs) -> @@ -97,10 +95,7 @@ let del_domain cons id = let con = find_domain cons id in Hashtbl.remove cons.domains id; (match Connection.get_domain con with - | Some d -> - (match Domain.get_port d with - | Some p -> Hashtbl.remove cons.ports p - | None -> ()) + | Some d -> Hashtbl.remove cons.ports (Domain.get_local_port d) | None -> ()); del_watches cons con; Connection.close con diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml index d59a9401e211..ecdd65f3209a 100644 --- a/tools/ocaml/xenstored/domain.ml +++ b/tools/ocaml/xenstored/domain.ml @@ -19,14 +19,31 @@ open Printf let debug fmt = Logging.debug "domain" fmt let warn fmt = Logging.warn "domain" fmt +(* An event channel port pair. The remote port, and the local port it is + bound to. *) +type port_pair = +{ + local: Xeneventchn.t; + remote: int; +} + +(* Sentinal port_pair with both set to EVTCHN_INVALID *) +let invalid_ports = +{ + local = Xeneventchn.of_int 0; + remote = 0 +} + +let string_of_port_pair p = + sprintf "(l %d, r %d)" (Xeneventchn.to_int p.local) p.remote + type t = { id: Xenctrl.domid; mfn: nativeint; interface: Xenmmap.mmap_interface; eventchn: Event.t; - mutable remote_port: int; - mutable port: Xeneventchn.t option; + mutable ports: port_pair; mutable bad_client: bool; mutable io_credit: int; (* the rounds of ring process left to do, default is 0, usually set to 1 when there is work detected, could @@ -41,8 +58,8 @@ let is_dom0 d = d.id = 0 let get_id domain = domain.id let get_interface d = d.interface let get_mfn d = d.mfn -let get_remote_port d = d.remote_port -let get_port d = d.port +let get_remote_port d = d.ports.remote +let get_local_port d = d.ports.local let is_bad_domain domain = domain.bad_client let mark_as_bad domain = domain.bad_client <- true @@ -56,54 +73,36 @@ let is_paused_for_conflict dom = dom.conflict_credit <= 0.0 let is_free_to_conflict = is_dom0 -let string_of_port = function - | None -> "None" - | Some x -> string_of_int (Xeneventchn.to_int x) - let dump d chan = - fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.remote_port + fprintf chan "dom,%d,%nd,%d\n" d.id d.mfn d.ports.remote let rebind_evtchn d remote_port = - begin match d.port with - | None -> () - | Some p -> Event.unbind d.eventchn p - end; + Event.unbind d.eventchn d.ports.local; let local = Event.bind_interdomain d.eventchn d.id remote_port in - debug "domain %d rebind (l %s, r %d) => (l %d, r %d)" - d.id (string_of_port d.port) d.remote_port - (Xeneventchn.to_int local) remote_port; - d.remote_port <- remote_port; - d.port <- Some (local) + let ports = { local; remote = remote_port } in + debug "domain %d rebind %s => %s" + d.id (string_of_port_pair d.ports) (string_of_port_pair ports); + d.ports <- ports let notify dom = - match dom.port with - | None -> warn "domain %d: attempt to notify on unknown port" dom.id - | Some port -> Event.notify dom.eventchn port - -let bind_interdomain dom = - begin match dom.port with - | None -> () - | Some port -> Event.unbind dom.eventchn port - end; - dom.port <- Some (Event.bind_interdomain dom.eventchn dom.id dom.remote_port); - debug "bound domain %d remote port %d to local port %s" dom.id dom.remote_port (string_of_port dom.port) - + Event.notify dom.eventchn dom.ports.local let close dom = - debug "domain %d unbound port %s" dom.id (string_of_port dom.port); - begin match dom.port with - | None -> () - | Some port -> Event.unbind dom.eventchn port - end; + debug "domain %d unbind %s" dom.id (string_of_port_pair dom.ports); + Event.unbind dom.eventchn dom.ports.local; + dom.ports <- invalid_ports; Xenmmap.unmap dom.interface -let make id mfn remote_port interface eventchn = { +let make id mfn remote_port interface eventchn = + let local = Event.bind_interdomain eventchn id remote_port in + let ports = { local; remote = remote_port } in + debug "domain %d bind %s" id (string_of_port_pair ports); +{ id = id; mfn = mfn; - remote_port = remote_port; + ports; interface = interface; eventchn = eventchn; - port = None; bad_client = false; io_credit = 0; conflict_credit = !Define.conflict_burst_limit; diff --git a/tools/ocaml/xenstored/domains.ml b/tools/ocaml/xenstored/domains.ml index 26018ac0dd3d..2ab0c5f4d8d0 100644 --- a/tools/ocaml/xenstored/domains.ml +++ b/tools/ocaml/xenstored/domains.ml @@ -126,7 +126,6 @@ let create doms domid mfn remote_port = let interface = Xenctrl.map_foreign_range xc domid (Xenmmap.getpagesize()) mfn in let dom = Domain.make domid mfn remote_port interface doms.eventchn in Hashtbl.add doms.table domid dom; - Domain.bind_interdomain dom; dom let xenstored_kva = ref "" @@ -144,7 +143,6 @@ let create0 doms = let dom = Domain.make 0 Nativeint.zero remote_port interface doms.eventchn in Hashtbl.add doms.table 0 dom; - Domain.bind_interdomain dom; Domain.notify dom; dom
Inter-domain event channels are always a pair of local and remote ports. Right now the handling is asymmetric, caused by the fact that the evtchn is bound after the associated Domain object is constructed. First, move binding of the event channel into the Domain.make() constructor. This means the local port no longer needs to be an option. It also removes the final callers of Domain.bind_interdomain. Next, introduce a new port_pair type to encapsulate the fact that these two should be updated together, and replace the previous port and remote_port fields. This refactoring also changes the Domain.get_port interface (removing an option) so take the opportunity to name it get_local_port instead. Also, this fixes a use-after-free risk with Domain.close. Once the evtchn has been unbound, the same local port number can be reused for a different purpose, so explicitly invalidate the ports to prevent their accidental misuse in the future. This also cleans up some of the debugging, to always print a port pair. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Christian Lindig <christian.lindig@citrix.com> CC: David Scott <dave@recoil.org> CC: Edwin Torok <edvin.torok@citrix.com> CC: Rob Hoes <Rob.Hoes@citrix.com> v2: * New --- tools/ocaml/xenstored/connections.ml | 9 +---- tools/ocaml/xenstored/domain.ml | 75 ++++++++++++++++++------------------ tools/ocaml/xenstored/domains.ml | 2 - 3 files changed, 39 insertions(+), 47 deletions(-)