Re: [Xen-devel] [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()

[Julien Grall <julien.grall@xxxxxxx>]

Hi George,

On 16/10/2019 11:38, George Dunlap wrote:
> On 10/16/19 11:31 AM, Julien Grall wrote:
> > On 16/10/2019 11:22, George Dunlap wrote:
> > > On 10/16/19 11:18 AM, Ian Jackson wrote:
> > > > Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use
> > > > _end in is_xen_fixed_mfn()"):
> > > > > My suggestion is going to work: "the compiler sees through casts"
> > > > > referred to comparisons between pointers, where we temporarily casted
> > > > > both pointers to integers and back to pointers via a MACRO. That case
> > > > > was iffy because the MACRO was clearly a workaround the spec.
> > > > >
> > > > > Here the situation is different. For one, we are doing arithmetic. Also
> > > > > virt_to_maddr already takes a vaddr_t as argument. So instead of doing
> > > > > pointers arithmetic, then converting to vaddr_t, we are converting to
> > > > > vaddr_t first, then doing arithmetics, which is fine both from a C99
> > > > > point of view and even a MISRA C point of view. I can't see a problem
> > > > > with that. I am sure as I reasonable can be :-)
> > > > FTAOD I think you are suggesting this:
> > > >    - +     (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
> > > >    + +     (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
> > > >
> > > > virt_to_maddr(va) is a macro which expands to
> > > >      __virt_to_maddr((vaddr_t)(va))
> > > >
> > > > So what is happening here is that the cast to an integer type is being
> > > > done before the subtraction.
> > > >
> > > > Without the cast, you are calculating the pointer value _end-1 from
> > > > the value _end, which is UB.  With the cast you are calculating an
> > > > integer value.  vaddr_t is unsigned, so all arithmetic operations are
> > > > defined.  Nothing casts the result back to the "forbidden" (with this
> > > > provenance) pointer value, so all is well.
> > > >
> > > > (With the macro expansion the cast happens twice.  This is probably
> > > > better than using __virt_to_maddr here.)
> > > >
> > > > Ie, in this case I agree with Stefano.  The cast is both necessary and
> > > > sufficient.
> > > Maybe I missed something, but why are we using `<=` at all?  Why not use
> > > `<`?
> > >
> > > Or is this some weird C pointer comparison UB thing?
> > _end may not be mapped in the virtual address space. This is the case
> > when the size of Xen is page-aligned. So _end will point to the next page.
> >
> > virt_to_maddr() cannot fail so it should only be called on valid virtual
> > address. The behavior is undefined in all the other cases.
> >
> > On x86, you might be able to get away because you translate the virtual
> > address to physical address in software.
> >
> > On Arm, we are using the hardware instruction to do the translation. As
> > _end is not always mapped, then the translation may fail. In other word,
> > Xen will crash.
> None of this explains my question.
>
> Is it not the case that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)`
> is true, then `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will be true,
> and that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)` is false, then
> `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will also be false?
>
> Under what conditions would they be different?
>
> And if they're the same, then you can just use `<` instead of `<=`, and
> not have to worry about casting before subtracting.
_end is not part of the binary but points one past it. So there is no guarantee 
this address will be mapped in the virtual address space.
As I pointed out in my previous e-mail the result of virt_to_maddr() will be 
undefined in this case. On Arm, this will actually crash Xen.
So you can't use '<' here.

Cheers,

--
Julien Grall

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