Re: [Xen-devel] [PATCH for-4.8] tools/oxenstored: Avoid allocating invalid transaction ids

[David Scott <dave@xxxxxxxxxx>]

> On 26 Oct 2016, at 10:34, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> 
> The transaction id of 0 is reserved, meaning "not in a transaction".  It is up
> to the xenstored server to allocate transaction ids.  While oxenstored starts
> its ids at 1, but insufficient care is taken with truncation cases.
> 
> A 32bit oxenstored has an int with 31 bits of width, meaning that the
> transaction id will wrap around to 0 after 2 billion transactions.
> 
> A 64bit oxenstored has an int with 63 bits of width, meaning that once 4
> billion transactions are used, the allocated id will be truncated when written
> into the uin32_t field in the ring.  This causes the client to reply with the
> truncated id, breaking any further attempt to use any transactions.
> 
> Limit all transaction ids to the range between 1 and 0x7ffffffe.  This is the
> best which can be done without making oxenstored depend on Stdint or Cstruct,
> yet still work for 32bit builds.

Good catch, looks good to me!

> 
> Also check that the proposed new transaction id isn't currently in use.  For
> the first 2 billion transactions there is no chance of a collision, and after
> that, the chance is at most 20 (the default open transaction quota) in 2
> billion.

That makes sense to me. There seems little chance of the hash table filling up 
when the quota is set to 20 :-)

Acked-by: David Scott <dave@xxxxxxxxxx>

Cheers,
Dave

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: David Scott <dave@xxxxxxxxxx>
> ---
> tools/ocaml/xenstored/connection.ml | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/connection.ml 
> b/tools/ocaml/xenstored/connection.ml
> index b18336f..0b47009 100644
> --- a/tools/ocaml/xenstored/connection.ml
> +++ b/tools/ocaml/xenstored/connection.ml
> @@ -216,14 +216,23 @@ let fire_watch watch path =
>       let data = Utils.join_by_null [ new_path; watch.token; "" ] in
>       send_reply watch.con Transaction.none 0 Xenbus.Xb.Op.Watchevent data
> 
> -let find_next_tid con =
> -     let ret = con.next_tid in con.next_tid <- con.next_tid + 1; ret
> +(* Search for a valid unused transaction id. *)
> +let rec valid_transaction_id con proposed_id =
> +     (* Clip proposed_id to the range [1, 0x7ffffffe] *)
> +     let id = if proposed_id <= 0 || proposed_id >= 0x7fffffff then 1 else 
> proposed_id in
> +
> +     if Hashtbl.mem con.transactions id then (
> +             (* Outstanding transaction with this id.  Try the next. *)
> +             valid_transaction_id con (id + 1)
> +     ) else
> +             id
> 
> let start_transaction con store =
>       if !Define.maxtransaction > 0 && not (is_dom0 con)
>       && Hashtbl.length con.transactions > !Define.maxtransaction then
>               raise Quota.Transaction_opened;
> -     let id = find_next_tid con in
> +     let id = valid_transaction_id con con.next_tid in
> +     con.next_tid <- id + 1;
>       let ntrans = Transaction.make id store in
>       Hashtbl.add con.transactions id ntrans;
>       Logging.start_transaction ~tid:id ~con:(get_domstr con);
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel