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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3294 bytes --]

On 3/17/2016 12:48 PM, Jason Gunthorpe wrote:
> On Thu, Mar 17, 2016 at 10:01:55PM +0530, Devesh Sharma wrote:
>> On Thu, Mar 17, 2016 at 9:42 PM, Jason Gunthorpe
>> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>>> On Thu, Mar 17, 2016 at 09:38:30PM +0530, Devesh Sharma wrote:
>>>
>>>> To my mind mutex is *not* solving the problem completely unless we
>>>> make it a coarser grained lock. The possible deadlock problem still
>>>> lingers around it.
>>>
>>> Review the last version I sent, with this statement in mind:
>>
>> I am sorry I lost the track of it, which one you are point..we have
>> been discussion for a quite some time now!
> 
> Not saying it is perfect, but it should be close:

Jason, this is your patch since you completely tossed Devesh's work and
did your own.  Can you please post a corrected version of this with a
proper changelog and Signed-off-by.  Thanks.

> --- 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->cleanup_mutex);
> +	if (file->ucontext) {
> +		ib_uverbs_cleanup_ucontext(file, file->ucontext);
> +		file->ucontext = NULL;
> +	}
> +	mutex_unlock(&file->cleanup_mutex);
>  
>  	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);
> -	if (ucontext)
> -		ib_uverbs_cleanup_ucontext(file, ucontext);
>  
>  	if (file->async_file)
>  		kref_put(&file->async_file->ref, ib_uverbs_release_event_file);
> @@ -1177,26 +1179,28 @@ 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) {
> +			file->ucontext = NULL;
> +			mutex_unlock(&file->cleanup_mutex);
> +			ib_dev->disassociate_ucontext(file->ucontext);
> +			ib_uverbs_cleanup_ucontext(file, file->ucontext);
>  		}
> +		else
> +			mutex_unlock(&file->cleanup_mutex);
>  
>  		mutex_lock(&uverbs_dev->lists_mutex);
>  		kref_put(&file->ref, ib_uverbs_release_file);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]