Refcount mismatch when unregistering netdevice from kernel

Refcount mismatch when unregistering netdevice from kernel

[stranche]

Hi everyone,

We've recently been investigating a refcount problem when unregistering 
a netdevice from the kernel. It seems that the IPv6 module is still 
holding various references to the inet6_dev associated with the main 
netdevice struct that are not being released, preventing the 
unregistration from completing.

After tracing the various locations that take/release refcounts to these 
two structs, we see that there are mismatches in the refcount for the 
inet6_dev in the DST path when there are routes flushed with the 
rt6_uncached_list_flush_dev() function during rt6_disable_ip() when the 
device is unregistering. It seems that usually these references are 
freed via ip6_dst_ifdown() in the dst_ops->ifdown callback from 
dst_dev_put(), but this callback is not executed in the 
rt6_uncached_list_flush_dev() function. Instead, a reference to the 
inet6_dev is simply dropped to account for the inet6_dev_get() in 
ip6_rt_copy_init().

Unfortunately, this does not seem to be sufficient, as these uncached 
routes have an additional refcount on the inet6_device attached to them 
from the DST allocation. In the normal case, this reference from the DST 
allocation will happen in the dst_ops->destroy() callback in the 
dst_destroy() function when the DST is being freed. However, since 
rt6_uncached_list_flush_dev() changes the inet6_device stored in the DST 
to the loopback device, the dst_ops->destroy() callback doesn't 
decrement the refcount on the correct inet6_dev struct.

We're wondering if it would be appropriate to put() the refcount 
additionally for the uncached routes when flushing out the list for the 
unregistering device. Perhaps something like the following?

diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 6602f43..554b07b 
100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -156,9 +156,11 @@ void rt6_uncached_list_del(struct rt6_info *rt)

   char rt6_uncached_list_flush_dev_log1[1000][512];
   int rt6_uncached_list_flush_dev_log1_iter = 0; -static void 
rt6_uncached_list_flush_dev(struct net *net, struct net_device *dev)
+static void rt6_uncached_list_flush_dev(struct net *net, struct 
net_device *dev,
+                                        unsigned long event)
   {
          struct net_device *loopback_dev = net->loopback_dev;
+       bool unreg = event == NETDEV_UNREGISTER;
          int cpu;

          if (dev == loopback_dev)
@@ -190,6 +192,10 @@ static void rt6_uncached_list_flush_dev(struct net 
*net, struct net_device *dev)
                          }

                          if (rt_idev->dev == dev) {
+                               if (rt->dst.ops->ifdown)
+                                       rt->dst.ops->ifdown(&rt->dst, 
dev,
+                                                           unreg);
+
                                  rt->rt6i_idev = 
in6_dev_get(loopback_dev);
                                  in6_dev_put(rt_idev);
                          }
@@ -4781,7 +4787,7 @@ void rt6_sync_down_dev(struct net_device *dev, 
unsigned long event)
   void rt6_disable_ip(struct net_device *dev, unsigned long event)
   {
          rt6_sync_down_dev(dev, event);
-       rt6_uncached_list_flush_dev(dev_net(dev), dev);
+       rt6_uncached_list_flush_dev(dev_net(dev), dev, event);
          neigh_ifdown(&nd_tbl, dev);
   }


For reference, here are some samples of the refcounts on the 
inet6_device. In this log we saw the inet6_device had a final refcount 
of 4 while unregistering.
holds                 puts
ip6_rt_copy_init 17   13 ip6_dst_ifdown
xfrm6_fill_dst   6     5 xfrm6_dst_destroy
                        1 rt6_disable_ip
icmp6_dst_alloc  28   25 ip6_dst_destroy
                        3 rt6_disable_ip

Thanks,
Sean

Re: Refcount mismatch when unregistering netdevice from kernel

[Eric Dumazet]

On 12/8/20 4:55 AM, stranche@codeaurora.org wrote:
> Hi everyone,
> 
> We've recently been investigating a refcount problem when unregistering a netdevice from the kernel. It seems that the IPv6 module is still holding various references to the inet6_dev associated with the main netdevice struct that are not being released, preventing the unregistration from completing.
> 
> After tracing the various locations that take/release refcounts to these two structs, we see that there are mismatches in the refcount for the inet6_dev in the DST path when there are routes flushed with the rt6_uncached_list_flush_dev() function during rt6_disable_ip() when the device is unregistering. It seems that usually these references are freed via ip6_dst_ifdown() in the dst_ops->ifdown callback from dst_dev_put(), but this callback is not executed in the rt6_uncached_list_flush_dev() function. Instead, a reference to the inet6_dev is simply dropped to account for the inet6_dev_get() in ip6_rt_copy_init().
> 
> Unfortunately, this does not seem to be sufficient, as these uncached routes have an additional refcount on the inet6_device attached to them from the DST allocation. In the normal case, this reference from the DST allocation will happen in the dst_ops->destroy() callback in the dst_destroy() function when the DST is being freed. However, since rt6_uncached_list_flush_dev() changes the inet6_device stored in the DST to the loopback device, the dst_ops->destroy() callback doesn't decrement the refcount on the correct inet6_dev struct.
> 
> We're wondering if it would be appropriate to put() the refcount additionally for the uncached routes when flushing out the list for the unregistering device. Perhaps something like the following?
> 

dev refcount imbalances are quite common, particularly on old kernel versions, lacking few fixes.

First thing is to let us know on which kernel version you see this, and how you reproduce it.

Re: Refcount mismatch when unregistering netdevice from kernel

[Wei Wang]

Hi Sean,

Thanks for reporting the issue. Like Eric said, if you could provide
the kernel version you are using, that would be great. And if you have
a reproducer, that would be even better.
I have a few comments inline below.

On Tue, Dec 8, 2020 at 7:08 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 12/8/20 4:55 AM, stranche@codeaurora.org wrote:
> > Hi everyone,
> >
> > We've recently been investigating a refcount problem when unregistering a netdevice from the kernel. It seems that the IPv6 module is still holding various references to the inet6_dev associated with the main netdevice struct that are not being released, preventing the unregistration from completing.
> >
> > After tracing the various locations that take/release refcounts to these two structs, we see that there are mismatches in the refcount for the inet6_dev in the DST path when there are routes flushed with the rt6_uncached_list_flush_dev() function during rt6_disable_ip() when the device is unregistering. It seems that usually these references are freed via ip6_dst_ifdown() in the dst_ops->ifdown callback from dst_dev_put(), but this callback is not executed in the rt6_uncached_list_flush_dev() function. Instead, a reference to the inet6_dev is simply dropped to account for the inet6_dev_get() in ip6_rt_copy_init().
> >

rt6_uncached_list_flush_dev() actually tries to replace the inet6_dev
with loopback_dev, and release the reference to the previous inet6_dev
by calling in6_dev_put(), which is actually doing the same thing as
ip6_dst_ifdown(). I don't understand why you say " a reference to the
inet6_dev is simply dropped".

> > Unfortunately, this does not seem to be sufficient, as these uncached routes have an additional refcount on the inet6_device attached to them from the DST allocation. In the normal case, this reference from the DST allocation will happen in the dst_ops->destroy() callback in the dst_destroy() function when the DST is being freed. However, since rt6_uncached_list_flush_dev() changes the inet6_device stored in the DST to the loopback device, the dst_ops->destroy() callback doesn't decrement the refcount on the correct inet6_dev struct.
> >

The additional refcount to the DST is also released by doing the following:
                        if (rt_dev == dev) {
                                rt->dst.dev = blackhole_netdev;
                                dev_hold(rt->dst.dev);
                                dev_put(rt_dev);
                        }
Am I missing something?


> > We're wondering if it would be appropriate to put() the refcount additionally for the uncached routes when flushing out the list for the unregistering device. Perhaps something like the following?
> >
>
> dev refcount imbalances are quite common, particularly on old kernel versions, lacking few fixes.
>
> First thing is to let us know on which kernel version you see this, and how you reproduce it.
>

Re: Refcount mismatch when unregistering netdevice from kernel

[stranche]

Hi Wei and Eric,

Thanks for the replies.

This was reported to us on the 5.4.61 kernel during a customer 
regression suite, so we don't have an exact reproducer unfortunately. 
 From the trace logs we've added it seems like this is happening during 
IPv6 transport mode XFRM data transfer and the device is unregistered in 
the middle of it, but we've been unable to reproduce it ourselves.. 
We're open to trying out and sharing debug patches if needed though.

> rt6_uncached_list_flush_dev() actually tries to replace the inet6_dev
> with loopback_dev, and release the reference to the previous inet6_dev
> by calling in6_dev_put(), which is actually doing the same thing as
> ip6_dst_ifdown(). I don't understand why you say " a reference to the
> inet6_dev is simply dropped".

Fair. I was going off the semantics used by the dst_dev_put() function 
which calls dst_ops->ifdown() explicitly. At least in the case of 
xfrm6_dst_ifdown() this swap of the loopback device and putting the 
refcount seems like it could be missing a few things.

> The additional refcount to the DST is also released by doing the 
> following:
>                         if (rt_dev == dev) {
>                                 rt->dst.dev = blackhole_netdev;
>                                 dev_hold(rt->dst.dev);
>                                 dev_put(rt_dev);
>                         }
> Am I missing something?

That dev_put() is on the actual netdevice struct, not the inet6_dev 
associated with it. We're seeing many calls to icmp6_dst_alloc() and 
xfrm6_fill_dst() here, both of which seem to associate a reference to 
the inet6_dev struct with the DST in addition to the standard dev_hold() 
on the netdevice during the dst_alloc()/dst_init().

Thanks,
Sean

Re: Refcount mismatch when unregistering netdevice from kernel

[Wei Wang]

On Tue, Dec 8, 2020 at 11:13 AM <stranche@codeaurora.org> wrote:
>
> Hi Wei and Eric,
>
> Thanks for the replies.
>
> This was reported to us on the 5.4.61 kernel during a customer
> regression suite, so we don't have an exact reproducer unfortunately.
>  From the trace logs we've added it seems like this is happening during
> IPv6 transport mode XFRM data transfer and the device is unregistered in
> the middle of it, but we've been unable to reproduce it ourselves..
> We're open to trying out and sharing debug patches if needed though.
>

I double checked 5.4.61, and I didn't find any missing fixes in this
area AFAICT.

> > rt6_uncached_list_flush_dev() actually tries to replace the inet6_dev
> > with loopback_dev, and release the reference to the previous inet6_dev
> > by calling in6_dev_put(), which is actually doing the same thing as
> > ip6_dst_ifdown(). I don't understand why you say " a reference to the
> > inet6_dev is simply dropped".
>
> Fair. I was going off the semantics used by the dst_dev_put() function
> which calls dst_ops->ifdown() explicitly. At least in the case of
> xfrm6_dst_ifdown() this swap of the loopback device and putting the
> refcount seems like it could be missing a few things.
>

Looking more into the xfrm code, I think the major difference between
xfrm dst and non-xfrm dst is that, xfrm code creates a list of dst
entries in one dst_entry linked by xfrm_dst_child().
In xfrm_bundle_create(), which I believe is the main function to
create xfrm dst bundles, it allocates the main dst entry and its
children, and it calls xfrm_fill_dst() for each of them. So I think
each dst in the list (including all the children) are added into the
uncached_list.
The difference between the current code in
rt6_uncached_list_flush_dev() vs dst_ops->ifdown() is that, the
current code only releases the refcnt to inet6_dev associated with the
main dst, while xfrm6_dst_ifdown() tries to release inet6_dev
associated with every dst linked by xfrm_dst_child(). However, since
xfrm_bundle_create() anyway adds each child dst to the uncached list,
I don't see how the current code could miss any refcnt.
BTW, have you tried your previous proposed patch and confirmed it
would fix the issue?

> > The additional refcount to the DST is also released by doing the
> > following:
> >                         if (rt_dev == dev) {
> >                                 rt->dst.dev = blackhole_netdev;
> >                                 dev_hold(rt->dst.dev);
> >                                 dev_put(rt_dev);
> >                         }
> > Am I missing something?
>
> That dev_put() is on the actual netdevice struct, not the inet6_dev
> associated with it. We're seeing many calls to icmp6_dst_alloc() and
> xfrm6_fill_dst() here, both of which seem to associate a reference to
> the inet6_dev struct with the DST in addition to the standard dev_hold()
> on the netdevice during the dst_alloc()/dst_init().
>

Could we further distinguish between dst added to the uncached list by
icmp6_dst_alloc() and xfrm6_fill_dst(), and confirm which ones are the
ones leaking reference?
I suspect it would be the xfrm ones, but I think it is worth verifying.


> Thanks,
> Sean

Re: Refcount mismatch when unregistering netdevice from kernel

[David Ahern]

On 12/8/20 2:51 PM, Wei Wang wrote:
> On Tue, Dec 8, 2020 at 11:13 AM <stranche@codeaurora.org> wrote:
>>
>> Hi Wei and Eric,
>>
>> Thanks for the replies.
>>
>> This was reported to us on the 5.4.61 kernel during a customer
>> regression suite, so we don't have an exact reproducer unfortunately.
>>  From the trace logs we've added it seems like this is happening during
>> IPv6 transport mode XFRM data transfer and the device is unregistered in
>> the middle of it, but we've been unable to reproduce it ourselves..
>> We're open to trying out and sharing debug patches if needed though.
>>
> 
> I double checked 5.4.61, and I didn't find any missing fixes in this
> area AFAICT.
> 
>>> rt6_uncached_list_flush_dev() actually tries to replace the inet6_dev
>>> with loopback_dev, and release the reference to the previous inet6_dev
>>> by calling in6_dev_put(), which is actually doing the same thing as
>>> ip6_dst_ifdown(). I don't understand why you say " a reference to the
>>> inet6_dev is simply dropped".
>>
>> Fair. I was going off the semantics used by the dst_dev_put() function
>> which calls dst_ops->ifdown() explicitly. At least in the case of
>> xfrm6_dst_ifdown() this swap of the loopback device and putting the
>> refcount seems like it could be missing a few things.
>>
> 
> Looking more into the xfrm code, I think the major difference between
> xfrm dst and non-xfrm dst is that, xfrm code creates a list of dst
> entries in one dst_entry linked by xfrm_dst_child().
> In xfrm_bundle_create(), which I believe is the main function to
> create xfrm dst bundles, it allocates the main dst entry and its
> children, and it calls xfrm_fill_dst() for each of them. So I think
> each dst in the list (including all the children) are added into the
> uncached_list.
> The difference between the current code in
> rt6_uncached_list_flush_dev() vs dst_ops->ifdown() is that, the
> current code only releases the refcnt to inet6_dev associated with the
> main dst, while xfrm6_dst_ifdown() tries to release inet6_dev
> associated with every dst linked by xfrm_dst_child(). However, since
> xfrm_bundle_create() anyway adds each child dst to the uncached list,
> I don't see how the current code could miss any refcnt.
> BTW, have you tried your previous proposed patch and confirmed it
> would fix the issue?
> 
>>> The additional refcount to the DST is also released by doing the
>>> following:
>>>                         if (rt_dev == dev) {
>>>                                 rt->dst.dev = blackhole_netdev;
>>>                                 dev_hold(rt->dst.dev);
>>>                                 dev_put(rt_dev);
>>>                         }
>>> Am I missing something?
>>
>> That dev_put() is on the actual netdevice struct, not the inet6_dev
>> associated with it. We're seeing many calls to icmp6_dst_alloc() and
>> xfrm6_fill_dst() here, both of which seem to associate a reference to
>> the inet6_dev struct with the DST in addition to the standard dev_hold()
>> on the netdevice during the dst_alloc()/dst_init().
>>
> 
> Could we further distinguish between dst added to the uncached list by
> icmp6_dst_alloc() and xfrm6_fill_dst(), and confirm which ones are the
> ones leaking reference?
> I suspect it would be the xfrm ones, but I think it is worth verifying.
> 

Finally found the reference:

tools/testing/selftests/net/l2tp.sh at one point was triggering a
refcount leak:

https://lore.kernel.org/netdev/20190801235421.8344-1-dsahern@kernel.org/

And then Colin found more problems with it:

https://lore.kernel.org/netdev/450f5abb-5fe8-158d-d267-4334e15f8e58@canonical.com/


running that on a 5.8 kernel on Ubuntu 20.10 did not trigger the
problem. Neither did Ubuntu 20.04 with 5.4.0-51-generic.

Can you run it on your 5.4 version and see?

Re: Refcount mismatch when unregistering netdevice from kernel

[stranche]

>> BTW, have you tried your previous proposed patch and confirmed it
>> would fix the issue?
>> 

Yes, we shared this with the customer and the refcount mismatch still 
occurred, so this doesn't seem sufficient either.

>> Could we further distinguish between dst added to the uncached list by
>> icmp6_dst_alloc() and xfrm6_fill_dst(), and confirm which ones are the
>> ones leaking reference?
>> I suspect it would be the xfrm ones, but I think it is worth 
>> verifying.
>> 

After digging into the DST allocation/destroy a bit more, it seems that 
there are some cases where the DST's refcount does not hit zero, causing 
them to never be freed and release their references.
One case comes from here on the IPv6 packet output path (these DST 
structs would hold references to both the inet6_dev and the netdevice)
ip6_pol_route_output+0x20/0x2c -> ip6_pol_route+0x1dc/0x34c -> 
rt6_make_pcpu_route+0x18/0xf4 -> ip6_rt_pcpu_alloc+0xb4/0x19c

We also see two DSTs where they are stored as the xdst->rt entry on the 
XFRM path that do not get released. One is allocated by the same path as 
above, and the other like this
xfrm6_esp_err+0x7c/0xd4 -> esp6_err+0xc8/0x100 -> 
ip6_update_pmtu+0xc8/0x100 -> __ip6_rt_update_pmtu+0x248/0x434 -> 
ip6_rt_cache_alloc+0xa0/0x1dc

 From those alloc paths it seems like the problem might not be coming 
from the uncached list after all.

> 
> Finally found the reference:
> 
> tools/testing/selftests/net/l2tp.sh at one point was triggering a
> refcount leak:
> 
> https://lore.kernel.org/netdev/20190801235421.8344-1-dsahern@kernel.org/
> 
> And then Colin found more problems with it:
> 
> https://lore.kernel.org/netdev/450f5abb-5fe8-158d-d267-4334e15f8e58@canonical.com/
> 
> 
> running that on a 5.8 kernel on Ubuntu 20.10 did not trigger the
> problem. Neither did Ubuntu 20.04 with 5.4.0-51-generic.
> 
> Can you run it on your 5.4 version and see?

We let that run for two days on our setup and didn't see anything, 
unfortunately.

Thanks,
Sean

Re: Refcount mismatch when unregistering netdevice from kernel

[David Ahern]

On 12/10/20 6:12 PM, stranche@codeaurora.org wrote:
>>> BTW, have you tried your previous proposed patch and confirmed it
>>> would fix the issue?
>>>
> 
> Yes, we shared this with the customer and the refcount mismatch still
> occurred, so this doesn't seem sufficient either.
> 
>>> Could we further distinguish between dst added to the uncached list by
>>> icmp6_dst_alloc() and xfrm6_fill_dst(), and confirm which ones are the
>>> ones leaking reference?
>>> I suspect it would be the xfrm ones, but I think it is worth verifying.
>>>
> 
> After digging into the DST allocation/destroy a bit more, it seems that
> there are some cases where the DST's refcount does not hit zero, causing
> them to never be freed and release their references.
> One case comes from here on the IPv6 packet output path (these DST
> structs would hold references to both the inet6_dev and the netdevice)
> ip6_pol_route_output+0x20/0x2c -> ip6_pol_route+0x1dc/0x34c ->
> rt6_make_pcpu_route+0x18/0xf4 -> ip6_rt_pcpu_alloc+0xb4/0x19c

This is the normal data path, and this refers to a per-cpu dst cache.
Delete the route and the cached entries get removed.

> 
> We also see two DSTs where they are stored as the xdst->rt entry on the
> XFRM path that do not get released. One is allocated by the same path as
> above, and the other like this
> xfrm6_esp_err+0x7c/0xd4 -> esp6_err+0xc8/0x100 ->
> ip6_update_pmtu+0xc8/0x100 -> __ip6_rt_update_pmtu+0x248/0x434 ->
> ip6_rt_cache_alloc+0xa0/0x1dc

This entry goes into an exception cache. I have lost track of kernel
versions and features. Try listing the route cache to see these:  ip -6
ro ls cache

Re: Refcount mismatch when unregistering netdevice from kernel

[stranche]

On 2020-12-11 09:10, David Ahern wrote:

>>>> Could we further distinguish between dst added to the uncached list 
>>>> by
>>>> icmp6_dst_alloc() and xfrm6_fill_dst(), and confirm which ones are 
>>>> the
>>>> ones leaking reference?
>>>> I suspect it would be the xfrm ones, but I think it is worth 
>>>> verifying.
>>>> 
>> 
>> After digging into the DST allocation/destroy a bit more, it seems 
>> that
>> there are some cases where the DST's refcount does not hit zero, 
>> causing
>> them to never be freed and release their references.
>> One case comes from here on the IPv6 packet output path (these DST
>> structs would hold references to both the inet6_dev and the netdevice)
>> ip6_pol_route_output+0x20/0x2c -> ip6_pol_route+0x1dc/0x34c ->
>> rt6_make_pcpu_route+0x18/0xf4 -> ip6_rt_pcpu_alloc+0xb4/0x19c
> 
> This is the normal data path, and this refers to a per-cpu dst cache.
> Delete the route and the cached entries get removed.
> 

After tracing all the DST entries created by the system, we've been able 
to see
that all unfreed DST entries belong to the same route on the system. One 
is the
main rt6_info struct it references and the rest are percpu copies of it.

>> 
>> We also see two DSTs where they are stored as the xdst->rt entry on 
>> the
>> XFRM path that do not get released. One is allocated by the same path 
>> as
>> above, and the other like this
>> xfrm6_esp_err+0x7c/0xd4 -> esp6_err+0xc8/0x100 ->
>> ip6_update_pmtu+0xc8/0x100 -> __ip6_rt_update_pmtu+0x248/0x434 ->
>> ip6_rt_cache_alloc+0xa0/0x1dc
> 
> This entry goes into an exception cache. I have lost track of kernel
> versions and features. Try listing the route cache to see these:  ip -6
> ro ls cache

Thanks for the tip here. We've further seen that the route that refers 
to these
unfreed DST is always a cached exception route. After tracing the routes 
as well,
we can see that the fib6_info struct for this route is never freed 
either, thus
preventing any of the DSTs associated with it from being cleaned up and 
releasing
their refcounts on the device. In fact, we can see that the fib6_info 
struct is no
longer present in the main fib6 tree after a period of time. The last 
time we're
able to see the pointer to the route in the tree is during a route 
replace
operation from userspace, but it seems that the fib6_info is not fully 
released.
In particular, the exception cache is not flushed out for the route 
during the
replace operation like it is during a standard fib6_del_route() call.

We're able to reproduce the refcount mismatch after some experimentation 
as well.
Essentially, it consists of
1) adding a default route (ip -6 route add dev XXX default)
2) forcing the creation of an exception route via manually injecting an 
ICMPv6
Packet Too Big into the device.
3) Replace the default route (ip -6 route change dev XXX default)
4) Delete the device. (ip link del XXX)

After adding a call to flush out the exception cache for the route, the 
mismatch
is no longer seen:
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 7a0c877..95e4310 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1215,6 +1215,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, 
struct fib6_info *rt,
                 }
                 nsiblings = iter->fib6_nsiblings;
                 iter->fib6_node = NULL;
+               rt6_flush_exceptions(iter);
                 fib6_purge_rt(iter, fn, info->nl_net);
                 if (rcu_access_pointer(fn->rr_ptr) == iter)
                         fn->rr_ptr = NULL;

Re: Refcount mismatch when unregistering netdevice from kernel

[David Ahern]

On 1/4/21 8:05 PM, stranche@codeaurora.org wrote:
> 
> We're able to reproduce the refcount mismatch after some experimentation
> as well.
> Essentially, it consists of
> 1) adding a default route (ip -6 route add dev XXX default)
> 2) forcing the creation of an exception route via manually injecting an
> ICMPv6
> Packet Too Big into the device.
> 3) Replace the default route (ip -6 route change dev XXX default)
> 4) Delete the device. (ip link del XXX)
> 
> After adding a call to flush out the exception cache for the route, the
> mismatch
> is no longer seen:
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 7a0c877..95e4310 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -1215,6 +1215,7 @@ static int fib6_add_rt2node(struct fib6_node *fn,
> struct fib6_info *rt,
>                 }
>                 nsiblings = iter->fib6_nsiblings;
>                 iter->fib6_node = NULL;
> +               rt6_flush_exceptions(iter);
>                 fib6_purge_rt(iter, fn, info->nl_net);
>                 if (rcu_access_pointer(fn->rr_ptr) == iter)
>                         fn->rr_ptr = NULL;

Ah, I see now. rt6_flush_exceptions is called by fib6_del_route, but
that won't handle replace.

If you look at fib6_purge_rt it already has a call to remove pcpu
entries. This call to flush exceptions should go there and the existing
one in fib6_del_route can be removed.

Also, can you add the reproducer as another test case to
tools/testing/selftests/net/pmtu.sh? We definitely need one for this
sequence (route, exceptions, replace route).

Re: Refcount mismatch when unregistering netdevice from kernel

[Wei Wang]

On Mon, Jan 4, 2021 at 8:58 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/4/21 8:05 PM, stranche@codeaurora.org wrote:
> >
> > We're able to reproduce the refcount mismatch after some experimentation
> > as well.
> > Essentially, it consists of
> > 1) adding a default route (ip -6 route add dev XXX default)
> > 2) forcing the creation of an exception route via manually injecting an
> > ICMPv6
> > Packet Too Big into the device.
> > 3) Replace the default route (ip -6 route change dev XXX default)
> > 4) Delete the device. (ip link del XXX)
> >
> > After adding a call to flush out the exception cache for the route, the
> > mismatch
> > is no longer seen:
> > diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> > index 7a0c877..95e4310 100644
> > --- a/net/ipv6/ip6_fib.c
> > +++ b/net/ipv6/ip6_fib.c
> > @@ -1215,6 +1215,7 @@ static int fib6_add_rt2node(struct fib6_node *fn,
> > struct fib6_info *rt,
> >                 }
> >                 nsiblings = iter->fib6_nsiblings;
> >                 iter->fib6_node = NULL;
> > +               rt6_flush_exceptions(iter);
> >                 fib6_purge_rt(iter, fn, info->nl_net);
> >                 if (rcu_access_pointer(fn->rr_ptr) == iter)
> >                         fn->rr_ptr = NULL;
>
> Ah, I see now. rt6_flush_exceptions is called by fib6_del_route, but
> that won't handle replace.
>
> If you look at fib6_purge_rt it already has a call to remove pcpu
> entries. This call to flush exceptions should go there and the existing
> one in fib6_del_route can be removed.
>
Thanks for catching this!
Agree with this proposed fix.


> Also, can you add the reproducer as another test case to
> tools/testing/selftests/net/pmtu.sh? We definitely need one for this
> sequence (route, exceptions, replace route).

Re: Refcount mismatch when unregistering netdevice from kernel

[Alexei Starovoitov]

On Tue, Jan 5, 2021 at 11:11 AM Wei Wang <weiwan@google.com> wrote:
>
> On Mon, Jan 4, 2021 at 8:58 PM David Ahern <dsahern@gmail.com> wrote:
> >
> > On 1/4/21 8:05 PM, stranche@codeaurora.org wrote:
> > >
> > > We're able to reproduce the refcount mismatch after some experimentation
> > > as well.
> > > Essentially, it consists of
> > > 1) adding a default route (ip -6 route add dev XXX default)
> > > 2) forcing the creation of an exception route via manually injecting an
> > > ICMPv6
> > > Packet Too Big into the device.
> > > 3) Replace the default route (ip -6 route change dev XXX default)
> > > 4) Delete the device. (ip link del XXX)
> > >
> > > After adding a call to flush out the exception cache for the route, the
> > > mismatch
> > > is no longer seen:
> > > diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> > > index 7a0c877..95e4310 100644
> > > --- a/net/ipv6/ip6_fib.c
> > > +++ b/net/ipv6/ip6_fib.c
> > > @@ -1215,6 +1215,7 @@ static int fib6_add_rt2node(struct fib6_node *fn,
> > > struct fib6_info *rt,
> > >                 }
> > >                 nsiblings = iter->fib6_nsiblings;
> > >                 iter->fib6_node = NULL;
> > > +               rt6_flush_exceptions(iter);
> > >                 fib6_purge_rt(iter, fn, info->nl_net);
> > >                 if (rcu_access_pointer(fn->rr_ptr) == iter)
> > >                         fn->rr_ptr = NULL;
> >
> > Ah, I see now. rt6_flush_exceptions is called by fib6_del_route, but
> > that won't handle replace.
> >
> > If you look at fib6_purge_rt it already has a call to remove pcpu
> > entries. This call to flush exceptions should go there and the existing
> > one in fib6_del_route can be removed.
> >
> Thanks for catching this!
> Agree with this proposed fix.

Looks like this fix never landed?
Is it still needed or there was an alternative fix merged?

Re: Refcount mismatch when unregistering netdevice from kernel

[Jakub Kicinski]

On Thu, 11 Feb 2021 11:21:26 -0800 Alexei Starovoitov wrote:
> On Tue, Jan 5, 2021 at 11:11 AM Wei Wang <weiwan@google.com> wrote:
> > On Mon, Jan 4, 2021 at 8:58 PM David Ahern <dsahern@gmail.com> wrote:  
> > > On 1/4/21 8:05 PM, stranche@codeaurora.org wrote:  
> > Ah, I see now. rt6_flush_exceptions is called by fib6_del_route, but
> > > that won't handle replace.
> > >
> > > If you look at fib6_purge_rt it already has a call to remove pcpu
> > > entries. This call to flush exceptions should go there and the existing
> > > one in fib6_del_route can be removed.
> > >  
> > Thanks for catching this!
> > Agree with this proposed fix.  
> 
> Looks like this fix never landed?
> Is it still needed or there was an alternative fix merged?

Wasn't it:

d8f5c29653c3 ("net: ipv6: fib: flush exceptions when purging route")

?

Re: Refcount mismatch when unregistering netdevice from kernel

[Alexei Starovoitov]

On Thu, Feb 11, 2021 at 5:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 11 Feb 2021 11:21:26 -0800 Alexei Starovoitov wrote:
> > On Tue, Jan 5, 2021 at 11:11 AM Wei Wang <weiwan@google.com> wrote:
> > > On Mon, Jan 4, 2021 at 8:58 PM David Ahern <dsahern@gmail.com> wrote:
> > > > On 1/4/21 8:05 PM, stranche@codeaurora.org wrote:
> > > Ah, I see now. rt6_flush_exceptions is called by fib6_del_route, but
> > > > that won't handle replace.
> > > >
> > > > If you look at fib6_purge_rt it already has a call to remove pcpu
> > > > entries. This call to flush exceptions should go there and the existing
> > > > one in fib6_del_route can be removed.
> > > >
> > > Thanks for catching this!
> > > Agree with this proposed fix.
> >
> > Looks like this fix never landed?
> > Is it still needed or there was an alternative fix merged?
>
> Wasn't it:
>
> d8f5c29653c3 ("net: ipv6: fib: flush exceptions when purging route")
>
> ?

Ahh. Thanks!