netinet6: Fix memory leak on auto_linklocal

[pouria]

Summary

nd6_prelist_add will do refcount_init(refcnt, 2) if
pr is not null. since we're passing the pointer of pr from
stack, do nd6_prefix_rele again to decrease it's reference just like
in6_addifaddr.

MFC to: stable/15
MFC after: 1 week

Test Plan

Observe the memory leak by:

vmstat -m | grep ndp
ifconfig lo1 create inet6 auto_linklocal up
vmstat -m | grep ndp
ifconfig lo1 destroy
vmstat -m | grep ndp

Repeat it.

[zlei]

I'd rather prefer this, to ease my brain ;)

diff --git a/sys/netinet6/nd6_rtr.c b/sys/netinet6/nd6_rtr.c
index ba2f025b7db1..3a6f5f501f8f 100644
--- a/sys/netinet6/nd6_rtr.c
+++ b/sys/netinet6/nd6_rtr.c
@@ -1370,7 +1370,7 @@ nd6_prelist_add(struct nd_prefixctl *pr, struct nd_defrouter *dr,
        new = malloc(sizeof(*new), M_IP6NDP, M_NOWAIT | M_ZERO);
        if (new == NULL)
                return (ENOMEM);
-       refcount_init(&new->ndpr_refcnt, newp != NULL ? 2 : 1);
+       refcount_init(&new->ndpr_refcnt, 1);
        new->ndpr_ifp = pr->ndpr_ifp;
        new->ndpr_prefix = pr->ndpr_prefix;
        new->ndpr_plen = pr->ndpr_plen;
@@ -1410,8 +1410,10 @@ nd6_prelist_add(struct nd_prefixctl *pr, struct nd_defrouter *dr,
 
        if (dr != NULL)
                pfxrtr_add(new, dr);
-       if (newp != NULL)
+       if (newp != NULL) {
+               nd6_prefix_ref(new);
                *newp = new;
+       }
        return (0);
 }

@markj How about the above proposal ?

[zlei]

Looks good to me.

@pouria
A tag is useful to track the commit,

Fixes: e58c8da0683d Map IPv6 link-local prefix to the link-local ifa

[zlei]

sys/netinet6/in6_ifattach.c

631

I thought the ndpr_refcnt should be bumped when the prefix is referenced by interface's IPv6 addresses, and thought @melifaro made that on purpose. Actually the prefix use ndpr_addrcnt to track linked IPv6 addresses so I was wrong.

[pouria]

In D55593#1271927, @zlei wrote:

I'd rather prefer this, to ease my brain ;)

diff --git a/sys/netinet6/nd6_rtr.c b/sys/netinet6/nd6_rtr.c
index ba2f025b7db1..3a6f5f501f8f 100644
--- a/sys/netinet6/nd6_rtr.c
+++ b/sys/netinet6/nd6_rtr.c
@@ -1370,7 +1370,7 @@ nd6_prelist_add(struct nd_prefixctl *pr, struct nd_defrouter *dr,
        new = malloc(sizeof(*new), M_IP6NDP, M_NOWAIT | M_ZERO);
        if (new == NULL)
                return (ENOMEM);
-       refcount_init(&new->ndpr_refcnt, newp != NULL ? 2 : 1);
+       refcount_init(&new->ndpr_refcnt, 1);
        new->ndpr_ifp = pr->ndpr_ifp;
        new->ndpr_prefix = pr->ndpr_prefix;
        new->ndpr_plen = pr->ndpr_plen;
@@ -1410,8 +1410,10 @@ nd6_prelist_add(struct nd_prefixctl *pr, struct nd_defrouter *dr,
 
        if (dr != NULL)
                pfxrtr_add(new, dr);
-       if (newp != NULL)
+       if (newp != NULL) {
+               nd6_prefix_ref(new);
                *newp = new;
+       }
        return (0);
 }

@markj How about the above proposal ?

I believe this function could be optimized more and we could avoid set the refcount to 2 in first place, but that's a different topic and I'll work on it in another review.
Before that, I need my other revision on D55267 get reviewed to isolate pi processing.
I'd like to hear @markj input too.

In D55593#1271933, @zlei wrote:

A tag is useful to track the commit,

Fixes: e58c8da0683d Map IPv6 link-local prefix to the link-local ifa

Thank you!, I will.
Just to be safe, I'll wait for another reviewer since I want to MFC it.

[pouria]

sys/netinet6/in6_ifattach.c

631

I thought the ndpr_refcnt should be bumped when the prefix is referenced by interface's IPv6 addresses, and thought @melifaro made that on purpose. Actually the prefix use ndpr_addrcnt to track linked IPv6 addresses so I was wrong.

Yes it uses ndpr_addrcnt, thank you for updates!

[zlei]

@pouria

MFC to: stable/15

I have not tested yet, but I believe stable/14 and stable/13 are also affected.

[pouria]

In D55593#1271953, @zlei wrote:

@pouria

MFC to: stable/15

I have not tested yet, but I believe stable/14 and stable/13 are also affected.

Based on the source code in stable/14, it should be affected.
But I don't think we should MFC it in 14.4 because there's almost no time left for testing it.
It's probably not worth the rush.

[pouria]

Since @markj just announce that he's AFK, could you take a look at this please? @glebius

[ivy]

re: MFC, you can MFC this to stable/14, even though it's too late for 14.4, we will be doing a 14.5 release.

the change itself seems fine based on the rationale, although i'm not really familiar with this code. but, it's not immediately clear why pr can't be NULL on line 634, so it might be worth adding a short explanatory comment that nd6_prelist_add will store into pr. otherwise, it looks like a bug at first glance.