Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 9:55 PM, Cong Wang wrote:
> Why not add a printk and play with my patch to see the difference?

I have other things to do. If you believe your patch fixes the problem,
send it and let Andrey verify.

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 9:55 PM, Cong Wang wrote:
> Why not add a printk and play with my patch to see the difference?

I have other things to do. If you believe your patch fixes the problem,
send it and let Andrey verify.

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 7:43 PM, David Ahern  wrote:
> On 5/3/17 5:35 PM, Cong Wang wrote:
>> Ah, we need:
>>
>> @@ -4024,7 +4027,7 @@ static struct pernet_operations ip6_route_net_late_ops 
>> = {
>>
>>  static struct notifier_block ip6_route_dev_notifier = {
>> .notifier_call = ip6_route_dev_notify,
>> -   .priority = 0,
>> +   .priority = -10, /* Must be called after addrconf_notify!! */
>>  };
>>
>
>
> It's not a notifier problem; the null_entry is created in ip6_route_init
> which is an init function.

Only init_net's null entry is created here.

>
> For network namespaces other than init_net, it is never initialized. See
> ip6_route_net_init.

I don't understand what you are talking about...

It is obviously initialized in 3 places:

1) The template itself, as we use memdup()
2) Code around dst_init_metrics() in ip6_route_net_init()
3) ip6_route_dev_notify(), which is supposed to initialize it after
loopback registers (the order needs to fix, as shown in my patch)

Why not add a printk and play with my patch to see the difference?

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 7:43 PM, David Ahern  wrote:
> On 5/3/17 5:35 PM, Cong Wang wrote:
>> Ah, we need:
>>
>> @@ -4024,7 +4027,7 @@ static struct pernet_operations ip6_route_net_late_ops 
>> = {
>>
>>  static struct notifier_block ip6_route_dev_notifier = {
>> .notifier_call = ip6_route_dev_notify,
>> -   .priority = 0,
>> +   .priority = -10, /* Must be called after addrconf_notify!! */
>>  };
>>
>
>
> It's not a notifier problem; the null_entry is created in ip6_route_init
> which is an init function.

Only init_net's null entry is created here.

>
> For network namespaces other than init_net, it is never initialized. See
> ip6_route_net_init.

I don't understand what you are talking about...

It is obviously initialized in 3 places:

1) The template itself, as we use memdup()
2) Code around dst_init_metrics() in ip6_route_net_init()
3) ip6_route_dev_notify(), which is supposed to initialize it after
loopback registers (the order needs to fix, as shown in my patch)

Why not add a printk and play with my patch to see the difference?

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 5:35 PM, Cong Wang wrote:
> Ah, we need:
> 
> @@ -4024,7 +4027,7 @@ static struct pernet_operations ip6_route_net_late_ops 
> = {
> 
>  static struct notifier_block ip6_route_dev_notifier = {
> .notifier_call = ip6_route_dev_notify,
> -   .priority = 0,
> +   .priority = -10, /* Must be called after addrconf_notify!! */
>  };
> 


It's not a notifier problem; the null_entry is created in ip6_route_init
which is an init function.

For network namespaces other than init_net, it is never initialized. See
ip6_route_net_init.

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 5:35 PM, Cong Wang wrote:
> Ah, we need:
> 
> @@ -4024,7 +4027,7 @@ static struct pernet_operations ip6_route_net_late_ops 
> = {
> 
>  static struct notifier_block ip6_route_dev_notifier = {
> .notifier_call = ip6_route_dev_notify,
> -   .priority = 0,
> +   .priority = -10, /* Must be called after addrconf_notify!! */
>  };
> 


It's not a notifier problem; the null_entry is created in ip6_route_init
which is an init function.

For network namespaces other than init_net, it is never initialized. See
ip6_route_net_init.

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 3:09 PM, David Ahern  wrote:
> On 5/3/17 4:02 PM, Cong Wang wrote:
>> On Wed, May 3, 2017 at 11:22 AM, David Ahern  wrote:
>>> On 5/3/17 11:02 AM, Cong Wang wrote:
 A quick glance shows we need to simply check local->rt6i_idev
 since we do the same check for sprt right above.
>>>
>>> As I recall, rt6i_idev is set for all routes except null_entry and it is
>>> not set on null_entry only because of initialization order.
>>
>> Are you sure?
>>
>> if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
>> net->ipv6.ip6_null_entry->dst.dev = dev;
>> net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
>> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>> net->ipv6.ip6_prohibit_entry->dst.dev = dev;
>> net->ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(dev);
>> net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
>> net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
>> #endif
>> }
>>
>> Loopback is the first one to register, so null entry is set to
>> loopback dev initially. Similar for init_net.
>>
>
> Why don't you add a printk and see ;-)

Ah, we need:

@@ -4024,7 +4027,7 @@ static struct pernet_operations ip6_route_net_late_ops = {

 static struct notifier_block ip6_route_dev_notifier = {
.notifier_call = ip6_route_dev_notify,
-   .priority = 0,
+   .priority = -10, /* Must be called after addrconf_notify!! */
 };

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 3:09 PM, David Ahern  wrote:
> On 5/3/17 4:02 PM, Cong Wang wrote:
>> On Wed, May 3, 2017 at 11:22 AM, David Ahern  wrote:
>>> On 5/3/17 11:02 AM, Cong Wang wrote:
 A quick glance shows we need to simply check local->rt6i_idev
 since we do the same check for sprt right above.
>>>
>>> As I recall, rt6i_idev is set for all routes except null_entry and it is
>>> not set on null_entry only because of initialization order.
>>
>> Are you sure?
>>
>> if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
>> net->ipv6.ip6_null_entry->dst.dev = dev;
>> net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
>> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
>> net->ipv6.ip6_prohibit_entry->dst.dev = dev;
>> net->ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(dev);
>> net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
>> net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
>> #endif
>> }
>>
>> Loopback is the first one to register, so null entry is set to
>> loopback dev initially. Similar for init_net.
>>
>
> Why don't you add a printk and see ;-)

Ah, we need:

@@ -4024,7 +4027,7 @@ static struct pernet_operations ip6_route_net_late_ops = {

 static struct notifier_block ip6_route_dev_notifier = {
.notifier_call = ip6_route_dev_notify,
-   .priority = 0,
+   .priority = -10, /* Must be called after addrconf_notify!! */
 };

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 4:02 PM, Cong Wang wrote:
> On Wed, May 3, 2017 at 11:22 AM, David Ahern  wrote:
>> On 5/3/17 11:02 AM, Cong Wang wrote:
>>> A quick glance shows we need to simply check local->rt6i_idev
>>> since we do the same check for sprt right above.
>>
>> As I recall, rt6i_idev is set for all routes except null_entry and it is
>> not set on null_entry only because of initialization order.
> 
> Are you sure?
> 
> if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
> net->ipv6.ip6_null_entry->dst.dev = dev;
> net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> net->ipv6.ip6_prohibit_entry->dst.dev = dev;
> net->ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(dev);
> net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
> net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
> #endif
> }
> 
> Loopback is the first one to register, so null entry is set to
> loopback dev initially. Similar for init_net.
> 

Why don't you add a printk and see ;-)

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 4:02 PM, Cong Wang wrote:
> On Wed, May 3, 2017 at 11:22 AM, David Ahern  wrote:
>> On 5/3/17 11:02 AM, Cong Wang wrote:
>>> A quick glance shows we need to simply check local->rt6i_idev
>>> since we do the same check for sprt right above.
>>
>> As I recall, rt6i_idev is set for all routes except null_entry and it is
>> not set on null_entry only because of initialization order.
> 
> Are you sure?
> 
> if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
> net->ipv6.ip6_null_entry->dst.dev = dev;
> net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
> #ifdef CONFIG_IPV6_MULTIPLE_TABLES
> net->ipv6.ip6_prohibit_entry->dst.dev = dev;
> net->ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(dev);
> net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
> net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
> #endif
> }
> 
> Loopback is the first one to register, so null entry is set to
> loopback dev initially. Similar for init_net.
> 

Why don't you add a printk and see ;-)

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 11:22 AM, David Ahern  wrote:
> On 5/3/17 11:02 AM, Cong Wang wrote:
>> A quick glance shows we need to simply check local->rt6i_idev
>> since we do the same check for sprt right above.
>
> As I recall, rt6i_idev is set for all routes except null_entry and it is
> not set on null_entry only because of initialization order.

Are you sure?

if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
net->ipv6.ip6_null_entry->dst.dev = dev;
net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
net->ipv6.ip6_prohibit_entry->dst.dev = dev;
net->ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(dev);
net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
#endif
}

Loopback is the first one to register, so null entry is set to
loopback dev initially. Similar for init_net.

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 11:22 AM, David Ahern  wrote:
> On 5/3/17 11:02 AM, Cong Wang wrote:
>> A quick glance shows we need to simply check local->rt6i_idev
>> since we do the same check for sprt right above.
>
> As I recall, rt6i_idev is set for all routes except null_entry and it is
> not set on null_entry only because of initialization order.

Are you sure?

if (event == NETDEV_REGISTER && (dev->flags & IFF_LOOPBACK)) {
net->ipv6.ip6_null_entry->dst.dev = dev;
net->ipv6.ip6_null_entry->rt6i_idev = in6_dev_get(dev);
#ifdef CONFIG_IPV6_MULTIPLE_TABLES
net->ipv6.ip6_prohibit_entry->dst.dev = dev;
net->ipv6.ip6_prohibit_entry->rt6i_idev = in6_dev_get(dev);
net->ipv6.ip6_blk_hole_entry->dst.dev = dev;
net->ipv6.ip6_blk_hole_entry->rt6i_idev = in6_dev_get(dev);
#endif
}

Loopback is the first one to register, so null entry is set to
loopback dev initially. Similar for init_net.

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 11:02 AM, Cong Wang wrote:
> A quick glance shows we need to simply check local->rt6i_idev
> since we do the same check for sprt right above.

As I recall, rt6i_idev is set for all routes except null_entry and it is
not set on null_entry only because of initialization order.

> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index a1bf426..61ec3c8 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -511,7 +511,7 @@ static inline struct rt6_info
> *rt6_device_match(struct net *net,
> sprt->rt6i_idev->dev->ifindex != oif) {
> if (flags & RT6_LOOKUP_F_IFACE)
> continue;
> -   if (local &&
> +   if (local && local->rt6i_idev &&
> 
> local->rt6i_idev->dev->ifindex == oif)
> continue;
> }
> 

Or don't consider null_entry at all:


@@ -515,7 +528,8 @@ static inline struct rt6_info
*rt6_device_match(struct net *net,

local->rt6i_idev->dev->ifindex == oif)
continue;
}
-   local = sprt;
+   if (sprt != net->ipv6.ip6_null_entry)
+   local = sprt;
}

Re: net/ipv6: GPF in rt6_device_match

[David Ahern]

On 5/3/17 11:02 AM, Cong Wang wrote:
> A quick glance shows we need to simply check local->rt6i_idev
> since we do the same check for sprt right above.

As I recall, rt6i_idev is set for all routes except null_entry and it is
not set on null_entry only because of initialization order.

> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index a1bf426..61ec3c8 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -511,7 +511,7 @@ static inline struct rt6_info
> *rt6_device_match(struct net *net,
> sprt->rt6i_idev->dev->ifindex != oif) {
> if (flags & RT6_LOOKUP_F_IFACE)
> continue;
> -   if (local &&
> +   if (local && local->rt6i_idev &&
> 
> local->rt6i_idev->dev->ifindex == oif)
> continue;
> }
> 

Or don't consider null_entry at all:


@@ -515,7 +528,8 @@ static inline struct rt6_info
*rt6_device_match(struct net *net,

local->rt6i_idev->dev->ifindex == oif)
continue;
}
-   local = sprt;
+   if (sprt != net->ipv6.ip6_null_entry)
+   local = sprt;
}

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 9:33 AM, Andrey Konovalov  wrote:
> Hi David,
>
> Got another report related to fib6.
>
> I'm on 89c9fea3c8034cdb2fd745f551cde0b507fd6893 with your last patch applied.
>
> A reproducer and .config are attached.
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault:  [#1] SMP KASAN
> Modules linked in:
> CPU: 1 PID: 4059 Comm: a.out Not tainted 4.11.0+ #312
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: 880064d11600 task.stack: 88006b0e8000
> RIP: 0010:rt6_device_match net/ipv6/route.c:515
> RIP: 0010:ip6_pol_route_lookup+0x2f2/0xa90 net/ipv6/route.c:885
> RSP: 0018:88006b0ef0b8 EFLAGS: 00010246
> RAX: 11000d00f20b RBX: 880066dd96c0 RCX: 
> RDX:  RSI: 88006b0ef5b8 RDI: 880068079058
> RBP: 88006b0ef1e0 R08: 0002 R09: 4e432473
> R10: 88006b0ef0b8 R11: dc00 R12: 0009
> R13: 880068078f00 R14:  R15: dc00
> FS:  7fd68b4d5700() GS:88006cb0() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 208e2fe0 CR3: 64c9b000 CR4: 06e0
> Call Trace:
>  fib6_rule_action+0x261/0x8a0 net/ipv6/fib6_rules.c:100
>  fib_rules_lookup+0x3cf/0xca0 net/core/fib_rules.c:279
>  fib6_rule_lookup+0x175/0x360 net/ipv6/fib6_rules.c:44
>  rt6_lookup+0x267/0x3b0 net/ipv6/route.c:924
>  ip6gre_tnl_link_config+0x774/0xc00 net/ipv6/ip6_gre.c:746
>  ip6gre_tunnel_locate+0x4ab/0x7c0 net/ipv6/ip6_gre.c:340
>  ip6gre_tunnel_ioctl+0x5cd/0x21f0 net/ipv6/ip6_gre.c:878
>  dev_ifsioc+0x53f/0x9f0 net/core/dev_ioctl.c:338

A quick glance shows we need to simply check local->rt6i_idev
since we do the same check for sprt right above.

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a1bf426..61ec3c8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -511,7 +511,7 @@ static inline struct rt6_info
*rt6_device_match(struct net *net,
sprt->rt6i_idev->dev->ifindex != oif) {
if (flags & RT6_LOOKUP_F_IFACE)
continue;
-   if (local &&
+   if (local && local->rt6i_idev &&

local->rt6i_idev->dev->ifindex == oif)
continue;
}

Re: net/ipv6: GPF in rt6_device_match

[Cong Wang]

On Wed, May 3, 2017 at 9:33 AM, Andrey Konovalov  wrote:
> Hi David,
>
> Got another report related to fib6.
>
> I'm on 89c9fea3c8034cdb2fd745f551cde0b507fd6893 with your last patch applied.
>
> A reproducer and .config are attached.
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault:  [#1] SMP KASAN
> Modules linked in:
> CPU: 1 PID: 4059 Comm: a.out Not tainted 4.11.0+ #312
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: 880064d11600 task.stack: 88006b0e8000
> RIP: 0010:rt6_device_match net/ipv6/route.c:515
> RIP: 0010:ip6_pol_route_lookup+0x2f2/0xa90 net/ipv6/route.c:885
> RSP: 0018:88006b0ef0b8 EFLAGS: 00010246
> RAX: 11000d00f20b RBX: 880066dd96c0 RCX: 
> RDX:  RSI: 88006b0ef5b8 RDI: 880068079058
> RBP: 88006b0ef1e0 R08: 0002 R09: 4e432473
> R10: 88006b0ef0b8 R11: dc00 R12: 0009
> R13: 880068078f00 R14:  R15: dc00
> FS:  7fd68b4d5700() GS:88006cb0() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 208e2fe0 CR3: 64c9b000 CR4: 06e0
> Call Trace:
>  fib6_rule_action+0x261/0x8a0 net/ipv6/fib6_rules.c:100
>  fib_rules_lookup+0x3cf/0xca0 net/core/fib_rules.c:279
>  fib6_rule_lookup+0x175/0x360 net/ipv6/fib6_rules.c:44
>  rt6_lookup+0x267/0x3b0 net/ipv6/route.c:924
>  ip6gre_tnl_link_config+0x774/0xc00 net/ipv6/ip6_gre.c:746
>  ip6gre_tunnel_locate+0x4ab/0x7c0 net/ipv6/ip6_gre.c:340
>  ip6gre_tunnel_ioctl+0x5cd/0x21f0 net/ipv6/ip6_gre.c:878
>  dev_ifsioc+0x53f/0x9f0 net/core/dev_ioctl.c:338

A quick glance shows we need to simply check local->rt6i_idev
since we do the same check for sprt right above.

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index a1bf426..61ec3c8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -511,7 +511,7 @@ static inline struct rt6_info
*rt6_device_match(struct net *net,
sprt->rt6i_idev->dev->ifindex != oif) {
if (flags & RT6_LOOKUP_F_IFACE)
continue;
-   if (local &&
+   if (local && local->rt6i_idev &&

local->rt6i_idev->dev->ifindex == oif)
continue;
}