On Wed, Mar 9, 2016 at 10:18 PM, Yishai Hadas <yishaih@xxxxxxxxxxxxxxxxxx> wrote: > On 3/8/2016 7:53 PM, Jason Gunthorpe wrote: >> >> 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. > > > 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. > > The reason for the extra "completion" that I suggested comes to make sure > that when an application returns from its close API the underlying resources > were really freed, this is open in current code even if the race *wasn't* > hit. > > As we need to enable parallel closing it seems to be the preferred way to > go. > > Devesh, can you send V3 with above suggestion to help people reviewing it ? > if you have some other solution with mutex that addressed above points > please come it to the list for a review. Sure, I should be able to post it toady. > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
