Re: [PATCH rdma-rc] RDMA/cma: Limit join multicast to UD QP type only

Jason Gunthorpe
2022-04-08T18:24:47Z

On Mon, Apr 04, 2022 at 05:52:18PM +0300, Leon Romanovsky wrote:
> -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);

This should be called in the couple of places that are actually
allowed to set a default qkey. We have some confusion about when that
is supposed to happen and when a 0 qkey can be presented.

But isn't this not the same? The original behavior was to make the
set_default a NOP if the id_priv already had a qkey:

- if (id_priv->qkey) {
- if (qkey && id_priv->qkey != qkey)

But that is gone now?

I got this:

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 3e315fc0ac16cb..ef980ea7153e51 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1102,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;

@@ -4430,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);
@@ -4685,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;

> static void cma_translate_ib(struct sockaddr_ib *sib, struct rdma_dev_addr *dev_addr)
> {
> dev_addr->dev_type = ARPHRD_INFINIBAND;
> @@ -4762,8 +4764,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);

Why isn't this symetrical with the IB side:

ret = cma_set_default_qkey(id_priv);
if (ret)
return ret;
rec.qkey = cpu_to_be32(id_priv->qkey);

??

It fells like set_default_qkey() is the right thing to do incase the
qkey was already set by something, just as IB does it.

> @@ -4815,6 +4816,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;
> +

This makes sense

Jason