* [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
@ 2015-07-13 14:28 YOSHIFUJI Hideaki/吉藤英明
2015-07-13 15:23 ` Hajime Tazaki
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: YOSHIFUJI Hideaki/吉藤英明 @ 2015-07-13 14:28 UTC (permalink / raw)
To: davem, netdev
Cc: ek, thehajime, YOSHIFUJI Hideaki/吉藤英明
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-13 14:28 [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr() YOSHIFUJI Hideaki/吉藤英明
@ 2015-07-13 15:23 ` Hajime Tazaki
2015-07-13 15:31 ` Tom Herbert
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Hajime Tazaki @ 2015-07-13 15:23 UTC (permalink / raw)
To: hideaki.yoshifuji; +Cc: davem, netdev, ek
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-13 14:28 [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr() YOSHIFUJI Hideaki/吉藤英明
2015-07-13 15:23 ` Hajime Tazaki
@ 2015-07-13 15:31 ` Tom Herbert
2015-07-14 12:44 ` YOSHIFUJI Hideaki/吉藤英明
2015-07-16 4:07 ` David Miller
2015-07-15 9:15 ` Erik Kline
2015-07-16 4:07 ` David Miller
3 siblings, 2 replies; 9+ messages in thread
From: Tom Herbert @ 2015-07-13 15:31 UTC (permalink / raw)
To: YOSHIFUJI Hideaki/吉藤英明
Cc: David S. Miller, Linux Kernel Network Developers, Erik Kline,
thehajime
I am testing this patch which may be a little simpler. Also idev needs
to be checked after __in6_dev_get
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-13 15:31 ` Tom Herbert
@ 2015-07-14 12:44 ` YOSHIFUJI Hideaki/吉藤英明
2015-07-14 15:22 ` Tom Herbert
2015-07-16 4:07 ` David Miller
1 sibling, 1 reply; 9+ messages in thread
From: YOSHIFUJI Hideaki/吉藤英明 @ 2015-07-14 12:44 UTC (permalink / raw)
To: Tom Herbert
Cc: hideaki.yoshifuji, David S. Miller,
Linux Kernel Network Developers, Erik Kline, thehajime
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
--
吉藤英明 <hideaki.yoshifuji@miraclelinux.com>
ミラクル・リナックス株式会社 技術本部 サポート部
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-14 12:44 ` YOSHIFUJI Hideaki/吉藤英明
@ 2015-07-14 15:22 ` Tom Herbert
0 siblings, 0 replies; 9+ messages in thread
From: Tom Herbert @ 2015-07-14 15:22 UTC (permalink / raw)
To: YOSHIFUJI Hideaki/吉藤英明
Cc: David S. Miller, Linux Kernel Network Developers, Erik Kline,
Hajime Tazaki
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>
> ミラクル・リナックス株式会社 技術本部 サポート部
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-13 14:28 [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr() YOSHIFUJI Hideaki/吉藤英明
2015-07-13 15:23 ` Hajime Tazaki
2015-07-13 15:31 ` Tom Herbert
@ 2015-07-15 9:15 ` Erik Kline
2015-07-15 10:22 ` Erik Kline
2015-07-16 4:07 ` David Miller
3 siblings, 1 reply; 9+ messages in thread
From: Erik Kline @ 2015-07-15 9:15 UTC (permalink / raw)
To: YOSHIFUJI Hideaki/吉藤英明
Cc: David Miller, netdev, thehajime
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-15 9:15 ` Erik Kline
@ 2015-07-15 10:22 ` Erik Kline
0 siblings, 0 replies; 9+ messages in thread
From: Erik Kline @ 2015-07-15 10:22 UTC (permalink / raw)
To: YOSHIFUJI Hideaki/吉藤英明
Cc: David Miller, netdev, Hajime Tazaki
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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-13 15:31 ` Tom Herbert
2015-07-14 12:44 ` YOSHIFUJI Hideaki/吉藤英明
@ 2015-07-16 4:07 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2015-07-16 4:07 UTC (permalink / raw)
To: tom; +Cc: hideaki.yoshifuji, netdev, ek, thehajime
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
2015-07-13 14:28 [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr() YOSHIFUJI Hideaki/吉藤英明
` (2 preceding siblings ...)
2015-07-15 9:15 ` Erik Kline
@ 2015-07-16 4:07 ` David Miller
3 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2015-07-16 4:07 UTC (permalink / raw)
To: hideaki.yoshifuji; +Cc: netdev, ek, thehajime
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-07-16 4:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-13 14:28 [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr() YOSHIFUJI Hideaki/吉藤英明
2015-07-13 15:23 ` Hajime Tazaki
2015-07-13 15:31 ` Tom Herbert
2015-07-14 12:44 ` YOSHIFUJI Hideaki/吉藤英明
2015-07-14 15:22 ` Tom Herbert
2015-07-16 4:07 ` David Miller
2015-07-15 9:15 ` Erik Kline
2015-07-15 10:22 ` Erik Kline
2015-07-16 4:07 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).