Re: [PATCH V2] IB/uverbs: Fix race between uverbs_close and remove_one

[Yishai Hadas <yishaih-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 3/9/2016 9:03 PM, Jason Gunthorpe wrote:
> On Wed, Mar 09, 2016 at 06:48:08PM +0200, Yishai Hadas wrote:
>
>> The srcu with NULL checking by itself can prevent the race, no need for the
>> "completion" mechanism. ib_uverbs_free_hw_resources uses synchronize_srcu
>> just after that ib_dev was set to NULL as part of ib_uverbs_remove_one.
>
> No, I don't think that is true, the completion looks like it is
> actually needed because the goto out in ib_uverbs_close needs to wait
> for ib_uverbs_free_hw_resources to do the cleanups ib_uverbs_close
> skipped over before it can go ahead and kref_put things.

Why not ? the final cleanup as part of uverbs_close doesn't depend on 
ib_dev, the kref should be fine for that. The race is *only* for 
ib_uverbs_cleanup_ucontext that uses ib_dev and it should be solved as 
of above suggestion.

> So, this is too ugly, do not create a mutex out of srcu and completion.
>
> Your performance reason not to use the existing lists_mutex seems
> reasonable, so add a new cleanup mutex for this purpose.
>
> Something like this. I would also get rid of file->is_closed and use
> list_del_init & list_empty instead.

Your suggestion is wrong, it doesn't handle the race and might end up in 
other case with deadlock, see below.

> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 39680aed99dd..8d192234fdd6 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -953,18 +953,20 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp)
>   {
>   	struct ib_uverbs_file *file = filp->private_data;
>   	struct ib_uverbs_device *dev = file->device;
> -	struct ib_ucontext *ucontext = NULL;
>
>   	mutex_lock(&file->device->lists_mutex);
> -	ucontext = file->ucontext;
> -	file->ucontext = NULL;
>   	if (!file->is_closed) {
>   		list_del(&file->list);
>   		file->is_closed = 1;
>   	}
>   	mutex_unlock(&file->device->lists_mutex);

At that point file was deleted from the list and there is *no* sync any 
more with ib_uverbs_free_hw_resources relates to that file.

If here ib_uverbs_free_hw_resource will run to its end freeing ib_dev we 
hit the race as part of ib_uverbs_cleanup_ucontext below, the new added 
lock won't help.

> -	if (ucontext)
> -		ib_uverbs_cleanup_ucontext(file, ucontext);
> +
> +	mutex_lock(&file->cleanup_mutex);
> +	if (file->ucontext) {
> +		ib_uverbs_cleanup_ucontext(file, file->ucontext);
> +		file->ucontext = NULL;
> +	}
> +	mutex_unlock(&file->cleanup_mutex);
>
>   	if (file->async_file)
>   		kref_put(&file->async_file->ref, ib_uverbs_release_event_file);
> @@ -1177,26 +1179,26 @@ static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev,
>
>   	mutex_lock(&uverbs_dev->lists_mutex);
>   	while (!list_empty(&uverbs_dev->uverbs_file_list)) {
> -		struct ib_ucontext *ucontext;
> -
>   		file = list_first_entry(&uverbs_dev->uverbs_file_list,
>   					struct ib_uverbs_file, list);
>   		file->is_closed = 1;
> -		ucontext = file->ucontext;
>   		list_del(&file->list);
> -		file->ucontext = NULL;
>   		kref_get(&file->ref);
>   		mutex_unlock(&uverbs_dev->lists_mutex);
> +
>   		/* We must release the mutex before going ahead and calling
>   		 * disassociate_ucontext. disassociate_ucontext might end up
>   		 * indirectly calling uverbs_close, for example due to freeing
>   		 * the resources (e.g mmput).
>   		 */
>   		ib_uverbs_event_handler(&file->event_handler, &event);
> -		if (ucontext) {
> -			ib_dev->disassociate_ucontext(ucontext);
> -			ib_uverbs_cleanup_ucontext(file, ucontext);
> +		mutex_lock(&file->cleanup_mutex);
> +		if (file->ucontext) {
> +			ib_dev->disassociate_ucontext(file->ucontext);

This might end up with deadlock, what is the difference between taking 
this cleanup mutex comparing the list mutex ? see above comment re 
calling disassociate_ucontext under the lock.

> +			ib_uverbs_cleanup_ucontext(file, file->ucontext);
> +			file->ucontext = NULL;
>   		}
> +		mutex_unlock(&file->cleanup_mutex);
>
>   		mutex_lock(&uverbs_dev->lists_mutex);
>   		kref_put(&file->ref, ib_uverbs_release_file);
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html