On 3/10/2016 11:05 PM, Jason Gunthorpe wrote:
On Thu, Mar 10, 2016 at 01:26:04PM +0200, Yishai Hadas wrote:
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.
Why not ? the final cleanup as part of uverbs_close doesn't depend on ib_dev,
the kref should be fine for that. The race is *only* for
ib_uverbs_cleanup_ucontext that uses ib_dev and it should be solved as of
above suggestion.
It has nothing to do with ib_dev, srcu doesn't lock the list:
CPU 0 CPU 1
rcu_assign_pointer(ib_dev, null)
ib_uverbs_free_hw_resources()
synchronize_srcu();
ib_uverbs_close()
srcu_read_lock
.. goto out
kref_put(file->ref) .. kfree(file)
ib_uverbs_free_hw_resources()
mutex_lock(&lists_mutex);
while (!list_empty(&uverbs_dev->uverbs_file_list))
.. Boom, use after free of file->list ..
Ie, as I said, we can't put until we know the list_del is done, and
the goto skips over list_del.
The completion is preventing the above scenario, can't remove it.
Why do we need the completion for that ? the race can be prevented by
below flow as part of uverbs_close.
ib_uverbs_close()
{
..
struct ib_ucontext *ucontext = NULL;
+ struct ib_device *ib_dev;
+ srcu_read_lock(...)
+ ib_dev = srcu_dereference(...);
mutex_lock(&file->device->lists_mutex);
ucontext = file->ucontext;
file->ucontext = NULL;
- if (!file->is_closed) {
+ if (ib_dev) {
list_del(&file->list);
file->is_closed = 1;
}
mutex_unlock(&file->device->lists_mutex);
if (ucontext)
ib_uverbs_cleanup_ucontext(file, ucontext);
+ srcu_read_unlock()
if (file->async_file)
kref_put(&file->async_file->ref, ib_uverbs_release_event_file);
...
}
--
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
