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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> +
> +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.

> +
>  #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

This way, you don't need LPT_ASSERT(), or play games with the existing
macros to avoid having them evaluate their parameters.

~Andrew

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