Skip to contentSkip to conversation

Update CMA to use net namespace

PATCH In review Leon Romanovsky 4 participants 14 messages Jan 9, 2018 – Jan 23, 2018 7 messages not in the archive InfiniBand

lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

cover Update CMA to use net namespace

Hi,

This series updates several CMA paths to use net namespaces, those patches don't have Fixes line, because the original code wasn't supposed to work with namespaces.

Thanks

Parav Pandit (6):
RDMA/cma: Check existence of netdevice during port validation RDMA/cma: Refactor to access multiple fields of rdma_dev_addr RDMA/cma: Update cma_validate_port to honor net namespace RDMA/cma: Update RoCE multicast routines to use net namespace RDMA/cma: Protect ifindex access during IPv6 route lookup RDMA/cma: Protect ifindex access during IPv4 route lookup

 drivers/infiniband/core/cma.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

--
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180109135859.7676-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

author
#2 lore raw

PATCH 1/6 RDMA/cma: Check existence of netdevice during port validation

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

If valid netdevice is not found for RoCE, GID table should not be searched with NULL netdevice.

Fixes abae1b71dd37 ("IB/cma: cma_validate_port should verify the port and netdevice")
Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +5 -3
index 169d3a3bbf71..683428cffd54 100644
@@ -624,11 +624,13 @@ static inline int cma_validate_port(struct ib_device *device, u8 port,
 	if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
 		return ret;
 
-	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port))
+	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
 		ndev = dev_get_by_index(&init_net, bound_if_index);
-	else
+		if (!ndev)
+			return ret;
+	} else {
 		gid_type = IB_GID_TYPE_IB;
-
+	}
 
 	ret = ib_find_cached_gid_by_port(device, gid, gid_type, port,
 					 ndev, NULL);
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180109135859.7676-2-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#4 lore raw

Re: [PATCH rdma-next 1/6] RDMA/cma: Check existence of netdevice during port validation

Quoted 16 lines · Leon Romanovsky

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

If valid netdevice is not found for RoCE, GID table should not be searched with NULL netdevice.

Fixes abae1b71dd37 ("IB/cma: cma_validate_port should verify the port and netdevice")
Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/cma.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +1 -0
index 169d3a3bbf71..683428cffd54 100644
+++ b/drivers/infiniband/core/cma.c

drivers/infiniband/core/cma.c static inline int cma_validate_port(struct ib_device *device, u8 port, · line 628

Quoted 6 lines
drivers/infiniband/core/cma.c +1 -1
@@ -624,11 +624,13 @@ static inline int cma_validate_port(struct ib_device *device, u8 port,
 	if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
 		return ret;
 
-	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port))
+	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
 		ndev = dev_get_by_index(&init_net, bound_if_index);

Why are we using indexes as a long term handle to netdevs? Is there some reason for this you know of?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180122184807.GN14358-VPRAkNaXOzVWk0Htik3J/[email protected]

#6 lore raw

RE: [PATCH rdma-next 1/6] RDMA/cma: Check existence of netdevice during port validation

Quoted 34 lines

-----Original Message-----
From: Jason Gunthorpe
Sent: Monday, January 22, 2018 12:48 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; RDMA mailing list <linux- [email protected]>; Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>; Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 1/6] RDMA/cma: Check existence of netdevice
during port validation

Review static inline int cma_validate_port(struct ib_device

Quoted 18 lines · Leon Romanovsky

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

If valid netdevice is not found for RoCE, GID table should not be searched with NULL netdevice.

Fixes abae1b71dd37 ("IB/cma: cma_validate_port should verify the port

and netdevice")

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

drivers/infiniband/core/cma.c | 8 +++++---

 1 file changed, 5 insertions(+), 3 deletions(-)
(quoted diff) +0 -0

b/drivers/infiniband/core/cma.c index 169d3a3bbf71..683428cffd54 100644
+++ b/drivers/infiniband/core/cma.c

(quoted diff) +0 -0
@@ -624,11 +624,13 @@ static inline int cma_validate_port(struct ib_device

*device, u8 port,

Review static inline int cma_validate_port(struct ib_device · line 624

Quoted 1 line
(quoted diff) +0 -0
 	if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device,

port))

Quoted 8 lines

drivers/infiniband/core/cma.c static inline int cma_validate_port(struct ib_device *device, u8 port, · line 628

Quoted 5 lines
drivers/infiniband/core/cma.c +1 -1
 		return ret;
 
-	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port))
+	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
 		ndev = dev_get_by_index(&init_net, bound_if_index);

Why are we using indexes as a long term handle to netdevs? Is there some reason for this you know of?

I am not sure why index was used instead of netdev pointer in beginning.
I do prefer pointer. Sometime back I tried to look at this code but dropped the idea due to convoluted code of rdma_translate_ip() and rdma_copy_addr().

Quoted 2 lines

Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): VI1PR0502MB30080B452158077BBA9C9EE4D1EC0-o1MPJYiShExKsLr+rGaxW8DSnupUy6xnnBOFsp37pqbUKgpGm//BTAC/[email protected]

#8 lore raw

Re: [PATCH rdma-next 1/6] RDMA/cma: Check existence of netdevice during port validation

Quoted 11 lines · Parav Pandit
Quoted 6 lines

drivers/infiniband/core/cma.c static inline int cma_validate_port(struct ib_device *device, u8 port, · line 628

Quoted 3 lines
drivers/infiniband/core/cma.c +1 -1
-	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port))
+	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
 		ndev = dev_get_by_index(&init_net, bound_if_index);

Why are we using indexes as a long term handle to netdevs? Is there some reason for this you know of?

I am not sure why index was used instead of netdev pointer in beginning. I do prefer pointer. Sometime back I tried to look at this code but dropped the idea due to convoluted code of rdma_translate_ip() and rdma_copy_addr().

I think we should change it.

I tried to convince myself these patches were right, and that net and bound_if_index are actually coherent and it was far too hard in general.

The best I could do was to convince myself that the listen/accept path was probably OK..

This whole thing seems so baroque these days, and now that ifindexes can be aliased across net namespaces, and even changed during a single netdevice lifetime, it seems dangerous..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#9 lore raw

PATCH 2/6 RDMA/cma: Refactor to access multiple fields of rdma_dev_addr

In reply to a message not in the archive

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

Pass the rdma_cm_id so that multiple fields of the rdma_dev_addr structure can be accessed, instead of passing each individual fields.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/infiniband/core/cma.c +8 -7
index 683428cffd54..78b1ce5789e6 100644
@@ -612,11 +612,14 @@ static int cma_translate_addr(struct sockaddr *addr, struct rdma_dev_addr *dev_a
 
 static inline int cma_validate_port(struct ib_device *device, u8 port,
 				    enum ib_gid_type gid_type,
-				      union ib_gid *gid, int dev_type,
-				      int bound_if_index)
+				    union ib_gid *gid,
+				    struct rdma_id_private *id_priv)
 {
-	int ret = -ENODEV;
+	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
+	int bound_if_index = dev_addr->bound_dev_if;
+	int dev_type = dev_addr->dev_type;
 	struct net_device *ndev = NULL;
+	int ret = -ENODEV;
 
 	if ((dev_type == ARPHRD_INFINIBAND) && !rdma_protocol_ib(device, port))
 		return ret;

@@ -671,8 +674,7 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv,
 					rdma_protocol_ib(cma_dev->device, port) ?
 					IB_GID_TYPE_IB :
 					listen_id_priv->gid_type, gidp,
-					dev_addr->dev_type,
-					dev_addr->bound_dev_if);
+					id_priv);
 		if (!ret) {
 			id_priv->id.port_num = port;
 			goto out;

@@ -693,8 +695,7 @@ static int cma_acquire_dev(struct rdma_id_private *id_priv,
 						rdma_protocol_ib(cma_dev->device, port) ?
 						IB_GID_TYPE_IB :
 						cma_dev->default_gid_type[port - 1],
-						gidp, dev_addr->dev_type,
-						dev_addr->bound_dev_if);
+						gidp, id_priv);
 			if (!ret) {
 				id_priv->id.port_num = port;
 				goto out;
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#10 lore raw

PATCH 3/6 RDMA/cma: Update cma_validate_port to honor net namespace

In reply to a message not in the archive

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

cma_validate_port uses rdma_dev_addr to validate the port of the cm_id. It needs to honor the net namespace which is setup during cm_id creation when finding netdevice.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/cma.c +1 -1
index 78b1ce5789e6..09bde1b13a64 100644
@@ -628,7 +628,7 @@ static inline int cma_validate_port(struct ib_device *device, u8 port,
 		return ret;
 
 	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
-		ndev = dev_get_by_index(&init_net, bound_if_index);
+		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
 		if (!ndev)
 			return ret;
 	} else {
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#11 lore raw

PATCH 4/6 RDMA/cma: Update RoCE multicast routines to use net namespace

In reply to a message not in the archive

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

rdma_dev_addr contains the net namespace pointer, while referring bound_dev_if of the rdma_dev_addr, refer to the net namespace of rdma_cm_id stored in rdma_dev_addr.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +3 -3
index 09bde1b13a64..90dead30de4c 100644
@@ -3930,7 +3930,7 @@ static int cma_ib_mc_handler(int status, struct ib_sa_multicast *multicast)
 		struct rdma_dev_addr *dev_addr =
 			&id_priv->id.route.addr.dev_addr;
 		struct net_device *ndev =
-			dev_get_by_index(&init_net, dev_addr->bound_dev_if);
+			dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);
 		enum ib_gid_type gid_type =
 			id_priv->cma_dev->default_gid_type[id_priv->id.port_num -
 			rdma_start_port(id_priv->cma_dev->device)];

@@ -4120,7 +4120,7 @@ static int cma_iboe_join_multicast(struct rdma_id_private *id_priv,
 		mc->multicast.ib->rec.qkey = cpu_to_be32(RDMA_UDP_QKEY);
 
 	if (dev_addr->bound_dev_if)
-		ndev = dev_get_by_index(&init_net, dev_addr->bound_dev_if);
+		ndev = dev_get_by_index(dev_addr->net, dev_addr->bound_dev_if);
 	if (!ndev) {
 		err = -ENODEV;
 		goto out2;

@@ -4238,7 +4238,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr)
 					struct net_device *ndev = NULL;
 
 					if (dev_addr->bound_dev_if)
-						ndev = dev_get_by_index(&init_net,
+						ndev = dev_get_by_index(dev_addr->net,
 									dev_addr->bound_dev_if);
 					if (ndev) {
 						cma_igmp_send(ndev,
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#12 lore raw

PATCH 5/6 RDMA/cma: Protect ifindex access during IPv6 route lookup

In reply to a message not in the archive

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

Netdev ifindex can change while performing IPv6 rt6_lookup(). Therefore access ifindex under rcu lock.
This ensures that ifindex won't change while lookup is in progress.

Fixes f887f2ac87c2 ("IB/cma: Validate routing of incoming requests")
Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +7 -3
index 90dead30de4c..690ed4238d72 100644
@@ -1333,11 +1333,15 @@ static bool validate_ipv6_net_dev(struct net_device *net_dev,
 #if IS_ENABLED(CONFIG_IPV6)
 	const int strict = ipv6_addr_type(&dst_addr->sin6_addr) &
 			   IPV6_ADDR_LINKLOCAL;
-	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
-					 &src_addr->sin6_addr, net_dev->ifindex,
-					 strict);
+	struct rt6_info *rt;
 	bool ret;
 
+	rcu_read_lock();
+	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
+			&src_addr->sin6_addr, net_dev->ifindex,
+			strict);
+	rcu_read_unlock();
+
 	if (!rt)
 		return false;
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180109135859.7676-6-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#14 lore raw

Re: [PATCH rdma-next 5/6] RDMA/cma: Protect ifindex access during IPv6 route lookup

Quoted 21 lines · Leon Romanovsky

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

Netdev ifindex can change while performing IPv6 rt6_lookup(). Therefore access ifindex under rcu lock.
This ensures that ifindex won't change while lookup is in progress.

Fixes f887f2ac87c2 ("IB/cma: Validate routing of incoming requests")
Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/cma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +1 -1
index 90dead30de4c..690ed4238d72 100644
+++ b/drivers/infiniband/core/cma.c

@@ -1333,11 +1333,15 @@ static bool validate_ipv6_net_dev(struct net_device *net_dev,
 #if IS_ENABLED(CONFIG_IPV6)
 	const int strict = ipv6_addr_type(&dst_addr->sin6_addr) &
 			   IPV6_ADDR_LINKLOCAL;
-	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,

drivers/infiniband/core/cma.c static bool validate_ipv6_net_dev(struct net_device *net_dev, · line 1343

Quoted 10 lines
drivers/infiniband/core/cma.c +6 -2
-					 &src_addr->sin6_addr, net_dev->ifindex,
-					 strict);
+	struct rt6_info *rt;
 	bool ret;
 
+	rcu_read_lock();
+	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
+			&src_addr->sin6_addr, net_dev->ifindex,
+			strict);
+	rcu_read_unlock();

This doesn't make sense to me.

rcu is not supposed to be used in cases where two variables can change concurrently.

For instance if the ifindex is changed due to dev_change_net_namespace() then you have this:

	dev_net_set(dev, net);
	if (__dev_get_by_index(net, dev->ifindex))
		dev->ifindex = dev_new_index(net);

Racing with this:

	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
			&src_addr->sin6_addr, net_dev->ifindex,
			strict);

And we will get a racy incoherent result for dev_net(net_dev) and net_dev->ifindex.

Same comment for the next patch too.

It kind of looks to me like the locking scheme in the netdev side shuts down the netdev while moving it. So the RCU protected test should be if the netdev is DOWN ?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180122183820.GM14358-VPRAkNaXOzVWk0Htik3J/[email protected]

author
#16 lore raw

Re: [PATCH rdma-next 5/6] RDMA/cma: Protect ifindex access during IPv6 route lookup

Quoted 45 lines · Jason Gunthorpe
Quoted 21 lines · Leon Romanovsky

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

Netdev ifindex can change while performing IPv6 rt6_lookup(). Therefore access ifindex under rcu lock.
This ensures that ifindex won't change while lookup is in progress.

Fixes f887f2ac87c2 ("IB/cma: Validate routing of incoming requests")
Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/cma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +1 -1
index 90dead30de4c..690ed4238d72 100644
+++ b/drivers/infiniband/core/cma.c

@@ -1333,11 +1333,15 @@ static bool validate_ipv6_net_dev(struct net_device *net_dev,
 #if IS_ENABLED(CONFIG_IPV6)
 	const int strict = ipv6_addr_type(&dst_addr->sin6_addr) &
 			   IPV6_ADDR_LINKLOCAL;
-	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,

drivers/infiniband/core/cma.c static bool validate_ipv6_net_dev(struct net_device *net_dev, · line 1343

Quoted 10 lines
drivers/infiniband/core/cma.c +6 -2
-					 &src_addr->sin6_addr, net_dev->ifindex,
-					 strict);
+	struct rt6_info *rt;
 	bool ret;
 
+	rcu_read_lock();
+	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
+			&src_addr->sin6_addr, net_dev->ifindex,
+			strict);
+	rcu_read_unlock();

This doesn't make sense to me.

rcu is not supposed to be used in cases where two variables can change concurrently.

For instance if the ifindex is changed due to dev_change_net_namespace() then you have this:

	dev_net_set(dev, net);
	if (__dev_get_by_index(net, dev->ifindex))
		dev->ifindex = dev_new_index(net);
Quoted 8 lines

Racing with this:

	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
			&src_addr->sin6_addr, net_dev->ifindex,
			strict);

And we will get a racy incoherent result for dev_net(net_dev) and net_dev->ifindex.

I think that rcu_derefence over net_dev will solve the race.

Quoted 6 lines

Same comment for the next patch too.

It kind of looks to me like the locking scheme in the netdev side shuts down the netdev while moving it. So the RCU protected test should be if the netdev is DOWN ?

It is not the case for ipv6 tunnel interface, they use RCU protection for live netdevs too.

Quoted 6 lines

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180123085014.GO1393-U/[email protected]

#18 lore raw

Re: [PATCH rdma-next 5/6] RDMA/cma: Protect ifindex access during IPv6 route lookup

Quoted 9 lines · Leon Romanovsky
Quoted 9 lines

For instance if the ifindex is changed due to dev_change_net_namespace() then you have this:

	dev_net_set(dev, net);
	if (__dev_get_by_index(net, dev->ifindex))
		dev->ifindex = dev_new_index(net);

Racing with this:

Quoted 8 lines
Quoted 6 lines
	rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
			&src_addr->sin6_addr, net_dev->ifindex,
			strict);

And we will get a racy incoherent result for dev_net(net_dev) and net_dev->ifindex.

I think that rcu_derefence over net_dev will solve the race.

Nope, net_dev is a stack pointer, it is not RCU protected.

Quoted 6 lines
Quoted 3 lines

It kind of looks to me like the locking scheme in the netdev side shuts down the netdev while moving it. So the RCU protected test should be if the netdev is DOWN ?

It is not the case for ipv6 tunnel interface, they use RCU protection for live netdevs too.

Pointer? Doesn't mean it is right :(

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#19 lore raw

PATCH 6/6 RDMA/cma: Protect ifindex access during IPv4 route lookup

In reply to a message not in the archive

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

Netdev ifindex can change while performing IPv4 fib_lookup(), therefore access to ifindex under RCU lock.

This ensures that ifindex won't change while lookup is in progress.

Fixes f887f2ac87c2 ("IB/cma: Validate routing of incoming requests")
Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/cma.c +1 -1
index 690ed4238d72..1d5dc0d13105 100644
@@ -1314,11 +1314,11 @@ static bool validate_ipv4_net_dev(struct net_device *net_dev,
 		return false;
 
 	memset(&fl4, 0, sizeof(fl4));
-	fl4.flowi4_iif = net_dev->ifindex;
 	fl4.daddr = daddr;
 	fl4.saddr = saddr;
 
 	rcu_read_lock();
+	fl4.flowi4_iif = net_dev->ifindex;
 	err = fib_lookup(dev_net(net_dev), &fl4, &res, 0);
 	ret = err == 0 && FIB_RES_DEV(res) == net_dev;
 	rcu_read_unlock();
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#20 lore raw
In reply to a message not in the archive
Quoted 6 lines · Leon Romanovsky

Hi,

This series updates several CMA paths to use net namespaces, those patches don't have Fixes line, because the original code wasn't supposed to work with namespaces.

Quoted 7 lines

Thanks

Parav Pandit (6):
RDMA/cma: Check existence of netdevice during port validation RDMA/cma: Refactor to access multiple fields of rdma_dev_addr RDMA/cma: Update cma_validate_port to honor net namespace RDMA/cma: Update RoCE multicast routines to use net namespace

I took these ones to for-next

Quoted 2 lines

RDMA/cma: Protect ifindex access during IPv6 route lookup RDMA/cma: Protect ifindex access during IPv4 route lookup

Left these pending more explanations/rework

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html