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

[Jason Gunthorpe <jgg@xxxxxxxxxx>]

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