[PATCH V1 for-next 0/4] IB core fixes 29-Jan-2015

[PATCH V1 for-next 0/4] IB core fixes 29-Jan-2015

[Or Gerlitz]

Sending V1 as the wrong instance was sent for patch #2 (Gerrit hardows)

Hi Roland,

This is a batch with IB core fixes, please apply for 3.20

The patch that deals with PSN touches the HW drivers b/c we added the 
qp attributes provided by the ULP as a param to the ib_modify_qp_is_ok() 
IB core helper.

Also, there's a pending CMA patch we sent for 3.19 and was approved
by Sean, please pick it too "RDMA/CMA: Mark IPv4 addresses correctly 
when the listener is IPv6" https://patchwork.kernel.org/patch/5298971/

Or.

Ilya Nelkenbaum (1):
  IB/core: When marshaling ucma path from user-space, clear unused fields

Majd Dibbiny (2):
  IB/core: Check mad_agent in ib_unregister_mad_agent before dereferencing it
  IB/core: Make sure that the PSN does not overflow

Moshe Lazer (1):
  IB/core: Fix deadlock on uverbs modify_qp error flow

 drivers/infiniband/core/cma.c               |    6 +++---
 drivers/infiniband/core/mad.c               |    4 ++++
 drivers/infiniband/core/ucma.c              |    3 +++
 drivers/infiniband/core/uverbs_cmd.c        |   13 +++++++------
 drivers/infiniband/core/verbs.c             |   10 ++++++++--
 drivers/infiniband/hw/ehca/ehca_qp.c        |    2 +-
 drivers/infiniband/hw/ipath/ipath_qp.c      |    2 +-
 drivers/infiniband/hw/mlx4/qp.c             |    6 +++---
 drivers/infiniband/hw/mlx5/qp.c             |    4 ++--
 drivers/infiniband/hw/mthca/mthca_qp.c      |    4 ++--
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.c |    7 ++++---
 drivers/infiniband/hw/qib/qib_qp.c          |    2 +-
 drivers/infiniband/ulp/ipoib/ipoib_cm.c     |    4 ++--
 include/rdma/ib_verbs.h                     |    7 ++++---
 14 files changed, 45 insertions(+), 29 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V1 for-next 1/4] IB/core: When marshaling ucma path from user-space, clear unused fields

[Or Gerlitz]

From: Ilya Nelkenbaum <ilyan-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

When marsheling a user path to the kernel struct ib_sa_path, need
to zero smac, dmac and set the vlan id to the "no vlan" value.

This is to ensure that Ethernet attributes are not used with
InfiniBand QPs.

Fixes: dd5f03beb4f7 ("IB/core: Ethernet L2 attributes in verbs/cm structures")
Signed-off-by: Ilya Nelkenbaum <ilyan-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---

FYI Sean, an easy way to reproduce the bug (and test the fix..) is have
ibacm running plus librdmacm instance which uses it (the default in
latest releases) and use rstream with gids, e.g

$ rstream -f gid -s fe80::f452:1403:1:da81

 drivers/infiniband/core/ucma.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 56a4b7c..45d67e9 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -1124,6 +1124,9 @@ static int ucma_set_ib_path(struct ucma_context *ctx,
 	if (!optlen)
 		return -EINVAL;
 
+	memset(&sa_path, 0, sizeof(sa_path));
+	sa_path.vlan_id = 0xffff;
+
 	ib_sa_unpack_path(path_data->path_rec, &sa_path);
 	ret = rdma_set_ib_paths(ctx->cm_id, &sa_path, 1);
 	if (ret)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V1 for-next 2/4] IB/core: Check mad_agent in ib_unregister_mad_agent before dereferencing it

[Or Gerlitz]

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

When ib_register_mad_agent fails, it returns a pointer with error which is not
NULL, therefore when calling ib_unregister_mad_agent we need to check that the
passed mad_agent is valid and not erroneous.

Signed-off-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/mad.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 74c30f4..8ec811e 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -619,6 +619,9 @@ int ib_unregister_mad_agent(struct ib_mad_agent *mad_agent)
 	struct ib_mad_agent_private *mad_agent_priv;
 	struct ib_mad_snoop_private *mad_snoop_priv;
 
+	if (IS_ERR(mad_agent))
+		goto out;
+
 	/* If the TID is zero, the agent can only snoop. */
 	if (mad_agent->hi_tid) {
 		mad_agent_priv = container_of(mad_agent,
@@ -631,6 +634,7 @@ int ib_unregister_mad_agent(struct ib_mad_agent *mad_agent)
 					      agent);
 		unregister_mad_snoop(mad_snoop_priv);
 	}
+out:
 	return 0;
 }
 EXPORT_SYMBOL(ib_unregister_mad_agent);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 for-next 2/4] IB/core: Check mad_agent in ib_unregister_mad_agent before dereferencing it

[Jason Gunthorpe]

On Thu, Jan 29, 2015 at 11:14:29AM +0200, Or Gerlitz wrote:
> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> When ib_register_mad_agent fails, it returns a pointer with error which is not
> NULL, therefore when calling ib_unregister_mad_agent we need to check that the
> passed mad_agent is valid and not erroneous.

This seems really weird, it is not idiomatic to randomly sprinkle
IS_ERR checks for function arguments.

The proper place for the IS_ERR check is directly after a
function that can return an ERR_PTR, then the caller shouldn't call
unregister,

Ie ib_sa_add_one already has this arrangement:

        for (i = 0; i <= e - s; ++i) {
                sa_dev->port[i].agent =
                        ib_register_mad_agent(device, i + s, IB_QPT_GSI,
                                              NULL, 0, send_handler,
                                              recv_handler, sa_dev);
                if (IS_ERR(sa_dev->port[i].agent))
                        goto err;
[..]

err:
        while (--i >= 0)
                if (rdma_port_get_link_layer(device, i + 1) == IB_LINK_LAYER_INFINIBAND)
                        ib_unregister_mad_agent(sa_dev->port[i].agent);


So it never calls ib_unregister_mad_agent with an ERR_PTR.

If there is a bug, then it is at a call site, not in the unregister.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 for-next 2/4] IB/core: Check mad_agent in ib_unregister_mad_agent before dereferencing it

[Or Gerlitz]

On 1/29/2015 7:42 PM, Jason Gunthorpe wrote:
> If there is a bug, then it is at a call site, not in the unregister.

Agree, will drop this patch and dig in our regression data-base to find 
the offending caller and fix it up, if it's not already fixed by now. 
Majd - looking into the mlx4 IB driver, I see calls to 
ib_unregister_mad_agent in the error flow of mlx4_ib_mad_init which 
looks suspicions...

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow

[Or Gerlitz]

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User
space applications shouldn't use the 8 most significant bits in the 32 bits
variables to avoid overflow in modify_qp.

Fixed the PSN usage for kernel ULPs and masked out the 8 most significant bits
for User-space applications.

>From now on, we check in ib_modify_qp_is_ok that the PSN doesn't overflow, and
we fail if so.

Signed-off-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cma.c               |    6 +++---
 drivers/infiniband/core/uverbs_cmd.c        |    4 ++--
 drivers/infiniband/core/verbs.c             |   10 ++++++++--
 drivers/infiniband/hw/ehca/ehca_qp.c        |    2 +-
 drivers/infiniband/hw/ipath/ipath_qp.c      |    2 +-
 drivers/infiniband/hw/mlx4/qp.c             |    6 +++---
 drivers/infiniband/hw/mlx5/qp.c             |    4 ++--
 drivers/infiniband/hw/mthca/mthca_qp.c      |    4 ++--
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.c |    7 ++++---
 drivers/infiniband/hw/qib/qib_qp.c          |    2 +-
 drivers/infiniband/ulp/ipoib/ipoib_cm.c     |    4 ++--
 include/rdma/ib_verbs.h                     |    7 ++++---
 12 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index d570030..26c203a 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -744,7 +744,7 @@ int rdma_init_qp_attr(struct rdma_cm_id *id, struct ib_qp_attr *qp_attr,
 						 qp_attr_mask);
 
 		if (qp_attr->qp_state == IB_QPS_RTR)
-			qp_attr->rq_psn = id_priv->seq_num;
+			qp_attr->rq_psn = id_priv->seq_num & 0xffffff;
 		break;
 	case RDMA_TRANSPORT_IWARP:
 		if (!id_priv->cm_id.iw) {
@@ -2807,7 +2807,7 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
 	req.service_id = rdma_get_service_id(&id_priv->id, cma_dst_addr(id_priv));
 	req.qp_num = id_priv->qp_num;
 	req.qp_type = id_priv->id.qp_type;
-	req.starting_psn = id_priv->seq_num;
+	req.starting_psn = id_priv->seq_num & 0xffffff;
 	req.responder_resources = conn_param->responder_resources;
 	req.initiator_depth = conn_param->initiator_depth;
 	req.flow_control = conn_param->flow_control;
@@ -2924,7 +2924,7 @@ static int cma_accept_ib(struct rdma_id_private *id_priv,
 
 	memset(&rep, 0, sizeof rep);
 	rep.qp_num = id_priv->qp_num;
-	rep.starting_psn = id_priv->seq_num;
+	rep.starting_psn = id_priv->seq_num & 0xffffff;
 	rep.private_data = conn_param->private_data;
 	rep.private_data_len = conn_param->private_data_len;
 	rep.responder_resources = conn_param->responder_resources;
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 532d8eb..ecb6430 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2053,8 +2053,8 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
 	attr->path_mtu 		  = cmd.path_mtu;
 	attr->path_mig_state 	  = cmd.path_mig_state;
 	attr->qkey 		  = cmd.qkey;
-	attr->rq_psn 		  = cmd.rq_psn;
-	attr->sq_psn 		  = cmd.sq_psn;
+	attr->rq_psn              = cmd.rq_psn & 0xffffff;
+	attr->sq_psn              = cmd.sq_psn & 0xffffff;
 	attr->dest_qp_num 	  = cmd.dest_qp_num;
 	attr->qp_access_flags 	  = cmd.qp_access_flags;
 	attr->pkey_index 	  = cmd.pkey_index;
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index f93eb8d..2cad161 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -827,8 +827,8 @@ static const struct {
 };
 
 int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
-		       enum ib_qp_type type, enum ib_qp_attr_mask mask,
-		       enum rdma_link_layer ll)
+		       enum ib_qp_type type, struct ib_qp_attr *attr,
+		       enum ib_qp_attr_mask mask, enum rdma_link_layer ll)
 {
 	enum ib_qp_attr_mask req_param, opt_param;
 
@@ -860,6 +860,12 @@ int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
 	if (mask & ~(req_param | opt_param | IB_QP_STATE))
 		return 0;
 
+	if ((mask & IB_QP_SQ_PSN) && (attr->sq_psn & 0xff000000))
+		return 0;
+
+	if ((mask & IB_QP_RQ_PSN) && (attr->rq_psn & 0xff000000))
+		return 0;
+
 	return 1;
 }
 EXPORT_SYMBOL(ib_modify_qp_is_ok);
diff --git a/drivers/infiniband/hw/ehca/ehca_qp.c b/drivers/infiniband/hw/ehca/ehca_qp.c
index 2e89356..c773cc2 100644
--- a/drivers/infiniband/hw/ehca/ehca_qp.c
+++ b/drivers/infiniband/hw/ehca/ehca_qp.c
@@ -1329,7 +1329,7 @@ static int internal_modify_qp(struct ib_qp *ibqp,
 	qp_new_state = attr_mask & IB_QP_STATE ? attr->qp_state : qp_cur_state;
 	if (!smi_reset2init &&
 	    !ib_modify_qp_is_ok(qp_cur_state, qp_new_state, ibqp->qp_type,
-				attr_mask, IB_LINK_LAYER_UNSPECIFIED)) {
+				attr, attr_mask, IB_LINK_LAYER_UNSPECIFIED)) {
 		ret = -EINVAL;
 		ehca_err(ibqp->device,
 			 "Invalid qp transition new_state=%x cur_state=%x "
diff --git a/drivers/infiniband/hw/ipath/ipath_qp.c b/drivers/infiniband/hw/ipath/ipath_qp.c
index face876..a3a3be7 100644
--- a/drivers/infiniband/hw/ipath/ipath_qp.c
+++ b/drivers/infiniband/hw/ipath/ipath_qp.c
@@ -462,7 +462,7 @@ int ipath_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 		attr->cur_qp_state : qp->state;
 	new_state = attr_mask & IB_QP_STATE ? attr->qp_state : cur_state;
 
-	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type,
+	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type, attr,
 				attr_mask, IB_LINK_LAYER_UNSPECIFIED))
 		goto inval;
 
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index cc37e01..96c607c 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -1898,9 +1898,9 @@ int mlx4_ib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 	}
 
 	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type,
-				attr_mask, ll)) {
-		pr_debug("qpn 0x%x: invalid attribute mask specified "
-			 "for transition %d to %d. qp_type %d,"
+				attr, attr_mask, ll)) {
+		pr_debug("qpn 0x%x: invalid attribute mask or attributes "
+			 "specified for transition %d to %d. qp_type %d,"
 			 " attr_mask 0x%x\n",
 			 ibqp->qp_num, cur_state, new_state,
 			 ibqp->qp_type, attr_mask);
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index be0cd35..9a120a8 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -1790,8 +1790,8 @@ int mlx5_ib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 	new_state = attr_mask & IB_QP_STATE ? attr->qp_state : cur_state;
 
 	if (ibqp->qp_type != MLX5_IB_QPT_REG_UMR &&
-	    !ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type, attr_mask,
-				IB_LINK_LAYER_UNSPECIFIED))
+	    !ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type, attr,
+				attr_mask, IB_LINK_LAYER_UNSPECIFIED))
 		goto out;
 
 	if ((attr_mask & IB_QP_PORT) &&
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index e354b2f..7822f64 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -860,8 +860,8 @@ int mthca_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, int attr_mask,
 
 	new_state = attr_mask & IB_QP_STATE ? attr->qp_state : cur_state;
 
-	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type, attr_mask,
-				IB_LINK_LAYER_UNSPECIFIED)) {
+	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type, attr,
+				attr_mask, IB_LINK_LAYER_UNSPECIFIED)) {
 		mthca_dbg(dev, "Bad QP transition (transport %d) "
 			  "%d->%d with attr 0x%08x\n",
 			  qp->transport, cur_state, new_state,
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
index fb8d8c4..b74dd07 100644
--- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
@@ -1348,9 +1348,10 @@ int ocrdma_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 		new_qps = old_qps;
 	spin_unlock_irqrestore(&qp->q_lock, flags);
 
-	if (!ib_modify_qp_is_ok(old_qps, new_qps, ibqp->qp_type, attr_mask,
-				IB_LINK_LAYER_ETHERNET)) {
-		pr_err("%s(%d) invalid attribute mask=0x%x specified for\n"
+	if (!ib_modify_qp_is_ok(old_qps, new_qps, ibqp->qp_type, attr,
+				attr_mask, IB_LINK_LAYER_ETHERNET)) {
+		pr_err("%s(%d) invalid attribute mask=0x%x or "
+		       "attributues specified for\n"
 		       "qpn=0x%x of type=0x%x old_qps=0x%x, new_qps=0x%x\n",
 		       __func__, dev->id, attr_mask, qp->id, ibqp->qp_type,
 		       old_qps, new_qps);
diff --git a/drivers/infiniband/hw/qib/qib_qp.c b/drivers/infiniband/hw/qib/qib_qp.c
index 6ddc026..3b4a176 100644
--- a/drivers/infiniband/hw/qib/qib_qp.c
+++ b/drivers/infiniband/hw/qib/qib_qp.c
@@ -584,7 +584,7 @@ int qib_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
 		attr->cur_qp_state : qp->state;
 	new_state = attr_mask & IB_QP_STATE ? attr->qp_state : cur_state;
 
-	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type,
+	if (!ib_modify_qp_is_ok(cur_state, new_state, ibqp->qp_type, attr,
 				attr_mask, IB_LINK_LAYER_UNSPECIFIED))
 		goto inval;
 
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 56959ad..bdf35b0 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -294,7 +294,7 @@ static int ipoib_cm_modify_rx_qp(struct net_device *dev,
 		ipoib_warn(priv, "failed to init QP attr for RTR: %d\n", ret);
 		return ret;
 	}
-	qp_attr.rq_psn = psn;
+	qp_attr.rq_psn = psn & 0xffffff;
 	ret = ib_modify_qp(qp, &qp_attr, qp_attr_mask);
 	if (ret) {
 		ipoib_warn(priv, "failed to modify QP to RTR: %d\n", ret);
@@ -433,7 +433,7 @@ static int ipoib_cm_send_rep(struct net_device *dev, struct ib_cm_id *cm_id,
 	rep.rnr_retry_count = req->rnr_retry_count;
 	rep.srq = ipoib_cm_has_srq(dev);
 	rep.qp_num = qp->qp_num;
-	rep.starting_psn = psn;
+	rep.starting_psn = psn & 0xffffff;
 	return ib_send_cm_rep(cm_id, &rep);
 }
 
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 0d74f1d..a8bb978 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1716,10 +1716,11 @@ static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len
 /**
  * ib_modify_qp_is_ok - Check that the supplied attribute mask
  * contains all required attributes and no attributes not allowed for
- * the given QP state transition.
+ * the given QP state transition, and checks the QP attributes.
  * @cur_state: Current QP state
  * @next_state: Next QP state
  * @type: QP type
+ * @attr: QP attributes
  * @mask: Mask of supplied QP attributes
  * @ll : link layer of port
  *
@@ -1730,8 +1731,8 @@ static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len
  * and that the attribute mask supplied is allowed for the transition.
  */
 int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
-		       enum ib_qp_type type, enum ib_qp_attr_mask mask,
-		       enum rdma_link_layer ll);
+		       enum ib_qp_type type, struct ib_qp_attr *attr,
+		       enum ib_qp_attr_mask mask, enum rdma_link_layer ll);
 
 int ib_register_event_handler  (struct ib_event_handler *event_handler);
 int ib_unregister_event_handler(struct ib_event_handler *event_handler);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow

[Jason Gunthorpe]

On Thu, Jan 29, 2015 at 11:14:30AM +0200, Or Gerlitz wrote:
> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User
> space applications shouldn't use the 8 most significant bits in the 32 bits
> variables to avoid overflow in modify_qp.
> 
> Fixed the PSN usage for kernel ULPs and masked out the 8 most significant bits
> for User-space applications.
> 
> From now on, we check in ib_modify_qp_is_ok that the PSN doesn't overflow, and
> we fail if so.

So, I guess the actual problem here is this:

        get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);

?

So what we need is

        get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
+       id_priv->seq_num &= 0xffffff;

>  		if (qp_attr->qp_state == IB_QPS_RTR)
> -			qp_attr->rq_psn = id_priv->seq_num;
> +			qp_attr->rq_psn = id_priv->seq_num & 0xffffff;

All this duplicitive masking should be centralized at the seq_num
assignment, not sprinkled

>  int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
> -		       enum ib_qp_type type, enum ib_qp_attr_mask mask,
> -		       enum rdma_link_layer ll)
> +		       enum ib_qp_type type, struct ib_qp_attr *attr,
> +		       enum ib_qp_attr_mask mask, enum rdma_link_layer ll)
>  {
>  	enum ib_qp_attr_mask req_param, opt_param;
>  
> @@ -860,6 +860,12 @@ int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
>  	if (mask & ~(req_param | opt_param | IB_QP_STATE))
>  		return 0;
>  
> +	if ((mask & IB_QP_SQ_PSN) && (attr->sq_psn & 0xff000000))
> +		return 0;
> +
> +	if ((mask & IB_QP_RQ_PSN) && (attr->rq_psn & 0xff000000))
> +		return 0;
> +

And since rdmacm has had this longstanding bug of generating > 24 bit
PSNs, this change seems really scary - very likely to break working
systems.

I think we'd be better off keeping the current behavior of masking
the PSN, not enforcing the limit.

> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> index 56959ad..bdf35b0 100644
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> @@ -294,7 +294,7 @@ static int ipoib_cm_modify_rx_qp(struct net_device *dev,
>  		ipoib_warn(priv, "failed to init QP attr for RTR: %d\n", ret);
>  		return ret;
>  	}
> -	qp_attr.rq_psn = psn;
> +	qp_attr.rq_psn = psn & 0xffffff;
>  	ret = ib_modify_qp(qp, &qp_attr, qp_attr_mask);

Please don't just sed/grep stuff without review. It was trivial for me
to find that this is unneeded:

        psn = prandom_u32() & 0xffffff;
        ret = ipoib_cm_modify_rx_qp(dev, cm_id, p->qp, psn);

> @@ -433,7 +433,7 @@ static int ipoib_cm_send_rep(struct net_device *dev, struct ib_cm_id *cm_id,
>  	rep.rnr_retry_count = req->rnr_retry_count;
>  	rep.srq = ipoib_cm_has_srq(dev);
>  	rep.qp_num = qp->qp_num;
> -	rep.starting_psn = psn;
> +	rep.starting_psn = psn & 0xffffff;

ditto:

        psn = prandom_u32() & 0xffffff;
[..]
        ret = ipoib_cm_send_rep(dev, cm_id, p->qp, &event->param.req_rcvd, psn);

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow

[Or Gerlitz]

On 1/29/2015 7:54 PM, Jason Gunthorpe wrote:
> On Thu, Jan 29, 2015 at 11:14:30AM +0200, Or Gerlitz wrote:
>> From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> The rq/sq->psn is 24 bits as defined in the IB spec, therefore ULPs and User
>> space applications shouldn't use the 8 most significant bits in the 32 bits
>> variables to avoid overflow in modify_qp.
>>
>> Fixed the PSN usage for kernel ULPs and masked out the 8 most significant bits
>> for User-space applications.
>>
>>  From now on, we check in ib_modify_qp_is_ok that the PSN doesn't overflow, and
>> we fail if so.
> So, I guess the actual problem here is this:
>
>          get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
>
> ?
>
> So what we need is
>
>          get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
> +       id_priv->seq_num &= 0xffffff;
>
>>   		if (qp_attr->qp_state == IB_QPS_RTR)
>> -			qp_attr->rq_psn = id_priv->seq_num;
>> +			qp_attr->rq_psn = id_priv->seq_num & 0xffffff;
> All this duplicitive masking should be centralized at the seq_num assignment, not sprinkled

sure, point taken, will fix.

>>   int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
>> -		       enum ib_qp_type type, enum ib_qp_attr_mask mask,
>> -		       enum rdma_link_layer ll)
>> +		       enum ib_qp_type type, struct ib_qp_attr *attr,
>> +		       enum ib_qp_attr_mask mask, enum rdma_link_layer ll)
>>   {
>>   	enum ib_qp_attr_mask req_param, opt_param;
>>   
>> @@ -860,6 +860,12 @@ int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
>>   	if (mask & ~(req_param | opt_param | IB_QP_STATE))
>>   		return 0;
>>   
>> +	if ((mask & IB_QP_SQ_PSN) && (attr->sq_psn & 0xff000000))
>> +		return 0;
>> +
>> +	if ((mask & IB_QP_RQ_PSN) && (attr->rq_psn & 0xff000000))
>> +		return 0;
>> +
> And since rdmacm has had this longstanding bug of generating > 24 bit PSNs, this change seems really scary - very likely to break working systems.

By IBTA the HW can only use 24 bits, also the IB CM also makes sure to 
only encode/decode 24 PSN bits to/from the wire (see the PSN related 
helpers in drivers/infiniband/core/cm_msgs.h), so in that respect, I 
don't see what other bits which are not 24 bits out of the 32 generated 
ones could be of some use to existing applications, please clarify.



>
> I think we'd be better off keeping the current behavior of masking the PSN, not enforcing the limit.
>
>> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
>> index 56959ad..bdf35b0 100644
>> +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
>> @@ -294,7 +294,7 @@ static int ipoib_cm_modify_rx_qp(struct net_device *dev,
>>   		ipoib_warn(priv, "failed to init QP attr for RTR: %d\n", ret);
>>   		return ret;
>>   	}
>> -	qp_attr.rq_psn = psn;
>> +	qp_attr.rq_psn = psn & 0xffffff;
>>   	ret = ib_modify_qp(qp, &qp_attr, qp_attr_mask);
> Please don't just sed/grep stuff without review. It was trivial for me
> to find that this is unneeded:
>
>          psn = prandom_u32() & 0xffffff;
>          ret = ipoib_cm_modify_rx_qp(dev, cm_id, p->qp, psn);

right,  will skip ipoib in this patch since it works just fine.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow

[Jason Gunthorpe]

On Sun, Feb 01, 2015 at 01:33:30PM +0200, Or Gerlitz wrote:

> >>@@ -860,6 +860,12 @@ int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
> >>  	if (mask & ~(req_param | opt_param | IB_QP_STATE))
> >>  		return 0;
> >>+	if ((mask & IB_QP_SQ_PSN) && (attr->sq_psn & 0xff000000))
> >>+		return 0;
> >>+
> >>+	if ((mask & IB_QP_RQ_PSN) && (attr->rq_psn & 0xff000000))
> >>+		return 0;
> >>+

> >And since rdmacm has had this longstanding bug of generating > 24
> >bit PSNs, this change seems really scary - very likely to break
> >working systems.
 
> By IBTA the HW can only use 24 bits, also the IB CM also makes sure
> to only encode/decode 24 PSN bits to/from the wire (see the PSN
> related helpers in drivers/infiniband/core/cm_msgs.h), so in that
> respect, I don't see what other bits which are not 24 bits out of
> the 32 generated ones could be of some use to existing applications,
> please clarify.

Maybe you can explain why this check is suddenly important now? It
seems risky with no rational?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V1 for-next 3/4] IB/core: Make sure that the PSN does not overflow

[Hefty, Sean]

> > >>@@ -860,6 +860,12 @@ int ib_modify_qp_is_ok(enum ib_qp_state
> cur_state, enum ib_qp_state next_state,
> > >>  	if (mask & ~(req_param | opt_param | IB_QP_STATE))
> > >>  		return 0;
> > >>+	if ((mask & IB_QP_SQ_PSN) && (attr->sq_psn & 0xff000000))
> > >>+		return 0;
> > >>+
> > >>+	if ((mask & IB_QP_RQ_PSN) && (attr->rq_psn & 0xff000000))
> > >>+		return 0;
> > >>+
> 
> > >And since rdmacm has had this longstanding bug of generating > 24
> > >bit PSNs, this change seems really scary - very likely to break
> > >working systems.
> 
> > By IBTA the HW can only use 24 bits, also the IB CM also makes sure
> > to only encode/decode 24 PSN bits to/from the wire (see the PSN
> > related helpers in drivers/infiniband/core/cm_msgs.h), so in that
> > respect, I don't see what other bits which are not 24 bits out of
> > the 32 generated ones could be of some use to existing applications,
> > please clarify.
> 
> Maybe you can explain why this check is suddenly important now? It
> seems risky with no rational?

To add to this, there's a difference between the code ignoring the upper 8-bits, versus mandating that they be 0.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V1 for-next 4/4] IB/core: Fix deadlock on uverbs modify_qp error flow

[Or Gerlitz]

From: Moshe Lazer <moshel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The deadlock occurs on __uverbs_modify_qp, we take a lock (idr_read_qp)
and in case of failure in ib_resolve_eth_l2_attrs we don't release
it (put_qp_read). Fix that.

Issue: 355606
Fixes: ed4c54e5b4ba ("IB/core: Resolve Ethernet L2 addresses when modifying QP")
Signed-off-by: Moshe Lazer <moshel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index ecb6430..a602ce9 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2097,20 +2097,21 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
 	if (qp->real_qp == qp) {
 		ret = ib_resolve_eth_l2_attrs(qp, attr, &cmd.attr_mask);
 		if (ret)
-			goto out;
+			goto release_qp;
 		ret = qp->device->modify_qp(qp, attr,
 			modify_qp_mask(qp->qp_type, cmd.attr_mask), &udata);
 	} else {
 		ret = ib_modify_qp(qp, attr, modify_qp_mask(qp->qp_type, cmd.attr_mask));
 	}
 
-	put_qp_read(qp);
-
 	if (ret)
-		goto out;
+		goto release_qp;
 
 	ret = in_len;
 
+release_qp:
+	put_qp_read(qp);
+
 out:
 	kfree(attr);
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html