[PATCH] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[PATCH] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Bernd Schubert]

We have just been investigating kernel panics related to
cq->ibcq.event_handler() completion calls.

Reason is that ib_destroy_qp() fails with -EBUSY. Further investigation
revealed qp->usecnt is not initialized. This counter was introduced
in linux-3.2 by commit 0e0ec7e0638ef48e0c661873dfcc8caccab984c6
and is only initialized for IB_QPT_XRC_TGT, but also checked in ib_destroy_qp()
for any qp type.

Signed-off-by: Bernd Schubert <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
Signed-off-by: Sven Breuner <sven.breuner-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
---
 drivers/infiniband/core/verbs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 602b1bd..575b780 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -421,6 +421,7 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 		qp->uobject    = NULL;
 		qp->qp_type    = qp_init_attr->qp_type;
 
+		atomic_set(&qp->usecnt, 0);
 		if (qp_init_attr->qp_type == IB_QPT_XRC_TGT) {
 			qp->event_handler = __ib_shared_qp_event_handler;
 			qp->qp_context = qp;
@@ -430,7 +431,6 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 			qp->xrcd = qp_init_attr->xrcd;
 			atomic_inc(&qp_init_attr->xrcd->usecnt);
 			INIT_LIST_HEAD(&qp->open_list);
-			atomic_set(&qp->usecnt, 0);
 
 			real_qp = qp;
 			qp = __ib_open_qp(real_qp, qp_init_attr->event_handler,

--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Hefty, Sean]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1804 bytes --]

> We have just been investigating kernel panics related to
> cq->ibcq.event_handler() completion calls.
> 
> Reason is that ib_destroy_qp() fails with -EBUSY. Further investigation
> revealed qp->usecnt is not initialized. This counter was introduced
> in linux-3.2 by commit 0e0ec7e0638ef48e0c661873dfcc8caccab984c6
> and is only initialized for IB_QPT_XRC_TGT, but also checked in
> ib_destroy_qp()
> for any qp type.
> 
> Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
> Signed-off-by: Sven Breuner <sven.breuner@itwm.fraunhofer.de>

Reviewed-by: Sean Hefty <sean.hefty@intel.com>

> ---

Good catch.  I did all my testing with mlx4, which just happens to use kzalloc when allocating the QP.

>  drivers/infiniband/core/verbs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 602b1bd..575b780 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -421,6 +421,7 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
>  		qp->uobject    = NULL;
>  		qp->qp_type    = qp_init_attr->qp_type;
> 
> +		atomic_set(&qp->usecnt, 0);
>  		if (qp_init_attr->qp_type == IB_QPT_XRC_TGT) {
>  			qp->event_handler = __ib_shared_qp_event_handler;
>  			qp->qp_context = qp;
> @@ -430,7 +431,6 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
>  			qp->xrcd = qp_init_attr->xrcd;
>  			atomic_inc(&qp_init_attr->xrcd->usecnt);
>  			INIT_LIST_HEAD(&qp->open_list);
> -			atomic_set(&qp->usecnt, 0);
> 
>  			real_qp = qp;
>  			qp = __ib_open_qp(real_qp, qp_init_attr->event_handler,

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Bernd Schubert]

Hmm, I think we do have serious problem with the hole approach. While 
the patch works for the kernel side, there is a problem with user space 
libraries. So I monitored our daemons and noticed ibv_destroy_cq() 
failed. The reason again seems to be the same issue as already fixed for 
kernel qp's. So in __ibv_create_qp() (libibverbs/src/verbs.c):

> __ibv_create_qp()

> 	struct ibv_qp *qp = pd->context->ops.create_qp(pd, qp_init_attr);
>
> 	if (qp) {
> 		qp->context    	     = pd->context;
> 		qp->qp_context 	     = qp_init_attr->qp_context;
> 		qp->pd         	     = pd;
> 		qp->send_cq    	     = qp_init_attr->send_cq;
[...]

I *guess* the qp allocated by pd->context->ops.create_qp() does not have 
qp->usecnt initialized (not does it know anything about it). So its 
random value will fail the destruction later. A simple workaround that 
would work for us, is to extend the patch I send to

diff --git a/drivers/infiniband/core/verbs.c 
b/drivers/infiniband/core/verbs.c
index 602b1bd..fba1675 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -874,7 +874,7 @@ int ib_destroy_qp(struct ib_qp *qp)
         struct ib_srq *srq;
         int ret;

-       if (atomic_read(&qp->usecnt))
+       if (qp->qp_type == IB_QPT_XRC_TGT && atomic_read(&qp->usecnt))
                 return -EBUSY;

         if (qp->real_qp != qp)



However, what is is with user space setting type to IB_QPT_XRC_TGT? I 
guess this could be solved by letting the kernel zero the memory 
returned by ->ops.create_qp(pd, qp_init_attr).
Btw, I didn't figure out yet, how this translates at all in kernel 
space? Is this op directly going to the device driver?

But even if we are properly going to initialize the qp, what is with 
user space mischievously trying to crash the system by manipulating 
struct ib_qp *qp?


Thanks,
Bernd


--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Roland Dreier]

On Fri, Jan 20, 2012 at 8:14 AM, Bernd Schubert
<bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org> wrote:
> I *guess* the qp allocated by pd->context->ops.create_qp() does not have
> qp->usecnt initialized (not does it know anything about it). So its random
> value will fail the destruction later. A simple workaround that would work
> for us, is to extend the patch I send to
>
> diff --git a/drivers/infiniband/core/verbs.c
> b/drivers/infiniband/core/verbs.c
> index 602b1bd..fba1675 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -874,7 +874,7 @@ int ib_destroy_qp(struct ib_qp *qp)
>        struct ib_srq *srq;
>        int ret;
>
> -       if (atomic_read(&qp->usecnt))
> +       if (qp->qp_type == IB_QPT_XRC_TGT && atomic_read(&qp->usecnt))
>                return -EBUSY;
>
>        if (qp->real_qp != qp)

It looks like this is sufficient and correct without the other patch?

>
>
> However, what is is with user space setting type to IB_QPT_XRC_TGT? I guess
> this could be solved by letting the kernel zero the memory returned by
> ->ops.create_qp(pd, qp_init_attr).
> Btw, I didn't figure out yet, how this translates at all in kernel space? Is
> this op directly going to the device driver?
>
> But even if we are properly going to initialize the qp, what is with user
> space mischievously trying to crash the system by manipulating struct ib_qp
> *qp?

I don't follow this.  Isn't *qp completely allocated and manipulated
in the kernel?  How can userspace touch it except by having the
kernel do something via the uverbs interface?

 - 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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Roland Dreier]

On Fri, Jan 20, 2012 at 10:40 AM, Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> wrote:
> On Fri, Jan 20, 2012 at 8:14 AM, Bernd Schubert
> <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org> wrote:
>> I *guess* the qp allocated by pd->context->ops.create_qp() does not have
>> qp->usecnt initialized (not does it know anything about it). So its random
>> value will fail the destruction later. A simple workaround that would work
>> for us, is to extend the patch I send to
>>
>> diff --git a/drivers/infiniband/core/verbs.c
>> b/drivers/infiniband/core/verbs.c
>> index 602b1bd..fba1675 100644
>> --- a/drivers/infiniband/core/verbs.c
>> +++ b/drivers/infiniband/core/verbs.c
>> @@ -874,7 +874,7 @@ int ib_destroy_qp(struct ib_qp *qp)
>>        struct ib_srq *srq;
>>        int ret;
>>
>> -       if (atomic_read(&qp->usecnt))
>> +       if (qp->qp_type == IB_QPT_XRC_TGT && atomic_read(&qp->usecnt))
>>                return -EBUSY;
>>
>>        if (qp->real_qp != qp)
>
> It looks like this is sufficient and correct without the other patch?

But maybe it's cleaner to initialize qp->usecnt in
both ib_create_qp() and ib_uverbs_create_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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Sven Breuner]

Hi,

Roland Dreier wrote on 01/20/2012 07:43 PM:
> On Fri, Jan 20, 2012 at 10:40 AM, Roland Dreier<roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>  wrote:
>> On Fri, Jan 20, 2012 at 8:14 AM, Bernd Schubert
>> <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>  wrote:
>>> I *guess* the qp allocated by pd->context->ops.create_qp() does not have
>>> qp->usecnt initialized (not does it know anything about it). So its random
>>> value will fail the destruction later. A simple workaround that would work
>>> for us, is to extend the patch I send to
>>>
>>> diff --git a/drivers/infiniband/core/verbs.c
>>> b/drivers/infiniband/core/verbs.c
>>> index 602b1bd..fba1675 100644
>>> --- a/drivers/infiniband/core/verbs.c
>>> +++ b/drivers/infiniband/core/verbs.c
>>> @@ -874,7 +874,7 @@ int ib_destroy_qp(struct ib_qp *qp)
>>>         struct ib_srq *srq;
>>>         int ret;
>>>
>>> -       if (atomic_read(&qp->usecnt))
>>> +       if (qp->qp_type == IB_QPT_XRC_TGT&&  atomic_read(&qp->usecnt))
>>>                 return -EBUSY;
>>>
>>>         if (qp->real_qp != qp)
>>
>> It looks like this is sufficient and correct without the other patch?
>
> But maybe it's cleaner to initialize qp->usecnt in
> both ib_create_qp() and ib_uverbs_create_qp().
>
>   - R.


is there any progress on this? We have already reached Linux stable 
3.2.2 and IB still appears to be broken (at least for mthca) because of 
this.

Best regards,
Sven

--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Roland Dreier]

On Fri, Jan 27, 2012 at 8:23 AM, Sven Breuner
<sven.breuner-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org> wrote:
> is there any progress on this? We have already reached Linux stable 3.2.2
> and IB still appears to be broken (at least for mthca) because of this.

I've applied it and will send it to Linus soon.

 - 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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Sven Breuner]

Hi Roland,

Roland Dreier wrote on 01/27/2012 06:20 PM:
> I've applied it and will send it to Linus soon.

thanks! Will you also take care of the Linux-stable (3.2.x) backport or 
should I send it for inclusion in Linux-stable?

Best regards,
Sven

--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Roland Dreier]

On Fri, Jan 27, 2012 at 10:49 AM, Sven Breuner
<sven.breuner-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org> wrote:
> thanks! Will you also take care of the Linux-stable (3.2.x) backport or
> should I send it for inclusion in Linux-stable?

I added the CC to stable in the commit so it should happen automatically.
--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Bernd Schubert]

On 01/20/2012 07:40 PM, Roland Dreier wrote:
> On Fri, Jan 20, 2012 at 8:14 AM, Bernd Schubert
> <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>  wrote:
>> I *guess* the qp allocated by pd->context->ops.create_qp() does not have
>> qp->usecnt initialized (not does it know anything about it). So its random
>> value will fail the destruction later. A simple workaround that would work
>> for us, is to extend the patch I send to
>>
>> diff --git a/drivers/infiniband/core/verbs.c
>> b/drivers/infiniband/core/verbs.c
>> index 602b1bd..fba1675 100644
>> --- a/drivers/infiniband/core/verbs.c
>> +++ b/drivers/infiniband/core/verbs.c
>> @@ -874,7 +874,7 @@ int ib_destroy_qp(struct ib_qp *qp)
>>         struct ib_srq *srq;
>>         int ret;
>>
>> -       if (atomic_read(&qp->usecnt))
>> +       if (qp->qp_type == IB_QPT_XRC_TGT&&  atomic_read(&qp->usecnt))
>>                 return -EBUSY;
>>
>>         if (qp->real_qp != qp)
>
> It looks like this is sufficient and correct without the other patch?
>
>>
>>
>> However, what is is with user space setting type to IB_QPT_XRC_TGT? I guess
>> this could be solved by letting the kernel zero the memory returned by
>> ->ops.create_qp(pd, qp_init_attr).
>> Btw, I didn't figure out yet, how this translates at all in kernel space? Is
>> this op directly going to the device driver?
>>
>> But even if we are properly going to initialize the qp, what is with user
>> space mischievously trying to crash the system by manipulating struct ib_qp
>> *qp?
>
> I don't follow this.  Isn't *qp completely allocated and manipulated
> in the kernel?  How can userspace touch it except by having the
> kernel do something via the uverbs interface?

Sorry, I first didn't see the ib_uverbs_create_qp() interface and 
copy_from/to_user() there.


Cheers,
Bernd

--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Greg KH]

On Thu, Jan 19, 2012 at 08:46:41PM +0100, Bernd Schubert wrote:
> We have just been investigating kernel panics related to
> cq->ibcq.event_handler() completion calls.
> 
> Reason is that ib_destroy_qp() fails with -EBUSY. Further investigation
> revealed qp->usecnt is not initialized. This counter was introduced
> in linux-3.2 by commit 0e0ec7e0638ef48e0c661873dfcc8caccab984c6
> and is only initialized for IB_QPT_XRC_TGT, but also checked in ib_destroy_qp()
> for any qp type.
> 
> Signed-off-by: Bernd Schubert <bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
> Signed-off-by: Sven Breuner <sven.breuner-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
> ---
>  drivers/infiniband/core/verbs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>
--
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] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Hefty, Sean]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3559 bytes --]

> However, what is is with user space setting type to IB_QPT_XRC_TGT? I
> guess this could be solved by letting the kernel zero the memory
> returned by ->ops.create_qp(pd, qp_init_attr).
> Btw, I didn't figure out yet, how this translates at all in kernel
> space? Is this op directly going to the device driver?

ops.create_qp basically ends up going into the kernel into ib_uverbs_create_qp().

> But even if we are properly going to initialize the qp, what is with
> user space mischievously trying to crash the system by manipulating
> struct ib_qp *qp?

There's cleanup in uverbs that ignores the return value from ib_destroy_qp(), basically because it shouldn't fail in those circumstances.  After calling ib_destroy_qp, uverbs will free some internal structures that some of the callback handlers expect to access.  This leads to the crashes that you're seeing.

I think the problem is that your first patch is incomplete.  ib_uverbs_create_qp() will create a QP by either calling ib_create_qp() or by calling the device directly (device->create_qp).  qp->usecnt needs to be initialized in both cases.  Can you try this modification to your original patch?

From: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>

From: Sean Hefty <sean.hefty@intel.com>

rdma/core: Fix kernel panic by always initializing qp->usecnt

We have just been investigating kernel panics related to
cq->ibcq.event_handler() completion calls.

Reason is that ib_destroy_qp() fails with -EBUSY.  Further investigation
revealed qp->usecnt is not initialized.  This counter was introduced
in linux-3.2 by commit 0e0ec7e0638ef48e0c661873dfcc8caccab984c6
and is only initialized for IB_QPT_XRC_TGT, but also checked in ib_destroy_qp()
for any qp type.

Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
Signed-off-by: Sven Breuner <sven.breuner@itwm.fraunhofer.de>
Signed-off-by: Sean Hefty <sean.hefty@intel.com>
---
 drivers/infiniband/core/uverbs_cmd.c |    1 +
 drivers/infiniband/core/verbs.c      |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index e26193f..e47dbf1 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1472,6 +1472,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 		qp->event_handler = attr.event_handler;
 		qp->qp_context	  = attr.qp_context;
 		qp->qp_type	  = attr.qp_type;
+		atomic_set(&qp->usecnt, 0);
 		atomic_inc(&pd->usecnt);
 		atomic_inc(&attr.send_cq->usecnt);
 		if (attr.recv_cq)
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 602b1bd..575b780 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -421,6 +421,7 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 		qp->uobject    = NULL;
 		qp->qp_type    = qp_init_attr->qp_type;
 
+		atomic_set(&qp->usecnt, 0);
 		if (qp_init_attr->qp_type == IB_QPT_XRC_TGT) {
 			qp->event_handler = __ib_shared_qp_event_handler;
 			qp->qp_context = qp;
@@ -430,7 +431,6 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 			qp->xrcd = qp_init_attr->xrcd;
 			atomic_inc(&qp_init_attr->xrcd->usecnt);
 			INIT_LIST_HEAD(&qp->open_list);
-			atomic_set(&qp->usecnt, 0);
 
 			real_qp = qp;
 			qp = __ib_open_qp(real_qp, qp_init_attr->event_handler,


N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH] core/verb.c: fix kernel panic: always initialize struct ib_qp *qp->usecnt

[Bernd Schubert]

On 01/20/2012 07:43 PM, Hefty, Sean wrote:
>> However, what is is with user space setting type to IB_QPT_XRC_TGT?
>> I guess this could be solved by letting the kernel zero the memory
>> returned by ->ops.create_qp(pd, qp_init_attr). Btw, I didn't figure
>> out yet, how this translates at all in kernel space? Is this op
>> directly going to the device driver?
>
> ops.create_qp basically ends up going into the kernel into
> ib_uverbs_create_qp().

Thanks, I didn't find this.

>
>> But even if we are properly going to initialize the qp, what is
>> with user space mischievously trying to crash the system by
>> manipulating struct ib_qp *qp?
>
> There's cleanup in uverbs that ignores the return value from
> ib_destroy_qp(), basically because it shouldn't fail in those
> circumstances.  After calling ib_destroy_qp, uverbs will free some
> internal structures that some of the callback handlers expect to
> access.  This leads to the crashes that you're seeing.
>
> I think the problem is that your first patch is incomplete.
> ib_uverbs_create_qp() will create a QP by either calling
> ib_create_qp() or by calling the device directly (device->create_qp).
> qp->usecnt needs to be initialized in both cases.  Can you try this
> modification to your original patch?

Thanks, this works either. But a question here, couldn't we just add the 
"struct ib_udata *udata" as third parameter to ib_create_qp() and then 
remove the if-condition in ib_uverbs_create_qp()?

	if (cmd.qp_type == IB_QPT_XRC_TGT)
		qp = ib_create_qp(pd, &attr);
	else
		qp = device->create_qp(pd, &attr, &udata);


So reduce this to
	qp = ib_create_qp(pd, &attr, &attr);


Other callers of ib_create_qp() are not that many and would pass NULL 
instead.


Cheers,
Bernd

>
> From: Bernd Schubert<bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
>
> From: Sean Hefty<sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> rdma/core: Fix kernel panic by always initializing qp->usecnt
>
> We have just been investigating kernel panics related to
> cq->ibcq.event_handler() completion calls.
>
> Reason is that ib_destroy_qp() fails with -EBUSY.  Further
> investigation revealed qp->usecnt is not initialized.  This counter
> was introduced in linux-3.2 by commit
> 0e0ec7e0638ef48e0c661873dfcc8caccab984c6 and is only initialized for
> IB_QPT_XRC_TGT, but also checked in ib_destroy_qp() for any qp type.
>
> Signed-off-by: Bernd Schubert<bernd.schubert-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
> Signed-off-by: Sven Breuner<sven.breuner-mPn0NPGs4xGatNDF+KUbs4QuADTiUCJX@public.gmane.org>
> Signed-off-by: Sean Hefty<sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> ---
> drivers/infiniband/core/uverbs_cmd.c |    1 +
> drivers/infiniband/core/verbs.c      |    2 +- 2 files changed, 2
> insertions(+), 1 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c
> b/drivers/infiniband/core/uverbs_cmd.c index e26193f..e47dbf1 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c +++
> b/drivers/infiniband/core/uverbs_cmd.c @@ -1472,6 +1472,7 @@ ssize_t
> ib_uverbs_create_qp(struct ib_uverbs_file *file, qp->event_handler =
> attr.event_handler; qp->qp_context	  = attr.qp_context; qp->qp_type
> = attr.qp_type; +		atomic_set(&qp->usecnt, 0);
> atomic_inc(&pd->usecnt); atomic_inc(&attr.send_cq->usecnt); if
> (attr.recv_cq) diff --git a/drivers/infiniband/core/verbs.c
> b/drivers/infiniband/core/verbs.c index 602b1bd..575b780 100644 ---
> a/drivers/infiniband/core/verbs.c +++
> b/drivers/infiniband/core/verbs.c @@ -421,6 +421,7 @@ struct ib_qp
> *ib_create_qp(struct ib_pd *pd, qp->uobject    = NULL; qp->qp_type
> = qp_init_attr->qp_type;
>
> +		atomic_set(&qp->usecnt, 0); if (qp_init_attr->qp_type ==
> IB_QPT_XRC_TGT) { qp->event_handler = __ib_shared_qp_event_handler;
> qp->qp_context = qp; @@ -430,7 +431,6 @@ struct ib_qp
> *ib_create_qp(struct ib_pd *pd, qp->xrcd = qp_init_attr->xrcd;
> atomic_inc(&qp_init_attr->xrcd->usecnt);
> INIT_LIST_HEAD(&qp->open_list); -			atomic_set(&qp->usecnt, 0);
>
> real_qp = qp; qp = __ib_open_qp(real_qp,
> qp_init_attr->event_handler,
>
>

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