CM cleanup

PATCH Applied Leon Romanovsky 4 participants 20 messages Oct 20, 2019 – Oct 28, 2019

Subsystems (3)

InfiniBand SCSI RDMA protocol (SRP) initiator SCSI RDMA protocol (SRP) target

lore

Share & tools

lore

Leon Romanovsky

author

2019-10-20 07:16 UTC #0 lore raw

cover CM cleanup

From: Leon Romanovsky <[email protected]>

Hi,

This is first part of CM cleanup series needed to effectively add IBTA Enhanced Connection Establishment (ECE) spec changes.

In this series, we don't do anything major, just small code cleanup with small change in srpt. This change will allow us to provide general getter and setter for all CM structures.

Thanks

Leon Romanovsky (6):
RDMA/cm: Delete unused cm_is_active_peer function RDMA/cm: Use specific keyword to check define RDMA/cm: Update copyright together with SPDX tag RDMA/cm: Delete useless QPN masking
RDMA/cm: Provide private data size to CM users RDMA/srpt: Use private_data_len instead of hardcoded value

 drivers/infiniband/core/cm.c          | 51 ++++++++-------------------
 drivers/infiniband/core/cm_msgs.h     | 32 ++---------------
 drivers/infiniband/core/cma.c         | 31 ++--------------
 drivers/infiniband/ulp/srpt/ib_srpt.c |  2 +-
 include/rdma/ib_cm.h                  | 33 +++--------------
 5 files changed, 24 insertions(+), 125 deletions(-)

--
2.20.1

Leon Romanovsky

author

2019-10-20 07:16 UTC #1 lore raw

PATCH 1/6 RDMA/cm: Delete unused cm_is_active_peer function

From: Leon Romanovsky <[email protected]>

Function cm_is_active_peer is not used, delete it.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cm.c | 8 --------
 1 file changed, 8 deletions(-)

drivers/infiniband/core/cm.c +0 -8

index c0aa3a4b4cfd..bc4abb05dbd4 100644
@@ -1525,14 +1525,6 @@ static int cm_issue_rej(struct cm_port *port,
 	return ret;
 }
 
-static inline int cm_is_active_peer(__be64 local_ca_guid, __be64 remote_ca_guid,
-				    __be32 local_qpn, __be32 remote_qpn)
-{
-	return (be64_to_cpu(local_ca_guid) > be64_to_cpu(remote_ca_guid) ||
-		((local_ca_guid == remote_ca_guid) &&
-		 (be32_to_cpu(local_qpn) > be32_to_cpu(remote_qpn))));
-}
-
 static bool cm_req_has_alt_path(struct cm_req_msg *req_msg)
 {
 	return ((req_msg->alt_local_lid) ||

Signature · 1 line

2.20.1

Leon Romanovsky

author

2019-10-20 07:16 UTC #2 lore raw

PATCH 2/6 RDMA/cm: Use specific keyword to check define

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

There is a specific define keyword to check if define exists or not, let's use it instead of open-coded variant.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cm_msgs.h | 2 +-
 include/rdma/ib_cm.h              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/core/cm_msgs.h +1 -1

index 3d16d614aff6..0a9e2d3fb9df 100644
@@ -31,7 +31,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS THE
  * SOFTWARE.
  */
-#if !defined(CM_MSGS_H)
+#ifndef CM_MSGS_H
 #define CM_MSGS_H
 
 #include <rdma/ib_mad.h>

include/rdma/ib_cm.h +1 -1

index 49f4f75499b3..5dc4ec986527 100644
@@ -32,7 +32,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-#if !defined(IB_CM_H)
+#ifndef IB_CM_H
 #define IB_CM_H
 
 #include <rdma/ib_mad.h>

Signature · 1 line

2.20.1

Leon Romanovsky

author

2019-10-20 07:16 UTC #3 lore raw

PATCH 3/6 RDMA/cm: Update copyright together with SPDX tag

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Add Mellanox to lust of copyright holders and replace copyright boilerplate with relevant SPDX tag.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cm.c      | 30 ++----------------------------
 drivers/infiniband/core/cm_msgs.h | 30 ++----------------------------
 drivers/infiniband/core/cma.c     | 31 ++-----------------------------
 include/rdma/ib_cm.h              | 30 ++----------------------------
 4 files changed, 8 insertions(+), 113 deletions(-)

drivers/infiniband/core/cm.c +2 ~~-28~~

index bc4abb05dbd4..7ffa16ea5fe3 100644
@@ -1,36 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
 /*
  * Copyright (c) 2004-2007 Intel Corporation.  All rights reserved.
  * Copyright (c) 2004 Topspin Corporation.  All rights reserved.
  * Copyright (c) 2004, 2005 Voltaire Corporation.  All rights reserved.
  * Copyright (c) 2005 Sun Microsystems, Inc. All rights reserved.
- *
- * This software is available to you under a choice of one of two
- * licenses.  You may choose to be licensed under the terms of the GNU
- * General Public License (GPL) Version 2, available from the file
- * COPYING in the main directory of this source tree, or the
- * OpenIB.org BSD license below:
- *
- *     Redistribution and use in source and binary forms, with or
- *     without modification, are permitted provided that the following
- *     conditions are met:
- *
- *      - Redistributions of source code must retain the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer.
- *
- *      - Redistributions in binary form must reproduce the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer in the documentation and/or other materials
- *        provided with the distribution.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
+ * Copyright (c) 2019, Mellanox Technologies inc.  All rights reserved.
  */
 
 #include <linux/completion.h>

drivers/infiniband/core/cm_msgs.h +2 ~~-28~~

index 0a9e2d3fb9df..92d7260ac913 100644
@@ -1,35 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
 /*
  * Copyright (c) 2004, 2011 Intel Corporation.  All rights reserved.
  * Copyright (c) 2004 Topspin Corporation.  All rights reserved.
  * Copyright (c) 2004 Voltaire Corporation.  All rights reserved.
- *
- * This software is available to you under a choice of one of two
- * licenses.  You may choose to be licensed under the terms of the GNU
- * General Public License (GPL) Version 2, available from the file
- * COPYING the madirectory of this source tree, or the
- * OpenIB.org BSD license below:
- *
- *     Redistribution and use source and binary forms, with or
- *     withmodification, are permitted provided that the following
- *     conditions are met:
- *
- *      - Redistributions of source code must retathe above
- *        copyright notice, this list of conditions and the following
- *        disclaimer.
- *
- *      - Redistributions binary form must reproduce the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer the documentation and/or other materials
- *        provided with the distribution.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHWARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS THE
- * SOFTWARE.
+ * Copyright (c) 2019, Mellanox Technologies inc.  All rights reserved.
  */
 #ifndef CM_MSGS_H
 #define CM_MSGS_H

drivers/infiniband/core/cma.c +2 ~~-29~~

index c8566a423719..8f318928f29d 100644
@@ -1,36 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
 /*
  * Copyright (c) 2005 Voltaire Inc.  All rights reserved.
  * Copyright (c) 2002-2005, Network Appliance, Inc. All rights reserved.
- * Copyright (c) 1999-2005, Mellanox Technologies, Inc. All rights reserved.
+ * Copyright (c) 1999-2019, Mellanox Technologies, Inc. All rights reserved.
  * Copyright (c) 2005-2006 Intel Corporation.  All rights reserved.
- *
- * This software is available to you under a choice of one of two
- * licenses.  You may choose to be licensed under the terms of the GNU
- * General Public License (GPL) Version 2, available from the file
- * COPYING in the main directory of this source tree, or the
- * OpenIB.org BSD license below:
- *
- *     Redistribution and use in source and binary forms, with or
- *     without modification, are permitted provided that the following
- *     conditions are met:
- *
- *      - Redistributions of source code must retain the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer.
- *
- *      - Redistributions in binary form must reproduce the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer in the documentation and/or other materials
- *        provided with the distribution.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
  */
 
 #include <linux/completion.h>

include/rdma/ib_cm.h +2 ~~-28~~

index 5dc4ec986527..b01a8a8d4de9 100644
@@ -1,36 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
 /*
  * Copyright (c) 2004, 2005 Intel Corporation.  All rights reserved.
  * Copyright (c) 2004 Topspin Corporation.  All rights reserved.
  * Copyright (c) 2004 Voltaire Corporation.  All rights reserved.
  * Copyright (c) 2005 Sun Microsystems, Inc. All rights reserved.
- *
- * This software is available to you under a choice of one of two
- * licenses.  You may choose to be licensed under the terms of the GNU
- * General Public License (GPL) Version 2, available from the file
- * COPYING in the main directory of this source tree, or the
- * OpenIB.org BSD license below:
- *
- *     Redistribution and use in source and binary forms, with or
- *     without modification, are permitted provided that the following
- *     conditions are met:
- *
- *      - Redistributions of source code must retain the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer.
- *
- *      - Redistributions in binary form must reproduce the above
- *        copyright notice, this list of conditions and the following
- *        disclaimer in the documentation and/or other materials
- *        provided with the distribution.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
- * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
- * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
- * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
- * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
- * SOFTWARE.
+ * Copyright (c) 2019, Mellanox Technologies inc.  All rights reserved.
  */
 #ifndef IB_CM_H
 #define IB_CM_H

Signature · 1 line

2.20.1

Leon Romanovsky

author

2019-10-20 07:16 UTC #4 lore raw

PATCH 4/6 RDMA/cm: Delete useless QPN masking

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

QPN is supplied by kernel users who controls and creates valid QPs, such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/infiniband/core/cm.c +1 -1

index 7ffa16ea5fe3..2eb8e1fab962 100644
@@ -2101,7 +2101,7 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id,
 	cm_id_priv->initiator_depth = param->initiator_depth;
 	cm_id_priv->responder_resources = param->responder_resources;
 	cm_id_priv->rq_psn = cm_rep_get_starting_psn(rep_msg);
-	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num & 0xFFFFFF);
+	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num);
 
 out:	spin_unlock_irqrestore(&cm_id_priv->lock, flags);
 	return ret;

Signature · 1 line

2.20.1

Or Gerlitz

2019-10-20 08:48 UTC #5 lore raw

Re: [PATCH rdma-next 4/6] RDMA/cm: Delete useless QPN masking

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
QPN is supplied by kernel users who controls and creates valid QPs,

AFAIK this can also arrive from user-space, agree?

Quoted 2 lines

such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.

Leon Romanovsky

author

2019-10-20 08:55 UTC #6 lore raw

Re: [PATCH rdma-next 4/6] RDMA/cm: Delete useless QPN masking

Quoted 7 lines · Or Gerlitz

Quoted 4 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
QPN is supplied by kernel users who controls and creates valid QPs,
AFAIK this can also arrive from user-space, agree?

Not in this path, anyway we are masking it again while writing to the spec struct,

Quoted 3 lines

Quoted 3 lines
such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.

Jason Gunthorpe

2019-10-28 12:52 UTC #7 lore raw

Re: [PATCH rdma-next 4/6] RDMA/cm: Delete useless QPN masking

In reply to Leon Romanovsky

Quoted 16 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
QPN is supplied by kernel users who controls and creates valid QPs, such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/cm.c +0 -0
index 7ffa16ea5fe3..2eb8e1fab962 100644

drivers/infiniband/core/cm.c int ib_send_cm_rep(struct ib_cm_id *cm_id, · line 2104

Quoted 5 lines

drivers/infiniband/core/cm.c +1 -1

@@ -2101,7 +2101,7 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id,
 	cm_id_priv->initiator_depth = param->initiator_depth;
 	cm_id_priv->responder_resources = param->responder_resources;
 	cm_id_priv->rq_psn = cm_rep_get_starting_psn(rep_msg);
-	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num & 0xFFFFFF);
+	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num);

It does seem like this value comes from userspace:

ucma_connect()
ucma_copy_conn_param()

    	dst->qp_num = src->qp_num
  rdma_connect(.., &dst)
	if (!id->qp) {
		id_priv->qp_num = conn_param->qp_num;

cma_accept_ib()

	rep.qp_num = id_priv->qp_num;

Maybe this needs to add some masking to ucma_copy_conn_param()?

Jason

Leon Romanovsky

author

2019-10-28 13:13 UTC #8 lore raw

Re: [PATCH rdma-next 4/6] RDMA/cm: Delete useless QPN masking

Quoted 32 lines · Jason Gunthorpe

Quoted 16 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
QPN is supplied by kernel users who controls and creates valid QPs, such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/cm.c +0 -0
index 7ffa16ea5fe3..2eb8e1fab962 100644
drivers/infiniband/core/cm.c int ib_send_cm_rep(struct ib_cm_id *cm_id, · line 2104
Quoted 5 lines
drivers/infiniband/core/cm.c +1 -1
@@ -2101,7 +2101,7 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id,
 	cm_id_priv->initiator_depth = param->initiator_depth;
 	cm_id_priv->responder_resources = param->responder_resources;
 	cm_id_priv->rq_psn = cm_rep_get_starting_psn(rep_msg);
-	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num & 0xFFFFFF);
+	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num);
It does seem like this value comes from userspace:
ucma_connect()
ucma_copy_conn_param()
    	dst->qp_num = src->qp_num
  rdma_connect(.., &dst)
	if (!id->qp) {
		id_priv->qp_num = conn_param->qp_num;

Quoted 6 lines

vs
cma_accept_ib()
	rep.qp_num = id_priv->qp_num;
Maybe this needs to add some masking to ucma_copy_conn_param()?

Thanks for the callstack, Or pointed it to me too, but I missed this flow. Let's create a pre-patch with QPN masking.

Thanks

Quoted 2 lines

Jason

Jason Gunthorpe

2019-10-28 13:44 UTC #9 lore raw

Re: [PATCH rdma-next 4/6] RDMA/cm: Delete useless QPN masking

Quoted 33 lines · Leon Romanovsky

Quoted 32 lines · Jason Gunthorpe
Quoted 14 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
QPN is supplied by kernel users who controls and creates valid QPs, such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/cm.c +1 -0
index 7ffa16ea5fe3..2eb8e1fab962 100644
+++ b/drivers/infiniband/core/cm.c
drivers/infiniband/core/cm.c int ib_send_cm_rep(struct ib_cm_id *cm_id, · line 2104
Quoted 5 lines
drivers/infiniband/core/cm.c +1 -1
@@ -2101,7 +2101,7 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id,
 	cm_id_priv->initiator_depth = param->initiator_depth;
 	cm_id_priv->responder_resources = param->responder_resources;
 	cm_id_priv->rq_psn = cm_rep_get_starting_psn(rep_msg);
-	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num & 0xFFFFFF);
+	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num);
It does seem like this value comes from userspace:
ucma_connect()
ucma_copy_conn_param()
    	dst->qp_num = src->qp_num
  rdma_connect(.., &dst)
	if (!id->qp) {
		id_priv->qp_num = conn_param->qp_num;
vs

Quoted 7 lines

Quoted 4 lines
cma_accept_ib()
	rep.qp_num = id_priv->qp_num;
Maybe this needs to add some masking to ucma_copy_conn_param()?
Thanks for the callstack, Or pointed it to me too, but I missed this flow. Let's create a pre-patch with QPN masking.

You'll need to check all the id_priv->qp_num users, I stopped when I found the above

Jason

Leon Romanovsky

author

2019-10-28 14:02 UTC #10 lore raw

Re: [PATCH rdma-next 4/6] RDMA/cm: Delete useless QPN masking

Quoted 37 lines · Jason Gunthorpe

Quoted 36 lines · Leon Romanovsky
Quoted 35 lines · Jason Gunthorpe
Quoted 14 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
QPN is supplied by kernel users who controls and creates valid QPs, such flow ensures that QPN is limited to 24bits and no need to mask already valid QPN.
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/cm.c +1 -0
index 7ffa16ea5fe3..2eb8e1fab962 100644
+++ b/drivers/infiniband/core/cm.c
drivers/infiniband/core/cm.c int ib_send_cm_rep(struct ib_cm_id *cm_id, · line 2104
Quoted 5 lines
drivers/infiniband/core/cm.c +1 -1
@@ -2101,7 +2101,7 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id,
 	cm_id_priv->initiator_depth = param->initiator_depth;
 	cm_id_priv->responder_resources = param->responder_resources;
 	cm_id_priv->rq_psn = cm_rep_get_starting_psn(rep_msg);
-	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num & 0xFFFFFF);
+	cm_id_priv->local_qpn = cpu_to_be32(param->qp_num);
It does seem like this value comes from userspace:
ucma_connect()
ucma_copy_conn_param()
    	dst->qp_num = src->qp_num
  rdma_connect(.., &dst)
	if (!id->qp) {
		id_priv->qp_num = conn_param->qp_num;
vs
cma_accept_ib()
	rep.qp_num = id_priv->qp_num;

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Maybe this needs to add some masking to ucma_copy_conn_param()?
Thanks for the callstack, Or pointed it to me too, but I missed this flow. Let's create a pre-patch with QPN masking.
You'll need to check all the id_priv->qp_num users, I stopped when I found the above

Initially, I wanted to add masking in rdma_connect(), but decided that it is not the cleanest approach, so I grepped to see rdma_conn_param users. I'll continue to grep.

Thanks

Quoted 2 lines

Jason

Leon Romanovsky

author

2019-10-20 07:16 UTC #11 lore raw

PATCH 5/6 RDMA/cm: Provide private data size to CM users

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Prepare code to removal IB_CM_*_PRIVATE_DATA_SIZE enum so we will store such size in adjacent to actual data.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cm.c | 11 +++++++++++
 include/rdma/ib_cm.h         |  1 +
 2 files changed, 12 insertions(+)

drivers/infiniband/core/cm.c +11 -0

index 2eb8e1fab962..ecd868954958 100644
@@ -1681,6 +1681,7 @@ static void cm_format_req_event(struct cm_work *work,
 	param->srq = cm_req_get_srq(req_msg);
 	param->ppath_sgid_attr = cm_id_priv->av.ah_attr.grh.sgid_attr;
 	work->cm_event.private_data = &req_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_REQ_PRIVATE_DATA_SIZE;
 }
 
 static void cm_process_work(struct cm_id_private *cm_id_priv,

@@ -2193,6 +2194,7 @@ static void cm_format_rep_event(struct cm_work *work, enum ib_qp_type qp_type)
 	param->rnr_retry_count = cm_rep_get_rnr_retry_count(rep_msg);
 	param->srq = cm_rep_get_srq(rep_msg);
 	work->cm_event.private_data = &rep_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_REP_PRIVATE_DATA_SIZE;
 }
 
 static void cm_dup_rep_handler(struct cm_work *work)

@@ -2395,6 +2397,7 @@ static int cm_rtu_handler(struct cm_work *work)
 		return -EINVAL;
 
 	work->cm_event.private_data = &rtu_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_RTU_PRIVATE_DATA_SIZE;
 
 	spin_lock_irq(&cm_id_priv->lock);
 	if (cm_id_priv->id.state != IB_CM_REP_SENT &&

@@ -2597,6 +2600,7 @@ static int cm_dreq_handler(struct cm_work *work)
 	}
 
 	work->cm_event.private_data = &dreq_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_DREQ_PRIVATE_DATA_SIZE;
 
 	spin_lock_irq(&cm_id_priv->lock);
 	if (cm_id_priv->local_qpn != cm_dreq_get_remote_qpn(dreq_msg))

@@ -2671,6 +2675,7 @@ static int cm_drep_handler(struct cm_work *work)
 		return -EINVAL;
 
 	work->cm_event.private_data = &drep_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_DREP_PRIVATE_DATA_SIZE;
 
 	spin_lock_irq(&cm_id_priv->lock);
 	if (cm_id_priv->id.state != IB_CM_DREQ_SENT &&

@@ -2770,6 +2775,7 @@ static void cm_format_rej_event(struct cm_work *work)
 	param->ari_length = cm_rej_get_reject_info_len(rej_msg);
 	param->reason = __be16_to_cpu(rej_msg->reason);
 	work->cm_event.private_data = &rej_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_REJ_PRIVATE_DATA_SIZE;
 }
 
 static struct cm_id_private * cm_acquire_rejected_id(struct cm_rej_msg *rej_msg)

@@ -2982,6 +2988,7 @@ static int cm_mra_handler(struct cm_work *work)
 		return -EINVAL;
 
 	work->cm_event.private_data = &mra_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_MRA_PRIVATE_DATA_SIZE;
 	work->cm_event.param.mra_rcvd.service_timeout =
 					cm_mra_get_service_timeout(mra_msg);
 	timeout = cm_convert_to_ms(cm_mra_get_service_timeout(mra_msg)) +

@@ -3214,6 +3221,7 @@ static int cm_lap_handler(struct cm_work *work)
 	param->alternate_path = &work->path[0];
 	cm_format_path_from_lap(cm_id_priv, param->alternate_path, lap_msg);
 	work->cm_event.private_data = &lap_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_LAP_PRIVATE_DATA_SIZE;
 
 	spin_lock_irq(&cm_id_priv->lock);
 	if (cm_id_priv->id.state != IB_CM_ESTABLISHED)

@@ -3367,6 +3375,7 @@ static int cm_apr_handler(struct cm_work *work)
 	work->cm_event.param.apr_rcvd.apr_info = &apr_msg->info;
 	work->cm_event.param.apr_rcvd.info_len = apr_msg->info_length;
 	work->cm_event.private_data = &apr_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_APR_PRIVATE_DATA_SIZE;
 
 	spin_lock_irq(&cm_id_priv->lock);
 	if (cm_id_priv->id.state != IB_CM_ESTABLISHED ||

@@ -3515,6 +3524,7 @@ static void cm_format_sidr_req_event(struct cm_work *work,
 	param->port = work->port->port_num;
 	param->sgid_attr = rx_cm_id->av.ah_attr.grh.sgid_attr;
 	work->cm_event.private_data = &sidr_req_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE;
 }
 
 static int cm_sidr_req_handler(struct cm_work *work)

@@ -3664,6 +3674,7 @@ static void cm_format_sidr_rep_event(struct cm_work *work,
 	param->info_len = sidr_rep_msg->info_length;
 	param->sgid_attr = cm_id_priv->av.ah_attr.grh.sgid_attr;
 	work->cm_event.private_data = &sidr_rep_msg->private_data;
+	work->cm_event.private_data_len = IB_CM_SIDR_REP_PRIVATE_DATA_SIZE;
 }
 
 static int cm_sidr_rep_handler(struct cm_work *work)

include/rdma/ib_cm.h +1 -0

index b01a8a8d4de9..b476e0e27ec9 100644
@@ -254,6 +254,7 @@ struct ib_cm_event {
 	} param;
 
 	void			*private_data;
+	u8			private_data_len;
 };
 
 #define CM_REQ_ATTR_ID		cpu_to_be16(0x0010)

--
2.20.1

Leon Romanovsky

author

2019-10-20 07:16 UTC #12 lore raw

PATCH 6/6 RDMA/srpt: Use private_data_len instead of hardcoded value

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Reuse recently introduced private_data_len to get IBTA REJ message private data size.

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -1

index daf811abf40a..e66366de11e9 100644
@@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);
 		break;
 	case IB_CM_RTU_RECEIVED:
 	case IB_CM_USER_ESTABLISHED:

--
2.20.1

Bart Van Assche

maintainer Reviewed-by on v1 6/6

2019-10-24 17:01 UTC #13 lore raw

Re: [PATCH rdma-next 6/6] RDMA/srpt: Use private_data_len instead of hardcoded value

Quoted 15 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
Reuse recently introduced private_data_len to get IBTA REJ message private data size.
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +0 -0
index daf811abf40a..e66366de11e9 100644

drivers/infiniband/ulp/srpt/ib_srpt.c static int srpt_cm_handler(struct ib_cm_id *cm_id, · line 2615

Quoted 8 lines

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -1

@@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);
 		break;
 	case IB_CM_RTU_RECEIVED:
 	case IB_CM_USER_ESTABLISHED:

Reviewed-by Bart Van Assche <[email protected]>

Jason Gunthorpe

2019-10-28 13:13 UTC #14 lore raw

Re: [PATCH rdma-next 6/6] RDMA/srpt: Use private_data_len instead of hardcoded value

In reply to Leon Romanovsky

drivers/infiniband/ulp/srpt/ib_srpt.c static int srpt_cm_handler(struct ib_cm_id *cm_id, · line 2612

Quoted 10 lines · Leon Romanovsky

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -1

index daf811abf40a..e66366de11e9 100644
@@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);

So, I took a look and found a heck of a lot more places assuming the size of private data that really should be checked if we are going to introduce a buffer length here.

This is the first couple I noticed, but there were many many more and they all should be handled..

drivers/infiniband/ulp/srp/ib_srp.c +9 -1

@@ -2677,9 +2677,14 @@ static void srp_ib_cm_rej_handler(struct ib_cm_id *cm_id,
                break;
 
        case IB_CM_REJ_CONSUMER_DEFINED:
+               if (event->private_data_len < sizeof(struct srp_login_rej)) {
+                       ch->status = -ECONNRESET;
+                       break;
+               }
+
 
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c

@@ -985,12 +985,15 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id,
 {
        struct ipoib_cm_tx *p = cm_id->context;
        struct ipoib_dev_priv *priv = ipoib_priv(p->dev);
        struct ipoib_cm_data *data = event->private_data;
        struct sk_buff_head skqueue;
        struct ib_qp_attr qp_attr;
        int qp_attr_mask, ret;
        struct sk_buff *skb;
 
+       if (event->private_data_len < sizeof(*data))
+               return -EINVAL;
+

Thanks,
Jason

Leon Romanovsky

author

2019-10-28 13:19 UTC #15 lore raw

Re: [PATCH rdma-next 6/6] RDMA/srpt: Use private_data_len instead of hardcoded value

Quoted 15 lines · Jason Gunthorpe

drivers/infiniband/ulp/srpt/ib_srpt.c static int srpt_cm_handler(struct ib_cm_id *cm_id, · line 2612
Quoted 10 lines · Leon Romanovsky
drivers/infiniband/ulp/srpt/ib_srpt.c +1 -1
index daf811abf40a..e66366de11e9 100644
@@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);
So, I took a look and found a heck of a lot more places assuming the size of private data that really should be checked if we are going to introduce a buffer length here.

But we are not interested to make it dynamic, "private_data_len" has constant size according to IBTA spec, I just don't want users to be aware of this.

Why should we add the below checks if it wasn't before?

Quoted 35 lines

This is the first couple I noticed, but there were many many more and they all should be handled..

drivers/infiniband/ulp/srp/ib_srp.c +5 -0

@@ -2677,9 +2677,14 @@ static void srp_ib_cm_rej_handler(struct ib_cm_id *cm_id,
                break;
 
        case IB_CM_REJ_CONSUMER_DEFINED:
+               if (event->private_data_len < sizeof(struct srp_login_rej)) {
+                       ch->status = -ECONNRESET;
+                       break;
+               }
+

drivers/infiniband/ulp/ipoib/ipoib_cm.c +3 -0

@@ -985,12 +985,15 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id,
 {
        struct ipoib_cm_tx *p = cm_id->context;
        struct ipoib_dev_priv *priv = ipoib_priv(p->dev);
        struct ipoib_cm_data *data = event->private_data;
        struct sk_buff_head skqueue;
        struct ib_qp_attr qp_attr;
        int qp_attr_mask, ret;
        struct sk_buff *skb;
 
+       if (event->private_data_len < sizeof(*data))
+               return -EINVAL;
+
 
 
 Thanks,

Jason

Jason Gunthorpe

2019-10-28 13:43 UTC #16 lore raw

Re: [PATCH rdma-next 6/6] RDMA/srpt: Use private_data_len instead of hardcoded value

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Jason Gunthorpe
drivers/infiniband/ulp/srpt/ib_srpt.c static int srpt_cm_handler(struct ib_cm_id *cm_id, · line 2612
Quoted 9 lines · Leon Romanovsky
drivers/infiniband/ulp/srpt/ib_srpt.c +2 -1
index daf811abf40a..e66366de11e9 100644
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);
So, I took a look and found a heck of a lot more places assuming the size of private data that really should be checked if we are going to introduce a buffer length here.

Quoted 5 lines

But we are not interested to make it dynamic, "private_data_len" has constant size according to IBTA spec, I just don't want users to be aware of this.
Why should we add the below checks if it wasn't before?

Why are we doing any of this then?

Jason

Leon Romanovsky

author

2019-10-28 14:06 UTC #17 lore raw

Re: [PATCH rdma-next 6/6] RDMA/srpt: Use private_data_len instead of hardcoded value

Quoted 17 lines · Jason Gunthorpe

Quoted 16 lines · Leon Romanovsky
Quoted 14 lines · Jason Gunthorpe
drivers/infiniband/ulp/srpt/ib_srpt.c static int srpt_cm_handler(struct ib_cm_id *cm_id, · line 2612
Quoted 9 lines · Leon Romanovsky
drivers/infiniband/ulp/srpt/ib_srpt.c +2 -1
index daf811abf40a..e66366de11e9 100644
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2609,7 +2609,7 @@ static int srpt_cm_handler(struct ib_cm_id *cm_id,
 	case IB_CM_REJ_RECEIVED:
 		srpt_cm_rej_recv(ch, event->param.rej_rcvd.reason,
 				 event->private_data,
-				 IB_CM_REJ_PRIVATE_DATA_SIZE);
+				 event->private_data_len);
So, I took a look and found a heck of a lot more places assuming the size of private data that really should be checked if we are going to introduce a buffer length here.

Quoted 7 lines

Quoted 5 lines
But we are not interested to make it dynamic, "private_data_len" has constant size according to IBTA spec, I just don't want users to be aware of this.
Why should we add the below checks if it wasn't before?
Why are we doing any of this then?

The final goal will be to present all CM messages as binary blobs with access through specific GET/SET helpers, those including access to private data. It is needed to completely eliminate be32 madness in IB/core code.

Thanks

Quoted 2 lines

Jason

Jason Gunthorpe

Applied

2019-10-28 13:17 UTC #18 lore raw

In reply to Leon Romanovsky

Quoted 11 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>
Hi,
This is first part of CM cleanup series needed to effectively add IBTA Enhanced Connection Establishment (ECE) spec changes.
In this series, we don't do anything major, just small code cleanup with small change in srpt. This change will allow us to provide general getter and setter for all CM structures.

Quoted 6 lines

Thanks
Leon Romanovsky (6):
RDMA/cm: Delete unused cm_is_active_peer function RDMA/cm: Use specific keyword to check define RDMA/cm: Update copyright together with SPDX tag

Applied to for-next

Quoted 3 lines

RDMA/cm: Delete useless QPN masking
RDMA/cm: Provide private data size to CM users RDMA/srpt: Use private_data_len instead of hardcoded value

These ones probably need a bit more work

Thanks,
Jason

Leon Romanovsky

author

2019-10-28 13:20 UTC #19 lore raw

Quoted 12 lines · Jason Gunthorpe

Quoted 11 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Hi,
This is first part of CM cleanup series needed to effectively add IBTA Enhanced Connection Establishment (ECE) spec changes.
In this series, we don't do anything major, just small code cleanup with small change in srpt. This change will allow us to provide general getter and setter for all CM structures.

Quoted 8 lines

Quoted 6 lines
Thanks
Leon Romanovsky (6):
RDMA/cm: Delete unused cm_is_active_peer function RDMA/cm: Use specific keyword to check define RDMA/cm: Update copyright together with SPDX tag
Applied to for-next

Thanks

Quoted 6 lines

Quoted 4 lines
RDMA/cm: Delete useless QPN masking
RDMA/cm: Provide private data size to CM users RDMA/srpt: Use private_data_len instead of hardcoded value
These ones probably need a bit more work

I disagree about that last patch, we don't want to make private_data_len dynamic.

Thanks

Quoted 3 lines

Thanks,
Jason