| Message ID | CALx6S34Bj_YtRqqL5U0zJ9RKEu_Uvp9zYyNJLgvjEqX9jNrYqA@mail.gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, Tom Herbert wrote: > I am testing this patch which may be a little simpler. Also idev needs > to be checked after __in6_dev_get We have to select source address on *given* interface for link-local/ multicast destinations. > > Tom > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 4ab74d5..d631ac3 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -1363,9 +1363,10 @@ 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 **in_score, > + struct ipv6_saddr_score **in_hiscore) > { > - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; > + struct ipv6_saddr_score *score = *in_score, *hiscore = *in_hiscore; > > read_lock_bh(&idev->lock); > list_for_each_entry(score->ifa, &idev->addr_list, if_list) { > @@ -1434,13 +1435,16 @@ static void __ipv6_dev_get_saddr(struct net *net, > } > out: > read_unlock_bh(&idev->lock); > + *in_hiscore = hiscore; > + *in_score = score; > } > > 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]; > + struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; > struct ipv6_saddr_dst dst; > struct inet6_dev *idev; > struct net_device *dev; > @@ -1475,18 +1479,19 @@ int ipv6_dev_get_saddr(struct net *net, const > struct net_device *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 (idev) > + use_oif_addr = true; > } > } > if (use_oif_addr) { > - __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores); > + __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, > &score, &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, &score, &hiscore); > } > } > rcu_read_unlock(); > > On Mon, Jul 13, 2015 at 7:28 AM, 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
On Tue, Jul 14, 2015 at 5:44 AM, YOSHIFUJI Hideaki/吉藤英明 <hideaki.yoshifuji@miraclelinux.com> wrote: > Hi, > > Tom Herbert wrote: >> I am testing this patch which may be a little simpler. Also idev needs >> to be checked after __in6_dev_get > > We have to select source address on *given* interface for link-local/ > multicast destinations. > I mean check that idev is not NULL. >> >> Tom >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 4ab74d5..d631ac3 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -1363,9 +1363,10 @@ 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 **in_score, >> + struct ipv6_saddr_score **in_hiscore) >> { >> - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; >> + struct ipv6_saddr_score *score = *in_score, *hiscore = *in_hiscore; >> >> read_lock_bh(&idev->lock); >> list_for_each_entry(score->ifa, &idev->addr_list, if_list) { >> @@ -1434,13 +1435,16 @@ static void __ipv6_dev_get_saddr(struct net *net, >> } >> out: >> read_unlock_bh(&idev->lock); >> + *in_hiscore = hiscore; >> + *in_score = score; >> } >> >> 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]; >> + struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; >> struct ipv6_saddr_dst dst; >> struct inet6_dev *idev; >> struct net_device *dev; >> @@ -1475,18 +1479,19 @@ int ipv6_dev_get_saddr(struct net *net, const >> struct net_device *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 (idev) >> + use_oif_addr = true; >> } >> } >> if (use_oif_addr) { >> - __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores); >> + __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, >> &score, &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, &score, &hiscore); >> } >> } >> rcu_read_unlock(); >> >> On Mon, Jul 13, 2015 at 7:28 AM, 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 > > -- > 吉藤英明 <hideaki.yoshifuji@miraclelinux.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
From: Tom Herbert <tom@herbertland.com> Date: Mon, 13 Jul 2015 08:31:05 -0700 > I am testing this patch which may be a little simpler. Also idev needs > to be checked after __in6_dev_get Tom, I'm applying Hideaki's fix for now. If you want to simplify the code, feel free to submit that as a follow-up, thank you! -- 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..d631ac3 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -1363,9 +1363,10 @@ 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 **in_score, + struct ipv6_saddr_score **in_hiscore) { - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; + struct ipv6_saddr_score *score = *in_score, *hiscore = *in_hiscore; read_lock_bh(&idev->lock); list_for_each_entry(score->ifa, &idev->addr_list, if_list) { @@ -1434,13 +1435,16 @@ static void __ipv6_dev_get_saddr(struct net *net, } out: read_unlock_bh(&idev->lock); + *in_hiscore = hiscore; + *in_score = score; } 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]; + struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1]; struct ipv6_saddr_dst dst; struct inet6_dev *idev; struct net_device *dev; @@ -1475,18 +1479,19 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *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 (idev) + use_oif_addr = true; } } if (use_oif_addr) { - __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores); + __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, &score, &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, &score, &hiscore);
I am testing this patch which may be a little simpler. Also idev needs to be checked after __in6_dev_get Tom } } rcu_read_unlock(); On Mon, Jul 13, 2015 at 7:28 AM, 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 -- 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