[PATCH rdma-next 0/6] CM cleanup

[PATCH rdma-next 0/6] CM cleanup

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

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

[PATCH rdma-next 1/6] RDMA/cm: Delete unused cm_is_active_peer function

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

Function cm_is_active_peer is not used, delete it.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index c0aa3a4b4cfd..bc4abb05dbd4 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -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) ||
-- 
2.20.1

[PATCH rdma-next 2/6] RDMA/cm: Use specific keyword to check define

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

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 <leonro@mellanox.com>
---
 drivers/infiniband/core/cm_msgs.h | 2 +-
 include/rdma/ib_cm.h              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/cm_msgs.h b/drivers/infiniband/core/cm_msgs.h
index 3d16d614aff6..0a9e2d3fb9df 100644
--- a/drivers/infiniband/core/cm_msgs.h
+++ b/drivers/infiniband/core/cm_msgs.h
@@ -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>
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 49f4f75499b3..5dc4ec986527 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -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>
-- 
2.20.1

[PATCH rdma-next 3/6] RDMA/cm: Update copyright together with SPDX tag

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

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

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 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(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index bc4abb05dbd4..7ffa16ea5fe3 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -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>
diff --git a/drivers/infiniband/core/cm_msgs.h b/drivers/infiniband/core/cm_msgs.h
index 0a9e2d3fb9df..92d7260ac913 100644
--- a/drivers/infiniband/core/cm_msgs.h
+++ b/drivers/infiniband/core/cm_msgs.h
@@ -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
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index c8566a423719..8f318928f29d 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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>
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 5dc4ec986527..b01a8a8d4de9 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -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
-- 
2.20.1

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

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

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 <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 7ffa16ea5fe3..2eb8e1fab962 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -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;
-- 
2.20.1

[PATCH rdma-next 5/6] RDMA/cm: Provide private data size to CM users

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

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 <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 11 +++++++++++
 include/rdma/ib_cm.h         |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 2eb8e1fab962..ecd868954958 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -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)
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index b01a8a8d4de9..b476e0e27ec9 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -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

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

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

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

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index daf811abf40a..e66366de11e9 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ 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);
 		break;
 	case IB_CM_RTU_RECEIVED:
 	case IB_CM_USER_ESTABLISHED:
--
2.20.1

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

[Or Gerlitz]

On Sun, Oct 20, 2019 at 10:18 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@mellanox.com>
>
> QPN is supplied by kernel users who controls and creates valid QPs,

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

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

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

[Leon Romanovsky]

On Sun, Oct 20, 2019 at 11:48:39AM +0300, Or Gerlitz wrote:
> On Sun, Oct 20, 2019 at 10:18 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > 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,

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

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

[Bart Van Assche]

On 10/20/19 12:15 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Reuse recently introduced private_data_len to get IBTA REJ message
> private data size.
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index daf811abf40a..e66366de11e9 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ 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);
>  		break;
>  	case IB_CM_RTU_RECEIVED:
>  	case IB_CM_USER_ESTABLISHED:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

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

[Jason Gunthorpe]

On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> 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 <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/cm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index 7ffa16ea5fe3..2eb8e1fab962 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -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;

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

Jason

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

[Jason Gunthorpe]

On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index daf811abf40a..e66366de11e9 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ 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.

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

--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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

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

[Leon Romanovsky]

On Mon, Oct 28, 2019 at 09:52:33AM -0300, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > 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 <leonro@mellanox.com>
> > ---
> >  drivers/infiniband/core/cm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index 7ffa16ea5fe3..2eb8e1fab962 100644
> > --- a/drivers/infiniband/core/cm.c
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -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;
>
> 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

>
> Jason

Re: [PATCH rdma-next 0/6] CM cleanup

[Jason Gunthorpe]

On Sun, Oct 20, 2019 at 10:15:53AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> 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

Applied to for-next

>   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

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

[Leon Romanovsky]

On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index daf811abf40a..e66366de11e9 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ 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.

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?

>
> This is the first couple I noticed, but there were many many more and
> they all should be handled..
>
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -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

Re: [PATCH rdma-next 0/6] CM cleanup

[Leon Romanovsky]

On Mon, Oct 28, 2019 at 10:17:25AM -0300, Jason Gunthorpe wrote:
> On Sun, Oct 20, 2019 at 10:15:53AM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > 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
>
> Applied to for-next

Thanks

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

>
> Thanks,
> Jason

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

[Jason Gunthorpe]

On Mon, Oct 28, 2019 at 03:19:17PM +0200, Leon Romanovsky wrote:
> On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> > On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > 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.
> 
> 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

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

[Jason Gunthorpe]

On Mon, Oct 28, 2019 at 03:13:33PM +0200, Leon Romanovsky wrote:
> On Mon, Oct 28, 2019 at 09:52:33AM -0300, Jason Gunthorpe wrote:
> > On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@mellanox.com>
> > >
> > > 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 <leonro@mellanox.com>
> > >  drivers/infiniband/core/cm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > > index 7ffa16ea5fe3..2eb8e1fab962 100644
> > > +++ b/drivers/infiniband/core/cm.c
> > > @@ -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;
> >
> > 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

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

[Leon Romanovsky]

On Mon, Oct 28, 2019 at 10:44:57AM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 28, 2019 at 03:13:33PM +0200, Leon Romanovsky wrote:
> > On Mon, Oct 28, 2019 at 09:52:33AM -0300, Jason Gunthorpe wrote:
> > > On Sun, Oct 20, 2019 at 10:15:57AM +0300, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@mellanox.com>
> > > >
> > > > 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 <leonro@mellanox.com>
> > > >  drivers/infiniband/core/cm.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > > > index 7ffa16ea5fe3..2eb8e1fab962 100644
> > > > +++ b/drivers/infiniband/core/cm.c
> > > > @@ -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;
> > >
> > > 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

>
> Jason

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

[Leon Romanovsky]

On Mon, Oct 28, 2019 at 10:43:51AM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 28, 2019 at 03:19:17PM +0200, Leon Romanovsky wrote:
> > On Mon, Oct 28, 2019 at 10:13:19AM -0300, Jason Gunthorpe wrote:
> > > On Sun, Oct 20, 2019 at 10:15:59AM +0300, Leon Romanovsky wrote:
> > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > 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.
> >
> > 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

>
> Jason