[PATCH rdma-next 0/6] Update CMA to use net namespace

[PATCH rdma-next 0/6] Update CMA to use net namespace

[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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

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

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/w@public.gmane.org>
Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 169d3a3bbf71..683428cffd54 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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);
-- 
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/6] RDMA/cma: Refactor to access multiple fields of rdma_dev_addr

[Leon Romanovsky]

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

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/w@public.gmane.org>
Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 683428cffd54..78b1ce5789e6 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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;
-- 
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/6] RDMA/cma: Update cma_validate_port to honor net namespace

[Leon Romanovsky]

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

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/w@public.gmane.org>
Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 78b1ce5789e6..09bde1b13a64 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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 {
-- 
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 4/6] RDMA/cma: Update RoCE multicast routines to use net namespace

[Leon Romanovsky]

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

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/w@public.gmane.org>
Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 09bde1b13a64..90dead30de4c 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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,
-- 
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 5/6] RDMA/cma: Protect ifindex access during IPv6 route lookup

[Leon Romanovsky]

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

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/w@public.gmane.org>
Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 90dead30de4c..690ed4238d72 100644
--- a/drivers/infiniband/core/cma.c
+++ 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,
-					 &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;
 
-- 
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 6/6] RDMA/cma: Protect ifindex access during IPv4 route lookup

[Leon Romanovsky]

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

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/w@public.gmane.org>
Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 690ed4238d72..1d5dc0d13105 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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();
-- 
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 5/6] RDMA/cma: Protect ifindex access during IPv6 route lookup

[Jason Gunthorpe]

On Tue, Jan 09, 2018 at 03:58:58PM +0200, Leon Romanovsky wrote:
> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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/w@public.gmane.org>
> Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>  drivers/infiniband/core/cma.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> 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,
> -					 &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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jason Gunthorpe]

On Tue, Jan 09, 2018 at 03:58:54PM +0200, Leon Romanovsky wrote:
> From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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/w@public.gmane.org>
> Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>  drivers/infiniband/core/cma.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 169d3a3bbf71..683428cffd54 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 0/6] Update CMA to use net namespace

[Jason Gunthorpe]

On Tue, Jan 09, 2018 at 03:58:53PM +0200, Leon Romanovsky wrote:
> 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

I took these ones to for-next

>   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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Parav Pandit]

> -----Original Message-----
> From: Jason Gunthorpe
> Sent: Monday, January 22, 2018 12:48 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>; Mark Bloch
> <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH rdma-next 1/6] RDMA/cma: Check existence of netdevice
> during port validation
> 
> On Tue, Jan 09, 2018 at 03:58:54PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > 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/w@public.gmane.org>
> > Reviewed-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > drivers/infiniband/core/cma.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cma.c
> > b/drivers/infiniband/core/cma.c index 169d3a3bbf71..683428cffd54
> > 100644
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -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?
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().

> 
> 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/6] RDMA/cma: Check existence of netdevice during port validation

[Jason Gunthorpe]

On Mon, Jan 22, 2018 at 01:39:19PM -0700, Parav Pandit wrote:

> > > -	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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

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

On Mon, Jan 22, 2018 at 11:38:20AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 09, 2018 at 03:58:58PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > 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/w@public.gmane.org>
> > Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >  drivers/infiniband/core/cma.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > 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,
> > -					 &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.

I think that rcu_derefence over net_dev will solve the race.

>
> 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.

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

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

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

[Jason Gunthorpe]

On Tue, Jan 23, 2018 at 10:50:14AM +0200, Leon Romanovsky wrote:

> > 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.
> 
> I think that rcu_derefence over net_dev will solve the race.

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

> > 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html