Re: [PATCH rdma-next] RDMA/core: Fix resolve_prepare_src error cleanup

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Dec 12, 2022 at 04:06:03PM +0200, Patrisious Haddad wrote:
> Btw there is the easy ugly fix obviously, which would be this patch +
> locking this function with the tree spin-lock(to avoid any race).
> 
> I'll check however if there is hope for a better possible design for this
> function.

The usual way I've fixed this is to avoid touching, in this case,
cma_dst_addr() in the call chain. eg we already pass in the correct
dst_addr

What you've done is made it so that in RDMA_CM_ROUTE_QUERY and beyond
the CM id's dst cannot change. The trick with this nasty code is that
it is trying to trigger auto-bind, and it has to do it blind because
of bad code structure

So, try something like this:

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 13e0ab785baa24..1d1f9cd01dd38f 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -3547,7 +3547,7 @@ static int cma_bind_addr(struct rdma_cm_id *id, struct sockaddr *src_addr,
 	struct sockaddr_storage zero_sock = {};
 
 	if (src_addr && src_addr->sa_family)
-		return rdma_bind_addr(id, src_addr);
+		return rdma_bind_addr_dst(id, src_addr, dst_addr);
 
 	/*
 	 * When the src_addr is not specified, automatically supply an any addr
@@ -3567,7 +3567,7 @@ static int cma_bind_addr(struct rdma_cm_id *id, struct sockaddr *src_addr,
 		((struct sockaddr_ib *)&zero_sock)->sib_pkey =
 			((struct sockaddr_ib *)dst_addr)->sib_pkey;
 	}
-	return rdma_bind_addr(id, (struct sockaddr *)&zero_sock);
+	return rdma_bind_addr_dst(id, (struct sockaddr *)&zero_sock, dst_addr);
 }
 
 /*
@@ -3582,17 +3582,14 @@ static int resolve_prepare_src(struct rdma_id_private *id_priv,
 {
 	int ret;
 
-	memcpy(cma_dst_addr(id_priv), dst_addr, rdma_addr_size(dst_addr));
 	if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_BOUND, RDMA_CM_ADDR_QUERY)) {
 		/* For a well behaved ULP state will be RDMA_CM_IDLE */
 		ret = cma_bind_addr(&id_priv->id, src_addr, dst_addr);
 		if (ret)
-			goto err_dst;
+			return ret;
 		if (WARN_ON(!cma_comp_exch(id_priv, RDMA_CM_ADDR_BOUND,
-					   RDMA_CM_ADDR_QUERY))) {
-			ret = -EINVAL;
-			goto err_dst;
-		}
+					   RDMA_CM_ADDR_QUERY)))
+			return -EINVAL;
 	}
 
 	if (cma_family(id_priv) != dst_addr->sa_family) {
@@ -3603,8 +3600,6 @@ static int resolve_prepare_src(struct rdma_id_private *id_priv,
 
 err_state:
 	cma_comp_exch(id_priv, RDMA_CM_ADDR_QUERY, RDMA_CM_ADDR_BOUND);
-err_dst:
-	memset(cma_dst_addr(id_priv), 0, rdma_addr_size(dst_addr));
 	return ret;
 }
 
@@ -4058,27 +4053,25 @@ int rdma_listen(struct rdma_cm_id *id, int backlog)
 }
 EXPORT_SYMBOL(rdma_listen);
 
-int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr)
+static int rdma_bind_addr_dst(struct rdma_id_private *id_priv,
+			      struct sockaddr *addr, struct sockaddr *daddr)
 {
-	struct rdma_id_private *id_priv;
 	int ret;
-	struct sockaddr  *daddr;
 
 	if (addr->sa_family != AF_INET && addr->sa_family != AF_INET6 &&
 	    addr->sa_family != AF_IB)
 		return -EAFNOSUPPORT;
 
-	id_priv = container_of(id, struct rdma_id_private, id);
 	if (!cma_comp_exch(id_priv, RDMA_CM_IDLE, RDMA_CM_ADDR_BOUND))
 		return -EINVAL;
 
-	ret = cma_check_linklocal(&id->route.addr.dev_addr, addr);
+	ret = cma_check_linklocal(&id_priv->id.route.addr.dev_addr, addr);
 	if (ret)
 		goto err1;
 
 	memcpy(cma_src_addr(id_priv), addr, rdma_addr_size(addr));
 	if (!cma_any_addr(addr)) {
-		ret = cma_translate_addr(addr, &id->route.addr.dev_addr);
+		ret = cma_translate_addr(addr, &id_priv->id.route.addr.dev_addr);
 		if (ret)
 			goto err1;
 
@@ -4098,8 +4091,14 @@ int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr)
 		}
 #endif
 	}
-	daddr = cma_dst_addr(id_priv);
+
+	/*
+	 * FIXME: This seems wrong, we can't just blidnly replace the sa_family
+	 * unless we know the daddr is zero. It will corrupt it.
+	 */
 	daddr->sa_family = addr->sa_family;
+	if (daddr != cma_dst_addr(id_priv))
+		memcpy(cma_dst_addr(id_priv), daddr, rdma_addr_size(addr));
 
 	ret = cma_get_port(id_priv);
 	if (ret)
@@ -4115,6 +4114,14 @@ int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr)
 	cma_comp_exch(id_priv, RDMA_CM_ADDR_BOUND, RDMA_CM_IDLE);
 	return ret;
 }
+
+int rdma_bind_addr(struct rdma_cm_id *id, struct sockaddr *addr)
+{
+	struct rdma_id_private *id_priv =
+		container_of(id, struct rdma_id_private, id);
+
+	return rdma_bind_addr_dst(id_priv, addr, cma_dst_addr(id_priv));
+}
 EXPORT_SYMBOL(rdma_bind_addr);
 
 static int cma_format_hdr(void *hdr, struct rdma_id_private *id_priv)