Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1635141 > unrolled thread

Re: net/ipv6: GPF in rt6_device_match

Started byCong Wang <xiyou.wangcong@gmail.com>
First post2017-05-03 19:10 +0200
Last post2017-05-04 06:20 +0200
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: net/ipv6: GPF in rt6_device_match Cong Wang <xiyou.wangcong@gmail.com> - 2017-05-03 19:10 +0200
    Re: net/ipv6: GPF in rt6_device_match David Ahern <dsahern@gmail.com> - 2017-05-03 20:30 +0200
      Re: net/ipv6: GPF in rt6_device_match Cong Wang <xiyou.wangcong@gmail.com> - 2017-05-04 00:10 +0200
        Re: net/ipv6: GPF in rt6_device_match David Ahern <dsahern@gmail.com> - 2017-05-04 00:10 +0200
          Re: net/ipv6: GPF in rt6_device_match Cong Wang <xiyou.wangcong@gmail.com> - 2017-05-04 01:40 +0200
            Re: net/ipv6: GPF in rt6_device_match David Ahern <dsahern@gmail.com> - 2017-05-04 04:50 +0200
              Re: net/ipv6: GPF in rt6_device_match Cong Wang <xiyou.wangcong@gmail.com> - 2017-05-04 06:00 +0200
                Re: net/ipv6: GPF in rt6_device_match David Ahern <dsahern@gmail.com> - 2017-05-04 06:20 +0200

#1635141 — Re: net/ipv6: GPF in rt6_device_match

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-05-03 19:10 +0200
SubjectRe: net/ipv6: GPF in rt6_device_match
Message-ID<tD5WO-8gC-3@gated-at.bofh.it>
On Wed, May 3, 2017 at 9:33 AM, Andrey Konovalov <andreyknvl@google.com> 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: 0000 [#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: ffff880064d11600 task.stack: ffff88006b0e8000
> 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:ffff88006b0ef0b8 EFLAGS: 00010246
> RAX: 1ffff1000d00f20b RBX: ffff880066dd96c0 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff88006b0ef5b8 RDI: ffff880068079058
> RBP: ffff88006b0ef1e0 R08: 0000000000000002 R09: 4e43247300000000
> R10: ffff88006b0ef0b8 R11: dffffc0000000000 R12: 0000000000090000
> R13: ffff880068078f00 R14: 0000000000000000 R15: dffffc0000000000
> FS:  00007fd68b4d5700(0000) GS:ffff88006cb00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000208e2fe0 CR3: 0000000064c9b000 CR4: 00000000000006e0
> 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;
                                }

[toc] | [next] | [standalone]

#1635192

FromDavid Ahern <dsahern@gmail.com>
Date2017-05-03 20:30 +0200
Message-ID<tD7ce-Ai-11@gated-at.bofh.it>
In reply to#1635141
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;
                        }

[toc] | [prev] | [next] | [standalone]

#1635300

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-05-04 00:10 +0200
Message-ID<tDaD8-2VX-7@gated-at.bofh.it>
In reply to#1635192
On Wed, May 3, 2017 at 11:22 AM, David Ahern <dsahern@gmail.com> 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.

[toc] | [prev] | [next] | [standalone]

#1635301

FromDavid Ahern <dsahern@gmail.com>
Date2017-05-04 00:10 +0200
Message-ID<tDaD7-2VX-5@gated-at.bofh.it>
In reply to#1635300
On 5/3/17 4:02 PM, Cong Wang wrote:
> On Wed, May 3, 2017 at 11:22 AM, David Ahern <dsahern@gmail.com> 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 ;-)

[toc] | [prev] | [next] | [standalone]

#1635337

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-05-04 01:40 +0200
Message-ID<tDc2d-3Nr-1@gated-at.bofh.it>
In reply to#1635301
On Wed, May 3, 2017 at 3:09 PM, David Ahern <dsahern@gmail.com> wrote:
> On 5/3/17 4:02 PM, Cong Wang wrote:
>> On Wed, May 3, 2017 at 11:22 AM, David Ahern <dsahern@gmail.com> 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!! */
 };

[toc] | [prev] | [next] | [standalone]

#1635385

FromDavid Ahern <dsahern@gmail.com>
Date2017-05-04 04:50 +0200
Message-ID<tDf05-5JP-1@gated-at.bofh.it>
In reply to#1635337
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.

[toc] | [prev] | [next] | [standalone]

#1635399

FromCong Wang <xiyou.wangcong@gmail.com>
Date2017-05-04 06:00 +0200
Message-ID<tDg5Q-6pk-5@gated-at.bofh.it>
In reply to#1635385
On Wed, May 3, 2017 at 7:43 PM, David Ahern <dsahern@gmail.com> 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?

[toc] | [prev] | [next] | [standalone]

#1635403

FromDavid Ahern <dsahern@gmail.com>
Date2017-05-04 06:20 +0200
Message-ID<tDgpb-6LM-7@gated-at.bofh.it>
In reply to#1635399
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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web