[PATCH rdma-next 0/3] Refactor rdma_cm query GID API

[PATCH rdma-next 0/3] Refactor rdma_cm query GID API

[Leon Romanovsky]

Hi,

The following patchset continues the previous refactoring series
targeted for rdma-next.

The goal of the folliwing patches is to simplify and provide consistient
interface to query GIDs from rdma_cm.

Thanks

Cc: Santosh Shilimkar <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Parav Pandit (3):
  RDMA/core: Remove RoCE support from rdma_addr_get_sgid()
  RDMA/cma: Introduce API to read GIDs for multiple transports
  RDMA: Use rdma_cm API to query GID

 drivers/infiniband/core/cma.c  | 18 ++++++++++++++++
 drivers/infiniband/core/ucma.c |  8 +++----
 include/rdma/ib_addr.h         | 49 +++++++++++++++++++-----------------------
 include/rdma/rdma_cm.h         | 19 ++++++++++++++++
 net/rds/ib.c                   |  6 ++----
 5 files changed, 65 insertions(+), 35 deletions(-)

--
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from rdma_addr_get_sgid()

[Leon Romanovsky]

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

There are number of issues with RoCE support in rdma_addr_get_sgid() call:

1. With IP based GIDs, SGID derived using iboe_addr_get_sgid() was
not used in connection establishment process. SGID was always derived
using ip2gid().

2. It returns the first IP address of the netdevice, while netdevice
can have multiple IP addresses and connection could have been established
using non first IP address.

3. It returns SGID based on IPv4 address. It doesn't cover IPv6 addresses.

4. As the comment of rdma_dev_addr src_dev_addr and dst_dev_addr suggest,
such members of rdma_dev_addr stores the mac address. Therefore it is not
the best place to store the RoCE GIDs. RoCE GIDs are derived from the IP
addresses and they are already available in cm_id->route->addr.***_addr.

It was only used to return SGID to rds/ib.c and ucma.c modules, where it
returned incorrect SGID in above matching conditions.

Therefore code is simplified to not support RoCE.

Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/rdma/ib_addr.h | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
index dc0b642e0175..fa809a7b48e7 100644
--- a/include/rdma/ib_addr.h
+++ b/include/rdma/ib_addr.h
@@ -197,34 +197,17 @@ static inline void rdma_gid2ip(struct sockaddr *out, const union ib_gid *gid)
 	}
 }
 
-static inline void iboe_addr_get_sgid(struct rdma_dev_addr *dev_addr,
+/*
+ * rdma_get/set_sgid/dgid() APIs are applicable to IB and iWARP.
+ * They are not applicable to RoCE, because RoCE GIDs are derived
+ * from the IP addresses.
+ */
+static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr,
 				      union ib_gid *gid)
 {
-	struct net_device *dev;
-	struct in_device *ip4;
-
-	dev = dev_get_by_index(&init_net, dev_addr->bound_dev_if);
-	if (dev) {
-		ip4 = in_dev_get(dev);
-		if (ip4 && ip4->ifa_list && ip4->ifa_list->ifa_address)
-			ipv6_addr_set_v4mapped(ip4->ifa_list->ifa_address,
-					       (struct in6_addr *)gid);
-
-		if (ip4)
-			in_dev_put(ip4);
-
-		dev_put(dev);
-	}
-}
-
-static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr, union ib_gid *gid)
-{
-	if (dev_addr->transport == RDMA_TRANSPORT_IB &&
-	    dev_addr->dev_type != ARPHRD_INFINIBAND)
-		iboe_addr_get_sgid(dev_addr, gid);
-	else
-		memcpy(gid, dev_addr->src_dev_addr +
-		       rdma_addr_gid_offset(dev_addr), sizeof *gid);
+	memcpy(gid,
+	       dev_addr->src_dev_addr + rdma_addr_gid_offset(dev_addr),
+	       sizeof(*gid));
 }
 
 static inline void rdma_addr_set_sgid(struct rdma_dev_addr *dev_addr, union ib_gid *gid)
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for multiple transports

[Leon Romanovsky]

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patch introduces an API that allows legacy applications to query
GIDs for a rdma_cm_id which is used during connection establishment.

GIDs are stored and created differently for IB, RoCE and iWARP transports.
Therefore rdma_read_gids() returns GID for all the transports hiding
such internal details to caller. It is usable for client side and server
side connections.

The rdma_read_gids() should not be used by any new ULPs.

Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 18 ++++++++++++++++++
 include/rdma/ib_addr.h        | 16 ++++++++++++++--
 include/rdma/rdma_cm.h        | 19 +++++++++++++++++++
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index f4c6c2cbc585..169d3a3bbf71 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -2037,6 +2037,24 @@ __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr)
 }
 EXPORT_SYMBOL(rdma_get_service_id);
 
+void rdma_read_gids(struct rdma_cm_id *cm_id, union ib_gid *sgid,
+		    union ib_gid *dgid)
+{
+	struct rdma_addr *addr = &cm_id->route.addr;
+
+	if (!cm_id->device)
+		return;
+
+	if (rdma_protocol_roce(cm_id->device, cm_id->port_num)) {
+		rdma_ip2gid((struct sockaddr *)&addr->src_addr, sgid);
+		rdma_ip2gid((struct sockaddr *)&addr->dst_addr, dgid);
+	} else {
+		rdma_addr_get_sgid(&addr->dev_addr, sgid);
+		rdma_addr_get_dgid(&addr->dev_addr, dgid);
+	}
+}
+EXPORT_SYMBOL(rdma_read_gids);
+
 static int cma_iw_handler(struct iw_cm_id *iw_id, struct iw_cm_event *iw_event)
 {
 	struct rdma_id_private *id_priv = iw_id->context;
diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
index fa809a7b48e7..edb1cdf2a892 100644
--- a/include/rdma/ib_addr.h
+++ b/include/rdma/ib_addr.h
@@ -165,6 +165,9 @@ static inline u16 rdma_vlan_dev_vlan_id(const struct net_device *dev)
 
 static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
 {
+	if (!gid)
+		return 0;
+
 	switch (addr->sa_family) {
 	case AF_INET:
 		ipv6_addr_set_v4mapped(((struct sockaddr_in *)
@@ -205,6 +208,9 @@ static inline void rdma_gid2ip(struct sockaddr *out, const union ib_gid *gid)
 static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr,
 				      union ib_gid *gid)
 {
+	if (!gid)
+		return;
+
 	memcpy(gid,
 	       dev_addr->src_dev_addr + rdma_addr_gid_offset(dev_addr),
 	       sizeof(*gid));
@@ -215,9 +221,15 @@ static inline void rdma_addr_set_sgid(struct rdma_dev_addr *dev_addr, union ib_g
 	memcpy(dev_addr->src_dev_addr + rdma_addr_gid_offset(dev_addr), gid, sizeof *gid);
 }
 
-static inline void rdma_addr_get_dgid(struct rdma_dev_addr *dev_addr, union ib_gid *gid)
+static inline void rdma_addr_get_dgid(struct rdma_dev_addr *dev_addr,
+				      union ib_gid *gid)
 {
-	memcpy(gid, dev_addr->dst_dev_addr + rdma_addr_gid_offset(dev_addr), sizeof *gid);
+	if (!gid)
+		return;
+
+	memcpy(gid,
+	       dev_addr->dst_dev_addr + rdma_addr_gid_offset(dev_addr),
+	       sizeof(*gid));
 }
 
 static inline void rdma_addr_set_dgid(struct rdma_dev_addr *dev_addr, union ib_gid *gid)
diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
index 3d2eed3c4e75..6538a5cc27b6 100644
--- a/include/rdma/rdma_cm.h
+++ b/include/rdma/rdma_cm.h
@@ -413,4 +413,23 @@ bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason);
 const void *rdma_consumer_reject_data(struct rdma_cm_id *id,
 				      struct rdma_cm_event *ev, u8 *data_len);
 
+/**
+ * rdma_read_gids - Return the SGID and DGID used for establishing
+ *                  connection. This can be used after rdma_resolve_addr()
+ *                  on client side. This can be use on new connection
+ *                  on server side. This is applicable to IB, RoCE, iWarp.
+ *                  If cm_id is not bound yet to the RDMA device, it doesn't
+ *                  copy and SGID or DGID to the given pointers.
+ * @id: Communication identifier whose GIDs are queried.
+ * @sgid: Pointer to SGID where SGID will be returned. It is optional.
+ * @dgid: Pointer to DGID where DGID will be returned. It is optional.
+ * Note: This API should not be used by any new ULPs or new code.
+ * Instead, users interested in querying GIDs should refer to path record
+ * of the rdma_cm_id to query the GIDs.
+ * This API is provided for compatibility for existing users.
+ */
+
+void rdma_read_gids(struct rdma_cm_id *cm_id, union ib_gid *sgid,
+		    union ib_gid *dgid);
+
 #endif /* RDMA_CM_H */
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 3/3] RDMA: Use rdma_cm API to query GID

[Leon Romanovsky]

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Make use of rdma_read_gids() function to read SGID and DGID which
returns correct GIDs for all transports.

Cc: Santosh Shilimkar <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/ucma.c | 8 ++++----
 net/rds/ib.c                   | 6 ++----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 29a81e6cfc96..8699b3a1d464 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -944,8 +944,8 @@ static ssize_t ucma_query_gid(struct ucma_context *ctx,
 	} else {
 		addr->sib_family = AF_IB;
 		addr->sib_pkey = (__force __be16) resp.pkey;
-		rdma_addr_get_sgid(&ctx->cm_id->route.addr.dev_addr,
-				   (union ib_gid *) &addr->sib_addr);
+		rdma_read_gids(ctx->cm_id, (union ib_gid *)&addr->sib_addr,
+			       NULL);
 		addr->sib_sid = rdma_get_service_id(ctx->cm_id, (struct sockaddr *)
 						    &ctx->cm_id->route.addr.src_addr);
 	}
@@ -957,8 +957,8 @@ static ssize_t ucma_query_gid(struct ucma_context *ctx,
 	} else {
 		addr->sib_family = AF_IB;
 		addr->sib_pkey = (__force __be16) resp.pkey;
-		rdma_addr_get_dgid(&ctx->cm_id->route.addr.dev_addr,
-				   (union ib_gid *) &addr->sib_addr);
+		rdma_read_gids(ctx->cm_id, NULL,
+			       (union ib_gid *)&addr->sib_addr);
 		addr->sib_sid = rdma_get_service_id(ctx->cm_id, (struct sockaddr *)
 						    &ctx->cm_id->route.addr.dst_addr);
 	}
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 36dd2099048a..b2a5067b4afe 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -301,13 +301,11 @@ static int rds_ib_conn_info_visitor(struct rds_connection *conn,
 	memset(&iinfo->dst_gid, 0, sizeof(iinfo->dst_gid));
 	if (rds_conn_state(conn) == RDS_CONN_UP) {
 		struct rds_ib_device *rds_ibdev;
-		struct rdma_dev_addr *dev_addr;
 
 		ic = conn->c_transport_data;
-		dev_addr = &ic->i_cm_id->route.addr.dev_addr;
 
-		rdma_addr_get_sgid(dev_addr, (union ib_gid *) &iinfo->src_gid);
-		rdma_addr_get_dgid(dev_addr, (union ib_gid *) &iinfo->dst_gid);
+		rdma_read_gids(ic->i_cm_id, (union ib_gid *)&iinfo->src_gid,
+			       (union ib_gid *)&iinfo->dst_gid);
 
 		rds_ibdev = ic->rds_ibdev;
 		iinfo->max_send_wr = ic->i_send_ring.w_nr;
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 3/3] RDMA: Use rdma_cm API to query GID

[Santosh Shilimkar]

On 1/9/2018 3:10 AM, Leon Romanovsky wrote:
> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Make use of rdma_read_gids() function to read SGID and DGID which
> returns correct GIDs for all transports.
> 
> Cc: Santosh Shilimkar <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>   drivers/infiniband/core/ucma.c | 8 ++++----
>   net/rds/ib.c                   | 6 ++----
>   2 files changed, 6 insertions(+), 8 deletions(-)

Acked-by: Santosh Shilimkar <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from rdma_addr_get_sgid()

[Jason Gunthorpe]

On Tue, Jan 09, 2018 at 01:10:56PM +0200, Leon Romanovsky wrote:
> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> There are number of issues with RoCE support in rdma_addr_get_sgid() call:
> 
> 1. With IP based GIDs, SGID derived using iboe_addr_get_sgid() was
> not used in connection establishment process. SGID was always derived
> using ip2gid().
> 
> 2. It returns the first IP address of the netdevice, while netdevice
> can have multiple IP addresses and connection could have been established
> using non first IP address.
> 
> 3. It returns SGID based on IPv4 address. It doesn't cover IPv6 addresses.
> 
> 4. As the comment of rdma_dev_addr src_dev_addr and dst_dev_addr suggest,
> such members of rdma_dev_addr stores the mac address. Therefore it is not
> the best place to store the RoCE GIDs. RoCE GIDs are derived from the IP
> addresses and they are already available in cm_id->route->addr.***_addr.
> 
> It was only used to return SGID to rds/ib.c and ucma.c modules, where it
> returned incorrect SGID in above matching conditions.

Yes, but if those conditions were not matched it did something useful
and now what happens?

All the callers in ucma.c are returning the result to userspace, so
this patch looks like it is changing the uapi in some way?

Can you define what the SGID/DGID should be in the rdma CM uapi when
working with rocee?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for multiple transports

[Jason Gunthorpe]

On Tue, Jan 09, 2018 at 01:10:57PM +0200, Leon Romanovsky wrote:
> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> This patch introduces an API that allows legacy applications to query
> GIDs for a rdma_cm_id which is used during connection establishment.
> 
> GIDs are stored and created differently for IB, RoCE and iWARP transports.
> Therefore rdma_read_gids() returns GID for all the transports hiding
> such internal details to caller. It is usable for client side and server
> side connections.
> 
> The rdma_read_gids() should not be used by any new ULPs.

This is weird statement. Why can't we change the few callers to do
whatever is correct now?

> +void rdma_read_gids(struct rdma_cm_id *cm_id, union ib_gid *sgid,
> +		    union ib_gid *dgid)
> +{
> +	struct rdma_addr *addr = &cm_id->route.addr;
> +
> +	if (!cm_id->device)
> +		return;

Nope. I looked at only one caller and saw this return leaks uninited
kernel stack memory to userspace. Probably best to zero the sgid/dgid
in the error case??

> diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
> index fa809a7b48e7..edb1cdf2a892 100644
> +++ b/include/rdma/ib_addr.h
> @@ -165,6 +165,9 @@ static inline u16 rdma_vlan_dev_vlan_id(const struct net_device *dev)
>  
>  static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
>  {
> +	if (!gid)
> +		return 0;
> +
>  	switch (addr->sa_family) {
>  	case AF_INET:
>  		ipv6_addr_set_v4mapped(((struct sockaddr_in *)
> @@ -205,6 +208,9 @@ static inline void rdma_gid2ip(struct sockaddr *out, const union ib_gid *gid)
>  static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr,
>  				      union ib_gid *gid)
>  {
> +	if (!gid)
> +		return;

I think it would be clearer to put this test in the rdma_read_gids.
It makes no sense to have a function with one output where the output
is optional. Particularly when it is inlined..

And the prior patch makes a bit more sense after looking here, but the
series seems in the wrong order -> the patch to break
rdma_addr_get_sgid should go after the only rocee code path in ucma is
switched to use rdma_read_gids which hopefully does something similar
to what it did before?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from rdma_addr_get_sgid()

[Parav Pandit]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgg-uk2M96/98Pc@public.gmane.org]
> Sent: Monday, January 15, 2018 4:08 PM
> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; RDMA mailing list <linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Parav Pandit
> <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from
> rdma_addr_get_sgid()
> 
> On Tue, Jan 09, 2018 at 01:10:56PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > There are number of issues with RoCE support in rdma_addr_get_sgid() call:
> >
> > 1. With IP based GIDs, SGID derived using iboe_addr_get_sgid() was not
> > used in connection establishment process. SGID was always derived
> > using ip2gid().
> >
> > 2. It returns the first IP address of the netdevice, while netdevice
> > can have multiple IP addresses and connection could have been
> > established using non first IP address.
> >
> > 3. It returns SGID based on IPv4 address. It doesn't cover IPv6 addresses.
> >
> > 4. As the comment of rdma_dev_addr src_dev_addr and dst_dev_addr
> > suggest, such members of rdma_dev_addr stores the mac address.
> > Therefore it is not the best place to store the RoCE GIDs. RoCE GIDs
> > are derived from the IP addresses and they are already available in cm_id-
> >route->addr.***_addr.
> >
> > It was only used to return SGID to rds/ib.c and ucma.c modules, where
> > it returned incorrect SGID in above matching conditions.
> 
> Yes, but if those conditions were not matched it did something useful and now
> what happens?
Now it returns correct SGID in matched and unmatched cases.

> 
> All the callers in ucma.c are returning the result to userspace, so this patch looks
> like it is changing the uapi in some way?
No. It doesn't change uapi. It fixes cases by returning appropriate GID.
> 
> Can you define what the SGID/DGID should be in the rdma CM uapi when
> working with rocee?
> 
It is the GIDs derived from the IP address using ip2gid() conversion.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for multiple transports

[Parav Pandit]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgg-uk2M96/98Pc@public.gmane.org]
> Sent: Monday, January 15, 2018 4:22 PM
> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; RDMA mailing list <linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>; Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Parav Pandit
> <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for
> multiple transports
> 
> On Tue, Jan 09, 2018 at 01:10:57PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > This patch introduces an API that allows legacy applications to query
> > GIDs for a rdma_cm_id which is used during connection establishment.
> >
> > GIDs are stored and created differently for IB, RoCE and iWARP transports.
> > Therefore rdma_read_gids() returns GID for all the transports hiding
> > such internal details to caller. It is usable for client side and
> > server side connections.
> >
> > The rdma_read_gids() should not be used by any new ULPs.
> 
> This is weird statement. Why can't we change the few callers to do whatever is
> correct now?
> 
Because rdmacm doesn't expose the GIDs. Rdmacm works on IP addresses.
It uses GIDs to internally do the binding and attachment to right ib device.
But GIDs are not UAPIs from rdmacm perspective.
Therefore it should not be exposed for any new ULPs/users.

> > +void rdma_read_gids(struct rdma_cm_id *cm_id, union ib_gid *sgid,
> > +		    union ib_gid *dgid)
> > +{
> > +	struct rdma_addr *addr = &cm_id->route.addr;
> > +
> > +	if (!cm_id->device)
> > +		return;
> 
> Nope. I looked at only one caller and saw this return leaks uninited kernel stack
> memory to userspace. Probably best to zero the sgid/dgid in the error case??
Ok. That can be done.

> 
> > diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h index
> > fa809a7b48e7..edb1cdf2a892 100644
> > +++ b/include/rdma/ib_addr.h
> > @@ -165,6 +165,9 @@ static inline u16 rdma_vlan_dev_vlan_id(const
> > struct net_device *dev)
> >
> >  static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid
> > *gid)  {
> > +	if (!gid)
> > +		return 0;
> > +
> >  	switch (addr->sa_family) {
> >  	case AF_INET:
> >  		ipv6_addr_set_v4mapped(((struct sockaddr_in *) @@ -205,6
> +208,9 @@
> > static inline void rdma_gid2ip(struct sockaddr *out, const union
> > ib_gid *gid)  static inline void rdma_addr_get_sgid(struct rdma_dev_addr
> *dev_addr,
> >  				      union ib_gid *gid)
> >  {
> > +	if (!gid)
> > +		return;
> 
> I think it would be clearer to put this test in the rdma_read_gids.
> It makes no sense to have a function with one output where the output is
> optional. Particularly when it is inlined..
Ok. Make sense to me.
> 
> And the prior patch makes a bit more sense after looking here, but the series
> seems in the wrong order -> the patch to break rdma_addr_get_sgid should go
rdma_addr_get_sgid() worked only in very limited case.
Since all patches are in same series, I don't see order breaks anything. But changing order is fine in revised series.

> after the only rocee code path in ucma is switched to use rdma_read_gids which
> hopefully does something similar to what it did before?
> 
Yes, it returns IP based GID for multiple missing cases and previous simple case too.

> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from rdma_addr_get_sgid()

[Jason Gunthorpe]

On Mon, Jan 15, 2018 at 10:34:13PM +0000, Parav Pandit wrote:

> > Can you define what the SGID/DGID should be in the rdma CM uapi when
> > working with rocee?
> > 
> It is the GIDs derived from the IP address using ip2gid() conversion.

So then why is it OK for this patch to start returning
dev_addr->src_dev_addr ?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from rdma_addr_get_sgid()

[Parav Pandit]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgg-uk2M96/98Pc@public.gmane.org]
> Sent: Monday, January 15, 2018 4:48 PM
> To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; Doug Ledford
> <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; RDMA mailing list <linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>;
> Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from
> rdma_addr_get_sgid()
> 
> On Mon, Jan 15, 2018 at 10:34:13PM +0000, Parav Pandit wrote:
> 
> > > Can you define what the SGID/DGID should be in the rdma CM uapi when
> > > working with rocee?
> > >
> > It is the GIDs derived from the IP address using ip2gid() conversion.
> 
> So then why is it OK for this patch to start returning dev_addr->src_dev_addr ?
> 
It is not ok for RoCE. Therefore as the subject line says, it is no longer supported for RoCE.
For other transports the code is unchanged. It continues to report src_dev_addr.
For example, for IB transport GIDs are not derived IP address.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for multiple transports

[Jason Gunthorpe]

On Mon, Jan 15, 2018 at 10:43:31PM +0000, Parav Pandit wrote:

> > And the prior patch makes a bit more sense after looking here, but the series
> > seems in the wrong order -> the patch to break rdma_addr_get_sgid should go

> rdma_addr_get_sgid() worked only in very limited case.  Since all
> patches are in same series, I don't see order breaks anything. But
> changing order is fine in revised series.

When you order the patches in a series you are trying to tell a story
to the reviewer. It isn't just a random collection of related patches.

Here the story is fixing the few places that are hard wired to use
GIDs to have sane GID information for RoCE.

Every patch should advance that story withou outgoing backwards. In
this case the first patch totally breaks the GIDs in RoCE which breaks
the understandability of the story - if you had put this patch at the
end of the list and said:

 Now that all RoCE users of rdma_addr_get_sgid() call rdma_read_gids()
 we can remove the sketchy RoCE specific support from
 rdma_addr_get_sgid()

Then the entire story makes a lot more sense, and the patch becomes a
conclusion, not a confusing starting point.

This also aide bisect-ability - we don't want to see patches break
working functionality in the middle of a series because 'git bisect'
does not know about series boundaries.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for multiple transports

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2602 bytes --]

On Mon, Jan 15, 2018 at 03:22:15PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 09, 2018 at 01:10:57PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > This patch introduces an API that allows legacy applications to query
> > GIDs for a rdma_cm_id which is used during connection establishment.
> >
> > GIDs are stored and created differently for IB, RoCE and iWARP transports.
> > Therefore rdma_read_gids() returns GID for all the transports hiding
> > such internal details to caller. It is usable for client side and server
> > side connections.
> >
> > The rdma_read_gids() should not be used by any new ULPs.
>
> This is weird statement. Why can't we change the few callers to do
> whatever is correct now?
>
> > +void rdma_read_gids(struct rdma_cm_id *cm_id, union ib_gid *sgid,
> > +		    union ib_gid *dgid)
> > +{
> > +	struct rdma_addr *addr = &cm_id->route.addr;
> > +
> > +	if (!cm_id->device)
> > +		return;
>
> Nope. I looked at only one caller and saw this return leaks uninited
> kernel stack memory to userspace. Probably best to zero the sgid/dgid
> in the error case??
>
> > diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
> > index fa809a7b48e7..edb1cdf2a892 100644
> > +++ b/include/rdma/ib_addr.h
> > @@ -165,6 +165,9 @@ static inline u16 rdma_vlan_dev_vlan_id(const struct net_device *dev)
> >
> >  static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid)
> >  {
> > +	if (!gid)
> > +		return 0;
> > +
> >  	switch (addr->sa_family) {
> >  	case AF_INET:
> >  		ipv6_addr_set_v4mapped(((struct sockaddr_in *)
> > @@ -205,6 +208,9 @@ static inline void rdma_gid2ip(struct sockaddr *out, const union ib_gid *gid)
> >  static inline void rdma_addr_get_sgid(struct rdma_dev_addr *dev_addr,
> >  				      union ib_gid *gid)
> >  {
> > +	if (!gid)
> > +		return;
>
> I think it would be clearer to put this test in the rdma_read_gids.
> It makes no sense to have a function with one output where the output
> is optional. Particularly when it is inlined..

Actually, I changed Parav's original code in this patch to be the construction
as you commented now. For me, rdma_read_gids() looked so ugly with sgid, dgid
checks so i didn't want to show it to anybody, you will see it in v1.

>
> And the prior patch makes a bit more sense after looking here, but the
> series seems in the wrong order -> the patch to break
> rdma_addr_get_sgid should go after the only rocee code path in ucma is
> switched to use rdma_read_gids which hopefully does something similar
> to what it did before?
>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]