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

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Tue, Mar 08, 2016 at 04:24:51PM +0530, Devesh Sharma wrote:

> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -962,9 +962,9 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp)
> >                 list_del(&file->list);
> >                 file->is_closed = 1;
> >         }
> > -       mutex_unlock(&file->device->lists_mutex);
> >         if (ucontext)
> >                 ib_uverbs_cleanup_ucontext(file, ucontext);
> > +       mutex_unlock(&file->device->lists_mutex);
> >
> >
> > ??
> 
> There is following comment about list_mutex in uverbs_main.c around
> line number 1200:
> /* 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).
>  */

Okay, now I remember this discussion, and being unhappy about this
during review.

However, this comment is talking about disassociate_ucontext, the bug
is with ib_uverbs_cleanup_ucontext. We can't re-entre uverbs_close
while we are already in uverbs_close, so that doesn't explain why it
cannot be in the mutex.

So, Yishai, what is the problem with the above lock placement?

The only issue you raised was with multi-file close concurrency, and
that is trivially solved with another mutex.

I'd rather see another mutex added then this ugly add-hoc
srcu/completion thing.

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