[PATCH] ib/core: Protect QP mcast list

[PATCH] ib/core: Protect QP mcast list

[Eli Cohen]

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/w@public.gmane.org>
Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c |   11 ++++++++++-
 include/rdma/ib_verbs.h              |    1 +
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index c426992..0209292 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -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);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 55cd0a0..bf86750 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib/core: Protect QP mcast list

[Roland Dreier]

On Sun, Dec 18, 2011 at 11:09 PM, Eli Cohen <eli-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> 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.

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

Re: [PATCH] ib/core: Protect QP mcast list

[Eli Cohen]

On Mon, Dec 19, 2011 at 11:54 PM, Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> wrote:
>
> Did you consider taking the QP's rwsem for writing across these operations
> instead?  How did that approach compare?
>
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.

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

RE: [PATCH] ib/core: Protect QP mcast list

[Hefty, Sean]

> @@ -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);

Does the lower level code protect against trying to join the same group twice?  The locking ends up looking racy here, with the lock being dropped between checking the list and adding the new entry.

> @@ -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);

I wonder if the uverbs code should prevent the user from trying to join a group twice and prevent it from leaving a group that it hasn't joined.  This is a separate question from these patches, but it would affect how the locking is held. 

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

Re: [PATCH] ib/core: Protect QP mcast list

[Roland Dreier]

On Mon, Dec 19, 2011 at 11:19 PM, Eli Cohen <eli-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> 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.

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

Re: [PATCH] ib/core: Protect QP mcast list

[Eli Cohen]

On Tue, Dec 20, 2011 at 04:36:35PM -0800, Roland Dreier wrote:
> On Mon, Dec 19, 2011 at 11:19 PM, Eli Cohen <eli-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> > 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.
> 
> 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.
> 

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