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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 18 Sep 2018 18:02:35 -0700
From:   Wei Wang <tracywwnj@...il.com>
To:     dsahern@...il.com
Cc:     Wei Wang <weiwan@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Cong Wang <xiyou.wangcong@...il.com>, sd@...asysnail.net
Subject: Re: [PATCH net 2/2] ipv6: fix memory leak on dst->_metrics

On Tue, Sep 18, 2018 at 4:25 PM David Ahern <dsahern@...il.com> wrote:
>
> 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.
Yes. Agree. Since both v4 and v6 use the same logic on handling
metrics, helpers can be useful.

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