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

On Sun, Mar 13, 2016 at 2:15 AM, Jason Gunthorpe
[off-list ref] wrote:

On Sat, Mar 12, 2016 at 10:18:47AM -0500, Devesh Sharma wrote:

quoted

CC: Yishai Hadas <redacted>

I'm still pretty convinced this is wrong... But even still:

quoted

@@ -954,6 +955,17 @@ 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;
+     struct ib_device *ib_dev;
+     int srcu_key;
+
+     srcu_key = srcu_read_lock(&dev->disassociate_srcu);
+     ib_dev = srcu_dereference(dev->ib_dev,
+                               &dev->disassociate_srcu);
+     if (!ib_dev) {
+             srcu_read_unlock(&dev->disassociate_srcu, srcu_key);
+             wait_for_completion(&file->fcomp);
+             goto out;

This jumps over this:

I am not sure, to my mind kref_put(...,ib_uverbs_release_file) should
also be skipped. Because,
by putting an wait_for_completion(), this context is effectively
waiting for ib_uverbs_cleanup_ucontext
to finish cleaning up this file pointer. if the other thread is
cleaning up, why do I need to put the kerf again?

quoted

      if (file->async_file)
              kref_put(&file->async_file->ref, ib_uverbs_release_event_file);

Which doesn't seem right.

As I've said, I'm not sure how this is any different from using
lists_mutex. The wait_for_completion will still block and deadlock if
ib_uverbs_close is entered during the disassociate flow.

Taking list-mutex is not stopping ib_dev pointer to become NULL, while
if we split the mutex
and wait_for_completion(), then effectively we are trying to sync
ib_uverb_close() and
remove_one() in such a way that no ib_uverb_close context hit ib_dev = NULL

I hope I was able to put the situation in simpler words?

Can you please explain how it can lead to a deadlock?

Jason

--
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