* [PATCH for-4.8] tools/oxenstored: Fix transaction handling in 32bit builds
@ 2016-10-31 13:21 Andrew Cooper
2016-10-31 14:19 ` Wei Liu
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2016-10-31 13:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Wei Liu, David Scott
In a 32bit build, the ocaml code 'proposed_id >= 0x7fffffff' compiles to:
8055eac: 83 fb ff cmp $0xffffffff,%ebx
8055eaf: 7d 0f jge 8055ec0 <...+0x20>
which in C is 'proposed_id >= INT_MIN', or in other words, tautologically
true. As a result, 32bit builds of oxenstored always try to allocate the
transaction id 1, and fall into an infinite loop of trying the next id if
transaction 1 is already in use.
Restrict the range down to 1 billion, to sit in the positive half of a 31 bit
ocaml integer. The compiled code is now:
8055eac: b9 ff ff ff 7f mov $0x7fffffff,%ecx
8055eb1: 39 cb cmp %ecx,%ebx
8055eb3: 7d 0b jge 8055ec0 <...+0x20>
which (other than non-optimal code generation because of the unnecessary use
of %ecx), isn't unconditionally true.
In principle, the check could be changed to 'proposed_id == 0x7fffffff' which
would still allow for 2 billion transaction in 32bit builds. However, in
64bit builds, this reintroduces a risk that if proposed_id is initially
greater than 0x7fffffff, it will not be clipped suitably into range.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: David Scott <dave@recoil.org>
---
tools/ocaml/xenstored/connection.ml | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml
index 0b47009..3ffd35b 100644
--- a/tools/ocaml/xenstored/connection.ml
+++ b/tools/ocaml/xenstored/connection.ml
@@ -218,8 +218,16 @@ let fire_watch watch path =
(* 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
+ (*
+ * Clip proposed_id to the range [1, 0x3ffffffe]
+ *
+ * The chosen id must not trucate when written into the uint32_t tx_id
+ * field, and needs to fit within the positive range of a 31 bit ocaml
+ * integer to function when compiled as 32bit.
+ *
+ * Oxenstored therefore supports only 1 billion open transactions.
+ *)
+ let id = if proposed_id <= 0 || proposed_id >= 0x3fffffff then 1 else proposed_id in
if Hashtbl.mem con.transactions id then (
(* Outstanding transaction with this id. Try the next. *)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH for-4.8] tools/oxenstored: Fix transaction handling in 32bit builds
2016-10-31 13:21 [PATCH for-4.8] tools/oxenstored: Fix transaction handling in 32bit builds Andrew Cooper
@ 2016-10-31 14:19 ` Wei Liu
2016-10-31 20:51 ` David Scott
0 siblings, 1 reply; 4+ messages in thread
From: Wei Liu @ 2016-10-31 14:19 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, David Scott, Ian Jackson, Xen-devel
On Mon, Oct 31, 2016 at 01:21:56PM +0000, Andrew Cooper wrote:
> In a 32bit build, the ocaml code 'proposed_id >= 0x7fffffff' compiles to:
>
> 8055eac: 83 fb ff cmp $0xffffffff,%ebx
> 8055eaf: 7d 0f jge 8055ec0 <...+0x20>
>
> which in C is 'proposed_id >= INT_MIN', or in other words, tautologically
> true. As a result, 32bit builds of oxenstored always try to allocate the
> transaction id 1, and fall into an infinite loop of trying the next id if
> transaction 1 is already in use.
>
> Restrict the range down to 1 billion, to sit in the positive half of a 31 bit
> ocaml integer. The compiled code is now:
>
> 8055eac: b9 ff ff ff 7f mov $0x7fffffff,%ecx
> 8055eb1: 39 cb cmp %ecx,%ebx
> 8055eb3: 7d 0b jge 8055ec0 <...+0x20>
>
> which (other than non-optimal code generation because of the unnecessary use
> of %ecx), isn't unconditionally true.
>
> In principle, the check could be changed to 'proposed_id == 0x7fffffff' which
> would still allow for 2 billion transaction in 32bit builds. However, in
> 64bit builds, this reintroduces a risk that if proposed_id is initially
> greater than 0x7fffffff, it will not be clipped suitably into range.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH for-4.8] tools/oxenstored: Fix transaction handling in 32bit builds
2016-10-31 14:19 ` Wei Liu
@ 2016-10-31 20:51 ` David Scott
2016-10-31 21:03 ` Wei Liu
0 siblings, 1 reply; 4+ messages in thread
From: David Scott @ 2016-10-31 20:51 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Ian Jackson, Xen-devel
> On 31 Oct 2016, at 14:19, Wei Liu <wei.liu2@citrix.com> wrote:
>
> On Mon, Oct 31, 2016 at 01:21:56PM +0000, Andrew Cooper wrote:
>> In a 32bit build, the ocaml code 'proposed_id >= 0x7fffffff' compiles to:
>>
>> 8055eac: 83 fb ff cmp $0xffffffff,%ebx
>> 8055eaf: 7d 0f jge 8055ec0 <...+0x20>
>>
>> which in C is 'proposed_id >= INT_MIN', or in other words, tautologically
>> true. As a result, 32bit builds of oxenstored always try to allocate the
>> transaction id 1, and fall into an infinite loop of trying the next id if
>> transaction 1 is already in use.
>>
>> Restrict the range down to 1 billion, to sit in the positive half of a 31 bit
>> ocaml integer. The compiled code is now:
>>
>> 8055eac: b9 ff ff ff 7f mov $0x7fffffff,%ecx
>> 8055eb1: 39 cb cmp %ecx,%ebx
>> 8055eb3: 7d 0b jge 8055ec0 <...+0x20>
>>
>> which (other than non-optimal code generation because of the unnecessary use
>> of %ecx), isn't unconditionally true.
>>
>> In principle, the check could be changed to 'proposed_id == 0x7fffffff' which
>> would still allow for 2 billion transaction in 32bit builds. However, in
>> 64bit builds, this reintroduces a risk that if proposed_id is initially
>> greater than 0x7fffffff, it will not be clipped suitably into range.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: David Scott <dave@recoil.org>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH for-4.8] tools/oxenstored: Fix transaction handling in 32bit builds
2016-10-31 20:51 ` David Scott
@ 2016-10-31 21:03 ` Wei Liu
0 siblings, 0 replies; 4+ messages in thread
From: Wei Liu @ 2016-10-31 21:03 UTC (permalink / raw)
To: David Scott; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, Xen-devel
On Mon, Oct 31, 2016 at 08:51:07PM +0000, David Scott wrote:
>
> > On 31 Oct 2016, at 14:19, Wei Liu <wei.liu2@citrix.com> wrote:
> >
> > On Mon, Oct 31, 2016 at 01:21:56PM +0000, Andrew Cooper wrote:
> >> In a 32bit build, the ocaml code 'proposed_id >= 0x7fffffff' compiles to:
> >>
> >> 8055eac: 83 fb ff cmp $0xffffffff,%ebx
> >> 8055eaf: 7d 0f jge 8055ec0 <...+0x20>
> >>
> >> which in C is 'proposed_id >= INT_MIN', or in other words, tautologically
> >> true. As a result, 32bit builds of oxenstored always try to allocate the
> >> transaction id 1, and fall into an infinite loop of trying the next id if
> >> transaction 1 is already in use.
> >>
> >> Restrict the range down to 1 billion, to sit in the positive half of a 31 bit
> >> ocaml integer. The compiled code is now:
> >>
> >> 8055eac: b9 ff ff ff 7f mov $0x7fffffff,%ecx
> >> 8055eb1: 39 cb cmp %ecx,%ebx
> >> 8055eb3: 7d 0b jge 8055ec0 <...+0x20>
> >>
> >> which (other than non-optimal code generation because of the unnecessary use
> >> of %ecx), isn't unconditionally true.
> >>
> >> In principle, the check could be changed to 'proposed_id == 0x7fffffff' which
> >> would still allow for 2 billion transaction in 32bit builds. However, in
> >> 64bit builds, this reintroduces a risk that if proposed_id is initially
> >> greater than 0x7fffffff, it will not be clipped suitably into range.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>
> Acked-by: David Scott <dave@recoil.org>
>
Applied. Thanks.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-31 21:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-31 13:21 [PATCH for-4.8] tools/oxenstored: Fix transaction handling in 32bit builds Andrew Cooper
2016-10-31 14:19 ` Wei Liu
2016-10-31 20:51 ` David Scott
2016-10-31 21:03 ` Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.