ib/core: Protect QP mcast list

Multicast attach/detach operations on a QP are carried under the readers'/writers' lock of the QP. Such protection is not sufficient since a reader's lock allows more than one reader to acquire the lock. However, list manipulation implies write operations on the list variable. Use a spinlock to provide protection.
We have hit kernel oops when running applications that perform multicast joins/leaves. This patch solved the issue.

Reported by: Mike Dubman <miked-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/core/uverbs_cmd.c |   11 ++++++++++-
 include/rdma/ib_verbs.h              |    1 +
 2 files changed, 11 insertions(+), 1 deletions(-)

index c426992..0209292 100644
@@ -1132,6 +1132,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	if (ret)
 		goto err_destroy;
 
+	spin_lock_init(&qp->mcast_lock);
 	memset(&resp, 0, sizeof resp);
 	resp.qpn             = qp->qp_num;
 	resp.qp_handle       = obj->uevent.uobject.id;

@@ -1910,12 +1911,15 @@ ssize_t ib_uverbs_attach_mcast(struct ib_uverbs_file *file,
 
 	obj = container_of(qp->uobject, struct ib_uqp_object, uevent.uobject);
 
+	spin_lock(&qp->mcast_lock);
 	list_for_each_entry(mcast, &obj->mcast_list, list)
 		if (cmd.mlid == mcast->lid &&
 		    !memcmp(cmd.gid, mcast->gid.raw, sizeof mcast->gid.raw)) {
 			ret = 0;
+			spin_unlock(&qp->mcast_lock);
 			goto out_put;
 		}
+	spin_unlock(&qp->mcast_lock);
 
 	mcast = kmalloc(sizeof *mcast, GFP_KERNEL);
 	if (!mcast) {

@@ -1927,8 +1931,11 @@ ssize_t ib_uverbs_attach_mcast(struct ib_uverbs_file *file,
 	memcpy(mcast->gid.raw, cmd.gid, sizeof mcast->gid.raw);
 
 	ret = ib_attach_mcast(qp, &mcast->gid, cmd.mlid);
-	if (!ret)
+	if (!ret) {
+		spin_lock(&qp->mcast_lock);
 		list_add_tail(&mcast->list, &obj->mcast_list);
+		spin_unlock(&qp->mcast_lock);
+	}
 	else
 		kfree(mcast);
 

@@ -1961,6 +1968,7 @@ ssize_t ib_uverbs_detach_mcast(struct ib_uverbs_file *file,
 
 	obj = container_of(qp->uobject, struct ib_uqp_object, uevent.uobject);
 
+	spin_lock(&qp->mcast_lock);
 	list_for_each_entry(mcast, &obj->mcast_list, list)
 		if (cmd.mlid == mcast->lid &&
 		    !memcmp(cmd.gid, mcast->gid.raw, sizeof mcast->gid.raw)) {

@@ -1968,6 +1976,7 @@ ssize_t ib_uverbs_detach_mcast(struct ib_uverbs_file *file,
 			kfree(mcast);
 			break;
 		}
+	spin_unlock(&qp->mcast_lock);
 
 out_put:
 	put_qp_read(qp);

index 55cd0a0..bf86750 100644
@@ -895,6 +895,7 @@ struct ib_qp {
 	void		       *qp_context;
 	u32			qp_num;
 	enum ib_qp_type		qp_type;
+	spinlock_t              mcast_lock;
 };
 
 struct ib_mr {

1.7.8

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Did you consider taking the QP's rwsem for writing across these operations instead? How did that approach compare?

Assuming there's some problem with that, would it make sense to put the mcast_lock next to the mcast_list in struct ib_uqp_object, instead of hiding it in struct ib_qp?

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I considered this but that means that you serialize attach/detach operations at ib core. Using a spinlock to protect the list allows more concurrency. After all, we hit this bug since concurrency of such operations occur in real life applications.

Yes, it makes more sense to me.

If it is agreed to use a spinlock and put it next to mcast_list I will prepare another patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Looking closer at it, I don't see how we can avoid serializing multicast operations.

At least, I don't see how your proposed patch can work for all the crazy things userspace might do.

For example suppose two threads join the same QP to the same MCG at the same time. In that case it might happen that both threads check the list and don't find the group there, since you drop the lock before the actual low-level verbs attach (and you don't and can't hold the lock across that sleeping call).

In that case both calls to attach will succeed (underlying verb is idempotent) and the membership info will be added to the list twice.

Then all sorts of problems ensue, eg a detach will succeed but only remove one list entry, and so a subsequent re-attach will silently do nothing.

Also I'm sure racing attach/detach can get into trouble too, eg detach succeeds but list entry ends up still on the list.

I don't see any obvious way to close all these holes except to make sure that adding/removing a list entry is done atomically along with the actual attach/detach operation -- ie hold some sort of sleepable lock (like an rwsem) across checking the list, doing the attach/detach and doing the list add/remove.

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I see...
I think there is not much point in introducing a new semaphore. We can simply use the existing rw_semaphore. This requires defining idr_write_qp and put_qp_write. I will prepare a patch and run some testing.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html