[PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

The deletion of local addresses via netlink doesn't take the
prefix length into account resulting in the deletion of
the address that was added first given multiple addresses
exist only varying in the prefix length.

tgr:axs ~ ip -4 addr show dev lo
1: lo: <LOOPBACK,UP> mtu 16436 qdisc noqueue 
    inet 127.0.0.1/8 scope host lo
    inet 1.1.1.1/1 scope global lo
    inet 1.1.1.1/2 scope global lo
tgr:axs ~ ip addr del 1.1.1.1/2 dev lo
tgr:axs ~ ip -4 addr show dev lo
1: lo: <LOOPBACK,UP> mtu 16436 qdisc noqueue 
    inet 127.0.0.1/8 scope host lo
    inet 1.1.1.1/2 scope global lo

Signed-off-by: Thomas Graf <tgraf@suug.ch>

--- linux-2.6.11.orig/net/ipv4/devinet.c	2005-03-04 00:45:05.000000000 +0100
+++ linux-2.6.11/net/ipv4/devinet.c	2005-03-04 01:55:54.000000000 +0100
@@ -396,8 +396,9 @@
 	for (ifap = &in_dev->ifa_list; (ifa = *ifap) != NULL;
 	     ifap = &ifa->ifa_next) {
 		if ((rta[IFA_LOCAL - 1] &&
+		     (ifm->ifa_prefixlen != ifa->ifa_prefixlen ||
 		     memcmp(RTA_DATA(rta[IFA_LOCAL - 1]),
-			    &ifa->ifa_local, 4)) ||
+			    &ifa->ifa_local, 4))) ||
 		    (rta[IFA_LABEL - 1] &&
 		     rtattr_strcmp(rta[IFA_LABEL - 1], ifa->ifa_label)) ||
 		    (rta[IFA_ADDRESS - 1] &&

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

Thomas Graf <tgraf@suug.ch> wrote:
> The deletion of local addresses via netlink doesn't take the
> prefix length into account resulting in the deletion of
> the address that was added first given multiple addresses
> exist only varying in the prefix length.

This has the potential of breaking user-space scripts.  For example,
this won't work anymore:

ip a a dev eth0 192.168.0.1/24
ip a d dev eth0 192.168.0.1

> tgr:axs ~ ip -4 addr show dev lo
> 1: lo: <LOOPBACK,UP> mtu 16436 qdisc noqueue 
>    inet 127.0.0.1/8 scope host lo
>    inet 1.1.1.1/1 scope global lo
>    inet 1.1.1.1/2 scope global lo

Do we really need to handle this case? If we do then would it be better
to consider ifa_prefixlen only when there are multiple addresses present
which match but differ by prefix length?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

* Herbert Xu <E1D78DN-0002te-00@gondolin.me.apana.org.au> 2005-03-04 19:31
> Thomas Graf <tgraf@suug.ch> wrote:
> > The deletion of local addresses via netlink doesn't take the
> > prefix length into account resulting in the deletion of
> > the address that was added first given multiple addresses
> > exist only varying in the prefix length.
> 
> This has the potential of breaking user-space scripts.  For example,
> this won't work anymore:
> 
> ip a a dev eth0 192.168.0.1/24
> ip a d dev eth0 192.168.0.1

Yes I know.

> 
> > tgr:axs ~ ip -4 addr show dev lo
> > 1: lo: <LOOPBACK,UP> mtu 16436 qdisc noqueue 
> >    inet 127.0.0.1/8 scope host lo
> >    inet 1.1.1.1/1 scope global lo
> >    inet 1.1.1.1/2 scope global lo
> 
> Do we really need to handle this case? If we do then would it be better
> to consider ifa_prefixlen only when there are multiple addresses present
> which match but differ by prefix length?

I do not agree but I might have a better idea. Let's change iproute2
to provide a prefixlength of 0 if no prefix was specified and only
compare the prefixes if it is non zero. This allows for accurate
deletion, no scripts will break (except for really really broken ones).
Given there are multiple matching addresses only varying in prefix
length and no prefix was specified the first one will get deleted but
this is well defined.

--- linux-2.6.11.orig/net/ipv4/devinet.c	2005-03-04 14:08:14.000000000 +0100
+++ linux-2.6.11/net/ipv4/devinet.c	2005-03-04 14:06:33.000000000 +0100
@@ -396,8 +396,10 @@
 	for (ifap = &in_dev->ifa_list; (ifa = *ifap) != NULL;
 	     ifap = &ifa->ifa_next) {
 		if ((rta[IFA_LOCAL - 1] &&
+		     ((ifm->ifa_prefixlen &&
+		      ifm->ifa_prefixlen != ifa->ifa_prefixlen) ||
 		     memcmp(RTA_DATA(rta[IFA_LOCAL - 1]),
-			    &ifa->ifa_local, 4)) ||
+			    &ifa->ifa_local, 4))) ||
 		    (rta[IFA_LABEL - 1] &&
 		     rtattr_strcmp(rta[IFA_LABEL - 1], ifa->ifa_label)) ||
 		    (rta[IFA_ADDRESS - 1] &&

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

On Fri, Mar 04, 2005 at 02:14:19PM +0100, Thomas Graf wrote:
> 
> I do not agree but I might have a better idea. Let's change iproute2
> to provide a prefixlength of 0 if no prefix was specified and only
> compare the prefixes if it is non zero. This allows for accurate
> deletion, no scripts will break (except for really really broken ones).
> Given there are multiple matching addresses only varying in prefix
> length and no prefix was specified the first one will get deleted but
> this is well defined.

Yep this is a good solution.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

On Sat, Mar 05, 2005 at 07:40:00AM +1100, herbert wrote:
> On Fri, Mar 04, 2005 at 02:14:19PM +0100, Thomas Graf wrote:
> > 
> > I do not agree but I might have a better idea. Let's change iproute2
> > to provide a prefixlength of 0 if no prefix was specified and only
> > compare the prefixes if it is non zero. This allows for accurate
> > deletion, no scripts will break (except for really really broken ones).
> > Given there are multiple matching addresses only varying in prefix
> > length and no prefix was specified the first one will get deleted but
> > this is well defined.
> 
> Yep this is a good solution.

There is one hitch though.  iproute2 probably uses the same function to
parse the address used in adding as well as deleting addresses.  So you'll
need to be careful to only do this for the case of deleting.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

On Fri, Mar 04, 2005 at 02:14:19PM +0100, Thomas Graf wrote:
> 
> I do not agree but I might have a better idea. Let's change iproute2
> to provide a prefixlength of 0 if no prefix was specified and only
> compare the prefixes if it is non zero. This allows for accurate

A bigger problem with this approach is that we don't have a magical
way of getting people to upgrade their ip(8) binary.

To do this safely, we'll need a way of indicating that the ip(8) binary
is setting the prefixlen in the way you propose.  Perhaps this can be
done using a new IFA payload type.

Alternatively you can use the value of /32 to indicate a wildcard match
instead of /0.  After all, /0 has a specific meaning in this context so
it's just as arbitrary to choose /0 as opposed to /32.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

* Herbert Xu <20050304233212.GA27421@gondor.apana.org.au> 2005-03-05 10:32
> On Fri, Mar 04, 2005 at 02:14:19PM +0100, Thomas Graf wrote:
> > 
> > I do not agree but I might have a better idea. Let's change iproute2
> > to provide a prefixlength of 0 if no prefix was specified and only
> > compare the prefixes if it is non zero. This allows for accurate
> 
> A bigger problem with this approach is that we don't have a magical
> way of getting people to upgrade their ip(8) binary.
> 
> To do this safely, we'll need a way of indicating that the ip(8) binary
> is setting the prefixlen in the way you propose.  Perhaps this can be
> done using a new IFA payload type.
> 
> Alternatively you can use the value of /32 to indicate a wildcard match
> instead of /0.  After all, /0 has a specific meaning in this context so
> it's just as arbitrary to choose /0 as opposed to /32.

I've been thinking about this since yesterday and the best solution I
came up so far is to encode it in one of the yet unused bits in
the prefixlength field. After all we're only using 5bits of it. What
i'm thinking of in particular is to encode it as in 1 bit wildcard
flag 5 bits prefix length.

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

> I've been thinking about this since yesterday and the best solution I
> came up so far is to encode it in one of the yet unused bits in
> the prefixlength field. After all we're only using 5bits of it. What
> i'm thinking of in particular is to encode it as in 1 bit wildcard
> flag 5 bits prefix length.

6 bits i meant of course, silly me. ;->

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Patrick McHardy]

Thomas Graf wrote:
> I've been thinking about this since yesterday and the best solution I
> came up so far is to encode it in one of the yet unused bits in
> the prefixlength field. After all we're only using 5bits of it. What
> i'm thinking of in particular is to encode it as in 1 bit wildcard
> flag 5 bits prefix length.

I think that would be ok. Unrelated to this: for IFA_ADDRESS we don't
do an exact match, perhaps we should also do this for IFA_LOCAL for
consistency.

Regards
Patrick

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

On Sat, Mar 05, 2005 at 01:46:42AM +0100, Patrick McHardy wrote:
> 
> I think that would be ok. Unrelated to this: for IFA_ADDRESS we don't
> do an exact match, perhaps we should also do this for IFA_LOCAL for
> consistency.

You mean that we do do an exact match for IFA_ADDRESS? Yes that's
exactly what Thomas is trying to fix for IFA_LOCAL.  The only problem
is that people may have come to depend on the "wildcard" match behaviour
of IFA_LOCAL, i.e.:

ip a a dev eth0 192.168.0.1/24
ip a d dev eth0 192.168.0.1

His proposal of encoding a wildcard bit in IFA_LOCAL's prefix length
should solve the problem.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Mar 05, 2005 at 01:46:42AM +0100, Patrick McHardy wrote:
> 
>>I think that would be ok. Unrelated to this: for IFA_ADDRESS we don't
>>do an exact match, perhaps we should also do this for IFA_LOCAL for
>>consistency.
> 
> 
> You mean that we do do an exact match for IFA_ADDRESS?

No, I meant that IFA_ADDRESS matches on exact prefixlen, but uses
inet_ifa_match() for comparing the addresses. In any case we should
keep the behaviour that no given prefix is a wildcard, but if a
prefix is given we could do something similar as for IFA_ADDRESS.

Regards
Patrick

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

* Patrick McHardy <42290738.6050605@trash.net> 2005-03-05 02:11
> Herbert Xu wrote:
> >On Sat, Mar 05, 2005 at 01:46:42AM +0100, Patrick McHardy wrote:
> >
> >>I think that would be ok. Unrelated to this: for IFA_ADDRESS we don't
> >>do an exact match, perhaps we should also do this for IFA_LOCAL for
> >>consistency.
> >
> >
> >You mean that we do do an exact match for IFA_ADDRESS?
> 
> No, I meant that IFA_ADDRESS matches on exact prefixlen, but uses
> inet_ifa_match() for comparing the addresses. In any case we should
> keep the behaviour that no given prefix is a wildcard, but if a
> prefix is given we could do something similar as for IFA_ADDRESS.

This will change the behaviour and makes my work completely useless.

Assuming one adds 1.1.1.1/24, 1.1.1.2/24 and then deletes 1.1.1.2/24
one would expect 1.1.1.2/24 to be deleted but that wouldn't be the
case.

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Patrick McHardy]

Thomas Graf wrote:
> This will change the behaviour and makes my work completely useless.
> 
> Assuming one adds 1.1.1.1/24, 1.1.1.2/24 and then deletes 1.1.1.2/24
> one would expect 1.1.1.2/24 to be deleted but that wouldn't be the
> case.

True, that would be pretty stupid :)

Regards
Patrick

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

On Sat, Mar 05, 2005 at 01:29:10AM +0100, Thomas Graf wrote:
> 
> I've been thinking about this since yesterday and the best solution I
> came up so far is to encode it in one of the yet unused bits in
> the prefixlength field. After all we're only using 5bits of it. What
> i'm thinking of in particular is to encode it as in 1 bit wildcard
> flag 5 bits prefix length.

That's sound fine as long as we treat the current ip(8) prefix length
as a wild card.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

* Herbert Xu <20050305005911.GA27804@gondor.apana.org.au> 2005-03-05 11:59
> On Sat, Mar 05, 2005 at 01:29:10AM +0100, Thomas Graf wrote:
> > 
> > I've been thinking about this since yesterday and the best solution I
> > came up so far is to encode it in one of the yet unused bits in
> > the prefixlength field. After all we're only using 5bits of it. What
> > i'm thinking of in particular is to encode it as in 1 bit wildcard
> > flag 5 bits prefix length.
> 
> That's sound fine as long as we treat the current ip(8) prefix length
> as a wild card.

Although we should keep the behaviour of ip a a 1.1.1.1/24; ip a d
1.1.1.1, what about ip a a 1.1.1.1/24; ip a d 1.1.1.1/16? I think the
latter should not result in a deletion. We could achieve this by checking
the prefix length if either the exact-match flag is set or the 
prefixlength is != 32. This is of course quite minor and only affects
old iproute2 versions in combination with newer kernels.

Thougts?

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Herbert Xu]

On Sat, Mar 05, 2005 at 05:23:23PM +0100, Thomas Graf wrote:
> 
> Although we should keep the behaviour of ip a a 1.1.1.1/24; ip a d
> 1.1.1.1, what about ip a a 1.1.1.1/24; ip a d 1.1.1.1/16? I think the
> latter should not result in a deletion. We could achieve this by checking
> the prefix length if either the exact-match flag is set or the 
> prefixlength is != 32. This is of course quite minor and only affects
> old iproute2 versions in combination with newer kernels.

It's fine by me as long as the code doesn't get too complex because of it.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] [NET]: Fix deletion of equal local IPv4 addresses only varying in prefix length

[Thomas Graf]

The deletion of equal local IPv4 addresses only varying in prefix length
via netlink currently results in the deletion of the address that was added
first independently of the prefix length.

The fact that parts of the userspace, especially scripts, rely on the fact
that specifying no prefix has the meaning of a wildcard makes this fix non
trivial. In order to not break compatibilty the prefix length is only compared
if userspace explicitely requests an exact match by setting a flag or if
the prefix length is not 32 which means that it was specified by the user.

Assuming the addresses 1.1.1.1/1, 1.1.1.1/32, and 1.1.1.1/2 are added in
the given order the new behaviour looks as follows:

Deletion of            Unmodified userspace           Modified userspace
 1.1.1.1/2                   1.1.1.1/2                    1.1.1.1/2
 1.1.1.1                     1.1.1.1/1                    1.1.1.1/1
 1.1.1.1/32                  1.1.1.1/1                    1.1.1.1/32

While the old kernel behaviour would have deleted 1.1.1.1/1 in all
three cases.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

diff -Nru linux-2.6.11-bk3.orig/include/linux/inetdevice.h linux-2.6.11-bk3/include/linux/inetdevice.h
--- linux-2.6.11-bk3.orig/include/linux/inetdevice.h	2005-03-08 13:10:37.000000000 +0100
+++ linux-2.6.11-bk3/include/linux/inetdevice.h	2005-03-08 16:29:27.000000000 +0100
@@ -7,6 +7,7 @@
 #include <linux/netdevice.h>
 #include <linux/rcupdate.h>
 #include <linux/timer.h>
+#include <linux/rtnetlink.h>
 
 struct ipv4_devconf
 {
@@ -131,6 +132,25 @@
 	return 0;
 }
 
+static inline int inet_ifa_match_local_prefixlen(struct ifaddrmsg *ifm,
+						 struct in_ifaddr *ifa)
+{
+	int real_prefixlen = IFA_REAL_DEL_PREFIX(ifm->ifa_prefixlen);
+
+	/*
+	 * Since the prefix length hasn't been taken into account in
+	 * previous kernel versions, parts of the userspace rely on the fact
+	 * that the deletion of an address without specifying a prefix works.
+	 * We cannot break this and thus a prefix length of 32 still represents
+	 * a wildcard if no exact match is requested.
+	 */
+	if (real_prefixlen != 32 || ifm->ifa_prefixlen & IFA_PREFIX_EXACT_DEL)
+		if (real_prefixlen != ifa->ifa_prefixlen)
+			return 0;
+
+	return 1;
+}
+
 #define for_primary_ifa(in_dev)	{ struct in_ifaddr *ifa; \
   for (ifa = (in_dev)->ifa_list; ifa && !(ifa->ifa_flags&IFA_F_SECONDARY); ifa = ifa->ifa_next)
 
diff -Nru linux-2.6.11-bk3.orig/include/linux/rtnetlink.h linux-2.6.11-bk3/include/linux/rtnetlink.h
--- linux-2.6.11-bk3.orig/include/linux/rtnetlink.h	2005-03-08 16:28:04.000000000 +0100
+++ linux-2.6.11-bk3/include/linux/rtnetlink.h	2005-03-08 14:17:49.000000000 +0100
@@ -396,6 +396,19 @@
 
 #define IFA_MAX (__IFA_MAX - 1)
 
+/*
+ * Quirk for IPv4 address deletion to allow exact deletion of equal
+ * addresses varying only in prefix length. A explicit exact comparison
+ * of the prefix length will only be done if IFA_PREFIX_EXACT_DEL is
+ * ORed to ifa_prefixlen.
+ *
+ * Note: This special treatment is only understood while deleting
+ *       addresses and will lead to unexpected behaviour if used
+ *       otherwise.
+ */
+#define IFA_PREFIX_EXACT_DEL	0x40
+#define IFA_REAL_DEL_PREFIX(l)	((l) & 0x3f)
+
 /* ifa_flags */
 
 #define IFA_F_SECONDARY		0x01
diff -Nru linux-2.6.11-bk3.orig/net/ipv4/devinet.c linux-2.6.11-bk3/net/ipv4/devinet.c
--- linux-2.6.11-bk3.orig/net/ipv4/devinet.c	2005-03-08 13:10:44.000000000 +0100
+++ linux-2.6.11-bk3/net/ipv4/devinet.c	2005-03-08 16:29:49.000000000 +0100
@@ -389,6 +389,7 @@
 	struct in_device *in_dev;
 	struct ifaddrmsg *ifm = NLMSG_DATA(nlh);
 	struct in_ifaddr *ifa, **ifap;
+	int real_prefixlen = IFA_REAL_DEL_PREFIX(ifm->ifa_prefixlen);
 
 	ASSERT_RTNL();
 
@@ -399,12 +400,13 @@
 	for (ifap = &in_dev->ifa_list; (ifa = *ifap) != NULL;
 	     ifap = &ifa->ifa_next) {
 		if ((rta[IFA_LOCAL - 1] &&
+		    (!inet_ifa_match_local_prefixlen(ifm, ifa) ||
 		     memcmp(RTA_DATA(rta[IFA_LOCAL - 1]),
-			    &ifa->ifa_local, 4)) ||
+			    &ifa->ifa_local, 4))) ||
 		    (rta[IFA_LABEL - 1] &&
 		     rtattr_strcmp(rta[IFA_LABEL - 1], ifa->ifa_label)) ||
 		    (rta[IFA_ADDRESS - 1] &&
-		     (ifm->ifa_prefixlen != ifa->ifa_prefixlen ||
+		     (real_prefixlen != ifa->ifa_prefixlen ||
 		      !inet_ifa_match(*(u32*)RTA_DATA(rta[IFA_ADDRESS - 1]),
 			      	      ifa))))
 			continue;

Re: [PATCH] [NET]: Fix deletion of local addresses only varying in prefix length

[Thomas Graf]

* Herbert Xu <E1D78DN-0002te-00@gondolin.me.apana.org.au> 2005-03-04 19:31
> Do we really need to handle this case? If we do then would it be better
> to consider ifa_prefixlen only when there are multiple addresses present
> which match but differ by prefix length?

One argument that would speak for the original patch is that IPv6
already handles it this way so it would be more consistent.