* [PATCH net] ipv6: fix a dst leak when removing its exception
@ 2018-11-13 16:48 Xin Long
2018-11-14 19:03 ` David Ahern
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Xin Long @ 2018-11-13 16:48 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern
These is no need to hold dst before calling rt6_remove_exception_rt().
The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
which has been removed in Commit 93531c674315 ("net/ipv6: separate
handling of FIB entries from dst based routes"). Otherwise, it will
cause a dst leak.
This patch is to simply remove the dst_hold_safe() call before calling
rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
It's safe, because the removal of the exception that holds its dst's
refcnt is protected by rt6_exception_lock.
Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
Reported-by: Li Shuang <shuali@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/ipv6/route.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 2a7423c..14b422f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2232,8 +2232,7 @@ static void ip6_link_failure(struct sk_buff *skb)
if (rt) {
rcu_read_lock();
if (rt->rt6i_flags & RTF_CACHE) {
- if (dst_hold_safe(&rt->dst))
- rt6_remove_exception_rt(rt);
+ rt6_remove_exception_rt(rt);
} else {
struct fib6_info *from;
struct fib6_node *fn;
@@ -3214,8 +3213,8 @@ static int ip6_del_cached_rt(struct rt6_info *rt, struct fib6_config *cfg)
if (cfg->fc_flags & RTF_GATEWAY &&
!ipv6_addr_equal(&cfg->fc_gateway, &rt->rt6i_gateway))
goto out;
- if (dst_hold_safe(&rt->dst))
- rc = rt6_remove_exception_rt(rt);
+
+ rc = rt6_remove_exception_rt(rt);
out:
return rc;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-13 16:48 [PATCH net] ipv6: fix a dst leak when removing its exception Xin Long
@ 2018-11-14 19:03 ` David Ahern
2018-11-15 6:33 ` David Ahern
2018-11-15 19:49 ` David Ahern
2018-11-17 3:45 ` David Miller
2 siblings, 1 reply; 10+ messages in thread
From: David Ahern @ 2018-11-14 19:03 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem
On 11/13/18 8:48 AM, Xin Long wrote:
> These is no need to hold dst before calling rt6_remove_exception_rt().
> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
> which has been removed in Commit 93531c674315 ("net/ipv6: separate
> handling of FIB entries from dst based routes"). Otherwise, it will
> cause a dst leak.
>
> This patch is to simply remove the dst_hold_safe() call before calling
> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
> It's safe, because the removal of the exception that holds its dst's
> refcnt is protected by rt6_exception_lock.
>
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
> Reported-by: Li Shuang <shuali@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/ipv6/route.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
was this problem actually hit or is this patch based on a code analysis?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-14 19:03 ` David Ahern
@ 2018-11-15 6:33 ` David Ahern
2018-11-15 7:23 ` Xin Long
0 siblings, 1 reply; 10+ messages in thread
From: David Ahern @ 2018-11-15 6:33 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem
On 11/14/18 11:03 AM, David Ahern wrote:
> On 11/13/18 8:48 AM, Xin Long wrote:
>> These is no need to hold dst before calling rt6_remove_exception_rt().
>> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
>> which has been removed in Commit 93531c674315 ("net/ipv6: separate
>> handling of FIB entries from dst based routes"). Otherwise, it will
>> cause a dst leak.
>>
>> This patch is to simply remove the dst_hold_safe() call before calling
>> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
>> It's safe, because the removal of the exception that holds its dst's
>> refcnt is protected by rt6_exception_lock.
>>
>> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
>> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
>> Reported-by: Li Shuang <shuali@redhat.com>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/ipv6/route.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> was this problem actually hit or is this patch based on a code analysis?
>
I ask because I have not been able to reproduce the leak using existing
tests (e.g., pmtu) that I know create exceptions.
If this problem was hit, it would be good to get a test case for it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-15 6:33 ` David Ahern
@ 2018-11-15 7:23 ` Xin Long
2018-11-15 18:17 ` David Ahern
2018-11-20 2:16 ` David Miller
0 siblings, 2 replies; 10+ messages in thread
From: Xin Long @ 2018-11-15 7:23 UTC (permalink / raw)
To: David Ahern; +Cc: network dev, davem
[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]
On Thu, Nov 15, 2018 at 3:33 PM David Ahern <dsa@cumulusnetworks.com> wrote:
>
> On 11/14/18 11:03 AM, David Ahern wrote:
> > On 11/13/18 8:48 AM, Xin Long wrote:
> >> These is no need to hold dst before calling rt6_remove_exception_rt().
> >> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
> >> which has been removed in Commit 93531c674315 ("net/ipv6: separate
> >> handling of FIB entries from dst based routes"). Otherwise, it will
> >> cause a dst leak.
> >>
> >> This patch is to simply remove the dst_hold_safe() call before calling
> >> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
> >> It's safe, because the removal of the exception that holds its dst's
> >> refcnt is protected by rt6_exception_lock.
> >>
> >> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> >> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
> >> Reported-by: Li Shuang <shuali@redhat.com>
> >> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> >> ---
> >> net/ipv6/route.c | 7 +++----
> >> 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > was this problem actually hit or is this patch based on a code analysis?
> >
>
> I ask because I have not been able to reproduce the leak using existing
> tests (e.g., pmtu) that I know create exceptions.
>
> If this problem was hit, it would be good to get a test case for it.
The attachment is the ip6_dst.sh with IPVS.
# sh ip6_dst.sh
But this one triggers the kernel warnings caused by 2 places:
unregister_netdevice: waiting for br0 to become free. Usage count = 3
1. one is IPVS, I just posted the fix:
https://patchwork.ozlabs.org/patch/998123/ [1]
2. the other one is IPv6,
ip6_link_failure() will be hit.
So to make this reproduce clearly, you may want to apply
patch [1] firstly.
[-- Attachment #2: ip6_dst.sh --]
[-- Type: application/x-sh, Size: 2734 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-15 7:23 ` Xin Long
@ 2018-11-15 18:17 ` David Ahern
2018-11-15 19:13 ` Mika Penttilä
2018-11-20 2:16 ` David Miller
1 sibling, 1 reply; 10+ messages in thread
From: David Ahern @ 2018-11-15 18:17 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem
On 11/14/18 11:23 PM, Xin Long wrote:
> On Thu, Nov 15, 2018 at 3:33 PM David Ahern <dsa@cumulusnetworks.com> wrote:
>>
>> On 11/14/18 11:03 AM, David Ahern wrote:
>>> On 11/13/18 8:48 AM, Xin Long wrote:
>>>> These is no need to hold dst before calling rt6_remove_exception_rt().
>>>> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
>>>> which has been removed in Commit 93531c674315 ("net/ipv6: separate
>>>> handling of FIB entries from dst based routes"). Otherwise, it will
>>>> cause a dst leak.
>>>>
>>>> This patch is to simply remove the dst_hold_safe() call before calling
>>>> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
>>>> It's safe, because the removal of the exception that holds its dst's
>>>> refcnt is protected by rt6_exception_lock.
>>>>
>>>> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
>>>> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
>>>> Reported-by: Li Shuang <shuali@redhat.com>
>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>> ---
>>>> net/ipv6/route.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> was this problem actually hit or is this patch based on a code analysis?
>>>
>>
>> I ask because I have not been able to reproduce the leak using existing
>> tests (e.g., pmtu) that I know create exceptions.
>>
>> If this problem was hit, it would be good to get a test case for it.
> The attachment is the ip6_dst.sh with IPVS.
>
> # sh ip6_dst.sh
>
> But this one triggers the kernel warnings caused by 2 places:
> unregister_netdevice: waiting for br0 to become free. Usage count = 3
>
> 1. one is IPVS, I just posted the fix:
> https://patchwork.ozlabs.org/patch/998123/ [1]
> 2. the other one is IPv6,
> ip6_link_failure() will be hit.
>
> So to make this reproduce clearly, you may want to apply
> patch [1] firstly.
>
Thanks for the script. It does not replicate the problem using net-next
tree as of
commit 6d5db6c37929cb0a84e64ba0590a74593e5ce3b8
Merge: 15cef30974c5 bd3b5d462add
Author: David S. Miller <davem@davemloft.net>
Date: Wed Nov 14 08:51:28 2018 -0800
Merge branch 'nfp-abm-track-all-Qdiscs'
I would be really surprised if the fib6_info change introduced a need to
change the dst hold's for exception routes. I am not seeing the
connection, so I really want to see it reproduced.
Thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-15 18:17 ` David Ahern
@ 2018-11-15 19:13 ` Mika Penttilä
0 siblings, 0 replies; 10+ messages in thread
From: Mika Penttilä @ 2018-11-15 19:13 UTC (permalink / raw)
To: David Ahern, Xin Long; +Cc: network dev, davem
On 15.11.2018 20.17, David Ahern wrote:
> On 11/14/18 11:23 PM, Xin Long wrote:
>> On Thu, Nov 15, 2018 at 3:33 PM David Ahern <dsa@cumulusnetworks.com> wrote:
>>> On 11/14/18 11:03 AM, David Ahern wrote:
>>>> On 11/13/18 8:48 AM, Xin Long wrote:
>>>>> These is no need to hold dst before calling rt6_remove_exception_rt().
>>>>> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
>>>>> which has been removed in Commit 93531c674315 ("net/ipv6: separate
>>>>> handling of FIB entries from dst based routes"). Otherwise, it will
>>>>> cause a dst leak.
>>>>>
>>>>> This patch is to simply remove the dst_hold_safe() call before calling
>>>>> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
>>>>> It's safe, because the removal of the exception that holds its dst's
>>>>> refcnt is protected by rt6_exception_lock.
>>>>>
>>>>> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
>>>>> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
>>>>> Reported-by: Li Shuang <shuali@redhat.com>
>>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>>>> ---
>>>>> net/ipv6/route.c | 7 +++----
>>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>> was this problem actually hit or is this patch based on a code analysis?
>>>>
>>> I ask because I have not been able to reproduce the leak using existing
>>> tests (e.g., pmtu) that I know create exceptions.
>>>
>>> If this problem was hit, it would be good to get a test case for it.
>> The attachment is the ip6_dst.sh with IPVS.
>>
>> # sh ip6_dst.sh
>>
>> But this one triggers the kernel warnings caused by 2 places:
>> unregister_netdevice: waiting for br0 to become free. Usage count = 3
>>
>> 1. one is IPVS, I just posted the fix:
>> https://patchwork.ozlabs.org/patch/998123/ [1]
>> 2. the other one is IPv6,
>> ip6_link_failure() will be hit.
>>
>> So to make this reproduce clearly, you may want to apply
>> patch [1] firstly.
>>
> Thanks for the script. It does not replicate the problem using net-next
> tree as of
>
> commit 6d5db6c37929cb0a84e64ba0590a74593e5ce3b8
> Merge: 15cef30974c5 bd3b5d462add
> Author: David S. Miller <davem@davemloft.net>
> Date: Wed Nov 14 08:51:28 2018 -0800
>
> Merge branch 'nfp-abm-track-all-Qdiscs'
>
>
> I would be really surprised if the fib6_info change introduced a need to
> change the dst hold's for exception routes. I am not seeing the
> connection, so I really want to see it reproduced.
>
> Thanks
Maybe it's not 100% reproducer then, but I think the fix is obviously right.
--Mika
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-13 16:48 [PATCH net] ipv6: fix a dst leak when removing its exception Xin Long
2018-11-14 19:03 ` David Ahern
@ 2018-11-15 19:49 ` David Ahern
2018-11-17 3:45 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: David Ahern @ 2018-11-15 19:49 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: David Miller, Mika Penttilä
On 11/13/18 8:48 AM, Xin Long wrote:
> These is no need to hold dst before calling rt6_remove_exception_rt().
> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
> which has been removed in Commit 93531c674315 ("net/ipv6: separate
> handling of FIB entries from dst based routes"). Otherwise, it will
> cause a dst leak.
>
> This patch is to simply remove the dst_hold_safe() call before calling
> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
> It's safe, because the removal of the exception that holds its dst's
> refcnt is protected by rt6_exception_lock.
>
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
> Reported-by: Li Shuang <shuali@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/ipv6/route.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
Ok, I see now. commit ad65a2f05695 add the dst_hold_safe with
ip6_del_rt. ip6_del_rt called ip6_rt_put to release the reference taken
by the hold_safe. Those paths are gone now.
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-13 16:48 [PATCH net] ipv6: fix a dst leak when removing its exception Xin Long
2018-11-14 19:03 ` David Ahern
2018-11-15 19:49 ` David Ahern
@ 2018-11-17 3:45 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2018-11-17 3:45 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, dsa
From: Xin Long <lucien.xin@gmail.com>
Date: Wed, 14 Nov 2018 00:48:28 +0800
> These is no need to hold dst before calling rt6_remove_exception_rt().
> The call to dst_hold_safe() in ip6_link_failure() was for ip6_del_rt(),
> which has been removed in Commit 93531c674315 ("net/ipv6: separate
> handling of FIB entries from dst based routes"). Otherwise, it will
> cause a dst leak.
>
> This patch is to simply remove the dst_hold_safe() call before calling
> rt6_remove_exception_rt() and also do the same in ip6_del_cached_rt().
> It's safe, because the removal of the exception that holds its dst's
> refcnt is protected by rt6_exception_lock.
>
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Fixes: 23fb93a4d3f1 ("net/ipv6: Cleanup exception and cache route handling")
> Reported-by: Li Shuang <shuali@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-15 7:23 ` Xin Long
2018-11-15 18:17 ` David Ahern
@ 2018-11-20 2:16 ` David Miller
2018-11-20 2:19 ` David Ahern
1 sibling, 1 reply; 10+ messages in thread
From: David Miller @ 2018-11-20 2:16 UTC (permalink / raw)
To: lucien.xin; +Cc: dsa, netdev
From: Xin Long <lucien.xin@gmail.com>
Date: Thu, 15 Nov 2018 16:23:38 +0900
> The attachment is the ip6_dst.sh with IPVS.
>
> # sh ip6_dst.sh
Maybe a selftests candidate?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] ipv6: fix a dst leak when removing its exception
2018-11-20 2:16 ` David Miller
@ 2018-11-20 2:19 ` David Ahern
0 siblings, 0 replies; 10+ messages in thread
From: David Ahern @ 2018-11-20 2:19 UTC (permalink / raw)
To: David Miller, lucien.xin; +Cc: netdev
On 11/19/18 7:16 PM, David Miller wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Thu, 15 Nov 2018 16:23:38 +0900
>
>> The attachment is the ip6_dst.sh with IPVS.
>>
>> # sh ip6_dst.sh
>
> Maybe a selftests candidate?
>
That script was not a reliable reproducer for me.
I created a much simpler one that shows the problem every time - and
proves the change. It needs some adjustments to work as a selftest.
Specifically, it needs to check dmesg for net_device reference counts
after the rcu delay. I'll add to the to-do list.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-11-20 12:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13 16:48 [PATCH net] ipv6: fix a dst leak when removing its exception Xin Long
2018-11-14 19:03 ` David Ahern
2018-11-15 6:33 ` David Ahern
2018-11-15 7:23 ` Xin Long
2018-11-15 18:17 ` David Ahern
2018-11-15 19:13 ` Mika Penttilä
2018-11-20 2:16 ` David Miller
2018-11-20 2:19 ` David Ahern
2018-11-15 19:49 ` David Ahern
2018-11-17 3:45 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.