All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used
@ 2017-08-29 17:34 Roland Dreier
       [not found] ` <20170829173444.4289-1-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
  0 siblings, 1 reply; 5+ messages in thread
From: Roland Dreier @ 2017-08-29 17:34 UTC (permalink / raw)
  To: Doug Ledford, Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

A couple of places in the CM do

    spin_lock_irq(&cm_id_priv->lock);
    ...
    if (cm_alloc_response_msg(work->port, work->mad_recv_wc, &msg))

However when the underlying transport is RoCE, this leads to a sleeping function
being called with the lock held - the callchain is

    cm_alloc_response_msg() ->
      ib_create_ah_from_wc() ->
        ib_init_ah_from_wc() ->
          rdma_addr_find_l2_eth_by_grh() ->
            rdma_resolve_ip()

and rdma_resolve_ip() starts out by doing

    req = kzalloc(sizeof *req, GFP_KERNEL);

not to mention rdma_addr_find_l2_eth_by_grh() doing

    wait_for_completion(&ctx.comp);

to wait for the task that rdma_resolve_ip() queues up.

Fix this by moving the AH creation out of the lock.

Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/core/cm.c | 63 +++++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 2b4d613a3474..f814c5035c74 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -373,11 +373,19 @@ static int cm_alloc_msg(struct cm_id_private *cm_id_priv,
 	return ret;
 }
 
-static int cm_alloc_response_msg(struct cm_port *port,
-				 struct ib_mad_recv_wc *mad_recv_wc,
-				 struct ib_mad_send_buf **msg)
+static struct ib_mad_send_buf *cm_alloc_response_msg_no_ah(struct cm_port *port,
+							   struct ib_mad_recv_wc *mad_recv_wc)
+{
+	return ib_create_send_mad(port->mad_agent, 1, mad_recv_wc->wc->pkey_index,
+				  0, IB_MGMT_MAD_HDR, IB_MGMT_MAD_DATA,
+				  GFP_ATOMIC,
+				  IB_MGMT_BASE_VERSION);
+}
+
+static int cm_create_response_msg_ah(struct cm_port *port,
+				     struct ib_mad_recv_wc *mad_recv_wc,
+				     struct ib_mad_send_buf *msg)
 {
-	struct ib_mad_send_buf *m;
 	struct ib_ah *ah;
 
 	ah = ib_create_ah_from_wc(port->mad_agent->qp->pd, mad_recv_wc->wc,
@@ -385,27 +393,40 @@ static int cm_alloc_response_msg(struct cm_port *port,
 	if (IS_ERR(ah))
 		return PTR_ERR(ah);
 
-	m = ib_create_send_mad(port->mad_agent, 1, mad_recv_wc->wc->pkey_index,
-			       0, IB_MGMT_MAD_HDR, IB_MGMT_MAD_DATA,
-			       GFP_ATOMIC,
-			       IB_MGMT_BASE_VERSION);
-	if (IS_ERR(m)) {
-		rdma_destroy_ah(ah);
-		return PTR_ERR(m);
-	}
-	m->ah = ah;
-	*msg = m;
+	msg->ah = ah;
 	return 0;
 }
 
 static void cm_free_msg(struct ib_mad_send_buf *msg)
 {
-	rdma_destroy_ah(msg->ah);
+	if (msg->ah)
+		rdma_destroy_ah(msg->ah);
 	if (msg->context[0])
 		cm_deref_id(msg->context[0]);
 	ib_free_send_mad(msg);
 }
 
+static int cm_alloc_response_msg(struct cm_port *port,
+				 struct ib_mad_recv_wc *mad_recv_wc,
+				 struct ib_mad_send_buf **msg)
+{
+	struct ib_mad_send_buf *m;
+	int ret;
+
+	m = cm_alloc_response_msg_no_ah(port, mad_recv_wc);
+	if (IS_ERR(m))
+		return PTR_ERR(m);
+
+	ret = cm_create_response_msg_ah(port, mad_recv_wc, m);
+	if (ret) {
+		cm_free_msg(m);
+		return ret;
+	}
+
+	*msg = m;
+	return 0;
+}
+
 static void * cm_copy_private_data(const void *private_data,
 				   u8 private_data_len)
 {
@@ -2424,7 +2445,8 @@ static int cm_dreq_handler(struct cm_work *work)
 	case IB_CM_TIMEWAIT:
 		atomic_long_inc(&work->port->counter_group[CM_RECV_DUPLICATES].
 				counter[CM_DREQ_COUNTER]);
-		if (cm_alloc_response_msg(work->port, work->mad_recv_wc, &msg))
+		msg = cm_alloc_response_msg_no_ah(work->port, work->mad_recv_wc);
+		if (IS_ERR(msg))
 			goto unlock;
 
 		cm_format_drep((struct cm_drep_msg *) msg->mad, cm_id_priv,
@@ -2432,7 +2454,8 @@ static int cm_dreq_handler(struct cm_work *work)
 			       cm_id_priv->private_data_len);
 		spin_unlock_irq(&cm_id_priv->lock);
 
-		if (ib_post_send_mad(msg, NULL))
+		if (cm_create_response_msg_ah(work->port, work->mad_recv_wc, msg) ||
+		    ib_post_send_mad(msg, NULL))
 			cm_free_msg(msg);
 		goto deref;
 	case IB_CM_DREQ_RCVD:
@@ -2980,7 +3003,8 @@ static int cm_lap_handler(struct cm_work *work)
 	case IB_CM_MRA_LAP_SENT:
 		atomic_long_inc(&work->port->counter_group[CM_RECV_DUPLICATES].
 				counter[CM_LAP_COUNTER]);
-		if (cm_alloc_response_msg(work->port, work->mad_recv_wc, &msg))
+		msg = cm_alloc_response_msg_no_ah(work->port, work->mad_recv_wc);
+		if (IS_ERR(msg))
 			goto unlock;
 
 		cm_format_mra((struct cm_mra_msg *) msg->mad, cm_id_priv,
@@ -2990,7 +3014,8 @@ static int cm_lap_handler(struct cm_work *work)
 			      cm_id_priv->private_data_len);
 		spin_unlock_irq(&cm_id_priv->lock);
 
-		if (ib_post_send_mad(msg, NULL))
+		if (cm_create_response_msg_ah(work->port, work->mad_recv_wc, msg) ||
+		    ib_post_send_mad(msg, NULL))
 			cm_free_msg(msg);
 		goto deref;
 	case IB_CM_LAP_RCVD:
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] IB/core: Add might_sleep() annotation to ib_init_ah_from_wc()
       [not found] ` <20170829173444.4289-1-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
@ 2017-08-29 17:34   ` Roland Dreier
       [not found]     ` <20170829173444.4289-2-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
  2017-08-29 18:37   ` [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used Hefty, Sean
  2017-08-30 14:23   ` Doug Ledford
  2 siblings, 1 reply; 5+ messages in thread
From: Roland Dreier @ 2017-08-29 17:34 UTC (permalink / raw)
  To: Doug Ledford, Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

For RoCE, ib_init_ah_from_wc() can follow the path

    ib_init_ah_from_wc() ->
      rdma_addr_find_l2_eth_by_grh() ->
        rdma_resolve_ip()

and rdma_resolve_ip() will sleep in kzalloc() and wait_for_completion().

However, developers will not see any warnings if they use ib_init_ah_from_wc()
in an atomic context and test only on IB, because the function doesn't
sleep in that case.

Add a might_sleep() so that lockdep will catch bugs no matter what hardware is
used to test.

Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index b456e3ca1876..84868f76f8b9 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -478,6 +478,8 @@ int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
 	union ib_gid dgid;
 	union ib_gid sgid;
 
+	might_sleep();
+
 	memset(ah_attr, 0, sizeof *ah_attr);
 	ah_attr->type = rdma_ah_find_type(device, port_num);
 	if (rdma_cap_eth_ah(device, port_num)) {
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used
       [not found] ` <20170829173444.4289-1-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
  2017-08-29 17:34   ` [PATCH 2/2] IB/core: Add might_sleep() annotation to ib_init_ah_from_wc() Roland Dreier
@ 2017-08-29 18:37   ` Hefty, Sean
  2017-08-30 14:23   ` Doug Ledford
  2 siblings, 0 replies; 5+ messages in thread
From: Hefty, Sean @ 2017-08-29 18:37 UTC (permalink / raw)
  To: Roland Dreier, Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> 
> A couple of places in the CM do
> 
>     spin_lock_irq(&cm_id_priv->lock);
>     ...
>     if (cm_alloc_response_msg(work->port, work->mad_recv_wc, &msg))
> 
> However when the underlying transport is RoCE, this leads to a
> sleeping function being called with the lock held - the callchain is
> 
>     cm_alloc_response_msg() ->
>       ib_create_ah_from_wc() ->
>         ib_init_ah_from_wc() ->
>           rdma_addr_find_l2_eth_by_grh() ->
>             rdma_resolve_ip()
> 
> and rdma_resolve_ip() starts out by doing
> 
>     req = kzalloc(sizeof *req, GFP_KERNEL);
> 
> not to mention rdma_addr_find_l2_eth_by_grh() doing
> 
>     wait_for_completion(&ctx.comp);
> 
> to wait for the task that rdma_resolve_ip() queues up.
> 
> Fix this by moving the AH creation out of the lock.
> 
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

Reviewed-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used
       [not found] ` <20170829173444.4289-1-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
  2017-08-29 17:34   ` [PATCH 2/2] IB/core: Add might_sleep() annotation to ib_init_ah_from_wc() Roland Dreier
  2017-08-29 18:37   ` [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used Hefty, Sean
@ 2017-08-30 14:23   ` Doug Ledford
  2 siblings, 0 replies; 5+ messages in thread
From: Doug Ledford @ 2017-08-30 14:23 UTC (permalink / raw)
  To: Roland Dreier, Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA


[-- Attachment #1.1: Type: text/plain, Size: 1265 bytes --]

On 8/29/2017 1:34 PM, Roland Dreier wrote:
> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> 
> A couple of places in the CM do
> 
>     spin_lock_irq(&cm_id_priv->lock);
>     ...
>     if (cm_alloc_response_msg(work->port, work->mad_recv_wc, &msg))
> 
> However when the underlying transport is RoCE, this leads to a sleeping function
> being called with the lock held - the callchain is
> 
>     cm_alloc_response_msg() ->
>       ib_create_ah_from_wc() ->
>         ib_init_ah_from_wc() ->
>           rdma_addr_find_l2_eth_by_grh() ->
>             rdma_resolve_ip()
> 
> and rdma_resolve_ip() starts out by doing
> 
>     req = kzalloc(sizeof *req, GFP_KERNEL);
> 
> not to mention rdma_addr_find_l2_eth_by_grh() doing
> 
>     wait_for_completion(&ctx.comp);
> 
> to wait for the task that rdma_resolve_ip() queues up.
> 
> Fix this by moving the AH creation out of the lock.
> 
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

This looks good Roland, thanks for that.  Will apply today.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] IB/core: Add might_sleep() annotation to ib_init_ah_from_wc()
       [not found]     ` <20170829173444.4289-2-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
@ 2017-08-30 14:24       ` Doug Ledford
  0 siblings, 0 replies; 5+ messages in thread
From: Doug Ledford @ 2017-08-30 14:24 UTC (permalink / raw)
  To: Roland Dreier, Sean Hefty; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA


[-- Attachment #1.1: Type: text/plain, Size: 954 bytes --]

On 8/29/2017 1:34 PM, Roland Dreier wrote:
> From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> 
> For RoCE, ib_init_ah_from_wc() can follow the path
> 
>     ib_init_ah_from_wc() ->
>       rdma_addr_find_l2_eth_by_grh() ->
>         rdma_resolve_ip()
> 
> and rdma_resolve_ip() will sleep in kzalloc() and wait_for_completion().
> 
> However, developers will not see any warnings if they use ib_init_ah_from_wc()
> in an atomic context and test only on IB, because the function doesn't
> sleep in that case.
> 
> Add a might_sleep() so that lockdep will catch bugs no matter what hardware is
> used to test.
> 
> Signed-off-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>

Thanks, will pick this up today too.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-08-30 14:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-29 17:34 [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used Roland Dreier
     [not found] ` <20170829173444.4289-1-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2017-08-29 17:34   ` [PATCH 2/2] IB/core: Add might_sleep() annotation to ib_init_ah_from_wc() Roland Dreier
     [not found]     ` <20170829173444.4289-2-roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2017-08-30 14:24       ` Doug Ledford
2017-08-29 18:37   ` [PATCH 1/2] IB/cm: Fix sleeping in atomic when RoCE is used Hefty, Sean
2017-08-30 14:23   ` Doug Ledford

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.