ipv6: Do not iterate over all interfaces when finding source address on specific interface.
Subsystems (3)
PATCH v2 1/1 ipv6: Do not iterate over all interfaces when finding source address on specific interface.
If outgoing interface is specified and the candidate address is restricted to the outgoing interface, it is enough to iterate over that given interface only.
--- net/ipv6/addrconf.c | 197 ++++++++++++++++++++++++++++------------------------ 1 file changed, 107 insertions(+), 90 deletions(-)
net/ipv6/addrconf.c +107 -90
@@ -1358,15 +1358,94 @@ out:return ret; } +static void __ipv6_dev_get_saddr (struct net *net, +struct ipv6_saddr_dst *dst, +unsigned int prefs, +const struct in6_addr *saddr, +struct inet6_dev *idev, +struct ipv6_saddr_score *scores) +{ +struct ipv6_saddr_score *score = &scores[0 ], *hiscore = &scores[1 ]; + +read_lock_bh (&idev->lock); +list_for_each_entry (score->ifa, &idev->addr_list, if_list) { +int i; + +/* +* - Tentative Address (RFC2462 section 5.4) +* - A tentative address is not considered +* "assigned to an interface" in the traditional +* sense, unless it is also flagged as optimistic. +* - Candidate Source Address (section 4) +* - In any case, anycast addresses, multicast +* addresses, and the unspecified address MUST +* NOT be included in a candidate set. +*/ +if ((score->ifa->flags &IFA_F_TENTATIVE ) && + (!(score->ifa->flags &IFA_F_OPTIMISTIC ))) +continue ; + + score->addr_type =__ipv6_addr_type (&score->ifa->addr); + +if (unlikely (score->addr_type ==IPV6_ADDR_ANY || + score->addr_type &IPV6_ADDR_MULTICAST )) { +net_dbg_ratelimited ("ADDRCONF: unspecified / multicast address assigned as unicast address on %s" , + idev->dev->name); +continue ; + } + + score->rule =-1 ; +bitmap_zero (score->scorebits,IPV6_SADDR_RULE_MAX ); + +for (i =0 ; i <IPV6_SADDR_RULE_MAX ; i++) { +int minihiscore, miniscore; + + minihiscore =ipv6_get_saddr_eval (net, hiscore, dst, i); + miniscore =ipv6_get_saddr_eval (net, score, dst, i); + +if (minihiscore > miniscore) { +if (i ==IPV6_SADDR_RULE_SCOPE && + score->scopedist >0 ) { +/* +* special case: +* each remaining entry +* has too small (not enough) +* scope, because ifa entries +* are sorted by their scope +* values. +*/ + goto out; + } +break ; + }else if (minihiscore < miniscore) { +if (hiscore->ifa) +in6_ifa_put (hiscore->ifa); + +in6_ifa_hold (score->ifa); + +swap (hiscore, score); + +/* restore our iterator */ + score->ifa = hiscore->ifa; + +break ; + } + } + } +out: +read_unlock_bh (&idev->lock); +} +int ipv6_dev_get_saddr (struct net *net,const struct net_device *dst_dev,const struct in6_addr *daddr,unsigned int prefs,struct in6_addr *saddr) { -struct ipv6_saddr_score scores[2 ], - *score = &scores[0 ], *hiscore = &scores[1 ]; +struct ipv6_saddr_score scores[2 ], *hiscore = &scores[1 ];struct ipv6_saddr_dst dst; +struct inet6_dev *idev;struct net_device *dev;int dst_type; +bool use_oif_addr = false; dst_type =__ipv6_addr_type (daddr); dst.addr = daddr;
@@ -1380,97 +1459,35 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,rcu_read_lock (); -for_each_netdev_rcu (net, dev) { -struct inet6_dev *idev; - -/* Candidate Source Address (section 4) -* - multicast and link-local destination address, -* the set of candidate source address MUST only -* include addresses assigned to interfaces -* belonging to the same link as the outgoing -* interface. -* (- For site-local destination addresses, the -* set of candidate source addresses MUST only -* include addresses assigned to interfaces -* belonging to the same site as the outgoing -* interface.) -*/ -if (((dst_type &IPV6_ADDR_MULTICAST ) || - dst.scope <=IPV6_ADDR_SCOPE_LINKLOCAL ) && - dst.ifindex && dev->ifindex != dst.ifindex) -continue ; - - idev =__in6_dev_get (dev); -if (!idev) -continue ; - -read_lock_bh (&idev->lock); -list_for_each_entry (score->ifa, &idev->addr_list, if_list) { -int i; - -/* -* - Tentative Address (RFC2462 section 5.4) -* - A tentative address is not considered -* "assigned to an interface" in the traditional -* sense, unless it is also flagged as optimistic. -* - Candidate Source Address (section 4) -* - In any case, anycast addresses, multicast -* addresses, and the unspecified address MUST -* NOT be included in a candidate set. -*/ -if ((score->ifa->flags &IFA_F_TENTATIVE ) && - (!(score->ifa->flags &IFA_F_OPTIMISTIC ))) -continue ; - - score->addr_type =__ipv6_addr_type (&score->ifa->addr); +/* Candidate Source Address (section 4) +* - multicast and link-local destination address, +* the set of candidate source address MUST only +* include addresses assigned to interfaces +* belonging to the same link as the outgoing +* interface. +* (- For site-local destination addresses, the +* set of candidate source addresses MUST only +* include addresses assigned to interfaces +* belonging to the same site as the outgoing +* interface.) +*/ +if (dst_dev) { +if ((dst_type &IPV6_ADDR_MULTICAST ) || + dst.scope <=IPV6_ADDR_SCOPE_LINKLOCAL ) { + idev =__in6_dev_get (dst_dev); + use_oif_addr = true; + } + } -if (unlikely (score->addr_type ==IPV6_ADDR_ANY || - score->addr_type &IPV6_ADDR_MULTICAST )) { -net_dbg_ratelimited ("ADDRCONF: unspecified / multicast address assigned as unicast address on %s" , - dev->name); +if (use_oif_addr) { +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); + }else { +for_each_netdev_rcu (net, dev) { + idev =__in6_dev_get (dev); +if (!idev)continue ; - } - - score->rule =-1 ; -bitmap_zero (score->scorebits,IPV6_SADDR_RULE_MAX ); - -for (i =0 ; i <IPV6_SADDR_RULE_MAX ; i++) { -int minihiscore, miniscore; - - minihiscore =ipv6_get_saddr_eval (net, hiscore, &dst, i); - miniscore =ipv6_get_saddr_eval (net, score, &dst, i); - -if (minihiscore > miniscore) { -if (i ==IPV6_SADDR_RULE_SCOPE && - score->scopedist >0 ) { -/* -* special case: -* each remaining entry -* has too small (not enough) -* scope, because ifa entries -* are sorted by their scope -* values. -*/ - goto try_nextdev; - } -break ; - }else if (minihiscore < miniscore) { -if (hiscore->ifa) -in6_ifa_put (hiscore->ifa); - -in6_ifa_hold (score->ifa); - -swap (hiscore, score); - -/* restore our iterator */ - score->ifa = hiscore->ifa; - -break ; - } - } +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); } -try_nextdev: -read_unlock_bh (&idev->lock); }rcu_read_unlock ();
Signature · 1 line
1.9.1
From: YOSHIFUJI Hideaki/吉藤英明 <[email protected]>
Date: Fri, 10 Jul 2015 16:58:31 +0900
Quoted 6 lines
If outgoing interface is specified and the candidate address is restricted to the outgoing interface, it is enough to iterate over that given interface only.
Signed-off-by YOSHIFUJI Hideaki <[email protected]>Acked-by Erik Kline <[email protected]>
Applied, thanks!
Hmm, when I run a UML linux with this patch (which, I'm ashamed to say, I failed to do before) I get these kinds of errors:
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
Perhaps they're unrelated... I'm still investigating.
Quoted 11 lines · David Miller
From: YOSHIFUJI Hideaki/吉藤英明 <[email protected]>
Date: Fri, 10 Jul 2015 16:58:31 +0900Quoted 6 lines
If outgoing interface is specified and the candidate address is restricted to the outgoing interface, it is enough to iterate over that given interface only.
Signed-off-by YOSHIFUJI Hideaki <[email protected]>Acked-by Erik Kline <[email protected]>Applied, thanks!
Hi,
Quoted 9 lines · Erik Kline
Hmm, when I run a UML linux with this patch (which, I'm ashamed to say, I failed to do before) I get these kinds of errors:
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1Perhaps they're unrelated... I'm still investigating.
Would you test attached patch please?
--yoshfuji
Quoted 13 lines · David Miller
Quoted 11 lines
From: YOSHIFUJI Hideaki/吉藤英明 <[email protected]>
Date: Fri, 10 Jul 2015 16:58:31 +0900Quoted 6 lines
If outgoing interface is specified and the candidate address is restricted to the outgoing interface, it is enough to iterate over that given interface only.
Signed-off-by YOSHIFUJI Hideaki <[email protected]>Acked-by Erik Kline <[email protected]>Applied, thanks!
Signature · 2 lines
Hideaki Yoshifuji <[email protected]> Technical Division, MIRACLE LINUX CORPORATION
Quoted 1 line
From
38c5a10a5876ea47766ffc05b5a131a210d6e1aaMon Sep 17 00:00:00 2001
From: YOSHIFUJI Hideaki <[email protected]>
Date: Mon, 13 Jul 2015 15:23:02 +0900
Subject: [PATCH] ipv6: Avoid NULL pointer dereference in
__ipv6_dev_get_saddr().
Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when finding source address on specific interface.") introduced possible NULL pointer dereference if outgoing device is specified.
iterate over all interfaces when finding source address on specific interface.")
--- net/ipv6/addrconf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv6/addrconf.c +2 -1
@@ -1480,7 +1480,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev, }if (use_oif_addr) { -__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); +if (idev) +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); }else {for_each_netdev_rcu (net, dev) { idev =__in6_dev_get (dev);
Signature · 1 line
1.9.1
Quoted 6 lines · YOSHIFUJI Hideaki
Hi,
Quoted 3 lines · Erik Kline
Hmm, when I run a UML linux with this patch (which, I'm ashamed to say, I failed to do before) I get these kinds of errors:
Quoted 8 lines
Quoted 6 lines
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1Perhaps they're unrelated... I'm still investigating.
Would you test attached patch please?
That does look logically correct, so +1 to it regardless, but it does not seem to have fixed the issue I'm seeing.
I still haven't produced the smallest possible demo test program.
Yoshifuji-san,
At Mon, 13 Jul 2015 17:38:48 +0900,
Quoted 14 lines · Erik Kline
Quoted 11 lines · YOSHIFUJI Hideaki
Hi,
Quoted 8 lines · Erik Kline
Hmm, when I run a UML linux with this patch (which, I'm ashamed to say, I failed to do before) I get these kinds of errors:
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Perhaps they're unrelated... I'm still investigating.
Would you test attached patch please?
That does look logically correct, so +1 to it regardless, but it does not seem to have fixed the issue I'm seeing.
I still haven't produced the smallest possible demo test program.
sorry to jump-in, but there is a side-effect with this patch, which my tcp and dccp tests (ipv6) are failed.
because newly added function (__ipv6_dev_get_saddr) won't update a variable 'hiscore' (it swaps with 'score' in some case), the caller (ipv6_dev_get_saddr) can't fill an appropriate saddr in the end.
I don't know if this is a good patch but the following diff makes my test happy.
-- Hajime
net/ipv6/addrconf.c +7 -3
@@ -1363,7 +1363,8 @@ static void __ipv6_dev_get_saddr(struct net *net,unsigned int prefs, const struct in6_addr *saddr, struct inet6_dev *idev, - struct ipv6_saddr_score *scores) + struct ipv6_saddr_score *scores, + struct ipv6_saddr_score **in_hiscore) { struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1];
@@ -1424,6 +1425,7 @@ static void __ipv6_dev_get_saddr(struct net *net,in6_ifa_hold (score->ifa);swap (hiscore, score); + *in_hiscore = hiscore;/* restore our iterator */ score->ifa = hiscore->ifa;
@@ -1480,13 +1482,15 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev, }if (use_oif_addr) { -__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, + scores, &hiscore); }else {for_each_netdev_rcu (net, dev) { idev =__in6_dev_get (dev);if (!idev)continue ; -__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, + scores, &hiscore); } }rcu_read_unlock ();
Hi,
Quoted 31 lines · Hajime Tazaki
Yoshifuji-san,
At Mon, 13 Jul 2015 17:38:48 +0900,
Quoted 14 lines · Erik Kline
Quoted 11 lines · YOSHIFUJI Hideaki
Hi,
Quoted 8 lines · Erik Kline
Hmm, when I run a UML linux with this patch (which, I'm ashamed to say, I failed to do before) I get these kinds of errors:
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1
unregister_netdevice: waiting for <TAPdevice> to become free.
Usage count = 1Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Perhaps they're unrelated... I'm still investigating.
Would you test attached patch please?
That does look logically correct, so +1 to it regardless, but it does not seem to have fixed the issue I'm seeing.
I still haven't produced the smallest possible demo test program.
sorry to jump-in, but there is a side-effect with this patch, which my tcp and dccp tests (ipv6) are failed.
Quoted 7 lines
because newly added function (
__ipv6_dev_get_saddr) won't update a variable 'hiscore' (it swaps with 'score' in some case), the caller (ipv6_dev_get_saddr) can't fill an appropriate saddr in the end.I don't know if this is a good patch but the following diff makes my test happy.
We should update score as well...
Quoted 44 lines
-- Hajime
net/ipv6/addrconf.c +7
-3@@ -1363,7 +1363,8 @@ static void __ipv6_dev_get_saddr(struct net *net,unsigned int prefs, const struct in6_addr *saddr, struct inet6_dev *idev, - struct ipv6_saddr_score *scores) + struct ipv6_saddr_score *scores, + struct ipv6_saddr_score **in_hiscore) { struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1];@@ -1424,6 +1425,7 @@ static void __ipv6_dev_get_saddr(struct net *net,in6_ifa_hold (score->ifa);swap (hiscore, score); + *in_hiscore = hiscore;/* restore our iterator */ score->ifa = hiscore->ifa;@@ -1480,13 +1482,15 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev, }if (use_oif_addr) { -__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, + scores, &hiscore); }else {for_each_netdev_rcu (net, dev) { idev =__in6_dev_get (dev);if (!idev)continue ; -__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, scores); +__ipv6_dev_get_saddr (net, &dst, prefs, saddr, idev, + scores, &hiscore); } }rcu_read_unlock ();
Signature · 2 lines
Hideaki Yoshifuji <[email protected]> Technical Division, MIRACLE LINUX CORPORATION