| Message ID | 98466723510491a832171031f591c77e5691979a.1650362467.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [rdma-rc,v1] RDMA/cma: Limit join multicast to UD QP type only | expand |
On Tue, Apr 19, 2022 at 01:03:21PM +0300, Leon Romanovsky wrote: > @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey) > default: > break; > } > + > return ret; > } > > +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey) > +{ > + if (!qkey) > + return cma_set_default_qkey(id_priv); The point was to get rid of this if since we don't need it once all the 0 qkey means default cases were split out. The remaining call sites: static int cma_sidr_rep_handler(struct ib_cm_id *cm_id, const struct ib_cm_event *ib_event) { ret = cma_set_qkey(id_priv, rep->qkey); 'rep' is CM_SIDR_REP_Q_KEY and 0 does not mean default in a MAD (0 is an error) static void cma_make_mc_event(int status, struct rdma_id_private *id_priv, struct ib_sa_multicast *multicast, struct rdma_cm_event *event, struct cma_multicast *mc) { status = cma_set_qkey(id_priv, be32_to_cpu(multicast->rec.qkey)); Comes from the SA in the IB case (zero is error) Is wired to ib.rec.qkey = cpu_to_be32(RDMA_UDP_QKEY); for roce case static int cma_send_sidr_rep(struct rdma_id_private *id_priv, enum ib_cm_sidr_status status, u32 qkey, const void *private_data, int private_data_len) { ret = cma_set_qkey(id_priv, qkey); Is rdma_conn_param::qkey, which is only ever set here: dst->qkey = (id->route.addr.src_addr.ss_family == AF_IB) ? src->qkey : 0; Which is the only other place that wants a default set, so I'd prefer to see the default set open coded here and the normal cma_set_qkey() return error for 0 qkey. > @@ -4683,7 +4681,7 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, > if (ret) > return ret; > > - ret = cma_set_qkey(id_priv, 0); > + ret = cma_set_default_qkey(id_priv); > if (ret) > return ret; I'm still not convinced this is right. I think the flow has the caller do a cma_send_sidr_rep() which will set a non-default qkey as above, and then do cma_join_ib_multicast which is supposed to follow the non-default qkey. So this should be conditional on not already having a set qkey. > @@ -4762,8 +4760,7 @@ 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 = cpu_to_be32(RDMA_UDP_QKEY); And the same logic should apply here, we can't just ignore the qkey that was set by cma_send_sidr_rep() and program in the UDP_QKEY to the QP, that would break it. (though that seems like another commit) Jason
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index fabca5e51e3d..ef980ea7153e 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -496,22 +496,11 @@ static inline unsigned short cma_family(struct rdma_id_private *id_priv) return id_priv->id.route.addr.src_addr.ss_family; } -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: @@ -528,9 +517,22 @@ static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey) default: break; } + return ret; } +static int cma_set_qkey(struct rdma_id_private *id_priv, u32 qkey) +{ + if (!qkey) + return cma_set_default_qkey(id_priv); + + if (id_priv->qkey && (id_priv->qkey != qkey)) + return -EINVAL; + + id_priv->qkey = qkey; + return 0; +} + static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr) { dev_addr->dev_type = ARPHRD_INFINIBAND; @@ -1100,7 +1102,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; @@ -4428,14 +4430,10 @@ int rdma_accept(struct rdma_cm_id *id, struct rdma_conn_param *conn_param) if (rdma_cap_ib_cm(id->device, id->port_num)) { if (id->qp_type == IB_QPT_UD) { - if (conn_param) - ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS, - conn_param->qkey, - conn_param->private_data, - conn_param->private_data_len); - else - ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS, - 0, NULL, 0); + ret = cma_send_sidr_rep(id_priv, IB_SIDR_SUCCESS, + conn_param->qkey, + conn_param->private_data, + conn_param->private_data_len); } else { if (conn_param) ret = cma_accept_ib(id_priv, conn_param); @@ -4683,7 +4681,7 @@ static int cma_join_ib_multicast(struct rdma_id_private *id_priv, if (ret) return ret; - ret = cma_set_qkey(id_priv, 0); + ret = cma_set_default_qkey(id_priv); if (ret) return ret; @@ -4762,8 +4760,7 @@ 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 = 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); @@ -4815,6 +4812,9 @@ int rdma_join_multicast(struct rdma_cm_id *id, struct sockaddr *addr, READ_ONCE(id_priv->state) != RDMA_CM_ADDR_RESOLVED)) return -EINVAL; + if (id_priv->id.qp_type != IB_QPT_UD) + return -EINVAL; + mc = kzalloc(sizeof(*mc), GFP_KERNEL); if (!mc) return -ENOMEM;