Re: [PATCH 1/2] xen/arm: Convert runstate address during hypcall

[Julien Grall <julien@xxxxxxx>]

On 12/06/2020 02:09, Stefano Stabellini wrote:
> On Thu, 11 Jun 2020, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 11/06/2020 19:50, Stefano Stabellini wrote:
> > > On Thu, 11 Jun 2020, Julien Grall wrote:
> > > > > > +        return -EINVAL;
> > > > > >        }
> > > > > >
> > > > > > -    __copy_to_guest(runstate_guest(v), &runstate, 1);
> > > > > > +    v->arch.runstate_guest.page = page;
> > > > > > +    v->arch.runstate_guest.offset = offset;
> > > > > > +
> > > > > > +    spin_unlock(&v->arch.runstate_guest.lock);
> > > > > > +
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > > +
> > > > > > +/* Update per-VCPU guest runstate shared memory area (if registered).
> > > > > > */
> > > > > > +static void update_runstate_area(struct vcpu *v)
> > > > > > +{
> > > > > > +    struct vcpu_runstate_info *guest_runstate;
> > > > > > +    void *p;
> > > > > > +
> > > > > > +    spin_lock(&v->arch.runstate_guest.lock);
> > > > > >
> > > > > > -    if ( guest_handle )
> > > > > > +    if ( v->arch.runstate_guest.page )
> > > > > >        {
> > > > > > -        runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> > > > > > +        p = __map_domain_page(v->arch.runstate_guest.page);
> > > > > > +        guest_runstate = p + v->arch.runstate_guest.offset;
> > > > > > +
> > > > > > +        if ( VM_ASSIST(v->domain, runstate_update_flag) )
> > > > > > +        {
> > > > > > +            v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> > > > > > +            guest_runstate->state_entry_time |= XEN_RUNSTATE_UPDATE;
> > > > > I think that this write to guest_runstate should use write_atomic or
> > > > > another atomic write operation.
> > > > I thought about suggesting the same, but  guest_copy_* helpers may not
> > > > do a single memory write to state_entry_time.
> > > > What are you trying to prevent with the write_atomic()?
> > > I am thinking that without using an atomic write, it would be (at least
> > > theoretically) possible for a guest to see a partial write to
> > > state_entry_time, which is not good.
> > It is already the case with existing implementation as Xen may write byte by
> > byte. So are you suggesting the existing code is also buggy?
It looks like I may have misread the code as we only copy one byte. But 
I still think this is fragile.
For this context, I agree that a write_atomic() should do the job.

However, I still want to answer to your comments below.

> Writing byte by byte is a different case. That is OK. In that case, the
> guest could see the state after 3 bytes written and it would be fine and
> consistent.
Why? What does actually prevent a guest to see an in-between value?

To give a concrete example, if the original value is 0xabc and you want 
to write 0xdef. Why would the guest never see 0xabf or 0xaec?
> If this hadn't been the case, then yes, the existing code
> would also be buggy.
>
> So if we did the write with a memcpy, it would be fine, no need for
> atomics:
>
>    memcpy(&guest_runstate->state_entry_time,
>           &v->runstate.state_entry_time,
>           XXX);
>
>
> The |= case is different: GCC could implement it in any way it likes,
> including going through a zero-write to any of the bytes in the word, or
> doing an addition then a subtraction. GCC doesn't make any guarantees.
> If we want guarantees we need to use atomics.
Yes GCC can generate assembly for |= in any way it likes. But so does 
for memcpy(). So I still don't understand why one would be fine for you 
and not the other...
Cheers,

--
Julien Grall