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

[Wei Wang <tracywwnj@gmail.com>]

On Tue, Sep 18, 2018 at 4:25 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 9/18/18 1:45 PM, Wei Wang wrote:
> > From: Wei Wang <weiwan@google.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@queasysnail.net>
> > Signed-off-by: Wei Wang <weiwan@google.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.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@gmail.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.