Re: [PATCH rdma-rc] RDMA/cma: Allow UD qp_type to join multicast only

From: Jason Gunthorpe <jgg@nvidia.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: Mark Zhang <markzhang@nvidia.com>,
	linux-rdma@vger.kernel.org,
	syzbot+8fcbb77276d43cc8b693@syzkaller.appspotmail.com
Subject: Re: [PATCH rdma-rc] RDMA/cma: Allow UD qp_type to join multicast only
Date: Mon, 16 Jan 2023 13:30:23 -0400	[thread overview]
Message-ID: <Y8WJr3ETfPB9SOEy@nvidia.com> (raw)
In-Reply-To: <236616934e3b6485428671d482d131175f5c1cdd.1672821452.git.leonro@nvidia.com>

On Wed, Jan 04, 2023 at 10:38:09AM +0200, Leon Romanovsky wrote:
> From: Mark Zhang <markzhang@nvidia.com>
> 
> 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);
