netdev - Re: [PATCH net 2/2] ipv6: fix memory leak on dst->

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Sep 2018 16:23:11 -0700
From:   David Ahern <dsahern@...il.com>
To:     Wei Wang <weiwan@...gle.com>, David Miller <davem@...emloft.net>,
        netdev@...r.kernel.org
Cc:     Eric Dumazet <edumazet@...gle.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Sabrina Dubroca <sd@...asysnail.net>
Subject: Re: [PATCH net 2/2] ipv6: fix memory leak on dst->_metrics

On 9/18/18 1:45 PM, Wei Wang wrote:
> From: Wei Wang <weiwan@...gle.com>
> 
> When dst->_metrics and f6i->fib6_metrics share the same memory, both
> take reference count on the dst_metrics structure. However, when dst is
> destroyed, ip6_dst_destroy() only invokes dst_destroy_metrics_generic()
> which does not take care of READONLY metrics and does not release refcnt.
> This causes memory leak.
> Similar to ipv4 logic, the fix is to properly release refcnt and free
> the memory space pointed by dst->_metrics if refcnt becomes 0.
> 
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Reported-by: Sabrina Dubroca <sd@...asysnail.net>
> Signed-off-by: Wei Wang <weiwan@...gle.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  net/ipv6/route.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b5d3e6b294ab..826b14de7dbb 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -364,11 +364,14 @@ EXPORT_SYMBOL(ip6_dst_alloc);
>  
>  static void ip6_dst_destroy(struct dst_entry *dst)
>  {
> +	struct dst_metrics *p = (struct dst_metrics *)DST_METRICS_PTR(dst);
>  	struct rt6_info *rt = (struct rt6_info *)dst;
>  	struct fib6_info *from;
>  	struct inet6_dev *idev;
>  
> -	dst_destroy_metrics_generic(dst);
> +	if (p != &dst_default_metrics && refcount_dec_and_test(&p->refcnt))
> +		kfree(p);
> +
>  	rt6_uncached_list_del(rt);
>  
>  	idev = rt->rt6i_idev;
> 

Reviewed-by: David Ahern <dsahern@...il.com>

With the revert in patch 1 we are back to my original code after
93531c67431 ("net/ipv6: separate handling of FIB entries from dst based
routes").

My intention with that series was to make IPv6 handling of metrics as
identical to IPv4 as possible (v6 does have differences for example due
to autoconf and changing metrics after installing a route). The change
in this patch is what I missed back in April.

Comparing IPv4 and IPv6 code for memory allocation and freeing for FIB
entries, transferring metrics to dst_entry and cleanup of dst_entry all
look nearly identical - to the point that net-next could have common
helpers to manage the refcnt'ing. I can submit those after this change
hits net-next.

Thanks for your time getting to the bottom of the leak.