Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 16/19] xen/arm: mm: Protect Xen page-table update with a spinlock

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 08/06/2019 01:17, Stefano Stabellini wrote:
> On Wed, 5 Jun 2019, Julien Grall wrote:
> > On 05/06/2019 00:11, Stefano Stabellini wrote:
> > > On Tue, 14 May 2019, Julien Grall wrote:
> > > > The function create_xen_entries() may be called concurrently. For
> > > > instance, while the vmap allocation is protected by a spinlock, the
> > > > mapping is not.
> > > Do you have an example of potential concurrent calls of
> > > create_xen_entries() which doesn't involve concurrent vmaps (because
> > > vmaps are already protected by their spinlock)? vmap + something_else
> > > for instance?
> > Well, I gave an example here. The vmap allocation (i.e vm_alloc) is protected
> > by a spinlock. However, when the mapping is done there are no spinlock to
> > protected against concurrent one.
> >
> > So the following scenario could happen:
> >
> > CPU0                                  | CPU1
> >                                       |
> > vmap()                                | vmap()
> >    vm_alloc()                         |   vm_alloc()
> >      spin_lock()                              |
> >      ...                                      |
> >      spin_unlock()                    |
> >                                       |     spin_lock()
> >      * interrupt *                    |     ...
> >                                       |     spin_unlock()
> >                                       |
> >      map_pages_to_xen()               |     map_pages_to_xen()
> >         entry = &xen_second[X]            |         entry = &xen_second[Y]
> >         * entry invalid *             |         * entry invalid *
> >         create_xen_table()            |         create_xen_table()
> >         
> >
> > Assuming X == Y (i.e we the xen second entry is the same), then one will win
> > the race and therefore one mapping will be inexistent.
> >
> > But as I wrote, the chance it is happening is very limited.
> >
> > I can add that in the commit message.
> Thanks for the detailed explanation, I am just trying to understand and
> double-check the race. Wouldn't vm_alloc guarantee to return a different
> va in the two cases (CPU0 and CPU1 above), given that the selection of
> the va is done under spin_lock?
Yes vm_alloc() would guarantee you to have a different VA.

> But it would be still possible to have X
> and Y so that they select the same &xen_second entry, hence, the race
> with create_xen_table(). It looks like the race is there.
That's correct.

> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Thank you!

Cheers,

--
Julien Grall

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