IB/core: Fix race condition in ib_uverbs_open_qp

Commit Message

Or Gerlitz Sept. 23, 2014, 9:38 a.m. UTC

From: Jack Morgenstein <[e-mail]>

In ib_uverbs_open_qp, the sharable xrc target qp is created as a "pseudo" qp
and added to a list of qp's sharing the same physical QP. This is done before
the "pseudo" qp is assigned a uobject.

There is a race condition here if an async event arrives at the physical qp.
If the event is handled after the pseudo qp is added to the list, but before
it is assigned a uobject, the kernel crashes in ib_uverbs_qp_event_handler,
due to trying to dereference a NULL uobject pointer.

Note that simply checking for non-NULL is not enough, due to error flows
in ib_uverbs_open_qp. If the failure is after assigning the uobject, but
before the qp has fully been created, we still have a problem.

Thus, in ib_uverbs_qp_event_handler, we test that the uobject is present,
and also that it is live.

Reported-by: Matthew Finlay <[e-mail]>
Signed-off-by: Jack Morgenstein <[e-mail]>
Signed-off-by: Or Gerlitz <[e-mail]>
---
 drivers/infiniband/core/uverbs_main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Comments

Hefty, Sean Sept. 23, 2014, 3:55 p.m. UTC | #1

> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -502,6 +502,10 @@ void ib_uverbs_qp_event_handler(struct ib_event
> *event, void *context_ptr)
>  {
>  	struct ib_uevent_object *uobj;
> 
> +	/* for XRC target qp's, check that qp is live */
> +	if (!event->element.qp->uobject || !event->element.qp->uobject->live)
> +		return;

Isn't checking 'live' sufficient?

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

jackm Oct. 2, 2014, 10:21 a.m. UTC | #2

On Tue, 23 Sep 2014 15:55:14 +0000
"Hefty, Sean" <[e-mail]> wrote:

> > --- a/drivers/infiniband/core/uverbs_main.c
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -502,6 +502,10 @@ void ib_uverbs_qp_event_handler(struct ib_event
> > *event, void *context_ptr)
> >  {
> >  	struct ib_uevent_object *uobj;
> > 
> > +	/* for XRC target qp's, check that qp is live */
> > +	if (!event->element.qp->uobject
> > || !event->element.qp->uobject->live)
> > +		return;
> 
> Isn't checking 'live' sufficient?

Unfortunately, not -- uobject might be NULL, in which case we will get
a kernel Oops.

However, checking that uobject is NULL alone is not sufficient -- it
might not yet be live.

-Jack


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

Hefty, Sean Oct. 2, 2014, 2:34 p.m. UTC | #3

> > > +	/* for XRC target qp's, check that qp is live */
> > > +	if (!event->element.qp->uobject
> > > || !event->element.qp->uobject->live)
> > > +		return;
> >
> > Isn't checking 'live' sufficient?
> 
> Unfortunately, not -- uobject might be NULL, in which case we will get
> a kernel Oops.

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

Patch

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index c73b22a..bb6fea1 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -502,6 +502,10 @@  void ib_uverbs_qp_event_handler(struct ib_event *event, void *context_ptr)
 {
 	struct ib_uevent_object *uobj;
 
+	/* for XRC target qp's, check that qp is live */
+	if (!event->element.qp->uobject || !event->element.qp->uobject->live)
+		return;
+
 	uobj = container_of(event->element.qp->uobject,
 			    struct ib_uevent_object, uobject);