[PATCH linux-2.6.17-rc1] net: fix neigh_delete to handle mult. tables

[PATCH linux-2.6.17-rc1] net: fix neigh_delete to handle mult. tables

[Don Law]

The neigh_delete assumes that there is only one table with the desired
protocol family for the address being deleted.  This assumption is not
always valid.  For example, in a kernel configured with both the
typical Ethernet configuration (CONFIG_NET_ETHERNET=y etc.) as well
as ATM (CONFIG_ATM=y and CONFIG_ATM_CLIP=y), there are two tables, 
namely arp_tbl and clip_tbl, both with family AF_INET.

The problem fixed by this patch is that the "for" loop in neigh_delete
will not iterate further once it finds one table with a matching family.
The code changes refactor neigh_delete such that it will continue
to iterate through the neighbor tables until it finds a match or 
finishes the tables.

To see the symptom of the problem, you must have a kernel with more
than one AF_INET transport configured, then try to delete a neighbor
table entry.  The delete will fail with EINVAL.  For example:

    [root@bonefish ~]# ip neigh list dev eth4
    192.168.4.24 lladdr 00:90:fb:a1:07:2f nud reachable
    [root@bonefish ~]# ip neigh delete 192.168.4.24 dev eth4
    RTNETLINK answers: Invalid argument
    [root@bonefish ~]# ip neigh list dev eth4
    192.168.4.24 lladdr 00:90:fb:a1:07:2f nud reachable

One side effect of this change is that neigh_lookup is now called while
a read lock is held on neigh_tbl_lock.  neigh_lookup does take read
locks on the tbl->lock set.  However, this does not introduce any new lock
order dependencies, since we already have that precedent set in the
neightbl_set function.

Signed-off-by: Don Law <opendon@donlaw.com>

---

I have built this patch against the 2.6.17-rc1 kernel.

I tested this patch with the simple test case described above, plus let
it run for a few days on the box to make sure it behaves OK.  I also
verified that I can properly delete IPV6 neighbor entries.


--- linux-2.6.17-rc1/net/core/neighbour.c.orig  2006-04-11 16:28:10.000000000 -0
400
+++ linux-2.6.17-rc1/net/core/neighbour.c       2006-04-13 10:01:19.000000000 -0
400
@@ -1430,48 +1430,54 @@ int neigh_delete(struct sk_buff *skb, st
        struct rtattr **nda = arg;
        struct neigh_table *tbl;
        struct net_device *dev = NULL;
-       int err = -ENODEV;
+       int err;
 
        if (ndm->ndm_ifindex &&
-           (dev = dev_get_by_index(ndm->ndm_ifindex)) == NULL)
-               goto out;
+                       (dev = dev_get_by_index(ndm->ndm_ifindex)) == NULL)
+               return -ENODEV;
 
        read_lock(&neigh_tbl_lock);
-       for (tbl = neigh_tables; tbl; tbl = tbl->next) {
+       for (tbl = neigh_tables; ; tbl = tbl->next) {
                struct rtattr *dst_attr = nda[NDA_DST - 1];
                struct neighbour *n;
 
+               if (!tbl) {
+                       read_unlock(&neigh_tbl_lock);
+                       err = -EADDRNOTAVAIL;
+                       break;
+               }
                if (tbl->family != ndm->ndm_family)
                        continue;
-               read_unlock(&neigh_tbl_lock);
 
                err = -EINVAL;
-               if (!dst_attr || RTA_PAYLOAD(dst_attr) < tbl->key_len)
-                       goto out_dev_put;
+               if (!dst_attr || RTA_PAYLOAD(dst_attr) < tbl->key_len) {
+                       read_unlock(&neigh_tbl_lock);
+                       break;
+               }
 
                if (ndm->ndm_flags & NTF_PROXY) {
+                       read_unlock(&neigh_tbl_lock);
                        err = pneigh_delete(tbl, RTA_DATA(dst_attr), dev);
-                       goto out_dev_put;
+                       break;
                }
 
-               if (!dev)
-                       goto out;
+               if (!dev) {
+                       read_unlock(&neigh_tbl_lock);
+                       break;
+               }
 
                n = neigh_lookup(tbl, RTA_DATA(dst_attr), dev);
                if (n) {
-                       err = neigh_update(n, NULL, NUD_FAILED, 
+                       read_unlock(&neigh_tbl_lock);
+                       err = neigh_update(n, NULL, NUD_FAILED,
                                           NEIGH_UPDATE_F_OVERRIDE|
                                           NEIGH_UPDATE_F_ADMIN);
                        neigh_release(n);
+                       break;
                }
-               goto out_dev_put;
        }
-       read_unlock(&neigh_tbl_lock);
-       err = -EADDRNOTAVAIL;
-out_dev_put:
        if (dev)
                dev_put(dev);
-out:
        return err;
 }

Re: [PATCH linux-2.6.17-rc1] net: fix neigh_delete to handle mult. tables

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4448520C.5080508@donlaw.com> (at Thu, 20 Apr 2006 23:31:24 -0400), Don Law <opendon@donlaw.com> says:

> One side effect of this change is that neigh_lookup is now called while
> a read lock is held on neigh_tbl_lock.  neigh_lookup does take read
> locks on the tbl->lock set.  However, this does not introduce any new lock
> order dependencies, since we already have that precedent set in the
> neightbl_set function.

I don't think this is good change.

What you need to fix your problem is to remove the last
"goto out_dev_put;" isn't it?

--yoshfuji

Re: [PATCH linux-2.6.17-rc1] net: fix neigh_delete to handle mult. tables

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20060421.125543.126764691.yoshfuji@linux-ipv6.org> (at Fri, 21 Apr 2006 12:55:43 +0900 (JST)), YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> says:

> In article <4448520C.5080508@donlaw.com> (at Thu, 20 Apr 2006 23:31:24 -0400), Don Law <opendon@donlaw.com> says:
> 
> > One side effect of this change is that neigh_lookup is now called while
> > a read lock is held on neigh_tbl_lock.  neigh_lookup does take read
> > locks on the tbl->lock set.  However, this does not introduce any new lock
> > order dependencies, since we already have that precedent set in the
> > neightbl_set function.
> 
> I don't think this is good change.
> 
> What you need to fix your problem is to remove the last
> "goto out_dev_put;" isn't it?

Hmm, okay, I notice we need to hold lock for pneigh_delete().
How about this?  Also handles proxy neighbors.

Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 4cf878e..fa3227a 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1430,6 +1430,8 @@ int neigh_delete(struct sk_buff *skb, st
 	    (dev = dev_get_by_index(ndm->ndm_ifindex)) == NULL)
 		goto out;
 
+	err = -EADDRNOTAVAIL;
+
 	read_lock(&neigh_tbl_lock);
 	for (tbl = neigh_tables; tbl; tbl = tbl->next) {
 		struct rtattr *dst_attr = nda[NDA_DST - 1];
@@ -1437,32 +1439,33 @@ int neigh_delete(struct sk_buff *skb, st
 
 		if (tbl->family != ndm->ndm_family)
 			continue;
-		read_unlock(&neigh_tbl_lock);
 
-		err = -EINVAL;
-		if (!dst_attr || RTA_PAYLOAD(dst_attr) < tbl->key_len)
-			goto out_dev_put;
+		if (!dst_attr || RTA_PAYLOAD(dst_attr) < tbl->key_len) {
+			err = -EINVAL;
+			break;
+		}
 
 		if (ndm->ndm_flags & NTF_PROXY) {
-			err = pneigh_delete(tbl, RTA_DATA(dst_attr), dev);
-			goto out_dev_put;
+			if (!pneigh_delete(tbl, RTA_DATA(dst_attr), dev)) {
+				read_unlock(&neigh_tbl_lock);
+				break;
+			}
 		}
 
 		if (!dev)
-			goto out;
+			continue;
 
 		n = neigh_lookup(tbl, RTA_DATA(dst_attr), dev);
 		if (n) {
+			read_unlock(&neigh_tbl_lock);
 			err = neigh_update(n, NULL, NUD_FAILED, 
 					   NEIGH_UPDATE_F_OVERRIDE|
 					   NEIGH_UPDATE_F_ADMIN);
 			neigh_release(n);
+			break;
 		}
-		goto out_dev_put;
 	}
 	read_unlock(&neigh_tbl_lock);
-	err = -EADDRNOTAVAIL;
-out_dev_put:
 	if (dev)
 		dev_put(dev);
 out:

-- 
YOSHIFUJI Hideaki @ USAGI Project  <yoshfuji@linux-ipv6.org>
GPG-FP  : 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

[PATCH linux-2.6.17-rc1] net: fix neigh_delete to handle mult. tables

[Don Law]

I'm resending the patch I just sent. Sorry - it looks like my
mailer likes to damage the white space in the patch.

This version should be cleaner:

Signed-off-by: Don Law <opendon@donlaw.com>

--- linux-2.6.17-rc1/net/core/neighbour.c.orig	2006-04-11 16:28:10.000000000 -0400
+++ linux-2.6.17-rc1/net/core/neighbour.c	2006-04-26 23:36:04.535992104 -0400
@@ -1430,48 +1430,55 @@ int neigh_delete(struct sk_buff *skb, st
 	struct rtattr **nda = arg;
 	struct neigh_table *tbl;
 	struct net_device *dev = NULL;
-	int err = -ENODEV;
+	int err;
 
 	if (ndm->ndm_ifindex &&
-	    (dev = dev_get_by_index(ndm->ndm_ifindex)) == NULL)
-		goto out;
+			(dev = dev_get_by_index(ndm->ndm_ifindex)) == NULL)
+		return -ENODEV;
 
 	read_lock(&neigh_tbl_lock);
-	for (tbl = neigh_tables; tbl; tbl = tbl->next) {
+	for (tbl = neigh_tables; ; tbl = tbl->next) {
 		struct rtattr *dst_attr = nda[NDA_DST - 1];
 		struct neighbour *n;
 
+		if (!tbl) {
+			read_unlock(&neigh_tbl_lock);
+			err = -EADDRNOTAVAIL;
+			break;
+		}
 		if (tbl->family != ndm->ndm_family)
 			continue;
-		read_unlock(&neigh_tbl_lock);
 
 		err = -EINVAL;
-		if (!dst_attr || RTA_PAYLOAD(dst_attr) < tbl->key_len)
-			goto out_dev_put;
+		if (!dst_attr || RTA_PAYLOAD(dst_attr) < tbl->key_len) {
+			read_unlock(&neigh_tbl_lock);
+			break;
+		}
 
 		if (ndm->ndm_flags & NTF_PROXY) {
-			err = pneigh_delete(tbl, RTA_DATA(dst_attr), dev);
-			goto out_dev_put;
+			if (!pneigh_delete(tbl, RTA_DATA(dst_attr), dev)) {
+				read_unlock(&neigh_tbl_lock);
+				break;
+			}
 		}
 
-		if (!dev)
-			goto out;
+		if (!dev) {
+			read_unlock(&neigh_tbl_lock);
+			break;
+		}
 
 		n = neigh_lookup(tbl, RTA_DATA(dst_attr), dev);
 		if (n) {
-			err = neigh_update(n, NULL, NUD_FAILED, 
+			read_unlock(&neigh_tbl_lock);
+			err = neigh_update(n, NULL, NUD_FAILED,
 					   NEIGH_UPDATE_F_OVERRIDE|
 					   NEIGH_UPDATE_F_ADMIN);
 			neigh_release(n);
+			break;
 		}
-		goto out_dev_put;
 	}
-	read_unlock(&neigh_tbl_lock);
-	err = -EADDRNOTAVAIL;
-out_dev_put:
 	if (dev)
 		dev_put(dev);
-out:
 	return err;
 }