Hi,
When adding an IPv6 address to a given interface, I'm allowed to
add that address multiple time, e.g.:
[root@abulafia root]# ip addr add 3ffe:4242:4242::1 dev eth0
[root@abulafia root]# ip addr add 3ffe:4242:4242::1 dev eth0
[root@abulafia root]# ip addr add 3ffe:4242:4242::1 dev eth0
[root@abulafia root]# ip addr show dev eth0
2: eth0: <BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast qlen 100
link/ether 00:48:54:1b:25:30 brd ff:ff:ff:ff:ff:ff
inet6 3ffe:4242:4242::1/128 scope global
inet6 3ffe:4242:4242::1/128 scope global
inet6 3ffe:4242:4242::1/128 scope global
inet6 fe80::248:54ff:fe1b:2530/10 scope link
Apparently, this is not a stability problem, because I'm allowed to
delete 3 times that address before receving a "not found" error,
but there's no reason to allow multiple instances of the same address
on the same interface, so this is a bug nonetheless.
Bug is confirmed on:
- 2.4.20
- 2.5.66
- latest -usagi
Following is a patch attempting to fix this bug.
It's for 2.4.20 but sould apply cleanly on 2.5 too.
Credits:
Chad N. Tindel - discovered the bug and showed it to me
Peter Bieringer - confirmed it's a bug
Mauro Tortonesi - suggested sending a patch to this list.
Regards,
Simone Piunno
--- net/ipv6/addrconf.c.orig 2003-03-25 21:33:55.000000000 +0100
+++ net/ipv6/addrconf.c 2003-03-30 13:48:23.000000000 +0200
@@ -89,6 +89,8 @@
static struct inet6_ifaddr *inet6_addr_lst[IN6_ADDR_HSIZE];
static rwlock_t addrconf_hash_lock = RW_LOCK_UNLOCKED;
+static spinlock_t addrconf_add_lock = SPIN_LOCK_UNLOCKED;
+
/* Protects inet6 devices */
rwlock_t addrconf_lock = RW_LOCK_UNLOCKED;
@@ -621,6 +623,24 @@
return ifp != NULL;
}
+static struct inet6_ifaddr *
+ipv6_addr_already_present(struct in6_addr *addr, struct net_device *dev)
+{
+ struct inet6_ifaddr *ifp;
+ u8 hash = ipv6_addr_hash(addr);
+
+ read_lock_bh(&addrconf_hash_lock);
+ for (ifp = inet6_addr_lst[hash]; ifp; ifp = ifp->lst_next) {
+ if (ipv6_addr_cmp(&ifp->addr, addr) == 0 && ifp->idev->dev == dev) {
+ read_unlock_bh(&addrconf_hash_lock);
+ return ifp;
+ }
+ }
+ read_unlock_bh(&addrconf_hash_lock);
+ return NULL;
+}
+
+
struct inet6_ifaddr * ipv6_get_ifaddr(struct in6_addr *addr, struct net_device *dev)
{
struct inet6_ifaddr * ifp;
@@ -908,7 +928,7 @@
return;
ok:
-
+ spin_lock_bh(&addrconf_add_lock);
ifp = ipv6_get_ifaddr(&addr, dev);
if (ifp == NULL && valid_lft) {
@@ -920,12 +940,14 @@
addr_type&IPV6_ADDR_SCOPE_MASK, 0);
if (ifp == NULL) {
+ spin_unlock_bh(&addrconf_add_lock);
in6_dev_put(in6_dev);
return;
}
addrconf_dad_start(ifp);
}
+ spin_unlock_bh(&addrconf_add_lock);
if (ifp && valid_lft == 0) {
ipv6_del_addr(ifp);
@@ -1033,11 +1055,19 @@
scope = ipv6_addr_scope(pfx);
+ spin_lock_bh(&addrconf_add_lock);
+ if (ipv6_addr_already_present(pfx, dev)) {
+ spin_unlock_bh(&addrconf_add_lock);
+ return -EEXIST;
+ }
+
if ((ifp = ipv6_add_addr(idev, pfx, plen, scope, IFA_F_PERMANENT)) != NULL) {
+ spin_unlock_bh(&addrconf_add_lock);
addrconf_dad_start(ifp);
in6_ifa_put(ifp);
return 0;
}
+ spin_unlock_bh(&addrconf_add_lock);
return -ENOBUFS;
}
--
Simone Piunno -- http://members.ferrara.linux.it/pioppo
.------- Adde parvum parvo magnus acervus erit -------.
Ferrara Linux Users Group - http://www.ferrara.linux.it
Deep Space 6, IPv6 on Linux - http://www.deepspace6.net
GNU Mailman, Mailing List Manager - http://www.list.org
`-------------------------------------------------------'
In article <[email protected]> (at Sun, 30 Mar 2003 14:27:05 +0200), Simone Piunno <[email protected]> says:
> When adding an IPv6 address to a given interface, I'm allowed to
> add that address multiple time, e.g.:
>
> [root@abulafia root]# ip addr add 3ffe:4242:4242::1 dev eth0
> [root@abulafia root]# ip addr add 3ffe:4242:4242::1 dev eth0
> [root@abulafia root]# ip addr add 3ffe:4242:4242::1 dev eth0
> [root@abulafia root]# ip addr show dev eth0
> 2: eth0: <BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast qlen 100
> link/ether 00:48:54:1b:25:30 brd ff:ff:ff:ff:ff:ff
> inet6 3ffe:4242:4242::1/128 scope global
> inet6 3ffe:4242:4242::1/128 scope global
> inet6 3ffe:4242:4242::1/128 scope global
> inet6 fe80::248:54ff:fe1b:2530/10 scope link
>
> Apparently, this is not a stability problem, because I'm allowed to
> delete 3 times that address before receving a "not found" error,
> but there's no reason to allow multiple instances of the same address
> on the same interface, so this is a bug nonetheless.
>
> Bug is confirmed on:
> - 2.4.20
> - 2.5.66
> - latest -usagi
usagi code does not act like that.
In my environment,
# ip addr add 3ffe:4242:4242::1 dev eth0
# ip addr add 3ffe:4242:4242::1 dev eth0
RTNETLINK answers: No buffer space available
# ip addr add 3ffe:4242:4242::1 dev eth0
RTNETLINK answers: No buffer space available
And, patch does not seem optimal. I'd take a look at very soon.
--
Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
In article <[email protected]> (at Sun, 30 Mar 2003 22:08:29 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:
> And, patch does not seem optimal. I'd take a look at very soon.
Here's our patch based on our fix in August, 2001.
Question: should we use spin_lock_bh() instead of spin_lock()?
--------
Don't assign a same IPv6 address on a same interface.
This patch is against linux-2.5.66.
We believe this fix should be suitable on linux-2.4 tree.
(This patch itself conflicts at the first chunk...)
Thanks in advance.
-------------------------------------------------------------------
Patch-Name: Don't assign a same IPv6 address on a same interface
Patch-Id: FIX_2_5_66_ADDRCONF_DUPADDR-20030330
Patch-Author: YOSHIFUJI Hideaki / USAGI Project <[email protected]>
Credit: Yuji SEKIYA / USAGI Project <[email protected]>,
YOSHIFUJI Hideaki / USAGI Project <[email protected]>,
Simone Piunno <[email protected]>
-------------------------------------------------------------------
Index: net/ipv6/addrconf.c
===================================================================
RCS file: /cvsroot/usagi/usagi-backport/linux25/net/ipv6/addrconf.c,v
retrieving revision 1.1.1.9
retrieving revision 1.1.1.9.2.3
diff -u -r1.1.1.9 -r1.1.1.9.2.3
--- net/ipv6/addrconf.c 25 Mar 2003 04:33:45 -0000 1.1.1.9
+++ net/ipv6/addrconf.c 30 Mar 2003 13:50:41 -0000 1.1.1.9.2.3
@@ -30,6 +30,8 @@
* address validation timer.
* YOSHIFUJI Hideaki @USAGI : Privacy Extensions (RFC3041)
* support.
+ * Yuji SEKIYA @USAGI : Don't assign a same IPv6
+ * address on a same interface.
*/
#include <linux/config.h>
@@ -126,6 +128,8 @@
static void addrconf_rs_timer(unsigned long data);
static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
+static int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev);
+
static struct notifier_block *inet6addr_chain;
struct ipv6_devconf ipv6_devconf =
@@ -492,10 +496,21 @@
{
struct inet6_ifaddr *ifa;
int hash;
+ static spinlock_t lock = SPIN_LOCK_UNLOCKED;
+
+ spin_lock(&lock);
+
+ /* Ignore adding duplicate addresses on an interface */
+ if (ipv6_chk_same_addr(addr, idev->dev)) {
+ spin_unlock(&lock);
+ ADBG(("ipv6_add_addr: already assigned\n"));
+ return NULL;
+ }
ifa = kmalloc(sizeof(struct inet6_ifaddr), GFP_ATOMIC);
if (ifa == NULL) {
+ spin_unlock(&lock);
ADBG(("ipv6_add_addr: malloc failed\n"));
return NULL;
}
@@ -514,6 +529,7 @@
if (idev->dead) {
read_unlock(&addrconf_lock);
kfree(ifa);
+ spin_unlock(&lock);
return NULL;
}
@@ -551,6 +567,7 @@
in6_ifa_hold(ifa);
write_unlock_bh(&idev->lock);
read_unlock(&addrconf_lock);
+ spin_unlock(&lock);
notifier_call_chain(&inet6addr_chain,NETDEV_UP,ifa);
@@ -921,6 +938,23 @@
!(ifp->flags&IFA_F_TENTATIVE)) {
if (dev == NULL || ifp->idev->dev == dev ||
!(ifp->scope&(IFA_LINK|IFA_HOST)))
+ break;
+ }
+ }
+ read_unlock_bh(&addrconf_hash_lock);
+ return ifp != NULL;
+}
+
+static
+int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev)
+{
+ struct inet6_ifaddr * ifp;
+ u8 hash = ipv6_addr_hash(addr);
+
+ read_lock_bh(&addrconf_hash_lock);
+ for(ifp = inet6_addr_lst[hash]; ifp; ifp=ifp->lst_next) {
+ if (ipv6_addr_cmp(&ifp->addr, addr) == 0) {
+ if (dev != NULL && ifp->idev->dev == dev)
break;
}
}
--
Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
On Sun, Mar 30, 2003 at 11:58:09PM +0900, YOSHIFUJI Hideaki wrote:
> > And, patch does not seem optimal. I'd take a look at very soon.
>
> Here's our patch based on our fix in August, 2001.
> Question: should we use spin_lock_bh() instead of spin_lock()?
Because everywhere else in the file {read,write}_lock_bh() is used
instead of {read,write}_lock(), so I'm assuming that _bh is required
but I really don't know why.
Anyway I have some critics over your patch:
- locking inside ipv6_add_addr() is simpler and more linear but
semantically wrong because you're unable to tell the user why his
"ip addr add" failed. E.g. you answer ENOBUFS instead of EEXIST.
- your ipv6_chk_same_addr() does a useless check for (dev != NULL)
> +static
> +int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev)
> +{
> + struct inet6_ifaddr * ifp;
> + u8 hash = ipv6_addr_hash(addr);
> +
> + read_lock_bh(&addrconf_hash_lock);
> + for(ifp = inet6_addr_lst[hash]; ifp; ifp=ifp->lst_next) {
> + if (ipv6_addr_cmp(&ifp->addr, addr) == 0) {
> + if (dev != NULL && ifp->idev->dev == dev)
> break;
> }
your never "break" if dev == NULL, so you could return 0 before
even acquiring the lock.
Regards,
Simone
--
Simone Piunno -- http://members.ferrara.linux.it/pioppo
.------- Adde parvum parvo magnus acervus erit -------.
Ferrara Linux Users Group - http://www.ferrara.linux.it
Deep Space 6, IPv6 on Linux - http://www.deepspace6.net
GNU Mailman, Mailing List Manager - http://www.list.org
`-------------------------------------------------------'
In article <[email protected]> (at Sun, 30 Mar 2003 18:36:56 +0200), Simone Piunno <[email protected]> says:
> Because everywhere else in the file {read,write}_lock_bh() is used
> instead of {read,write}_lock(), so I'm assuming that _bh is required
> but I really don't know why.
maybe.
> - locking inside ipv6_add_addr() is simpler and more linear but
> semantically wrong because you're unable to tell the user why his
> "ip addr add" failed. E.g. you answer ENOBUFS instead of EEXIST.
We don't want to create duplicate address in any case.
ipv6_add_addr() IS right place.
And, we can return error code by using IS_ERR() etc.
I'll fix this.
> - your ipv6_chk_same_addr() does a useless check for (dev != NULL)
>
> > +static
> > +int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev)
> > +{
> > + struct inet6_ifaddr * ifp;
> > + u8 hash = ipv6_addr_hash(addr);
> > +
> > + read_lock_bh(&addrconf_hash_lock);
> > + for(ifp = inet6_addr_lst[hash]; ifp; ifp=ifp->lst_next) {
> > + if (ipv6_addr_cmp(&ifp->addr, addr) == 0) {
> > + if (dev != NULL && ifp->idev->dev == dev)
> > break;
> > }
>
> your never "break" if dev == NULL, so you could return 0 before
> even acquiring the lock.
It is not a problem because dev is always non-NULL.
However, it should be dev == NULL || ifp->idev->dev == dev.
Thanks.
(I don't understand what you mean by "you could return 0
before even acquiring the lock.")
--
Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
In article <[email protected]> (at Mon, 31 Mar 2003 03:35:24 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:
> In article <[email protected]> (at Sun, 30 Mar 2003 18:36:56 +0200), Simone Piunno <[email protected]> says:
>
> > - locking inside ipv6_add_addr() is simpler and more linear but
> > semantically wrong because you're unable to tell the user why his
> > "ip addr add" failed. E.g. you answer ENOBUFS instead of EEXIST.
>
> We don't want to create duplicate address in any case.
> ipv6_add_addr() IS right place.
> And, we can return error code by using IS_ERR() etc.
> I'll fix this.
Here's the revised patch.
Thank you.
Index: net/ipv6/addrconf.c
===================================================================
RCS file: /cvsroot/usagi/usagi-backport/linux25/net/ipv6/addrconf.c,v
retrieving revision 1.1.1.9
retrieving revision 1.1.1.9.2.6
diff -u -r1.1.1.9 -r1.1.1.9.2.6
--- net/ipv6/addrconf.c 25 Mar 2003 04:33:45 -0000 1.1.1.9
+++ net/ipv6/addrconf.c 30 Mar 2003 18:51:29 -0000 1.1.1.9.2.6
@@ -30,6 +30,8 @@
* address validation timer.
* YOSHIFUJI Hideaki @USAGI : Privacy Extensions (RFC3041)
* support.
+ * Yuji SEKIYA @USAGI : Don't assign a same IPv6
+ * address on a same interface.
*/
#include <linux/config.h>
@@ -126,6 +128,8 @@
static void addrconf_rs_timer(unsigned long data);
static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifa);
+static int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev);
+
static struct notifier_block *inet6addr_chain;
struct ipv6_devconf ipv6_devconf =
@@ -492,12 +496,23 @@
{
struct inet6_ifaddr *ifa;
int hash;
+ static spinlock_t lock = SPIN_LOCK_UNLOCKED;
+
+ spin_lock_bh(&lock);
+
+ /* Ignore adding duplicate addresses on an interface */
+ if (ipv6_chk_same_addr(addr, idev->dev)) {
+ spin_unlock_bh(&lock);
+ ADBG(("ipv6_add_addr: already assigned\n"));
+ return ERR_PTR(-EEXIST);
+ }
ifa = kmalloc(sizeof(struct inet6_ifaddr), GFP_ATOMIC);
if (ifa == NULL) {
+ spin_unlock_bh(&lock);
ADBG(("ipv6_add_addr: malloc failed\n"));
- return NULL;
+ return ERR_PTR(-ENOBUFS);
}
memset(ifa, 0, sizeof(struct inet6_ifaddr));
@@ -513,8 +528,9 @@
read_lock(&addrconf_lock);
if (idev->dead) {
read_unlock(&addrconf_lock);
+ spin_unlock_bh(&lock);
kfree(ifa);
- return NULL;
+ return ERR_PTR(-ENODEV); /*XXX*/
}
inet6_ifa_count++;
@@ -551,6 +567,7 @@
in6_ifa_hold(ifa);
write_unlock_bh(&idev->lock);
read_unlock(&addrconf_lock);
+ spin_unlock_bh(&lock);
notifier_call_chain(&inet6addr_chain,NETDEV_UP,ifa);
@@ -697,7 +714,7 @@
ift = ipv6_count_addresses(idev) < IPV6_MAX_ADDRESSES ?
ipv6_add_addr(idev, &addr, tmp_plen,
ipv6_addr_type(&addr)&IPV6_ADDR_SCOPE_MASK, IFA_F_TEMPORARY) : 0;
- if (!ift) {
+ if (IS_ERR(ift)) {
in6_dev_put(idev);
in6_ifa_put(ifp);
printk(KERN_INFO
@@ -928,6 +945,23 @@
return ifp != NULL;
}
+static
+int ipv6_chk_same_addr(const struct in6_addr *addr, struct net_device *dev)
+{
+ struct inet6_ifaddr * ifp;
+ u8 hash = ipv6_addr_hash(addr);
+
+ read_lock_bh(&addrconf_hash_lock);
+ for(ifp = inet6_addr_lst[hash]; ifp; ifp=ifp->lst_next) {
+ if (ipv6_addr_cmp(&ifp->addr, addr) == 0) {
+ if (dev == NULL || ifp->idev->dev == dev)
+ break;
+ }
+ }
+ read_unlock_bh(&addrconf_hash_lock);
+ return ifp != NULL;
+}
+
struct inet6_ifaddr * ipv6_get_ifaddr(struct in6_addr *addr, struct net_device *dev)
{
struct inet6_ifaddr * ifp;
@@ -1344,7 +1378,7 @@
ifp = ipv6_add_addr(in6_dev, &addr, pinfo->prefix_len,
addr_type&IPV6_ADDR_SCOPE_MASK, 0);
- if (ifp == NULL) {
+ if (IS_ERR(ifp)) {
in6_dev_put(in6_dev);
return;
}
@@ -1499,13 +1533,14 @@
scope = ipv6_addr_scope(pfx);
- if ((ifp = ipv6_add_addr(idev, pfx, plen, scope, IFA_F_PERMANENT)) != NULL) {
+ ifp = ipv6_add_addr(idev, pfx, plen, scope, IFA_F_PERMANENT);
+ if (!IS_ERR(ifp)) {
addrconf_dad_start(ifp);
in6_ifa_put(ifp);
return 0;
}
- return -ENOBUFS;
+ return PTR_ERR(ifp);
}
static int inet6_addr_del(int ifindex, struct in6_addr *pfx, int plen)
@@ -1597,7 +1632,7 @@
if (addr.s6_addr32[3]) {
ifp = ipv6_add_addr(idev, &addr, 128, scope, IFA_F_PERMANENT);
- if (ifp) {
+ if (!IS_ERR(ifp)) {
spin_lock_bh(&ifp->lock);
ifp->flags &= ~IFA_F_TENTATIVE;
spin_unlock_bh(&ifp->lock);
@@ -1633,7 +1668,7 @@
ifp = ipv6_add_addr(idev, &addr, plen, flag,
IFA_F_PERMANENT);
- if (ifp) {
+ if (!IS_ERR(ifp)) {
spin_lock_bh(&ifp->lock);
ifp->flags &= ~IFA_F_TENTATIVE;
spin_unlock_bh(&ifp->lock);
@@ -1660,7 +1695,7 @@
}
ifp = ipv6_add_addr(idev, &in6addr_loopback, 128, IFA_HOST, IFA_F_PERMANENT);
- if (ifp) {
+ if (!IS_ERR(ifp)) {
spin_lock_bh(&ifp->lock);
ifp->flags &= ~IFA_F_TENTATIVE;
spin_unlock_bh(&ifp->lock);
@@ -1674,7 +1709,7 @@
struct inet6_ifaddr * ifp;
ifp = ipv6_add_addr(idev, addr, 64, IFA_LINK, IFA_F_PERMANENT);
- if (ifp) {
+ if (!IS_ERR(ifp)) {
addrconf_dad_start(ifp);
in6_ifa_put(ifp);
}
--
Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
Hi,
just my 2 cents, I already saw, that newest USAGI snapshot include a fix.
--On Sunday, March 30, 2003 02:27:05 PM +0200 Simone Piunno
<[email protected]> wrote:
> When adding an IPv6 address to a given interface, I'm allowed to
> add that address multiple time, e.g.:
...
I didn't dig into any patch and also not into related drafts/RFCs, but one
scenario should be catched I think - or to be discussed:
Scenario:
Address was already added by autoconfiguration on receiving advertisement
(limited lifetime). Now the same address would be added manually (unlimited
lifetime).
What (should) happen?
Mho: manual add is allowed, both addresses need to be listed.
Peter
--
Dr. Peter Bieringer http://www.bieringer.de/pb/
GPG/PGP Key 0x958F422D mailto: pb at bieringer dot de
Deep Space 6 Co-Founder and Core Member http://www.deepspace6.net/
From: YOSHIFUJI Hideaki / 吉藤英明 <[email protected]>
Date: Mon, 31 Mar 2003 10:34:51 +0900 (JST)
In article <[email protected]> (at Mon, 31 Mar 2003 03:35:24 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:
> In article <[email protected]> (at Sun, 30 Mar 2003 18:36:56 +0200), Simone Piunno <[email protected]> says:
>
> > - locking inside ipv6_add_addr() is simpler and more linear but
> > semantically wrong because you're unable to tell the user why his
> > "ip addr add" failed. E.g. you answer ENOBUFS instead of EEXIST.
>
> We don't want to create duplicate address in any case.
> ipv6_add_addr() IS right place.
> And, we can return error code by using IS_ERR() etc.
> I'll fix this.
Here's the revised patch.
Applied to both 2.4.x and 2.5.x.
BTW, 2.4.x patch failed in two spots, one was author comment
which I easily fixed, second was in privacy code which I did not
apply yet to 2.4.x (I fixed this too, don't worry).
I do not want to put privacy code into 2.4.x until crypto is there.
I plan to put crypto lib into 2.4.22-pre1.
On Mon, Mar 31, 2003 at 08:23:58PM +0200, Peter Bieringer wrote:
> Address was already added by autoconfiguration on receiving advertisement
> (limited lifetime). Now the same address would be added manually (unlimited
> lifetime).
>
> What (should) happen?
>
> Mho: manual add is allowed, both addresses need to be listed.
I'd prefer this variant:
manual add is allowed and overwrites the autoconfigured address.
--
Simone Piunno -- http://members.ferrara.linux.it/pioppo
.------- Adde parvum parvo magnus acervus erit -------.
Ferrara Linux Users Group - http://www.ferrara.linux.it
Deep Space 6, IPv6 on Linux - http://www.deepspace6.net
GNU Mailman, Mailing List Manager - http://www.list.org
`-------------------------------------------------------'