From: Jason Gunthorpe <jgg@nvidia.com> To: Danil Kipnis <danil.kipnis@cloud.ionos.com>, Doug Ledford <dledford@redhat.com>, Christoph Hellwig <hch@lst.de>, Jack Wang <jinpu.wang@cloud.ionos.com>, Keith Busch <kbusch@kernel.org>, <linux-nvme@lists.infradead.org>, <linux-rdma@vger.kernel.org>, Max Gurtovoy <mgurtovoy@nvidia.com>, <netdev@vger.kernel.org>, <rds-devel@oss.oracle.com>, Sagi Grimberg <sagi@grimberg.me>, Santosh Shilimkar <santosh.shilimkar@oracle.com> Cc: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>, Leon Romanovsky <leonro@nvidia.com> Subject: [PATCH] RDMA: Add rdma_connect_locked() Date: Mon, 26 Oct 2020 11:25:49 -0300 [thread overview] Message-ID: <0-v1-75e124dbad74+b05-rdma_connect_locking_jgg@nvidia.com> (raw) There are two flows for handling RDMA_CM_EVENT_ROUTE_RESOLVED, either the handler triggers a completion and another thread does rdma_connect() or the handler directly calls rdma_connect(). In all cases rdma_connect() needs to hold the handler_mutex, but when handler's are invoked this is already held by the core code. This causes ULPs using the 2nd method to deadlock. Provide a rdma_connect_locked() and have all ULPs call it from their handlers. Reported-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com> Fixes: 2a7cec538169 ("RDMA/cma: Fix locking for the RDMA_CM_CONNECT state" Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/infiniband/core/cma.c | 39 +++++++++++++++++++++--- drivers/infiniband/ulp/iser/iser_verbs.c | 2 +- drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 +-- drivers/nvme/host/rdma.c | 10 +++--- include/rdma/rdma_cm.h | 13 +------- net/rds/ib_cm.c | 5 +-- 6 files changed, 47 insertions(+), 26 deletions(-) Seems people are not testing these four ULPs against rdma-next.. Here is a quick fix for the issue: https://lore.kernel.org/r/3b1f7767-98e2-93e0-b718-16d1c5346140@cloud.ionos.com Jason diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 7c2ab1f2fbea37..2eaaa1292fb847 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -405,10 +405,10 @@ static int cma_comp_exch(struct rdma_id_private *id_priv, /* * The FSM uses a funny double locking where state is protected by both * the handler_mutex and the spinlock. State is not allowed to change - * away from a handler_mutex protected value without also holding + * to/from a handler_mutex protected value without also holding * handler_mutex. */ - if (comp == RDMA_CM_CONNECT) + if (comp == RDMA_CM_CONNECT || exch == RDMA_CM_CONNECT) lockdep_assert_held(&id_priv->handler_mutex); spin_lock_irqsave(&id_priv->lock, flags); @@ -4038,13 +4038,20 @@ static int cma_connect_iw(struct rdma_id_private *id_priv, return ret; } -int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) +/** + * rdma_connect_locked - Initiate an active connection request. + * @id: Connection identifier to connect. + * @conn_param: Connection information used for connected QPs. + * + * Same as rdma_connect() but can only be called from the + * RDMA_CM_EVENT_ROUTE_RESOLVED handler callback. + */ +int rdma_connect_locked(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) { struct rdma_id_private *id_priv = container_of(id, struct rdma_id_private, id); int ret; - mutex_lock(&id_priv->handler_mutex); if (!cma_comp_exch(id_priv, RDMA_CM_ROUTE_RESOLVED, RDMA_CM_CONNECT)) { ret = -EINVAL; goto err_unlock; @@ -4071,6 +4078,30 @@ int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) err_state: cma_comp_exch(id_priv, RDMA_CM_CONNECT, RDMA_CM_ROUTE_RESOLVED); err_unlock: + return ret; +} +EXPORT_SYMBOL(rdma_connect_locked); + +/** + * rdma_connect - Initiate an active connection request. + * @id: Connection identifier to connect. + * @conn_param: Connection information used for connected QPs. + * + * Users must have resolved a route for the rdma_cm_id to connect with by having + * called rdma_resolve_route before calling this routine. + * + * This call will either connect to a remote QP or obtain remote QP information + * for unconnected rdma_cm_id's. The actual operation is based on the + * rdma_cm_id's port space. + */ +int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) +{ + struct rdma_id_private *id_priv = + container_of(id, struct rdma_id_private, id); + int ret; + + mutex_lock(&id_priv->handler_mutex); + ret = rdma_connect_locked(id, conn_param); mutex_unlock(&id_priv->handler_mutex); return ret; } diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c index 2f3ebc0a75d924..2bd18b00689341 100644 --- a/drivers/infiniband/ulp/iser/iser_verbs.c +++ b/drivers/infiniband/ulp/iser/iser_verbs.c @@ -620,7 +620,7 @@ static void iser_route_handler(struct rdma_cm_id *cma_id) conn_param.private_data = (void *)&req_hdr; conn_param.private_data_len = sizeof(struct iser_cm_hdr); - ret = rdma_connect(cma_id, &conn_param); + ret = rdma_connect_locked(cma_id, &conn_param); if (ret) { iser_err("failure connecting: %d\n", ret); goto failure; diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c index 776e89231c52f7..f298adc02acba2 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c @@ -1674,9 +1674,9 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con) uuid_copy(&msg.sess_uuid, &sess->s.uuid); uuid_copy(&msg.paths_uuid, &clt->paths_uuid); - err = rdma_connect(con->c.cm_id, ¶m); + err = rdma_connect_locked(con->c.cm_id, ¶m); if (err) - rtrs_err(clt, "rdma_connect(): %d\n", err); + rtrs_err(clt, "rdma_connect_locked(): %d\n", err); return err; } diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index aad829a2b50d0f..f488dc5f4c2c61 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1730,11 +1730,10 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue, req->result = cqe->result; if (wc->wc_flags & IB_WC_WITH_INVALIDATE) { - if (unlikely(!req->mr || - wc->ex.invalidate_rkey != req->mr->rkey)) { + if (unlikely(wc->ex.invalidate_rkey != req->mr->rkey)) { dev_err(queue->ctrl->ctrl.device, "Bogus remote invalidation for rkey %#x\n", - req->mr ? req->mr->rkey : 0); + req->mr->rkey); nvme_rdma_error_recovery(queue->ctrl); } } else if (req->mr) { @@ -1890,10 +1889,10 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue) priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize); } - ret = rdma_connect(queue->cm_id, ¶m); + ret = rdma_connect_locked(queue->cm_id, ¶m); if (ret) { dev_err(ctrl->ctrl.device, - "rdma_connect failed (%d).\n", ret); + "rdma_connect_locked failed (%d).\n", ret); goto out_destroy_queue_ib; } @@ -1927,6 +1926,7 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id, complete(&queue->cm_done); return 0; case RDMA_CM_EVENT_REJECTED: + nvme_rdma_destroy_queue_ib(queue); cm_error = nvme_rdma_conn_rejected(queue, ev); break; case RDMA_CM_EVENT_ROUTE_ERROR: diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h index c672ae1da26bb5..937d55611cd073 100644 --- a/include/rdma/rdma_cm.h +++ b/include/rdma/rdma_cm.h @@ -227,19 +227,8 @@ void rdma_destroy_qp(struct rdma_cm_id *id); int rdma_init_qp_attr(struct rdma_cm_id *id, struct ib_qp_attr *qp_attr, int *qp_attr_mask); -/** - * rdma_connect - Initiate an active connection request. - * @id: Connection identifier to connect. - * @conn_param: Connection information used for connected QPs. - * - * Users must have resolved a route for the rdma_cm_id to connect with - * by having called rdma_resolve_route before calling this routine. - * - * This call will either connect to a remote QP or obtain remote QP - * information for unconnected rdma_cm_id's. The actual operation is - * based on the rdma_cm_id's port space. - */ int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param); +int rdma_connect_locked(struct rdma_cm_id *id, struct rdma_conn_param *conn_param); int rdma_connect_ece(struct rdma_cm_id *id, struct rdma_conn_param *conn_param, struct rdma_ucm_ece *ece); diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c index 06603dd1c8aa38..b36b60668b1da9 100644 --- a/net/rds/ib_cm.c +++ b/net/rds/ib_cm.c @@ -956,9 +956,10 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6) rds_ib_cm_fill_conn_param(conn, &conn_param, &dp, conn->c_proposed_version, UINT_MAX, UINT_MAX, isv6); - ret = rdma_connect(cm_id, &conn_param); + ret = rdma_connect_locked(cm_id, &conn_param); if (ret) - rds_ib_conn_error(conn, "rdma_connect failed (%d)\n", ret); + rds_ib_conn_error(conn, "rdma_connect_locked failed (%d)\n", + ret); out: /* Beware - returning non-zero tells the rdma_cm to destroy -- 2.28.0
WARNING: multiple messages have this Message-ID (diff)
From: Jason Gunthorpe <jgg@nvidia.com> To: Danil Kipnis <danil.kipnis@cloud.ionos.com>, Doug Ledford <dledford@redhat.com>, Christoph Hellwig <hch@lst.de>, Jack Wang <jinpu.wang@cloud.ionos.com>, Keith Busch <kbusch@kernel.org>, <linux-nvme@lists.infradead.org>, <linux-rdma@vger.kernel.org>, Max Gurtovoy <mgurtovoy@nvidia.com>, <netdev@vger.kernel.org>, <rds-devel@oss.oracle.com>, Sagi Grimberg <sagi@grimberg.me>, Santosh Shilimkar <santosh.shilimkar@oracle.com> Cc: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>, Leon Romanovsky <leonro@nvidia.com> Subject: [PATCH] RDMA: Add rdma_connect_locked() Date: Mon, 26 Oct 2020 11:25:49 -0300 [thread overview] Message-ID: <0-v1-75e124dbad74+b05-rdma_connect_locking_jgg@nvidia.com> (raw) There are two flows for handling RDMA_CM_EVENT_ROUTE_RESOLVED, either the handler triggers a completion and another thread does rdma_connect() or the handler directly calls rdma_connect(). In all cases rdma_connect() needs to hold the handler_mutex, but when handler's are invoked this is already held by the core code. This causes ULPs using the 2nd method to deadlock. Provide a rdma_connect_locked() and have all ULPs call it from their handlers. Reported-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com> Fixes: 2a7cec538169 ("RDMA/cma: Fix locking for the RDMA_CM_CONNECT state" Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/infiniband/core/cma.c | 39 +++++++++++++++++++++--- drivers/infiniband/ulp/iser/iser_verbs.c | 2 +- drivers/infiniband/ulp/rtrs/rtrs-clt.c | 4 +-- drivers/nvme/host/rdma.c | 10 +++--- include/rdma/rdma_cm.h | 13 +------- net/rds/ib_cm.c | 5 +-- 6 files changed, 47 insertions(+), 26 deletions(-) Seems people are not testing these four ULPs against rdma-next.. Here is a quick fix for the issue: https://lore.kernel.org/r/3b1f7767-98e2-93e0-b718-16d1c5346140@cloud.ionos.com Jason diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 7c2ab1f2fbea37..2eaaa1292fb847 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -405,10 +405,10 @@ static int cma_comp_exch(struct rdma_id_private *id_priv, /* * The FSM uses a funny double locking where state is protected by both * the handler_mutex and the spinlock. State is not allowed to change - * away from a handler_mutex protected value without also holding + * to/from a handler_mutex protected value without also holding * handler_mutex. */ - if (comp == RDMA_CM_CONNECT) + if (comp == RDMA_CM_CONNECT || exch == RDMA_CM_CONNECT) lockdep_assert_held(&id_priv->handler_mutex); spin_lock_irqsave(&id_priv->lock, flags); @@ -4038,13 +4038,20 @@ static int cma_connect_iw(struct rdma_id_private *id_priv, return ret; } -int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) +/** + * rdma_connect_locked - Initiate an active connection request. + * @id: Connection identifier to connect. + * @conn_param: Connection information used for connected QPs. + * + * Same as rdma_connect() but can only be called from the + * RDMA_CM_EVENT_ROUTE_RESOLVED handler callback. + */ +int rdma_connect_locked(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) { struct rdma_id_private *id_priv = container_of(id, struct rdma_id_private, id); int ret; - mutex_lock(&id_priv->handler_mutex); if (!cma_comp_exch(id_priv, RDMA_CM_ROUTE_RESOLVED, RDMA_CM_CONNECT)) { ret = -EINVAL; goto err_unlock; @@ -4071,6 +4078,30 @@ int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) err_state: cma_comp_exch(id_priv, RDMA_CM_CONNECT, RDMA_CM_ROUTE_RESOLVED); err_unlock: + return ret; +} +EXPORT_SYMBOL(rdma_connect_locked); + +/** + * rdma_connect - Initiate an active connection request. + * @id: Connection identifier to connect. + * @conn_param: Connection information used for connected QPs. + * + * Users must have resolved a route for the rdma_cm_id to connect with by having + * called rdma_resolve_route before calling this routine. + * + * This call will either connect to a remote QP or obtain remote QP information + * for unconnected rdma_cm_id's. The actual operation is based on the + * rdma_cm_id's port space. + */ +int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) +{ + struct rdma_id_private *id_priv = + container_of(id, struct rdma_id_private, id); + int ret; + + mutex_lock(&id_priv->handler_mutex); + ret = rdma_connect_locked(id, conn_param); mutex_unlock(&id_priv->handler_mutex); return ret; } diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c index 2f3ebc0a75d924..2bd18b00689341 100644 --- a/drivers/infiniband/ulp/iser/iser_verbs.c +++ b/drivers/infiniband/ulp/iser/iser_verbs.c @@ -620,7 +620,7 @@ static void iser_route_handler(struct rdma_cm_id *cma_id) conn_param.private_data = (void *)&req_hdr; conn_param.private_data_len = sizeof(struct iser_cm_hdr); - ret = rdma_connect(cma_id, &conn_param); + ret = rdma_connect_locked(cma_id, &conn_param); if (ret) { iser_err("failure connecting: %d\n", ret); goto failure; diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c index 776e89231c52f7..f298adc02acba2 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c @@ -1674,9 +1674,9 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con) uuid_copy(&msg.sess_uuid, &sess->s.uuid); uuid_copy(&msg.paths_uuid, &clt->paths_uuid); - err = rdma_connect(con->c.cm_id, ¶m); + err = rdma_connect_locked(con->c.cm_id, ¶m); if (err) - rtrs_err(clt, "rdma_connect(): %d\n", err); + rtrs_err(clt, "rdma_connect_locked(): %d\n", err); return err; } diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index aad829a2b50d0f..f488dc5f4c2c61 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1730,11 +1730,10 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue, req->result = cqe->result; if (wc->wc_flags & IB_WC_WITH_INVALIDATE) { - if (unlikely(!req->mr || - wc->ex.invalidate_rkey != req->mr->rkey)) { + if (unlikely(wc->ex.invalidate_rkey != req->mr->rkey)) { dev_err(queue->ctrl->ctrl.device, "Bogus remote invalidation for rkey %#x\n", - req->mr ? req->mr->rkey : 0); + req->mr->rkey); nvme_rdma_error_recovery(queue->ctrl); } } else if (req->mr) { @@ -1890,10 +1889,10 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue) priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize); } - ret = rdma_connect(queue->cm_id, ¶m); + ret = rdma_connect_locked(queue->cm_id, ¶m); if (ret) { dev_err(ctrl->ctrl.device, - "rdma_connect failed (%d).\n", ret); + "rdma_connect_locked failed (%d).\n", ret); goto out_destroy_queue_ib; } @@ -1927,6 +1926,7 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id, complete(&queue->cm_done); return 0; case RDMA_CM_EVENT_REJECTED: + nvme_rdma_destroy_queue_ib(queue); cm_error = nvme_rdma_conn_rejected(queue, ev); break; case RDMA_CM_EVENT_ROUTE_ERROR: diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h index c672ae1da26bb5..937d55611cd073 100644 --- a/include/rdma/rdma_cm.h +++ b/include/rdma/rdma_cm.h @@ -227,19 +227,8 @@ void rdma_destroy_qp(struct rdma_cm_id *id); int rdma_init_qp_attr(struct rdma_cm_id *id, struct ib_qp_attr *qp_attr, int *qp_attr_mask); -/** - * rdma_connect - Initiate an active connection request. - * @id: Connection identifier to connect. - * @conn_param: Connection information used for connected QPs. - * - * Users must have resolved a route for the rdma_cm_id to connect with - * by having called rdma_resolve_route before calling this routine. - * - * This call will either connect to a remote QP or obtain remote QP - * information for unconnected rdma_cm_id's. The actual operation is - * based on the rdma_cm_id's port space. - */ int rdma_connect(struct rdma_cm_id *id, struct rdma_conn_param *conn_param); +int rdma_connect_locked(struct rdma_cm_id *id, struct rdma_conn_param *conn_param); int rdma_connect_ece(struct rdma_cm_id *id, struct rdma_conn_param *conn_param, struct rdma_ucm_ece *ece); diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c index 06603dd1c8aa38..b36b60668b1da9 100644 --- a/net/rds/ib_cm.c +++ b/net/rds/ib_cm.c @@ -956,9 +956,10 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6) rds_ib_cm_fill_conn_param(conn, &conn_param, &dp, conn->c_proposed_version, UINT_MAX, UINT_MAX, isv6); - ret = rdma_connect(cm_id, &conn_param); + ret = rdma_connect_locked(cm_id, &conn_param); if (ret) - rds_ib_conn_error(conn, "rdma_connect failed (%d)\n", ret); + rds_ib_conn_error(conn, "rdma_connect_locked failed (%d)\n", + ret); out: /* Beware - returning non-zero tells the rdma_cm to destroy -- 2.28.0 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next reply other threads:[~2020-10-26 14:25 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-26 14:25 Jason Gunthorpe [this message] 2020-10-26 14:25 ` [PATCH] RDMA: Add rdma_connect_locked() Jason Gunthorpe 2020-10-26 16:01 ` santosh.shilimkar 2020-10-26 16:01 ` santosh.shilimkar 2020-10-27 2:01 ` Chao Leng 2020-10-27 2:01 ` Chao Leng 2020-10-27 12:00 ` Jason Gunthorpe 2020-10-27 12:00 ` Jason Gunthorpe 2020-10-27 7:33 ` Jinpu Wang 2020-10-27 7:33 ` Jinpu Wang 2020-10-27 8:04 ` Christoph Hellwig 2020-10-27 8:04 ` Christoph Hellwig 2020-10-27 12:05 ` Guoqing Jiang 2020-10-27 12:05 ` Guoqing Jiang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0-v1-75e124dbad74+b05-rdma_connect_locking_jgg@nvidia.com \ --to=jgg@nvidia.com \ --cc=danil.kipnis@cloud.ionos.com \ --cc=dledford@redhat.com \ --cc=guoqing.jiang@cloud.ionos.com \ --cc=hch@lst.de \ --cc=jinpu.wang@cloud.ionos.com \ --cc=kbusch@kernel.org \ --cc=leonro@nvidia.com \ --cc=linux-nvme@lists.infradead.org \ --cc=linux-rdma@vger.kernel.org \ --cc=mgurtovoy@nvidia.com \ --cc=netdev@vger.kernel.org \ --cc=rds-devel@oss.oracle.com \ --cc=sagi@grimberg.me \ --cc=santosh.shilimkar@oracle.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.