Re: [Xen-devel] [PATCH] x86/ctxt-switch: Document and improve GDT handling

[Jan Beulich <JBeulich@xxxxxxxx>]

On 04.07.2019 19:57, Andrew Cooper wrote:
> write_full_gdt_ptes() has a latent bug.  Using virt_to_mfn() and iterating
> with (mfn + i) is wrong, because of PDX compression.  The context switch path
> only functions correctly because NR_RESERVED_GDT_PAGES is 1.

Whether this is a (latent) bug depends on how the allocation gets
done. As long as it's a single alloc_xenheap_pages(), this is
perfectly fine. There are no individual allocations which can span
a PDX compression hole (or else MFN or struct page pointer
arithmetic wouldn't work either, independent of the involvement of
a virtual address).

> Also, it should now be very obvious to people that Xen's current GDT handling
> for non-PV vcpus is a recipe subtle bugs, if we ever manage to execute a stray
> mov/pop %sreg instruction.  We really ought to have Xen's regular GDT in an
> area where slots 0-13 are either mapped to the zero page, or not present, so
> we don't risk loading a non-faulting garbage selector.

Well, there's certainly room for improvement, but loading a stray
selector seems pretty unlikely an event to happen, and the
respective code having got slipped in without anyone noticing.
Other than in context switching code I don't think there are many
places at all where we write to the selector registers.

> @@ -1718,15 +1737,12 @@ static void __context_switch(void)
>   
>       psr_ctxt_switch_to(nd);
>   
> -    gdt = !is_pv_32bit_domain(nd) ? per_cpu(gdt_table, cpu) :
> -                                    per_cpu(compat_gdt_table, cpu);
> -
>       if ( need_full_gdt(nd) )
> -        write_full_gdt_ptes(gdt, n);
> +        update_xen_slot_in_full_gdt(n, cpu);
>   
>       if ( need_full_gdt(pd) &&
>            ((p->vcpu_id != n->vcpu_id) || !need_full_gdt(nd)) )
> -        load_default_gdt(gdt, cpu);
> +        load_default_gdt(cpu);

 From looking at this transformation I cannot see how, as said in
the description and as expressed by removing the gdt parameter
from load_default_gdt(), the gdt having got passed in here would
always have been per_cpu(gdt_table, cpu). It pretty clearly was
the compat one for nd being 32-bit PV. What am I missing? Or is
the description perhaps instead meaning to say that it doesn't
_need_ to be the compat one that we load here, as in case it is
the subsequent load_full_gdt() will replace it again anyway?

> @@ -2059,6 +2061,14 @@ void __init trap_init(void)
>           }
>       }
>   
> +    /* Cache {,compat_}gdt_table_l1e now that physically relocation is done. 
> */

"physical relocation" or "physically relocating"?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel