Re: [Xen-devel] [PATCH v7 07/15] argo: implement the register op
n Thu, Jan 31, 2019 at 8:19 AM Roger Pau Monné wrote:
>
> 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
> > Tested-by: Chris Patterson
>
> Just one style issue below that should be fixed before commit, and two
> comments:
>
> Reviewed-by: Roger Pau Monné
Thanks
> > +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
ack
> [...]
> > +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.
ack, thanks.
> > @@ -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.
ack, fixed.
Christopher
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 07/15] argo: implement the register op
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
> Tested-by: Chris Patterson
Just one style issue below that should be fixed before commit, and two
comments:
Reviewed-by: Roger Pau Monné
> +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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v7 07/15] argo: implement the register op
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 Tested-by: Chris Patterson --- v6 #09 Jan: add compat ABI v6 #07 Jan: add argo_message_header to xlat.lst and invoke the CHECK v6 #07 Jan: xlat.lst: move argo struct entries to alphabetical position v5 #07 Roger: add BUILD_BUG_ON for MAX_RING_SIZE, PAGE_SIZE v5 #07 Roger: gprintk(XENLOG_ERR,.. for denied existing ring v5: add compat validation macros to primary source file: common/argo.c v5 : convert hypercall arg structs to struct form for compat checking v5: dropped external file for compat macros: common/compat/argo.c v4 v3#07 Jan: shrink critical sections in register_ring v4 v3#07 Jan: revise register flag MASK in header, note 32-bitness of args v4 feedback: use standard data structures per common code, not loop macros v4 Andrew: use the single argo command line option list v4 #07 Jan: rewrite find_ring_mfn to use check_get_page_from_gfn v4 #07 Roger: add FIXME to ring_map_page for vmap contiguous ring mapping v3 #07 Jan: comment: minimum ring size is based on minimum-sized message v3 #04 Andrew: reference CONFIG_ARGO in the command line documentation v3 #07 Jan: register_ring: fold else, if into else-if to drop indent v3 #07 Jan: remove no longer used guest_handle_is_aligned macros v3 #07 Jan: remove dead code from find_ring_mfns v3 #07 Jan: fix format string indention in printks v3 #07 Jan: remove redundant bounds check on npage in find_ring_mfns v3 #08 self/Roger: improve dprintk output in find_ring_info like find_send_info v3 #07 Jan: rename ring_find_info to find_ring_info v3 #07 Jan: use array_index_nospec in ring_map_page v3 #07 Jan: fix numeric entries in printk format strings v3 #7 Jan: drop unneeded parentheses from ROUNDUP_MESSAGE defn v3 #10 Roger: move find functions to top of file and drop prototypes v3 #03 meld compat check for hypercall arg register struct v3 #04 Roger/Jan: make lock names clearer and assert their state v3 #04 Jan: port -> aport with type; distinguish argo port from evtchn v3 feedback #07 Eric: fix header max ring size comment units v3 feedback #04 Roger: mfn_mapping: void* instead of uint8_t* v3 use %u for printing unsigned ints in find_ring_mfns v3 feedback #04 Jan: uint32_t -> unsigned int for npage in register_ring v3 feedback #04 Roger: drop npages struct member, calculate from len v3 : register_ring: uint32_t -> unsigned int for private_tx_ptr v3 feedback Roger/Jan: ASSERT currd is current->domain or use 'd' variable name v3 feedback #07 Roger: use opt_argo_mac_permissive : a boolean opt v3 feedback #04 Roger: reorder #includes to alphabetical order v3 feedback #07 Roger: drop comment re: Intel EPT/AMD NPT for write-only mapping v3 feedback #07 Roger: drop ptr arithmetic in update_tx_ptr, use ring struct cast v3 feedback #07 Roger: drop newline in ring_map_page v3 feedback #07 Roger: drop unneeded null check before xfree v3 feedback #07 Roger: use return and drop out label in register_ring v3 Stefano: add 4K page constraint to header file comment & commit msg v3 Julien/Stefano: 4K granularity ok: use 64-bit gfns in register interface v2 self: disallow ring resize via reregister v2 feedback Jan: drop cookie, implement teardown v2 feedback Jan: drop message from argo_message_op v2 self: move hash_index function below locking comment v2 self: OVERHAUL v2 self/Jan: remove use of magic verification field and tidy up v2 self: merge max and min ring size check clauses v2 feedback v1#13 Roger: use OS-supplied roundup; drop from public header v2 feedback #9, Jan: use the argo-mac bootparam at point of introduction v2 feedback #9, Jan: rename boot opt va
