Re: [PATCH v7 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq

[Christopher Clark <christopher.w.clark@gmail.com>]

On Mon, Feb 4, 2019 at 6:41 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 31.01.19 at 05:28, <christopher.w.clark@gmail.com> wrote:
> > @@ -1237,6 +1864,54 @@ compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> >          break;
> >      }
> >
> > +    case XEN_ARGO_OP_sendv:
> > +    {
> > +        xen_argo_send_addr_t send_addr;
> > +        xen_argo_iov_t iovs[XEN_ARGO_MAXIOV];
> > +        unsigned int i;
> > +        unsigned int niov;
> > +
> > +        XEN_GUEST_HANDLE_PARAM(xen_argo_send_addr_t) send_addr_hnd =
> > +            guest_handle_cast(arg1, xen_argo_send_addr_t);
> > +        /* arg2: iovs, arg3: niov, arg4: message_type */
> > +
> > +        rc = copy_from_guest(&send_addr, send_addr_hnd, 1) ? -EFAULT : 0;
> > +        if ( rc )
> > +            break;
> > +
> > +        if ( unlikely(arg3 > XEN_ARGO_MAXIOV) )
> > +        {
> > +            rc = -EINVAL;
> > +            break;
> > +        }
> > +        niov = array_index_nospec(arg3, XEN_ARGO_MAXIOV + 1);
> > +
> > +        /*
> > +         * Limited scope for compat_iovs array: enables a single copy_from_guest
> > +         * call and discards the array from the stack before calling sendv.
> > +         */
>
> What makes you think the array gets removed from the stack again
> before the call? The typical way of setting up stack frames for a
> function is to allocate the full chunk of space needed at the start
> of the function, and remove it before returning. Without the
> argo_dprintk() after the switch() there would be the potential of
> the sendv() carried out as a tail call, but you can't rely on that.

OK. I've revised the comment.

> With the current XEN_ARGO_MAXIOV value of 8 the overall frame
> size is still tolerable, I would say. But I think you want to add
> BUILD_BUG_ON()s here and in the native handler, such that
> careless bumping of the value won't go unnoticed (but also see
> below).

ack, I've added BUILD_BUG_ON to both.

>
> > --- a/xen/include/public/argo.h
> > +++ b/xen/include/public/argo.h
> > @@ -43,6 +43,28 @@
> >  /* Fixed-width type for "argo port" number. Nothing to do with evtchns. */
> >  typedef uint32_t xen_argo_port_t;
> >
> > +/*
> > + * XEN_ARGO_MAXIOV : maximum number of iovs accepted in a single sendv.
> > + * Caution is required if this value is increased: this determines the size of
> > + * an array of xen_argo_iov_t structs on the hypervisor stack, so could cause
> > + * stack overflow if the value is too large.
> > + * The Linux Argo driver never passes more than two iovs.
> > + *
> > + * This value should also not exceed 128 to ensure that the total amount of data
> > + * posted in a single Argo sendv operation cannot exceed 2^31 bytes, to reduce
> > + * risk of integer overflow defects:
> > + * Each argo iov can hold ~ 2^24 bytes, so XEN_ARGO_MAXIOV <= 2^(31-24),
> > + * ie. keep XEN_ARGO_MAXIOV <= 128.
> > +*/
> > +#define XEN_ARGO_MAXIOV          8U
>
> How does 2^31 come into play here? uint32_t can hold up to 2^32, and
> you shouldn't be using signed arithmetic anywhere by this time anymore.
> I'm also struggling to see what the "~ 2^24 bytes" refers to - I see nothing
> along these lines added to the public header, and ...
>
> > +typedef struct xen_argo_iov
> > +{
> > +    XEN_GUEST_HANDLE(uint8) iov_hnd;
> > +    uint32_t iov_len;
>
> ... the field here allows for 2^32-1. Oh, it's XEN_ARGO_MAX_RING_SIZE.
> It would help if the comment cross referenced that name.

I've removed the second paragraph of the comment entirely as it's no longer
accurate or required due to the bounds checking in iov_count.

> Btw., neither of these two maximum values look to be architectural limits,
> so I wonder whether, before declaring the ABI stable, these constants
> shouldn't be purged and replaced by settings the guest is to retrieve via
> hypercall.

That could potentially be useful; though it hasn't been necessary so far.
(fwiw: A determined guest can already retrieve these settings via hypercall.)

To make Argo's current Experimental status clearer, with the ABI stability
status that accords, I propose the following addition to SUPPORT.md:

Within section: ## Virtual Hardware, Hypervisor

### Argo: Inter-domain message delivery by hypercall.

    Status: Experimental

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel