Skip to contentSkip to conversation

RDMA/addr: Use appropriate locking with for_each_netdev()

PATCH In review Eric Dumazet 6 participants 10 messages Nov 10, 2009 – Nov 23, 2009 6 messages not in the archive

Subsystems (3)
Networking Network drivers InfiniBand
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 RDMA/addr: Use appropriate locking with for_each_netdev()

for_each_netdev() should be used with RTNL or dev_base_lock held, or risk a crash.

Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/infiniband/core/addr.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
drivers/infiniband/core/addr.c +7 -2
index bd07803..5ca0b2c 100644
@@ -131,6 +131,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
+		read_lock(&dev_base_lock);
 		for_each_netdev(&init_net, dev) {
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) addr)->sin6_addr,

@@ -139,6 +140,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 				break;
 			}
 		}
+		read_unlock(&dev_base_lock);
 		break;
 #endif
 	}

@@ -391,15 +393,17 @@ static int addr_resolve_local(struct sockaddr *src_in,
 	{
 		struct in6_addr *a;
 
+		read_lock(&dev_base_lock);
 		for_each_netdev(&init_net, dev)
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) dst_in)->sin6_addr,
 					  dev, 1))
 				break;
 
-		if (!dev)
+		if (!dev) {
+			read_unlock(&dev_base_lock);
 			return -EADDRNOTAVAIL;
-
+		}
 		a = &((struct sockaddr_in6 *) src_in)->sin6_addr;
 
 		if (ipv6_addr_any(a)) {

@@ -416,6 +420,7 @@ static int addr_resolve_local(struct sockaddr *src_in,
 			if (!ret)
 				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
 		}
+		read_unlock(&dev_base_lock);
 		break;
 	}
 #endif

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

#2 lore raw
Quoted 4 lines

for_each_netdev() should be used with RTNL or dev_base_lock held, or risk a crash.

Signed-off-by Eric Dumazet <[email protected]>

Thanks - I'm working on a patch set in this area. Roland, I can merge Eric's changes into that patch set if it makes things easier.

drivers/infiniband/core/addr.c int rdma_translate_ip(struct sockaddr *addr, struct

Quoted 8 lines

drivers/infiniband/core/addr.c | 9 +++++++--

1 files changed, 7 insertions(+), 2 deletions(-)
drivers/infiniband/core/addr.c +0 -0
index bd07803..5ca0b2c 100644
@@ -131,6 +131,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct

The changes to this function are still valid.

drivers/infiniband/core/addr.c static int addr_resolve_local(struct sockaddr *src_in,

Quoted 1 line
drivers/infiniband/core/addr.c +0 -0
@@ -391,15 +393,17 @@ static int addr_resolve_local(struct sockaddr *src_in,

This function is going away.

- Sean

--
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): D5E1FB213939498DBFFEB5A3B83C08AD-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org

#4 lore raw
Quoted 8 lines
Quoted 5 lines

for_each_netdev() should be used with RTNL or dev_base_lock held, or risk a crash.

Signed-off-by Eric Dumazet <[email protected]>

Thanks - I'm working on a patch set in this area. Roland, I can merge Eric's changes into that patch set if it makes things easier.

Would it be possible for you to take Eric's patch as the first in your set (keeping his From: of course) and base your fixes on top of that? That seems the cleanest thing to me.

Thanks,
Roland
--
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): adaocn8x6j7.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org

A message that is not in the archive (sent off-list or not yet received): 8C10E584257A46DB9A0AD193520CF4A7-Zpru7NauK7drdx17CPfAsdBPR1lH4CV8@public.gmane.org

#8 lore raw
Quoted 5 lines · Sean Hefty
Quoted 2 lines

Would it be possible for you to take Eric's patch as the first in your set (keeping his From: of course) and base your fixes on top of that?

Will do.

Any news of this patch ?

We need it for 2.6.32 and also to prepare 2.6.33 with upcoming patches.

Thanks

[PATCH] RDMA/addr: Use appropriate locking with for_each_netdev()

for_each_netdev() should be used with RTNL or dev_base_lock held, or risk a crash.

Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/infiniband/core/addr.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
drivers/infiniband/core/addr.c +7 -2
index bd07803..5ca0b2c 100644
@@ -131,6 +131,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
+		read_lock(&dev_base_lock);
 		for_each_netdev(&init_net, dev) {
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) addr)->sin6_addr,

@@ -139,6 +140,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 				break;
 			}
 		}
+		read_unlock(&dev_base_lock);
 		break;
 #endif
 	}

@@ -391,15 +393,17 @@ static int addr_resolve_local(struct sockaddr *src_in,
 	{
 		struct in6_addr *a;
 
+		read_lock(&dev_base_lock);
 		for_each_netdev(&init_net, dev)
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) dst_in)->sin6_addr,
 					  dev, 1))
 				break;
 
-		if (!dev)
+		if (!dev) {
+			read_unlock(&dev_base_lock);
 			return -EADDRNOTAVAIL;
-
+		}
 		a = &((struct sockaddr_in6 *) src_in)->sin6_addr;
 
 		if (ipv6_addr_any(a)) {

@@ -416,6 +420,7 @@ static int addr_resolve_local(struct sockaddr *src_in,
 			if (!ret)
 				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
 		}
+		read_unlock(&dev_base_lock);
 		break;
 	}
 #endif

--
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): [email protected]

#10 lore raw

drivers/infiniband/core/addr.c int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr) · line 143

Quoted 9 lines · On Fri, 20 Nov 2009 06:28:00 +0100 Eric Dumazet…
drivers/infiniband/core/addr.c +2 -0
+		read_lock(&dev_base_lock);
 		for_each_netdev(&init_net, dev) {
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) addr)->sin6_addr,

@@ -139,6 +140,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 				break;
 			}
 		}
+		read_unlock(&dev_base_lock);

what about for_each_netdev_rcu() here instead...

Signature · 4 lines
--
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
Quoted 15 lines · Stephen Hemminger

drivers/infiniband/core/addr.c int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr) · line 143

Quoted 9 lines · On Fri, 20 Nov 2009 06:28:00 +0100 Eric Dumazet <[email protected]> wrote:
drivers/infiniband/core/addr.c +2 -0
+		read_lock(&dev_base_lock);
 		for_each_netdev(&init_net, dev) {
 			if (ipv6_chk_addr(&init_net,
 					  &((struct sockaddr_in6 *) addr)->sin6_addr,

@@ -139,6 +140,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 				break;
 			}
 		}
+		read_unlock(&dev_base_lock);

what about for_each_netdev_rcu() here instead...

Yes, in 2.6.33 :)

For 2.6.32, we need this patch

Thanks

#12 lore raw
In reply to a message not in the archive
Quoted 4 lines

Any news of this patch ?

We need it for 2.6.32 and also to prepare 2.6.33 with upcoming patches.

Sorry for not explicitly emailing about this patch. I applied it as part of Sean's series, but I just added it to my 2.6.33 queue. At this point in the release cycle it doesn't look severe enough for 2.6.32, unless I miss something.

- R.
--
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): adak4xl669z.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org

#14 lore raw
Quoted 8 lines · Roland Dreier
Quoted 3 lines

Any news of this patch ?

We need it for 2.6.32 and also to prepare 2.6.33 with upcoming patches.

Sorry for not explicitly emailing about this patch. I applied it as part of Sean's series, but I just added it to my 2.6.33 queue. At this point in the release cycle it doesn't look severe enough for 2.6.32, unless I miss something.

Its possible to get infinite loops or crashes.

I am not a RDMA user, so I personally dont care :)

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

#15 lore raw
Quoted 4 lines

Its possible to get infinite loops or crashes.

I am not a RDMA user, so I personally dont care :)

I think given that it's not a regression, and I've never heard of anyone hitting it in practice, we can wait until the merge window.

Thanks!