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)
