On Mon, Mar 14, 2016 at 05:55:23PM +0200, Yishai Hadas wrote:
> 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.
I'm not sure what this is trying to show.
> 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()
This doesn't do anything, there is no assurance that ib_uverbs_close
doesn't start after synchronize_srcu.
Did you mean to include a goto? In which case, it is broken because
skipping over the ib_uverbs_cleanup_ucontext and doing list_del will
leak something.
Seriously, sit down and make a proper patch for this already.
Jason
--
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
