[PATCH V2 for-next 0/5] IB core fixes 29-Jan-2015

[PATCH V2 for-next 0/5] IB core fixes 29-Jan-2015

[Or Gerlitz]

Hi Roland,

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

There's one fix which touches few HW drivers to make sure
they invoke ib_unregister_mad_agent() only for valid cases
in their error flows.

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/

Majs and Or.

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

Majd Dibbiny (3):
  IB/core: Fixes in ib_sa_add_one error flow
  IB/core: Call ib_unregister_mad_agent() only for valid agents
  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           |    1 +
 drivers/infiniband/core/sa_query.c      |   10 +++++++---
 drivers/infiniband/core/ucma.c          |    3 +++
 drivers/infiniband/core/uverbs_cmd.c    |   13 +++++++------
 drivers/infiniband/hw/mlx4/mad.c        |    2 +-
 drivers/infiniband/hw/mthca/mthca_mad.c |    2 +-
 drivers/infiniband/hw/qib/qib_mad.c     |    2 +-
 7 files changed, 21 insertions(+), 12 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 V2 for-next 1/5] 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>
---
 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 V2 for-next 2/5] IB/core: Fixes in ib_sa_add_one error flow

[Or Gerlitz]

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

Fixed off-by-one bug, we need to decrement the port number only after we
released the resources to the current port.

Call ib_unregister_mad_agent only for cases where ib_register_mad_agent succeeded.

Separate the ib_register_event_handler() call error flow from the loop error
flow. If the call to ib_register_event_handler fails, the client data must be
reset to NULL, (in case at some point ib_register_event_handler() is modified so
that it may return a non-zero (error) value).

issue: none
Change-Id: Ia79a2d5037f674d67d75494c620b5c197da86314
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/sa_query.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index c38f030..80461af 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -1202,7 +1202,7 @@ static void ib_sa_add_one(struct ib_device *device)
 
 	INIT_IB_EVENT_HANDLER(&sa_dev->event_handler, device, ib_sa_event);
 	if (ib_register_event_handler(&sa_dev->event_handler))
-		goto err;
+		goto reg_err;
 
 	for (i = 0; i <= e - s; ++i)
 		if (rdma_port_get_link_layer(device, i + 1) == IB_LINK_LAYER_INFINIBAND)
@@ -1210,9 +1210,13 @@ static void ib_sa_add_one(struct ib_device *device)
 
 	return;
 
+reg_err:
+	ib_set_client_data(device, &sa_client, NULL);
+	i = e - s;
 err:
-	while (--i >= 0)
-		if (rdma_port_get_link_layer(device, i + 1) == IB_LINK_LAYER_INFINIBAND)
+	for (; i >= 0; --i)
+		if (rdma_port_get_link_layer(device, i + 1) == IB_LINK_LAYER_INFINIBAND &&
+		    !IS_ERR(sa_dev->port[i].agent))
 			ib_unregister_mad_agent(sa_dev->port[i].agent);
 
 	kfree(sa_dev);
-- 
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 V2 for-next 3/5] IB/core: Call ib_unregister_mad_agent() only for valid agents

[Or Gerlitz]

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

In some error flows, ib_mad_unregister_agent is being invoked also in cases where
the ib_mad_register_agent call failed (resulting in an illegal pointer in the
agent field).  This causes a kernel crash in the error flow.

Fix this by calling ib_unregister_mad_agent only for cases where
ib_register_mad_agent succeeded.

issue: 312895
Change-Id: Iaff4289ff3668e7ec91484f3d82a7d15f88678c8
Signed-off-by: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/mad.c        |    2 +-
 drivers/infiniband/hw/mthca/mthca_mad.c |    2 +-
 drivers/infiniband/hw/qib/qib_mad.c     |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
index 82a7dd8..6be0d2c 100644
--- a/drivers/infiniband/hw/mlx4/mad.c
+++ b/drivers/infiniband/hw/mlx4/mad.c
@@ -907,7 +907,7 @@ int mlx4_ib_mad_init(struct mlx4_ib_dev *dev)
 err:
 	for (p = 0; p < dev->num_ports; ++p)
 		for (q = 0; q <= 1; ++q)
-			if (dev->send_agent[p][q])
+			if (!IS_ERR(dev->send_agent[p][q]))
 				ib_unregister_mad_agent(dev->send_agent[p][q]);
 
 	return ret;
diff --git a/drivers/infiniband/hw/mthca/mthca_mad.c b/drivers/infiniband/hw/mthca/mthca_mad.c
index 8881fa3..5f1a7ce 100644
--- a/drivers/infiniband/hw/mthca/mthca_mad.c
+++ b/drivers/infiniband/hw/mthca/mthca_mad.c
@@ -317,7 +317,7 @@ int mthca_create_agents(struct mthca_dev *dev)
 err:
 	for (p = 0; p < dev->limits.num_ports; ++p)
 		for (q = 0; q <= 1; ++q)
-			if (dev->send_agent[p][q])
+			if (!IS_ERR(dev->send_agent[p][q]))
 				ib_unregister_mad_agent(dev->send_agent[p][q]);
 
 	return ret;
diff --git a/drivers/infiniband/hw/qib/qib_mad.c b/drivers/infiniband/hw/qib/qib_mad.c
index 636be11..6c7cc80 100644
--- a/drivers/infiniband/hw/qib/qib_mad.c
+++ b/drivers/infiniband/hw/qib/qib_mad.c
@@ -2499,7 +2499,7 @@ int qib_create_agents(struct qib_ibdev *dev)
 err:
 	for (p = 0; p < dd->num_pports; p++) {
 		ibp = &dd->pport[p].ibport_data;
-		if (ibp->send_agent) {
+		if (!IS_ERR(ibp->send_agent)) {
 			agent = ibp->send_agent;
 			ibp->send_agent = NULL;
 			ib_unregister_mad_agent(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

[PATCH V2 for-next 4/5] 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 generation by the RDMA-CM to mask out the 8 most significant bits,
also mask out these bits in uverbs for attributes provided by user-space.

issue: 416925
Change-Id: I7db9acc42564462de15bae84f4c512233fa8a62c
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        |    1 +
 drivers/infiniband/core/uverbs_cmd.c |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index d570030..fab0ee5 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -512,6 +512,7 @@ struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
 	INIT_LIST_HEAD(&id_priv->listen_list);
 	INIT_LIST_HEAD(&id_priv->mc_list);
 	get_random_bytes(&id_priv->seq_num, sizeof id_priv->seq_num);
+	id_priv->seq_num &= 0xffffff;
 
 	return &id_priv->id;
 }
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;
-- 
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 V2 for-next 5/5] 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

Re: [PATCH V2 for-next 3/5] IB/core: Call ib_unregister_mad_agent() only for valid agents

[Jason Gunthorpe]

On Thu, Feb 05, 2015 at 12:12:16PM +0200, Or Gerlitz wrote:

> diff --git a/drivers/infiniband/hw/mlx4/mad.c b/drivers/infiniband/hw/mlx4/mad.c
> index 82a7dd8..6be0d2c 100644
> +++ b/drivers/infiniband/hw/mlx4/mad.c
> @@ -907,7 +907,7 @@ int mlx4_ib_mad_init(struct mlx4_ib_dev *dev)
>  err:
>  	for (p = 0; p < dev->num_ports; ++p)
>  		for (q = 0; q <= 1; ++q)
> -			if (dev->send_agent[p][q])
> +			if (!IS_ERR(dev->send_agent[p][q]))
>  				ib_unregister_mad_agent(dev->send_agent[p][q]);

This doesn't really address my comment. I said the check for err ptr
only belongs after ib_register_mad_agent, I'm also not sure why these
few call sites were picked for this change ...

It didn't take long to see that mlx4 already does:

int mlx4_ib_mad_init(struct mlx4_ib_dev *dev)
{
	struct ib_mad_agent *agent;
	int p, q;
	int ret;
	enum rdma_link_layer ll;

	for (p = 0; p < dev->num_ports; ++p) {
		ll = rdma_port_get_link_layer(&dev->ib_dev, p + 1);
		for (q = 0; q <= 1; ++q) {
			if (ll == IB_LINK_LAYER_INFINIBAND) {
				agent = ib_register_mad_agent(&dev->ib_dev, p + 1,
							      q ? IB_QPT_GSI : IB_QPT_SMI,
							      NULL, 0, send_handler,
							      NULL, NULL);
				if (IS_ERR(agent)) {
					ret = PTR_ERR(agent);
					goto err;
				}
				dev->send_agent[p][q] = agent;
			} else
				dev->send_agent[p][q] = NULL;
		}
	}

I can't see how send_agent can ever contain err_ptr - and the above is
a good idiom.

I leave checking the rest up to you.

[The set to null is either redundent or there is another bug here
 because the err unwind blindly unregisters everything]

Again: err_ptr checks belong after ib_register_mad_agent - not at the
unregister, err_ptr values should not be put into long term storage in
the first place.

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