Re: [Xen-devel] [PATCH] common/gnttab: Introduce command line feature controls

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/08/17 18:45, Juergen Gross wrote:
> On 24/08/17 17:16, Andrew Cooper wrote:
>> On 24/08/17 16:01, Juergen Gross wrote:
>>> On 24/08/17 16:50, Andrew Cooper wrote:
>>>> This patch was originally a workaround for XSA-226.  Since then, transitive
>>>> grants are believed to be functioning properly, and the defaults have 
>>>> changed
>>>> appropriately.
>>>>
>>>> However, for those people who chose to use the workaround (especially from 
>>>> an
>>>> attack surface mitigation point of view), retain the ability for the host
>>>> administrator to choose.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> CC: Tim Deegan <tim@xxxxxxx>
>>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>>> ---
>>>>  docs/misc/xen-command-line.markdown | 13 +++++++++++
>>>>  xen/common/grant_table.c            | 44 
>>>> +++++++++++++++++++++++++++++++++++--
>>>>  2 files changed, 55 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/docs/misc/xen-command-line.markdown 
>>>> b/docs/misc/xen-command-line.markdown
>>>> index 4002eab..78c7b51 100644
>>>> --- a/docs/misc/xen-command-line.markdown
>>>> +++ b/docs/misc/xen-command-line.markdown
>>>> @@ -868,6 +868,19 @@ Controls EPT related features.
>>>>  
>>>>  Specify which console gdbstub should use. See **console**.
>>>>  
>>>> +### gnttab
>>>> +> `= List of [ max_ver:<integer>, transitive ]`
>>>> +
>>>> +> Default: `gnttab=max_ver:2,transitive`
>>>> +
>>>> +Control various aspects of the grant table behaviour available to guests.
>>>> +
>>>> +* `max_ver` Select the maximum grant table version to offer to guests.  
>>>> Valid
>>>> +version are 1 and 2.
>>>> +* `transitive` Permit or disallow the use of transitive grants.  Note 
>>>> that the
>>>> +use of grant table v2 without transitive grants is an ABI breakage from 
>>>> the
>>>> +guests point of view.
>>> So shouldn't there be a way for the guest to query the support of
>>> transient grants?
>> Ideally yes, but how do you suggest doing this in a compatible way?
> An ELF note in the guest kernel indicating it is aware of that
> possibility in order to allow v2 grants for that guest without transient
> grants?
>
>> All Xen downstreams which haven't backported the eventual transitive
>> fixes will have this clobber in place, without any query-ability.
> So the only really compatible way would be to disallow v2 grants. OTOH
> I guess there aren't so many guests using v2 right now...
>
>> The reason this workaround was so effective, and why several large users
>> still wish to clobber them, is because nothing uses transitive grants.
> Right. And only very few guests use v2 grants.

We tried disabling gnttab v2 by default first, which is why the max_ver:
parameter is present in this patch.

Some vendor versions of Linux refused to fall back from gnttab v2 to
gnttab v1 on migrate, even though they are perfectly happy booting in a
v1-only situation.

Also
https://github.com/xenserver/win-xenbus/blob/c2a60d437b42f2349361629f15275e4fabdcc22e/src/xenbus/gnttab.c#L566-L571
which was discovered out of the blue, when all windows guests installed
on older versions of XenServer started turning blue.


Leaving v2 active and creating an ABI breakage turned out to be the
viable way to inhibit the use of transitive grants in cases which needed
to run arbitrary guests.

>
>>>> +
>>>>  ### gnttab\_max\_frames
>>>>  > `= <integer>`
>>>>  
>>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>>> index 36895aa..f9c313d 100644
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -50,6 +50,42 @@ integer_param("gnttab_max_nr_frames", 
>>>> max_nr_grant_frames);
>>>>  unsigned int __read_mostly max_grant_frames;
>>>>  integer_param("gnttab_max_frames", max_grant_frames);
>>>>  
>>>> +static unsigned int __read_mostly opt_gnttab_max_version = 2;
>>>> +static bool __read_mostly opt_transitive_grants = true;
>>>> +
>>>> +static void __init parse_gnttab(char *s)
>>> Do you mind using:
>>>
>>> static int __init parse_gnttab(const char *s)
>>>
>>> in order to comply with my runtime parameter series?
>> If the result will still compile.  What should the semantics be?  (I've
>> had a quick look through your series, but I can't see the semantics
>> stated anywhere obvious)
> The parsing routine must not change the parameter string and should
> return an error (e.g. -EINVAL) in case of an illegal parameter.

Let me see what I can do.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel