[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems


  • To: Andrew Cooper <amc96@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 18 Nov 2021 13:59:10 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=x896/+HDLB8updnHKtmxVhOZ8ahF8apnlW70H+1+4uw=; b=IhVEGQNUpCM+16RAtPpcKnI58k3xZTZCA8iNgDcNb/7bJgkslQ6I/q84oficHzuEuOFVOprdAOrAZZ/W5aQMyTOEtjQwp5y5MGO1atK9WM2yoI2wLwa29CH15lWWlr8a3SObQPWDKfKAr0RsOhIO0hp2XtEobltYOj9vXwkv7GfMin6lXUdMlGDbj+Lx1yHw249UgJ7HV3Vm86BpDCtnXDzh7kS1Xro6W+uqAvjXN8dMonaswvhsBQ11/AYtTZDWkAiSRYL9FnKZOedQQ5j0D2nejbyjQmhK0l/o6nbybjdltrXrRyJaxrDdA//bFjlw/peiTlkycKFcXWu+7c3JeQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cWl887JV+3RS0XafvagLZbcMeLs7DnSvOubJTLt8hRo3v1qFbPVgkmRjumZORImvFpPFdjmAoFxbf5ukHJGAJdN/rTQ836N6XHqwHN475pnno5OJ0HbNlb7oqi/QTpQnj6oiAp8cZvyR5jYohfEzh+ELUt78fuZ1I68oawV+Exu2A7DeOIBGlpZh2aE1YHI/LRxeg16bgickHlPeYK/SRZ3wafdF3cBjYgYqX4Wd+nQCK6tD8HcM4OGV25izQeq5LfsIDX8GudyQIdAh23awiod6rDvWg6vpdErdR6QVGoh9nkhCNNANfS6cVUovD7RHG7V6s7qDV+81h97GAcQQFQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 18 Nov 2021 12:59:17 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 18.11.2021 00:14, Andrew Cooper wrote:
> On 08/11/2021 09:50, Jan Beulich wrote:
>> On 05.11.2021 14:55, Andrew Cooper wrote:
>>> Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() which 
>>> means
>>> that if any other XSM module registers a handler, we'll break the hypercall
>>> ABI totally by having the behaviour depend on the selected XSM module.
>>>
>>> There are 2 options:
>>>   1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op.  If another XSM
>>>      module wants hypercalls, they'll have to introduce a new top-level
>>>      hypercall.
>>>   2) Make the current flask_op() be common, and require new hypercalls to 
>>> fit
>>>      compatibly with xen_flask_op_t.  This can be done fairly easily by
>>>      subdividing the .cmd space.
>>>
>>> In this patch, we implement option 2.
>>>
>>> Move the stub {do,compat}_xsm_op() implementation into a new xsm_op.c so we
>>> can more easily do multi-inclusion compat magic.  Also add a new private.h,
>>> because flask/hook.c's local declaration of {do,compat}_flask_op() wasn't
>>> remotely safe.
>>>
>>> The top level now dispatches into {do,compat}_flask_op() based on op.cmd, 
>>> and
>>> handles the primary copy in/out.
>> I'm not convinced this is the only reasonable way of implementing 2).
>> I could also see number space to be separated in different ways, ...
>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>
>>> Only lightly tested.  Slightly RFC.  There are several things which aren't
>>> great, but probably want addressing in due course.
>>>
>>>   1) The public headers probably want to lose the flask name (in a 
>>> compatible
>>>      way), now that the hypercall is common.  This probably wants to be
>>>      combined with no longer taking a void handle.
>> ... leaving per-module public headers and perhaps merely adding an
>> abstracting
>>
>> struct xen_xsm_op_t {
>>      uint32_t op;
>>      ... /* placeholder */
>> };
>>
>> or (making explicit one possible variant of number space splitting)
>>
>> union xen_xsm_op_t {
>>      uint32_t op;
>>      struct {
>>          uint16_t cmd;
>>          uint16_t mod;
>>      } u;
>>      ... /* placeholder */
>> };
>>
>> in, say, a new public/xsm.h.
> 
> That doesn't work.  The ABI is fixed at sizeof(xen_flask_op_t) for all 
> other modules, because a caller which doesn't know which module is in 
> use must not have Xen over-read/write the object passed while it's 
> trying to figure things out.

Well, imo figuring out which module is in use should be via a sysctl.
Then there would be no restrictions by one module's interface
definitions on other modules.

>> As a result I consider this change either going too far (because of
>> not knowing future needs) or not far enough (by e.g. leaving
>> do_xsm_op() to use xen_flask_op_t.
> 
> Well - what it does is prevent someone from breaking the ABI with 
> literally this patch
> 
> diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c
> index 3550dded7b4e..36b82fd3bd3e 100644
> --- a/xen/xsm/silo.c
> +++ b/xen/xsm/silo.c
> @@ -100,6 +100,11 @@ static int silo_argo_send(const struct domain *d1, 
> const struct domain *d2)
> 
>   #endif
> 
> +static long silo_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op)
> +{
> +    /* ... */
> +}
> +
>   static const struct xsm_ops __initconstrel silo_xsm_ops = {
>       .evtchn_unbound = silo_evtchn_unbound,
>       .evtchn_interdomain = silo_evtchn_interdomain,
> @@ -110,6 +115,7 @@ static const struct xsm_ops __initconstrel 
> silo_xsm_ops = {
>       .argo_register_single_source = silo_argo_register_single_source,
>       .argo_send = silo_argo_send,
>   #endif
> +    .do_xsm_op = silo_do_xsm_op,
>   };
> 
>   const struct xsm_ops *__init silo_init(void)

So I'm afraid I don't see any ABI breakage here. Provided of course
silo_do_xsm_op() avoids the FLASK_* number space and uses a struct
layout compatible with the head of struct xen_flask_op.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.