Re: RFC: Lightweight synchronization mechanism for gnumach v2.0

[Samuel Thibault]

Hello,

Agustina Arzille, on Wed 30 Mar 2016 20:38:21 -0300, wrote:
> On 03/30/2016 08:07 PM, Samuel Thibault wrote:
> >>For shared gsync objects, I'm using vm_map_lookup to find out the
> >>vm object and offset. The problem is that said function expects the map
> >>to be unlocked
> >Really?
> >
> >It starts with locking it, and finishes with unlocking it, and the lock
> >is recursive, so it's fine to take it several times.
> 
> Yes, but there are parts where it tries to upgrade from read to write,
> and viceversa as well, and from what I understand, that requires the
> lock recursion count to reach zero, which cannot happen if we locked
> the VM map before calling the function.
> 
> The comments preceding the implementation say that "the map should
> not be locked", so I don't know.

Ah, sorry, I mixed up vm_map_lookup_entry with vm_map_lookup by not
really looking at the details.

> >But there's still room between that call and the dereference. We really
> >need to have the map locked between the access check and the actual
> >dereference.
> 
> This I don't see. What I propose to do is basically:
> 
> unlock_read (task->map)
> vm_map_lookup (...)
> 
> lock_read (task->map)
> if (!valid_access_p (...))
>   error

Ok, I hadn't understood what you meant.  Actually, you perhaps
don't even need to lock the map before calling vm_map_lookup: does
gsync_fill_wait really need to do the valid_access_p() call itself? We
can make functions call gsync_fill_wait, then lock the map, then call
valid_access_p(), then dereference, and eventually unlock the map.

Samuel