Re: [RFC mach] vm: fix locking issues

[Samuel Thibault]

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?

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.

> @@ -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.

> 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.

> @@ -244,8 +246,11 @@ vm_object_t vm_object_allocate(
>       port = ipc_port_alloc_kernel();
>       if (port == IP_NULL)
>               panic("vm_object_allocate");
> +
> +     vm_object_lock(object);
>       object->pager_name = port;
>       ipc_kobject_set(port, (ipc_kobject_t) object, IKOT_PAGING_NAME);
> +     vm_object_unlock(object);

Ditto (and later with new_copy and result)

> @@ -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.

> @@ -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.

Samuel