net: ipv6: Do not fix up linklocal and loopback addresses

PATCH In review Mike Manning 3 participants 9 messages Apr 16, 2016 – Apr 18, 2016 1 message not in the archive

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

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

Mike Manning

author

2016-04-16 03:13 UTC #1 lore raw

PATCH 1/1 net: ipv6: Do not fix up linklocal and loopback addresses

f1705ec197e7 added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses are also retained which is not part of the objective of f1705ec197e7. Add check so that these addresses are not fixed up, given that a related fix 70af921db6f8 ensures that they are not kept in the first place, otherwise this incorrect fixup triggers a crash in fib6.

Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")

Signed-off-by Mike Manning <[email protected]>

---
 net/ipv6/addrconf.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

net/ipv6/addrconf.c +7 -6

index 23cec53..cba4e10 100644
@@ -3200,6 +3200,12 @@ static void l3mdev_check_host_rt(struct inet6_dev *idev,
 }
 #endif
 
+static bool addr_is_local(const struct in6_addr *addr)
+{
+	return ipv6_addr_type(addr) &
+		(IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
+}
+
 static int fixup_permanent_addr(struct inet6_dev *idev,
 				struct inet6_ifaddr *ifp)
 {

@@ -3238,6 +3244,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
 
 	list_for_each_entry_safe(ifp, tmp, &idev->addr_list, if_list) {
 		if ((ifp->flags & IFA_F_PERMANENT) &&
+		    !addr_is_local(&ifp->addr) &&
 		    fixup_permanent_addr(idev, ifp) < 0) {
 			write_unlock_bh(&idev->lock);
 			ipv6_del_addr(ifp);

@@ -3448,12 +3455,6 @@ static void addrconf_type_change(struct net_device *dev, unsigned long event)
 		ipv6_mc_unmap(idev);
 }
 
-static bool addr_is_local(const struct in6_addr *addr)
-{
-	return ipv6_addr_type(addr) &
-		(IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
-}
-
 static int addrconf_ifdown(struct net_device *dev, int how)
 {
 	struct net *net = dev_net(dev);

Signature · 1 line

1.7.10.4

Sergei Shtylyov

2016-04-16 14:31 UTC #2 lore raw

Hello.

Quoted 7 lines · Mike Manning

f1705ec197e7 added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses are also retained which is not part of the objective of f1705ec197e7. Add check so that these addresses are not fixed up, given that a related fix 70af921db6f8 ensures that they are not kept in

scripts/checkpatch.pl now enforces commit citing certain style: <12-digit
SHA1> ("<comit summary>").

Quoted 4 lines

the first place, otherwise this incorrect fixup triggers a crash in fib6.
Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by Mike Manning <[email protected]>

[...]

MBR, Sergei

David Ahern

Acked-by on v1 1/1

2016-04-16 18:26 UTC #3 lore raw

In reply to Mike Manning

Quoted 8 lines · Mike Manning

f1705ec197e7 added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses are also retained which is not part of the objective of f1705ec197e7. Add check so that these addresses are not fixed up, given that a related fix 70af921db6f8 ensures that they are not kept in the first place, otherwise this incorrect fixup triggers a crash in fib6.

oops in fib6_del?

Quoted 4 lines

Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by Mike Manning <[email protected]>
---

for the change

Acked-by David Ahern <[email protected]>

Mike Manning

author

2016-04-17 18:14 UTC #4 lore raw

Quoted 10 lines · David Ahern

Quoted 8 lines · Mike Manning
f1705ec197e7 added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses are also retained which is not part of the objective of f1705ec197e7. Add check so that these addresses are not fixed up, given that a related fix 70af921db6f8 ensures that they are not kept in the first place, otherwise this incorrect fixup triggers a crash in fib6.

Quoted 2 lines

oops in fib6_del?

[  285.734883]  [<ffffffff8125ad15>] ? dump_stack+0x5c/0x77
[  285.734887]  [<ffffffff8105d907>] ? warn_slowpath_common+0x77/0xb0
[  285.734889]  [<ffffffff814f3e40>] ? fib6_clean_tohost+0x50/0x50
[  285.734891]  [<ffffffff814fbe3e>] ? fib6_del+0x22e/0x290
[  285.734893]  [<ffffffff814fbf22>] ? fib6_clean_node+0x82/0x120
[  285.734895]  [<ffffffff814f9a0d>] ? fib6_walk_continue+0x16d/0x1a0
[  285.734897]  [<ffffffff814f9ad4>] ? fib6_walk+0x54/0x80
[  285.734899]  [<ffffffff814f9b42>] ? fib6_clean_tree+0x42/0x50
[  285.734901]  [<ffffffff814fbea0>] ? fib6_del+0x290/0x290
[  285.734902]  [<ffffffff814f3e40>] ? fib6_clean_tohost+0x50/0x50
[  285.734904]  [<ffffffff814f9ba8>] ? __fib6_clean_all+0x58/0x90
[  285.734906]  [<ffffffff814f9326>] ? rt6_ifdown+0x36/0x1e0
[  285.734908]  [<ffffffff814fbf0f>] ? fib6_clean_node+0x6f/0x120
[  285.734911]  [<ffffffff814eeb6b>] ? addrconf_ifdown+0x3b/0x580
[  285.734913]  [<ffffffff814f9b42>] ? fib6_clean_tree+0x42/0x50
[  285.734914]  [<ffffffff814f1ea0>] ? addrconf_notify+0xf0/0xb10
[  285.734916]  [<ffffffff814f9bb0>] ? __fib6_clean_all+0x60/0x90

Quoted 7 lines

Quoted 3 lines
Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by Mike Manning <[email protected]>
---
for the change
Acked-by David Ahern <[email protected]>

Mike Manning

author

2016-04-18 14:23 UTC #5 lore raw

PATCH 1/1 net: ipv6: Do not fix up linklocal and loopback addresses

In reply to Mike Manning

f1705ec197e7 "Make address flushing on ifdown optional" added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses were also retained which was not part of the objective of the original changes. The fix 70af921db6f8 "Do not keep linklocal and loopback addresses" ensures that these are no longer kept. Similarly, the present fix ensures that these addresses are not fixed up either, otherwise the incorrect fixup triggers a crash in fib6.

Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")

Signed-off-by Mike Manning <[email protected]>

---
 net/ipv6/addrconf.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

net/ipv6/addrconf.c +7 -6

index 23cec53..cba4e10 100644
@@ -3200,6 +3200,12 @@ static void l3mdev_check_host_rt(struct inet6_dev *idev,
 }
 #endif
 
+static bool addr_is_local(const struct in6_addr *addr)
+{
+	return ipv6_addr_type(addr) &
+		(IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
+}
+
 static int fixup_permanent_addr(struct inet6_dev *idev,
 				struct inet6_ifaddr *ifp)
 {

@@ -3238,6 +3244,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
 
 	list_for_each_entry_safe(ifp, tmp, &idev->addr_list, if_list) {
 		if ((ifp->flags & IFA_F_PERMANENT) &&
+		    !addr_is_local(&ifp->addr) &&
 		    fixup_permanent_addr(idev, ifp) < 0) {
 			write_unlock_bh(&idev->lock);
 			ipv6_del_addr(ifp);

@@ -3448,12 +3455,6 @@ static void addrconf_type_change(struct net_device *dev, unsigned long event)
 		ipv6_mc_unmap(idev);
 }
 
-static bool addr_is_local(const struct in6_addr *addr)
-{
-	return ipv6_addr_type(addr) &
-		(IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
-}
-
 static int addrconf_ifdown(struct net_device *dev, int how)
 {
 	struct net *net = dev_net(dev);

Signature · 1 line

1.7.10.4

David Ahern

Acked-by on v1 1/1

2016-04-18 14:39 UTC #6 lore raw

Quoted 10 lines · Mike Manning

f1705ec197e7 "Make address flushing on ifdown optional" added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses were also retained which was not part of the objective of the original changes. The fix 70af921db6f8 "Do not keep linklocal and loopback addresses" ensures that these are no longer kept. Similarly, the present fix ensures that these addresses are not fixed up either, otherwise the incorrect fixup triggers a crash in fib6.

Quoted 5 lines

Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by Mike Manning <[email protected]>
---
  net/ipv6/addrconf.c |   13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)

Is this a v2? Code change is the same.

Acked-by David Ahern <[email protected]>

Mike Manning

author

2016-04-18 15:14 UTC #7 lore raw

Quoted 11 lines · David Ahern

Quoted 10 lines · Mike Manning
f1705ec197e7 "Make address flushing on ifdown optional" added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses were also retained which was not part of the objective of the original changes. The fix 70af921db6f8 "Do not keep linklocal and loopback addresses" ensures that these are no longer kept. Similarly, the present fix ensures that these addresses are not fixed up either, otherwise the incorrect fixup triggers a crash in fib6.

Quoted 7 lines

Quoted 5 lines
Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by Mike Manning <[email protected]>
---
  net/ipv6/addrconf.c |   13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)
Is this a v2? Code change is the same.

This is the same changeset, but Sergei requested on Saturday to change the wording as follows:

scripts/checkpatch.pl now enforces commit citing certain style: <12-digit

SHA1> ("<comit summary>").

Quoted 2 lines

Acked-by David Ahern <[email protected]>

Sergei Shtylyov

2016-04-18 15:20 UTC #8 lore raw

In reply to Mike Manning

Hello.

Quoted 1 line · Mike Manning

f1705ec197e7 "Make address flushing on ifdown optional" added the option

Looking further, this doesn't seem like a complete commit summary. And you
need to enclose it in parens too.

Quoted 6 lines

to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses were also retained which was not part of the objective of the original changes. The fix 70af921db6f8 "Do not keep linklocal and loopback addresses" ensures that these are no

Again, () are needed around the summary.

Quoted 5 lines

longer kept. Similarly, the present fix ensures that these addresses are not fixed up either, otherwise the incorrect fixup triggers a crash in fib6.
Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")
Signed-off-by Mike Manning <[email protected]>

[...]

MBR, Sergei

Mike Manning

author

2016-04-18 15:28 UTC #9 lore raw

PATCH 1/1 net: ipv6: Do not fix up linklocal and loopback addresses

In reply to Mike Manning

f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional") added the option to retain user configured addresses on an admin down. A comment to one of the later revisions suggested using the IFA_F_PERMANENT flag rather than adding a user_managed boolean to the ifaddr struct. A side effect of this change is that link local and loopback addresses were also retained which was not part of the objective of the original changes. The fix 70af921db6f8 ("net: ipv6: Do not keep linklocal and loopback addresses") ensures that these are no longer kept. Similarly, the present fix ensures that these addresses are not fixed up, otherwise the incorrect fixup triggers a crash in fib6.

Fixes f1705ec197e7 ("net: ipv6: Make address flushing on ifdown optional")

Signed-off-by Mike Manning <[email protected]>

---
 net/ipv6/addrconf.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

net/ipv6/addrconf.c +7 -6

index 23cec53..cba4e10 100644
@@ -3200,6 +3200,12 @@ static void l3mdev_check_host_rt(struct inet6_dev *idev,
 }
 #endif
 
+static bool addr_is_local(const struct in6_addr *addr)
+{
+	return ipv6_addr_type(addr) &
+		(IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
+}
+
 static int fixup_permanent_addr(struct inet6_dev *idev,
 				struct inet6_ifaddr *ifp)
 {

@@ -3238,6 +3244,7 @@ static void addrconf_permanent_addr(struct net_device *dev)
 
 	list_for_each_entry_safe(ifp, tmp, &idev->addr_list, if_list) {
 		if ((ifp->flags & IFA_F_PERMANENT) &&
+		    !addr_is_local(&ifp->addr) &&
 		    fixup_permanent_addr(idev, ifp) < 0) {
 			write_unlock_bh(&idev->lock);
 			ipv6_del_addr(ifp);

@@ -3448,12 +3455,6 @@ static void addrconf_type_change(struct net_device *dev, unsigned long event)
 		ipv6_mc_unmap(idev);
 }
 
-static bool addr_is_local(const struct in6_addr *addr)
-{
-	return ipv6_addr_type(addr) &
-		(IPV6_ADDR_LINKLOCAL | IPV6_ADDR_LOOPBACK);
-}
-
 static int addrconf_ifdown(struct net_device *dev, int how)
 {
 	struct net *net = dev_net(dev);

Signature · 1 line

1.7.10.4