Andrew Cooper, on Tue 10 Nov 2015 10:46:44 +0000, wrote:
> ml_interface_{read,write}() would miscalculate the quantity of
> data/space in the ring if it crossed the ring boundary, and incorrectly
> return a short read/write.
>
> This causes a protocol stall, as either side of the ring ends up waiting
> for what they believe to be the other side needing to take the next
> action.
>
> Correct the calculations to cope with crossing the ring boundary.
>
> In addition, correct the error detection. It is a hard error if the
> producer index gets more than a ring size ahead of the consumer, or if
> the consumer ever overtakes the producer.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Reviewed-by: Samuel Thibault <samuel.thiba...@ens-lyon.org>
> ---
> CC: Ian Campbell <ian.campb...@citrix.com>
> CC: Ian Jackson <ian.jack...@eu.citrix.com>
> CC: Wei Liu <wei.l...@citrix.com>
> CC: David Scott <d...@recoil.org>
> CC: Samuel Thibault <samuel.thiba...@ens-lyon.org>
>
> v2:
> * Correct error handling adjustments
> * More fixes to space calculations
> * Fix whitespace errors
> * No longer RFC - passed XenServer sanity testing
> ---
> tools/ocaml/libs/xb/xs_ring_stubs.c | 64
> +++++++++++++++++++++++++------------
> 1 file changed, 44 insertions(+), 20 deletions(-)
>
> diff --git a/tools/ocaml/libs/xb/xs_ring_stubs.c
> b/tools/ocaml/libs/xb/xs_ring_stubs.c
> index fd561a2..4737870 100644
> --- a/tools/ocaml/libs/xb/xs_ring_stubs.c
> +++ b/tools/ocaml/libs/xb/xs_ring_stubs.c
> @@ -50,7 +50,7 @@ CAMLprim value ml_interface_read(value ml_interface,
>
> struct xenstore_domain_interface *intf = interface->addr;
> XENSTORE_RING_IDX cons, prod; /* offsets only */
> - int to_read;
> + int total_data, data;
> uint32_t connection;
>
> cons = *(volatile uint32_t*)&intf->req_cons;
> @@ -65,19 +65,28 @@ CAMLprim value ml_interface_read(value ml_interface,
> if ((prod - cons) > XENSTORE_RING_SIZE)
> caml_failwith("bad connection");
>
> - if (prod == cons) {
> + /* Check for any pending data at all. */
> + total_data = prod - cons;
> + if (total_data == 0) {
> + /* No pending data at all. */
> result = 0;
> goto exit;
> }
> - cons = MASK_XENSTORE_IDX(cons);
> - prod = MASK_XENSTORE_IDX(prod);
> - if (prod > cons)
> - to_read = prod - cons;
> - else
> - to_read = XENSTORE_RING_SIZE - cons;
> - if (to_read < len)
> - len = to_read;
> - memcpy(buffer, intf->req + cons, len);
> + else if (total_data < len)
> + /* Some data - make a partial read. */
> + len = total_data;
> +
> + /* Check whether data crosses the end of the ring. */
> + data = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons);
> + if (len < data)
> + /* Data within the remaining part of the ring. */
> + memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), len);
> + else {
> + /* Data crosses the ring boundary. Read both halves. */
> + memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), data);
> + memcpy(buffer + data, intf->req, len - data);
> + }
> +
> xen_mb();
> intf->req_cons += len;
> result = len;
> @@ -100,7 +109,7 @@ CAMLprim value ml_interface_write(value ml_interface,
>
> struct xenstore_domain_interface *intf = interface->addr;
> XENSTORE_RING_IDX cons, prod;
> - int can_write;
> + int total_space, space;
> uint32_t connection;
>
> cons = *(volatile uint32_t*)&intf->rsp_cons;
> @@ -111,17 +120,32 @@ CAMLprim value ml_interface_write(value ml_interface,
> caml_raise_constant(*caml_named_value("Xb.Reconnect"));
>
> xen_mb();
> - if ( (prod - cons) >= XENSTORE_RING_SIZE ) {
> +
> + if ((prod - cons) > XENSTORE_RING_SIZE)
> + caml_failwith("bad connection");
> +
> + /* Check for space to write the full message. */
> + total_space = XENSTORE_RING_SIZE - (prod - cons);
> + if (total_space == 0) {
> + /* No space at all - exit having done nothing. */
> result = 0;
> goto exit;
> }
> - if (MASK_XENSTORE_IDX(prod) >= MASK_XENSTORE_IDX(cons))
> - can_write = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
> - else
> - can_write = MASK_XENSTORE_IDX(cons) - MASK_XENSTORE_IDX(prod);
> - if (can_write < len)
> - len = can_write;
> - memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
> + else if (total_space < len)
> + /* Some space - make a partial write. */
> + len = total_space;
> +
> + /* Check for space until the ring wraps. */
> + space = XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod);
> + if (len < space)
> + /* Message fits inside the remaining part of the ring. */
> + memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len);
> + else {
> + /* Message wraps around the end of the ring. Write both halves.
> */
> + memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space);
> + memcpy(intf->rsp, buffer + space, len - space);
> + }
> +
> xen_mb();
> intf->rsp_prod += len;
> result = len;
> --
> 2.1.4
>
--
Samuel
As usual, this being a 1.3.x release, I haven't even compiled this
kernel yet. So if it works, you should be doubly impressed.
(Linus Torvalds, announcing kernel 1.3.3 on the linux-kernel mailing list.)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
