Commit d1e09f30 authored by Jason Gunthorpe's avatar Jason Gunthorpe Committed by Doug Ledford

IB/uverbs: Fix race between uverbs_close and remove_one

Fixes an oops that might happen if uverbs_close races with
remove_one.

Both contexts may run ib_uverbs_cleanup_ucontext, it depends
on the flow.

Currently, there is no protection for a case that remove_one
didn't make the cleanup it runs to its end, the underlying
ib_device was freed then uverbs_close will call
ib_uverbs_cleanup_ucontext and OOPs.

Above might happen if uverbs_close deleted the file from the list
then remove_one didn't find it and runs to its end.

Fixes to protect against that case by a new cleanup lock so that
ib_uverbs_cleanup_ucontext will be called always before that
remove_one is ended.

Fixes: 35d4a0b6 ("IB/uverbs: Fix race between ib_uverbs_open and remove_one")
Reported-by: default avatarDevesh Sharma <devesh.sharma@broadcom.com>
Signed-off-by: default avatarJason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: default avatarYishai Hadas <yishaih@mellanox.com>
Signed-off-by: default avatarLeon Romanovsky <leon@kernel.org>
Signed-off-by: default avatarDoug Ledford <dledford@redhat.com>
parent 380bae5b
...@@ -116,6 +116,7 @@ struct ib_uverbs_event_file { ...@@ -116,6 +116,7 @@ struct ib_uverbs_event_file {
struct ib_uverbs_file { struct ib_uverbs_file {
struct kref ref; struct kref ref;
struct mutex mutex; struct mutex mutex;
struct mutex cleanup_mutex; /* protect cleanup */
struct ib_uverbs_device *device; struct ib_uverbs_device *device;
struct ib_ucontext *ucontext; struct ib_ucontext *ucontext;
struct ib_event_handler event_handler; struct ib_event_handler event_handler;
......
...@@ -931,6 +931,7 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp) ...@@ -931,6 +931,7 @@ static int ib_uverbs_open(struct inode *inode, struct file *filp)
file->async_file = NULL; file->async_file = NULL;
kref_init(&file->ref); kref_init(&file->ref);
mutex_init(&file->mutex); mutex_init(&file->mutex);
mutex_init(&file->cleanup_mutex);
filp->private_data = file; filp->private_data = file;
kobject_get(&dev->kobj); kobject_get(&dev->kobj);
...@@ -956,18 +957,20 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp) ...@@ -956,18 +957,20 @@ static int ib_uverbs_close(struct inode *inode, struct file *filp)
{ {
struct ib_uverbs_file *file = filp->private_data; struct ib_uverbs_file *file = filp->private_data;
struct ib_uverbs_device *dev = file->device; struct ib_uverbs_device *dev = file->device;
struct ib_ucontext *ucontext = NULL;
mutex_lock(&file->device->lists_mutex); mutex_lock(&file->cleanup_mutex);
ucontext = file->ucontext; if (file->ucontext) {
ib_uverbs_cleanup_ucontext(file, file->ucontext);
file->ucontext = NULL; file->ucontext = NULL;
}
mutex_unlock(&file->cleanup_mutex);
mutex_lock(&file->device->lists_mutex);
if (!file->is_closed) { if (!file->is_closed) {
list_del(&file->list); list_del(&file->list);
file->is_closed = 1; file->is_closed = 1;
} }
mutex_unlock(&file->device->lists_mutex); mutex_unlock(&file->device->lists_mutex);
if (ucontext)
ib_uverbs_cleanup_ucontext(file, ucontext);
if (file->async_file) if (file->async_file)
kref_put(&file->async_file->ref, ib_uverbs_release_event_file); kref_put(&file->async_file->ref, ib_uverbs_release_event_file);
...@@ -1181,22 +1184,30 @@ static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev, ...@@ -1181,22 +1184,30 @@ static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev,
mutex_lock(&uverbs_dev->lists_mutex); mutex_lock(&uverbs_dev->lists_mutex);
while (!list_empty(&uverbs_dev->uverbs_file_list)) { while (!list_empty(&uverbs_dev->uverbs_file_list)) {
struct ib_ucontext *ucontext; struct ib_ucontext *ucontext;
file = list_first_entry(&uverbs_dev->uverbs_file_list, file = list_first_entry(&uverbs_dev->uverbs_file_list,
struct ib_uverbs_file, list); struct ib_uverbs_file, list);
file->is_closed = 1; file->is_closed = 1;
ucontext = file->ucontext;
list_del(&file->list); list_del(&file->list);
file->ucontext = NULL;
kref_get(&file->ref); kref_get(&file->ref);
mutex_unlock(&uverbs_dev->lists_mutex); 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); ib_uverbs_event_handler(&file->event_handler, &event);
mutex_lock(&file->cleanup_mutex);
ucontext = file->ucontext;
file->ucontext = NULL;
mutex_unlock(&file->cleanup_mutex);
/* At this point ib_uverbs_close cannot be running
* ib_uverbs_cleanup_ucontext
*/
if (ucontext) { if (ucontext) {
/* 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_dev->disassociate_ucontext(ucontext); ib_dev->disassociate_ucontext(ucontext);
ib_uverbs_cleanup_ucontext(file, ucontext); ib_uverbs_cleanup_ucontext(file, ucontext);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment