Re: Use after free in __dst_destroy_metrics_generic
On Sat, Sep 16, 2017 at 5:40 AM, Julian Anastasovwrote: > > Hello, > > On Fri, 15 Sep 2017, Subash Abhinov Kasiviswanathan wrote: > >> > May be I'm missing some posting but I don't see if >> > the patch was tested successfully. >> > >> Hi Julian >> >> I've had this patch being tested for the last 3-4 days in our regression rack >> and I haven't seen the same issue being reproduced or even a related crash >> or leak in dst. > > For now I see only its bug-hiding side effects, i.e. > it does not call kfree. Now if there is no double-free > there should be a leak, not for dst but for metrics. You make very good points. I need to take a deeper look. > >> The original issue was reported only once to us from the regression rack only >> so the exact steps to reproduce is unknown. > > OK, lets see, may be others can explain what happens. > This makes me thinking if it is possible that we no longer have the guarantee of metrics address aligned to at least 4? This seems unlikely since kmalloc() should return at least word-size-aligned address.
Re: Use after free in __dst_destroy_metrics_generic
On Fri, Sep 15, 2017 at 2:00 PM, Eric Dumazetwrote: > > Hi Cong > > I believe your patch makes a lot of sense, please submit it formally ? > I have been waiting for Subash's testing, since I myself never even run it.
Re: Use after free in __dst_destroy_metrics_generic
Hello, On Fri, 15 Sep 2017, Subash Abhinov Kasiviswanathan wrote: > > May be I'm missing some posting but I don't see if > > the patch was tested successfully. > > > Hi Julian > > I've had this patch being tested for the last 3-4 days in our regression rack > and I haven't seen the same issue being reproduced or even a related crash > or leak in dst. For now I see only its bug-hiding side effects, i.e. it does not call kfree. Now if there is no double-free there should be a leak, not for dst but for metrics. > The original issue was reported only once to us from the regression rack only > so the exact steps to reproduce is unknown. OK, lets see, may be others can explain what happens. Regards -- Julian Anastasov
Re: Use after free in __dst_destroy_metrics_generic
May be I'm missing some posting but I don't see if the patch was tested successfully. Regards -- Julian AnastasovHi Julian I've had this patch being tested for the last 3-4 days in our regression rack and I haven't seen the same issue being reproduced or even a related crash or leak in dst. The original issue was reported only once to us from the regression rack only so the exact steps to reproduce is unknown. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: Use after free in __dst_destroy_metrics_generic
Hello, On Fri, 15 Sep 2017, Eric Dumazet wrote: > On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote: > > On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan > >wrote: > > > We are seeing a possible use after free in ip6_dst_destroy. > > > > > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some > > > path > > > and allocated > > > to ion driver. ion driver has also freed it. Finally the memory is freed > > > by > > > the > > > fib gc and crashes since it is already deallocated. > > > > Does the attach (compile-only) patch help anything? > > > > From my _quick_ glance, it seems we miss the refcnt'ing > > right in __dst_destroy_metrics_generic(). > > > > Thanks! > > > Hi Cong > > I believe your patch makes a lot of sense, please submit it formally ? Cong's patch is wrong for few reasons: - it will stop to kfree non-refcounted metrics - report was for IPV6 and we set DST_METRICS_REFCOUNTED only for IPv4, for DST_METRICS_READ_ONLY metrics - __dst_destroy_metrics_generic is called for val without DST_METRICS_READ_ONLY flag and such metrics are not with DST_METRICS_REFCOUNTED flag - ->cow_metrics and dst_cow_metrics_generic are called with DST_METRICS_READ_ONLY flag set, there is no chance to write new value twice, especially to write DST_METRICS_REFCOUNTED flag and later to see this flag in __dst_destroy_metrics_generic So, I'm not sure where exactly is the bug with the metrics. May be I'm missing some posting but I don't see if the patch was tested successfully. Regards -- Julian Anastasov
Re: Use after free in __dst_destroy_metrics_generic
On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote: > On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan >wrote: > > We are seeing a possible use after free in ip6_dst_destroy. > > > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path > > and allocated > > to ion driver. ion driver has also freed it. Finally the memory is freed by > > the > > fib gc and crashes since it is already deallocated. > > Does the attach (compile-only) patch help anything? > > From my _quick_ glance, it seems we miss the refcnt'ing > right in __dst_destroy_metrics_generic(). > > Thanks! Hi Cong I believe your patch makes a lot of sense, please submit it formally ? Thanks !
Re: Use after free in __dst_destroy_metrics_generic
On 2017-09-08 10:10, Cong Wang wrote: On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathanwrote: We are seeing a possible use after free in ip6_dst_destroy. It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path and allocated to ion driver. ion driver has also freed it. Finally the memory is freed by the fib gc and crashes since it is already deallocated. Does the attach (compile-only) patch help anything? From my _quick_ glance, it seems we miss the refcnt'ing right in __dst_destroy_metrics_generic(). Thanks! Hi Cong Thanks for patch. I'll try this out. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: Use after free in __dst_destroy_metrics_generic
On Fri, 2017-09-08 at 10:19 -0700, David Miller wrote: > From: Eric Dumazet> Date: Fri, 08 Sep 2017 10:16:53 -0700 > > > On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote: > >> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan > >> wrote: > >> > We are seeing a possible use after free in ip6_dst_destroy. > >> > > >> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some > >> > path > >> > and allocated > >> > to ion driver. ion driver has also freed it. Finally the memory is freed > >> > by > >> > the > >> > fib gc and crashes since it is already deallocated. > >> > >> Does the attach (compile-only) patch help anything? > >> > >> From my _quick_ glance, it seems we miss the refcnt'ing > >> right in __dst_destroy_metrics_generic(). > >> > >> Thanks! > > > > But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing, > > since this was added in 4.12 > > It was backported via -stable. I hate working on lazy bug reports.
Re: Use after free in __dst_destroy_metrics_generic
From: Eric DumazetDate: Fri, 08 Sep 2017 10:16:53 -0700 > On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote: >> On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan >> wrote: >> > We are seeing a possible use after free in ip6_dst_destroy. >> > >> > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some >> > path >> > and allocated >> > to ion driver. ion driver has also freed it. Finally the memory is freed by >> > the >> > fib gc and crashes since it is already deallocated. >> >> Does the attach (compile-only) patch help anything? >> >> From my _quick_ glance, it seems we miss the refcnt'ing >> right in __dst_destroy_metrics_generic(). >> >> Thanks! > > But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing, > since this was added in 4.12 It was backported via -stable.
Re: Use after free in __dst_destroy_metrics_generic
On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote: > On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan >wrote: > > We are seeing a possible use after free in ip6_dst_destroy. > > > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path > > and allocated > > to ion driver. ion driver has also freed it. Finally the memory is freed by > > the > > fib gc and crashes since it is already deallocated. > > Does the attach (compile-only) patch help anything? > > From my _quick_ glance, it seems we miss the refcnt'ing > right in __dst_destroy_metrics_generic(). > > Thanks! But 4.9 kernels do not have yet the DST_METRICS_REFCOUNTED thing, since this was added in 4.12
Re: Use after free in __dst_destroy_metrics_generic
On Fri, 8 Sep 2017 09:12:09 -0700 Cong Wangwrote: > On Thu, Sep 7, 2017 at 5:56 PM, Stefano Brivio wrote: > > On Thu, 07 Sep 2017 18:52:02 -0600 > > Subash Abhinov Kasiviswanathan wrote: > > > >> We are seeing a possible use after free in ip6_dst_destroy. > >> > >> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some > >> path and allocated > >> to ion driver. ion driver has also freed it. Finally the memory is freed > >> by the > >> fib gc and crashes since it is already deallocated. > >> > >> Target is running an ARM64 Android based 4.9 kernel. > >> Issue was seen once on a regression rack (sorry, no reproducer). > >> Any pointers to debug this is highly appreciated. > >> > >> [ 3489.470581] [] object_err+0x4c/0x5c > >> [ 3489.470586] [] free_debug_processing+0x2e0/0x398 > >> [ 3489.470589] [] __slab_free+0x300/0x3e0 > >> [ 3489.470593] [] kfree+0x28c/0x290 > >> [ 3489.470601] [] > >> __dst_destroy_metrics_generic+0x6c/0x78 > >> [ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4 > > > > Should be fixed by: > > > > commit ad65a2f05695aced349e308193c6e2a6b1d87112 > > Author: Wei Wang > > Date: Sat Jun 17 10:42:35 2017 -0700 > > > > ipv6: call dst_hold_safe() properly > > Obviously it should not. One is dst metric, the other is dst. And obviously you're right. Sorry for the confusion, I blatantly misread the backtrace. -- Stefano
Re: Use after free in __dst_destroy_metrics_generic
On Thu, Sep 7, 2017 at 5:56 PM, Stefano Briviowrote: > On Thu, 07 Sep 2017 18:52:02 -0600 > Subash Abhinov Kasiviswanathan wrote: > >> We are seeing a possible use after free in ip6_dst_destroy. >> >> It appears as if memory of the __DST_METRICS_PTR(old) was freed in some >> path and allocated >> to ion driver. ion driver has also freed it. Finally the memory is freed >> by the >> fib gc and crashes since it is already deallocated. >> >> Target is running an ARM64 Android based 4.9 kernel. >> Issue was seen once on a regression rack (sorry, no reproducer). >> Any pointers to debug this is highly appreciated. >> >> [ 3489.470581] [] object_err+0x4c/0x5c >> [ 3489.470586] [] free_debug_processing+0x2e0/0x398 >> [ 3489.470589] [] __slab_free+0x300/0x3e0 >> [ 3489.470593] [] kfree+0x28c/0x290 >> [ 3489.470601] [] >> __dst_destroy_metrics_generic+0x6c/0x78 >> [ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4 > > Should be fixed by: > > commit ad65a2f05695aced349e308193c6e2a6b1d87112 > Author: Wei Wang > Date: Sat Jun 17 10:42:35 2017 -0700 > > ipv6: call dst_hold_safe() properly Obviously it should not. One is dst metric, the other is dst.
Re: Use after free in __dst_destroy_metrics_generic
On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathanwrote: > We are seeing a possible use after free in ip6_dst_destroy. > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path > and allocated > to ion driver. ion driver has also freed it. Finally the memory is freed by > the > fib gc and crashes since it is already deallocated. Does the attach (compile-only) patch help anything? >From my _quick_ glance, it seems we miss the refcnt'ing right in __dst_destroy_metrics_generic(). Thanks! diff --git a/net/core/dst.c b/net/core/dst.c index 00aa972ad1a1..b293aeae3018 100644 --- a/net/core/dst.c +++ b/net/core/dst.c @@ -241,8 +241,14 @@ void __dst_destroy_metrics_generic(struct dst_entry *dst, unsigned long old) new = ((unsigned long) _default_metrics) | DST_METRICS_READ_ONLY; prev = cmpxchg(>_metrics, old, new); - if (prev == old) - kfree(__DST_METRICS_PTR(old)); + if (prev == old) { + struct dst_metrics *old_p = (struct dst_metrics *)__DST_METRICS_PTR(old); + + if (prev & DST_METRICS_REFCOUNTED) { + if (atomic_dec_and_test(_p->refcnt)) + kfree(old_p); + } + } } EXPORT_SYMBOL(__dst_destroy_metrics_generic);
Re: Use after free in __dst_destroy_metrics_generic
[ 3489.194392] __ion_alloc+0x180/0x988 I do not see the __ion_alloc function in my tree. Hi David This function seems to be defined in an Android specific change. https://android.googlesource.com/kernel/msm/+/20a5411d0115b16826f3d327b6abb0192c8a2001 -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: Use after free in __dst_destroy_metrics_generic
From: Subash Abhinov KasiviswanathanDate: Thu, 07 Sep 2017 18:52:02 -0600 > [ 3489.194392] __ion_alloc+0x180/0x988 I do not see the __ion_alloc function in my tree.
Re: Use after free in __dst_destroy_metrics_generic
Should be fixed by: commit ad65a2f05695aced349e308193c6e2a6b1d87112 Author: Wei WangDate: Sat Jun 17 10:42:35 2017 -0700 ipv6: call dst_hold_safe() properly Thanks for the info Stefano. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: Use after free in __dst_destroy_metrics_generic
On Thu, 07 Sep 2017 18:52:02 -0600 Subash Abhinov Kasiviswanathanwrote: > We are seeing a possible use after free in ip6_dst_destroy. > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some > path and allocated > to ion driver. ion driver has also freed it. Finally the memory is freed > by the > fib gc and crashes since it is already deallocated. > > Target is running an ARM64 Android based 4.9 kernel. > Issue was seen once on a regression rack (sorry, no reproducer). > Any pointers to debug this is highly appreciated. > > [ 3489.470581] [] object_err+0x4c/0x5c > [ 3489.470586] [] free_debug_processing+0x2e0/0x398 > [ 3489.470589] [] __slab_free+0x300/0x3e0 > [ 3489.470593] [] kfree+0x28c/0x290 > [ 3489.470601] [] > __dst_destroy_metrics_generic+0x6c/0x78 > [ 3489.470609] [] ip6_dst_destroy+0xb0/0xb4 Should be fixed by: commit ad65a2f05695aced349e308193c6e2a6b1d87112 Author: Wei Wang Date: Sat Jun 17 10:42:35 2017 -0700 ipv6: call dst_hold_safe() properly -- Stefano