Re: [Xen-devel] [PATCH v4 1/2] xen: Switch parameter in get_page_from_gfn to use typesafe gfn

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2019 19:36, Julien Grall wrote:
> On 29/08/2019 17:41, Jan Beulich wrote:
>> On 19.08.2019 16:26, Julien Grall wrote:
>>> No functional change intended.
>>>
>>> Only reasonable clean-ups are done in this patch. The rest will use _gfn
>>> for the time being.
>>>
>>> The code in get_page_from_gfn is slightly reworked to also handle a bad
>>> behavior because it is not safe to use mfn_to_page(...) because
>>> mfn_valid(...) succeeds.
>>
>> I guess the 2nd "because" is meant to be "before", but even then I
>> don't think I can easily agree: mfn_to_page() is just calculations,
>> no dereference.
> 
> Regardless the s/because/before/. Do you disagree on the wording or the 
> change? If the former, I just paraphrased what Andrew wrote in the 
> previous version:
> 
> "This unfortunately propagates some bad behaviour, because it is not 
> safe to use mfn_to_page(mfn); before mfn_valid(mfn) succeeds.  (In 
> practice it works because mfn_to_page() is just pointer arithmetic.)"
> 
> This is x86 code, so please agree with Andrew on the approach/wording.

Andrew - I don't see much point altering the ordering in a mechanical
patch like this one, when there are (presumably) dozens of other places
doing the same. I agree it's a grey area, but I don't think doing the
pointer arithmetic up front can really be called UB in the sense that
it may cause the compiler to mis-optimize things. This is in particular
because we don't declare frame_table[]'s bounds anywhere, so the
compiler has to view this as an array extending all the way up to the
upper address space end.

If we really were concerned, we should go through and change all such
instances.

>>> --- a/xen/arch/x86/hvm/domain.c
>>> +++ b/xen/arch/x86/hvm/domain.c
>>> @@ -296,8 +296,10 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
>>> vcpu_hvm_context_t *ctx)
>>>       if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
>>>       {
>>>           /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
>>> -        struct page_info *page = get_page_from_gfn(v->domain,
>>> -                                 v->arch.hvm.guest_cr[3] >> PAGE_SHIFT,
>>> +        struct page_info *page;
>>> +
>>> +        page = get_page_from_gfn(v->domain,
>>> +                                 gaddr_to_gfn(v->arch.hvm.guest_cr[3]),
>>>                                    NULL, P2M_ALLOC);
>>
>> Iirc I've said so before: I consider use of gaddr_to_gfn() here more
>> misleading than a plain shift by PAGE_SHIFT. Neither is really correct,
>> but in no event does CR3 as a whole hold an address. (Same elsewhere
>> then, and sadly in quite a few places.)
> 
> Well, this code has not changed since v3 and you acked it... I only 
> dropped the ack here because the previous version was sent 9 months ago. 
> I also can't see such comment made on any version of this series.

Well, it may not have been this patch, but I clearly recall pointing
this aspect out before; I think it was even more than once.

> Anyway, I am more than happy to switch to _gfn((v->arch.hvm.guest_cr[3] 
>  >> PAGE_SHIFT)) if you prefer it.

Personally I'd much prefer introducing (and then using)

#define gcr3_to_gfn(cr3) gaddr_to_gfn((cr3) & X86_CR3_ADDR_MASK)

>>> --- a/xen/common/event_fifo.c
>>> +++ b/xen/common/event_fifo.c
>>> @@ -361,7 +361,7 @@ static const struct evtchn_port_ops 
>>> evtchn_port_ops_fifo =
>>>       .print_state   = evtchn_fifo_print_state,
>>>   };
>>>   
>>> -static int map_guest_page(struct domain *d, uint64_t gfn, void **virt)
>>> +static int map_guest_page(struct domain *d, gfn_t gfn, void **virt)
>>>   {
>>>       struct page_info *p;
>>>   
>>> @@ -422,7 +422,7 @@ static int setup_control_block(struct vcpu *v)
>>>       return 0;
>>>   }
>>>   
>>> -static int map_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
>>> +static int map_control_block(struct vcpu *v, gfn_t gfn, uint32_t offset)
>>>   {
>>>       void *virt;
>>>       unsigned int i;
>>
>> Just as a remark (no action expected) - this makes a truncation issue
>> pretty apparent: On 32-bit platforms the upper 32 bits of a passed in
>> GFN get ignored. Correct (imo) behavior would be to reject the upper
>> bits being non-zero.
> 
> This is not only here but on pretty much all the hypercalls taking a gfn 
> (on Arm it is 64-bit regardless the bitness).
> 
> I agree this is not nice, but I am afraid this is likely another can of 
> worm that I am not to open it yet.

Right - hence me saying "no action expected".

Jan

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