Re: [RFC mach] vm: fix locking issues

[Justus Winter]

Quoting Samuel Thibault (2015-08-27 00:56:23)
> Justus Winter, le Mon 17 Aug 2015 17:51:09 +0200, a écrit :
> > @@ -2157,10 +2162,13 @@ start_pass_1:
> >               /*
> >                *      Check for permanent objects in the destination.
> >                */
> > -
> > -             if ((entry->object.vm_object != VM_OBJECT_NULL) &&
> > -                        !entry->object.vm_object->temporary)
> > -                     contains_permanent_objects = TRUE;
> > +             object = entry->object.vm_object;
> > +             if ((object != VM_OBJECT_NULL)
> > +                 && ! contains_permanent_objects) {
> > +                     vm_object_lock(object);
> > +                     contains_permanent_objects = object->temporary;
> 
> This looks wrong, shouldn't it be !object->temporary?

Yes.

> Also, taking the lock just to read a boolean usually means there's
> something bigger wrong somewhere.  Either the boolean was not meant to
> be protected (e.g. because it is constant), or more than just reading it
> should be locked, for the whole coherency.  Here it never changes except
> when memory_object_set_attributes_common is called, but then there is
> nothing that will prevent memory_object_set_attributes_common from doing
> it between the time you take the lock, get the temporary field, release
> the lock, and the time when you actually do something about it, e.g.
> here return KERN_FAILURE because you believe that there's a permanent
> object while it has just been turned temporary actually. That's probably
> not a problem in practice because the two calls will probably have some
> dependency, but that's the actual issue here.

Right.

> 
> > @@ -2275,8 +2284,15 @@ start_pass_1:
> >                */
> >  
> >               object = entry->object.vm_object;
> > +             temporary = 0;
> > +             if (object != VM_OBJECT_NULL) {
> > +                     vm_object_lock(object);
> > +                     temporary = object->temporary;
> > +                     vm_object_unlock(object);
> > +             }
> > +
> 
> Here it's worse: the copy strategy changes. Perhaps we could think that
> it doesn't pose problem in practice because the two calls would probably
> have a dependency, I don't know about memory objects to know.

Right, I will look more closely at the fields and how they are meant
to be used, not how it is documented in the header.

> > diff --git a/vm/vm_object.c b/vm/vm_object.c
> > index deac0c2..1d3e727 100644
> > --- a/vm/vm_object.c
> > +++ b/vm/vm_object.c
> > @@ -217,9 +217,11 @@ static void _vm_object_setup(
> >       vm_size_t       size)
> >  {
> >       *object = vm_object_template;
> > -     queue_init(&object->memq);
> >       vm_object_lock_init(object);
> > +     vm_object_lock(object);
> > +     queue_init(&object->memq);
> >       object->size = size;
> > +     vm_object_unlock(object);
> 
> Here it's a fresh object, that's why there's no need to lock, but oh
> well.

Yes.  I pondered enclosing these with #if MACH_LDEBUG, but that's ugly
and so I postponed it until it turns out to be a bottleneck.

> > @@ -1474,14 +1491,11 @@ vm_object_t vm_object_copy_delayed(
> >        *      must be done carefully, to avoid deadlock.
> >        */
> >  
> > -     /*
> > -      *      Allocate a new copy object before locking, even
> > -      *      though we may not need it later.
> > -      */
> > +     vm_object_lock(src_object);
> >  
> >       new_copy = vm_object_allocate(src_object->size);
> >  
> > -     vm_object_lock(src_object);
> 
> Better make a copy of src_object->size to avoid keeping the object
> locked while we allocate stuff.

Ok.

> > @@ -252,6 +252,8 @@ vm_pageout_setup(
> >               vm_object_unlock(new_object);
> >       }
> >  
> > +     vm_object_lock(old_object);
> > +
> >       if (flush) {
> >               /*
> >                *      Create a place-holder page where the old one was,
> > @@ -262,7 +264,6 @@ vm_pageout_setup(
> >                                                       == VM_PAGE_NULL)
> >                       vm_page_more_fictitious();
> > -             vm_object_lock(old_object);
> 
> Why keeping the lock while allocating a page?
> 
> >               vm_page_lock_queues();
> >               vm_page_remove(m);
> >               vm_page_unlock_queues();
> > @@ -281,8 +282,6 @@ vm_pageout_setup(
> >                                       VM_EXTERNAL_STATE_EXISTS);
> >  #endif       /* MACH_PAGEMAP */
> >  
> > -             vm_object_unlock(old_object);
> 
> Why keeping it locked here?  For accessing old_object->internal?  It is
> constant across the life of an object, thus does not need to be
> protected by the lock.

Ok.  I wish that kind of knowledge was stated in the comments
describing the objects.


Cheers,
Justus