Refactor rdma_cm query GID API

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Monday, January 15, 2018 4:08 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; RDMA mailing list <linux- [email protected]>; Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from
rdma_addr_get_sgid()

Quoted 20 lines · Leon Romanovsky

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

Quoted 2 lines

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?

Quoted 3 lines

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.

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Monday, January 15, 2018 4:48 PM
To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc: Leon Romanovsky <[email protected]>; Doug Ledford <[email protected]>; RDMA mailing list <[email protected]>; Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 1/3] RDMA/core: Remove RoCE support from
rdma_addr_get_sgid()

Quoted 4 lines · Parav Pandit

Quoted 3 lines

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 ?

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

include/rdma/ib_addr.h +4 -0

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 *)

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Monday, January 15, 2018 4:22 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; RDMA mailing list <linux- [email protected]>; Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 2/3] RDMA/cma: Introduce API to read GIDs for
multiple transports

Quoted 10 lines · Leon Romanovsky

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

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.

Quoted 1 line

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?

drivers/infiniband/core/cma.c __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr) · line 2046

Quoted 7 lines

drivers/infiniband/core/cma.c +7 -0

+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??

Quoted 14 lines

include/rdma/ib_addr.h index renamed from include/rdma/ib_addr.h +0 -0

fa809a7b48e7..edb1cdf2a892 100644
+++ b/include/rdma/ib_addr.h

include/rdma/ib_addr.h index renamed from include/rdma/ib_addr.h +3 -0

@@ -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:

Quoted 1 line

ipv6_addr_set_v4mapped(((struct sockaddr_in *) @@ -205,6

+208,9 @@

Quoted 2 lines

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,

include/rdma/ib_addr.h index

Quoted 4 lines

include/rdma/ib_addr.h index renamed from include/rdma/ib_addr.h +2 -0

 				      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

Quoted 2 lines

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.

Quoted 11 lines · Leon Romanovsky

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

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?

drivers/infiniband/core/cma.c __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr) · line 2046

Quoted 7 lines

drivers/infiniband/core/cma.c +7 -0

+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??

Quoted 15 lines

include/rdma/ib_addr.h +4 -0

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,

include/rdma/ib_addr.h static inline void rdma_gid2ip(struct sockaddr *out, const union ib_gid *gid) · line 212

Quoted 4 lines

include/rdma/ib_addr.h +2 -0

 				      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

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

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

Cc Santosh Shilimkar <[email protected]>

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
  drivers/infiniband/core/ucma.c | 8 ++++----
  net/rds/ib.c                   | 6 ++----
  2 files changed, 6 insertions(+), 8 deletions(-)