Re: [PATCH V2] IB/uverbs: Fix race between uverbs_close and remove_one
From: Devesh Sharma <hidden>
Date: 2016-03-10 09:04:07
On Thu, Mar 10, 2016 at 12:33 AM, Jason Gunthorpe [off-list ref] wrote:
On Wed, Mar 09, 2016 at 06:48:08PM +0200, Yishai Hadas wrote:quoted
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.
Yes, I agree with the above explanation. Even my testing showed evidence.
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.
Below patch seems to be working in the code analysis. I will test though. I would test this approach as well before posting V3.
quoted hunk
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;
ib_dev pointer will not be NULL if cleanup_mutex is taken, thus no need to check here.
quoted hunk
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); + + 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); + 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