| Message ID | 55A3CAFA.3060404@miraclelinux.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
At Mon, 13 Jul 2015 23:28:10 +0900, YOSHIFUJI Hideaki/吉藤英明 wrote: > > Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when > finding source address on specific interface.") did not properly > update best source address available. Plus, it introduced > possible NULL pointer dereference. > > Bug was reported by Erik Kline <ek@google.com>. > Based on patch proposed by Hajime Tazaki <thehajime@gmail.com>. > > Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not > iterate over all interfaces when finding source address > on specific interface.") > Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> all of my tests passed with the patch on 14fe22e: Revert "ipv4: use skb coalescing in defragmentation". thanks for the prompt fix ! Acked-by: Hajime Tazaki <thehajime@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
All my tests pass with this applied to net-next/master. Many thanks! Acked-by: Erik Kline <ek@google.com> On 13 July 2015 at 23:28, YOSHIFUJI Hideaki/吉藤英明 <hideaki.yoshifuji@miraclelinux.com> wrote: > Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when > finding source address on specific interface.") did not properly > update best source address available. Plus, it introduced > possible NULL pointer dereference. > > Bug was reported by Erik Kline <ek@google.com>. > Based on patch proposed by Hajime Tazaki <thehajime@gmail.com>. > > Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not > iterate over all interfaces when finding source address > on specific interface.") > Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> > --- > net/ipv6/addrconf.c | 30 ++++++++++++++++++------------ > 1 file changed, 18 insertions(+), 12 deletions(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 4ab74d5..4c9a024 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -1358,14 +1358,15 @@ 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) > +static int __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, > + int hiscore_idx) > { > - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; > + struct ipv6_saddr_score *score = &scores[1 - hiscore_idx], *hiscore = &scores[hiscore_idx]; > > read_lock_bh(&idev->lock); > list_for_each_entry(score->ifa, &idev->addr_list, if_list) { > @@ -1424,6 +1425,7 @@ static void __ipv6_dev_get_saddr(struct net *net, > in6_ifa_hold(score->ifa); > > swap(hiscore, score); > + hiscore_idx = 1 - hiscore_idx; > > /* restore our iterator */ > score->ifa = hiscore->ifa; > @@ -1434,18 +1436,20 @@ static void __ipv6_dev_get_saddr(struct net *net, > } > out: > read_unlock_bh(&idev->lock); > + return hiscore_idx; > } > > 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], *hiscore = &scores[1]; > + struct ipv6_saddr_score scores[2], *hiscore; > struct ipv6_saddr_dst dst; > struct inet6_dev *idev; > struct net_device *dev; > int dst_type; > bool use_oif_addr = false; > + int hiscore_idx = 0; > > dst_type = __ipv6_addr_type(daddr); > dst.addr = daddr; > @@ -1454,8 +1458,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev, > dst.label = ipv6_addr_label(net, daddr, dst_type, dst.ifindex); > dst.prefs = prefs; > > - hiscore->rule = -1; > - hiscore->ifa = NULL; > + scores[hiscore_idx].rule = -1; > + scores[hiscore_idx].ifa = NULL; > > rcu_read_lock(); > > @@ -1480,17 +1484,19 @@ 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) > + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx); > } 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); > + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx); > } > } > rcu_read_unlock(); > > + hiscore = &scores[hiscore_idx]; > if (!hiscore->ifa) > return -EADDRNOTAVAIL; > > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
And I now have a use_oif_addr sysctl patch that, on top if this one, passes all my tests. On 15 July 2015 at 18:15, Erik Kline <ek@google.com> wrote: > All my tests pass with this applied to net-next/master. > > Many thanks! > > Acked-by: Erik Kline <ek@google.com> > > On 13 July 2015 at 23:28, YOSHIFUJI Hideaki/吉藤英明 > <hideaki.yoshifuji@miraclelinux.com> wrote: >> Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when >> finding source address on specific interface.") did not properly >> update best source address available. Plus, it introduced >> possible NULL pointer dereference. >> >> Bug was reported by Erik Kline <ek@google.com>. >> Based on patch proposed by Hajime Tazaki <thehajime@gmail.com>. >> >> Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not >> iterate over all interfaces when finding source address >> on specific interface.") >> Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> >> --- >> net/ipv6/addrconf.c | 30 ++++++++++++++++++------------ >> 1 file changed, 18 insertions(+), 12 deletions(-) >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 4ab74d5..4c9a024 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -1358,14 +1358,15 @@ 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) >> +static int __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, >> + int hiscore_idx) >> { >> - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; >> + struct ipv6_saddr_score *score = &scores[1 - hiscore_idx], *hiscore = &scores[hiscore_idx]; >> >> read_lock_bh(&idev->lock); >> list_for_each_entry(score->ifa, &idev->addr_list, if_list) { >> @@ -1424,6 +1425,7 @@ static void __ipv6_dev_get_saddr(struct net *net, >> in6_ifa_hold(score->ifa); >> >> swap(hiscore, score); >> + hiscore_idx = 1 - hiscore_idx; >> >> /* restore our iterator */ >> score->ifa = hiscore->ifa; >> @@ -1434,18 +1436,20 @@ static void __ipv6_dev_get_saddr(struct net *net, >> } >> out: >> read_unlock_bh(&idev->lock); >> + return hiscore_idx; >> } >> >> 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], *hiscore = &scores[1]; >> + struct ipv6_saddr_score scores[2], *hiscore; >> struct ipv6_saddr_dst dst; >> struct inet6_dev *idev; >> struct net_device *dev; >> int dst_type; >> bool use_oif_addr = false; >> + int hiscore_idx = 0; >> >> dst_type = __ipv6_addr_type(daddr); >> dst.addr = daddr; >> @@ -1454,8 +1458,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev, >> dst.label = ipv6_addr_label(net, daddr, dst_type, dst.ifindex); >> dst.prefs = prefs; >> >> - hiscore->rule = -1; >> - hiscore->ifa = NULL; >> + scores[hiscore_idx].rule = -1; >> + scores[hiscore_idx].ifa = NULL; >> >> rcu_read_lock(); >> >> @@ -1480,17 +1484,19 @@ 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) >> + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx); >> } 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); >> + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx); >> } >> } >> rcu_read_unlock(); >> >> + hiscore = &scores[hiscore_idx]; >> if (!hiscore->ifa) >> return -EADDRNOTAVAIL; >> >> -- >> 1.9.1 >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: YOSHIFUJI Hideaki/吉藤英明 <hideaki.yoshifuji@miraclelinux.com> Date: Mon, 13 Jul 2015 23:28:10 +0900 > Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when > finding source address on specific interface.") did not properly > update best source address available. Plus, it introduced > possible NULL pointer dereference. > > Bug was reported by Erik Kline <ek@google.com>. > Based on patch proposed by Hajime Tazaki <thehajime@gmail.com>. > > Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not > iterate over all interfaces when finding source address > on specific interface.") > Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 4ab74d5..4c9a024 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -1358,14 +1358,15 @@ 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) +static int __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, + int hiscore_idx) { - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; + struct ipv6_saddr_score *score = &scores[1 - hiscore_idx], *hiscore = &scores[hiscore_idx]; read_lock_bh(&idev->lock); list_for_each_entry(score->ifa, &idev->addr_list, if_list) { @@ -1424,6 +1425,7 @@ static void __ipv6_dev_get_saddr(struct net *net, in6_ifa_hold(score->ifa); swap(hiscore, score); + hiscore_idx = 1 - hiscore_idx; /* restore our iterator */ score->ifa = hiscore->ifa; @@ -1434,18 +1436,20 @@ static void __ipv6_dev_get_saddr(struct net *net, } out: read_unlock_bh(&idev->lock); + return hiscore_idx; } 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], *hiscore = &scores[1]; + struct ipv6_saddr_score scores[2], *hiscore; struct ipv6_saddr_dst dst; struct inet6_dev *idev; struct net_device *dev; int dst_type; bool use_oif_addr = false; + int hiscore_idx = 0; dst_type = __ipv6_addr_type(daddr); dst.addr = daddr; @@ -1454,8 +1458,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev, dst.label = ipv6_addr_label(net, daddr, dst_type, dst.ifindex); dst.prefs = prefs; - hiscore->rule = -1; - hiscore->ifa = NULL; + scores[hiscore_idx].rule = -1; + scores[hiscore_idx].ifa = NULL; rcu_read_lock(); @@ -1480,17 +1484,19 @@ 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) + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx); } 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); + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx); } } rcu_read_unlock(); + hiscore = &scores[hiscore_idx]; if (!hiscore->ifa) return -EADDRNOTAVAIL;
Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when finding source address on specific interface.") did not properly update best source address available. Plus, it introduced possible NULL pointer dereference. Bug was reported by Erik Kline <ek@google.com>. Based on patch proposed by Hajime Tazaki <thehajime@gmail.com>. Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not iterate over all interfaces when finding source address on specific interface.") Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.com> --- net/ipv6/addrconf.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)