Re: [Xen-devel] [PATCH] x86/mm: Make PV linear pagetables optional

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/17/2017 07:05 PM, Andrew Cooper wrote:
> On 17/10/17 18:10, George Dunlap wrote:
>> Allowing pagetables to point to other pagetables of the same level
>> (often called 'linear pagetables') has been included in Xen since its
>> inception; but recently it has been the source of a number of subtle
>> reference-counting bugs.
>>
>> It is not used by Linux or MiniOS; but it used used by NetBSD and
>> Novell Netware.  There are significant numbers of people who are never
>> going to use the feature, along with significant numbers who need the
>> feature.
>>
>> Add a Kconfig option for the feature (default to 'y').  Also add a
>> command-line option to control whether PV linear pagetables are
>> allowed (default to 'true').
>>
>> In order to make the code clean:
>> - Introduce LPT_ASSERT(), which only exists if CONFIG_PV_LINEAR_PT is defined
>> - Introduce zero_linear_entries() to set page->linear_pt_count to zero
>>   (or do nothing, as appropriate)
>>
>> Reported-by: Jann Horn <jannh@xxxxxxxxxx>
>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> 
> Definitely +1 to this kind of arrangement of user choices.  Some notes
> below.
> 
>> diff --git a/docs/misc/xen-command-line.markdown 
>> b/docs/misc/xen-command-line.markdown
>> index eb4995e68b..952368d3be 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1422,6 +1422,22 @@ The following resources are available:
>>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>>      sum of CBMs is fixed, that means actual `cos_max` in use will 
>> automatically
>>      reduce to half when CDP is enabled.
>> +    
>> +### pv-linear-pt
>> +> `= <boolean>`
>> +
>> +> Default: `false`
> 
> Only available if Xen is compiled with CONFIG_PV_LINEAR_PT support enabled.

Ack

> 
>> +
>> +Allow PV guests to have pagetable entries pointing to other pagetables
>> +of the same level (i.e., allowing L2 PTEs to point to other L2 pages).
>> +This technique is often called "linear pagetables", and is sometimes
>> +used to allow operating systems a simple way to consistently map the
>> +current process's pagetables into its own virtual address space.
>> +
>> +Linux and MiniOS don't use this technique.  NetBSD and Novell Netware
>> +do; there may be other custom operating systems which do.  If you're
>> +certain you don't plan on having PV guests which use this feature,
>> +turning it off can reduce the attack surface.
>>  
>>  ### rcu-idle-timer-period-ms
>>  > `= <integer>`
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index 62d313e3f5..5881b64608 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -654,6 +660,9 @@ static void dec_linear_uses(struct page_info *pg)
>>   *     frame if it is mapped by a different root table. This is sufficient 
>> and
>>   *     also necessary to allow validation of a root table mapping itself.
>>   */
>> +static bool __read_mostly pv_linear_pt_enable = true;
>> +boolean_param("pv-linear-pt", pv_linear_pt_enable);
> 
> The _enable suffix just makes the name longer, and (semi-upheld)
> convention would be for opt_pv_linear_pt, which is fine even in its used
> context below.

Ack

> 
>> +
>>  #define define_get_linear_pagetable(level)                                  
>> \
>>  static int                                                                  
>> \
>>  get_##level##_linear_pagetable(                                             
>> \
>> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
>> index 26f0153164..7825f36316 100644
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -177,10 +177,15 @@ struct page_info
>>           *   in use.
>>           */
>>          struct {
>> +#ifdef CONFIG_PV_LINEAR_PT
>>              u16 nr_validated_ptes:PAGETABLE_ORDER + 1;
>>              u16 :16 - PAGETABLE_ORDER - 1 - 2;
>>              s16 partial_pte:2;
>>              s16 linear_pt_count;
>> +#else
>> +            u16 nr_validated_ptes;
>> +            s8 partial_pte;
>> +#endif
> 
> I don't think this is a clever move.  Having CONFIG_PV_LINEAR_PT change
> the behaviour of nr_validated_ptes and partial_pte is a recipe for
> subtle bugs.
>
> An alternative would be to have the dec_linear_{uses,entries}()
> BUG_ON(pg->linear_pt_count != 0) when !CONFIG_PV_LINEAR_PT

Oh, I just noticed this was a union; so cutting out linear_pt_count
doesn't actually save you any space.

Yeah, in that case, leaving it in and adding ASSERTs that it's 0 makes
sense.  (I think an ASSERT is better than a BUG_ON() in this case.)

 -George


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