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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 5 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> 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? 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.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

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