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
