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

[Yishai Hadas <yishaih-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html