On Wed, Jan 04, 2023 at 10:38:09AM +0200, Leon Romanovsky wrote:
> From: Mark Zhang <markzhang@xxxxxxxxxx>
>
> Only UD qp_type is allowed to join multicast.
>
> This patch also fixes an uninit-value error: the ib->rec.qkey field is
> accessed without being initialized. This is because multicast join was
> allowed for all port spaces, even these that omit qkey.
This commit message really needs to capture some of the lengthy
discussion that led to this.
> -static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> +static int cma_set_default_qkey(struct rdma_id_private *id_priv)
> {
> struct ib_sa_mcmember_rec rec;
> int ret = 0;
>
> - if (id_priv->qkey) {
> - if (qkey && id_priv->qkey != qkey)
> - return -EINVAL;
> - return 0;
> - }
> -
> - if (qkey) {
> - id_priv->qkey = qkey;
> - return 0;
> - }
> -
> switch (id_priv->id.ps) {
> case RDMA_PS_UDP:
> case RDMA_PS_IB:
> @@ -649,9 +638,20 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> default:
> break;
> }
> +
> return ret;
Unncessary hunk
> }
>
> +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey)
> +{
> + if (!qkey ||
> + (id_priv->qkey && (id_priv->qkey != qkey)))
> + return -EINVAL;
> +
> + id_priv->qkey = qkey;
> + return 0;
> +}
Ok
> +
> static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> {
> dev_addr->dev_type = ARPHRD_INFINIBAND;
> @@ -1222,7 +1222,7 @@ static int cma_ib_init_qp_attr(struct rdma_id_private *id_priv,
> *qp_attr_mask = IB_QP_STATE | IB_QP_PKEY_INDEX | IB_QP_PORT;
>
> if (id_priv->id.qp_type == IB_QPT_UD) {
> - ret = cma_set_qkey(id_priv, 0);
> + ret = cma_set_default_qkey(id_priv);
> if (ret)
> return ret;
OK
> @@ -4551,7 +4551,10 @@ static int cma_send_sidr_rep(struct rdma_id_private *id_priv,
> memset(&rep, 0, sizeof rep);
> rep.status = status;
> if (status == IB_SIDR_SUCCESS) {
> - ret = cma_set_qkey(id_priv, qkey);
> + if (qkey)
> + ret = cma_set_qkey(id_priv, qkey);
> + else
> + ret = cma_set_default_qkey(id_priv);
OK
> @@ -4859,9 +4862,11 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv,
> if (ret)
> return ret;
>
> - ret = cma_set_qkey(id_priv, 0);
> - if (ret)
> - return ret;
> + if (!id_priv->qkey) {
> + ret = cma_set_default_qkey(id_priv);
> + if (ret)
> + return ret;
> + }
Seems reasonable
> @@ -4938,8 +4943,8 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
> cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
>
> ib.rec.pkey = cpu_to_be16(0xffff);
> - if (id_priv->id.ps == RDMA_PS_UDP)
> - ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
> + ib.rec.qkey = id_priv->qkey ?
> + cpu_to_be32(id_priv->qkey) : cpu_to_be32(RDMA_UDP_QKEY);
This seems obtuse, why do we put it in the rec then call set_qkey
hidden in the cma_make_mc_event ?
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 68721ff10255e2..8fcf1473fe6031 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4756,9 +4756,7 @@ static void cma_make_mc_event(int status, struct rdma_id_private *id_priv,
enum ib_gid_type gid_type;
struct net_device *ndev;
- if (!status)
- status = cma_set_qkey(id_priv, be32_to_cpu(multicast->rec.qkey));
- else
+ if (status)
pr_debug_ratelimited("RDMA CM: MULTICAST_ERROR: failed to join multicast. status %d\n",
status);
@@ -4786,7 +4784,7 @@ static void cma_make_mc_event(int status, struct rdma_id_private *id_priv,
}
event->param.ud.qp_num = 0xFFFFFF;
- event->param.ud.qkey = be32_to_cpu(multicast->rec.qkey);
+ event->param.ud.qkey = id_priv->qkey;
out:
if (ndev)
@@ -4805,8 +4803,11 @@ static int cma_ib_mc_handler(int status, struct ib_sa_multicast *multicast)
READ_ONCE(id_priv->state) == RDMA_CM_DESTROYING)
goto out;
- cma_make_mc_event(status, id_priv, multicast, &event, mc);
- ret = cma_cm_event_handler(id_priv, &event);
+ ret = cma_set_qkey(id_priv, multicast->rec.qkey);
+ if (!ret) {
+ cma_make_mc_event(status, id_priv, multicast, &event, mc);
+ ret = cma_cm_event_handler(id_priv, &event);
+ }
rdma_destroy_ah_attr(&event.param.ud.ah_attr);
WARN_ON(ret);
@@ -4938,9 +4939,6 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
cma_iboe_set_mgid(addr, &ib.rec.mgid, gid_type);
ib.rec.pkey = cpu_to_be16(0xffff);
- if (id_priv->id.ps == RDMA_PS_UDP)
- ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
-
if (dev_addr->bound_dev_if)
ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);
if (!ndev)
@@ -4966,6 +4964,9 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
if (err || !ib.rec.mtu)
return err ?: -EINVAL;
+ if (!id_priv->qkey)
+ cma_set_default_qkey(id_priv);
+
rdma_ip2gid((struct sockaddr *)&id_priv->id.route.addr.src_addr,
&ib.rec.port_gid);
INIT_WORK(&mc->iboe_join.work, cma_iboe_join_work_handler);
