Re: [Xen-devel] [PATCH v7 07/15] argo: implement the register op

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 30, 2019 at 08:28:12PM -0800, Christopher Clark wrote:
> The register op is used by a domain to register a region of memory for
> receiving messages from either a specified other domain, or, if specifying a
> wildcard, any domain.
> 
> This operation creates a mapping within Xen's private address space that
> will remain resident for the lifetime of the ring. In subsequent commits,
> the hypervisor will use this mapping to copy data from a sending domain into
> this registered ring, making it accessible to the domain that registered the
> ring to receive data.
> 
> Wildcard any-sender rings are default disabled and registration will be
> refused with EPERM unless they have been specifically enabled with the
> new mac-permissive flag that is added to the argo boot option here. The
> reason why the default for wildcard rings is 'deny' is that there is
> currently no means to protect the ring from DoS by a noisy domain
> spamming the ring, affecting other domains ability to send to it. This
> will be addressed with XSM policy controls in subsequent work.
> 
> Since denying access to any-sender rings is a significant functional
> constraint, the new option "mac-permissive" for the argo bootparam
> enables overriding this. eg: "argo=1,mac-permissive=1"
> 
> The p2m type of the memory supplied by the guest for the ring must be
> p2m_ram_rw and the memory will be pinned as PGT_writable_page while the ring
> is registered.
> 
> This hypercall op and its interface currently only supports 4K-sized pages.
> 
> Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
> Tested-by: Chris Patterson <pattersonc@xxxxxxxxxxxx>

Just one style issue below that should be fixed before commit, and two
comments:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> +static int
> +ring_map_page(const struct domain *d, struct argo_ring_info *ring_info,
> +              unsigned int i, void **out_ptr)
> +{
> +    ASSERT(LOCKING_L3(d, ring_info));
> +
> +    /*
> +     * FIXME: Investigate using vmap to create a single contiguous virtual
> +     * address space mapping of the ring instead of using the array of single
> +     * page mappings.
> +     * Affects logic in memcpy_to_guest_ring, the mfn_mapping array data
> +     * structure, and places where ring mappings are added or removed.
> +     */
> +
> +    if ( i >= ring_info->nmfns )
> +    {
> +        gprintk(XENLOG_ERR,
> +               "argo: ring (vm%u:%x vm%u) %p attempted to map page %u of 
> %u\n",
> +                ring_info->id.domain_id, ring_info->id.aport,
> +                ring_info->id.partner_id, ring_info, i, ring_info->nmfns);
> +        return -ENOMEM;
> +    }
> +    i = array_index_nospec(i, ring_info->nmfns);
> +
> +    if ( !ring_info->mfns || !ring_info->mfn_mapping)
                                                       ^ missing space
[...]
> +static int
> +copy_gfn_from_handle(XEN_GUEST_HANDLE_PARAM(void) gfn_hnd, bool compat,
> +                     unsigned int i, gfn_t *out_gfn)
> +{
> +    int ret;
> +
> +#ifdef CONFIG_COMPAT
> +    if ( compat )
> +    {
> +        XEN_GUEST_HANDLE_PARAM(compat_pfn_t) c_gfn_hnd =
> +            guest_handle_cast(gfn_hnd, compat_pfn_t);
> +        compat_pfn_t c_gfn;
> +
> +        ret = __copy_from_guest_offset(&c_gfn, c_gfn_hnd, i, 1) ? -EFAULT : 
> 0;
> +        *out_gfn = _gfn(c_gfn);
> +    }
> +    else

AFAICT you could place the #endif here and avoid the one below.

> @@ -579,7 +1105,6 @@ compat_argo_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg1,
>      argo_dprintk("<-compat_argo_op(%u)=%ld\n", cmd, rc);
>  
>      return rc;
> -

This looks like a stray fix that should have gone into a different
patch.

Roger.

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