Re: Regression: net/ipv6/mld running system out of memory (not a leak)

On 04.03.2020 07:45, Hangbin Liu wrote:

On Tue, Mar 03, 2020 at 10:23:12AM +0100, Rafał Miłecki wrote:

quoted

On 03.03.2020 10:11, Hangbin Liu wrote:

quoted

On Tue, Mar 03, 2020 at 05:00:35PM +0800, Hangbin Liu wrote:

quoted

On Tue, Mar 03, 2020 at 07:16:44AM +0100, Rafał Miłecki wrote:

quoted

It appears that every interface up & down sequence results in adding a
new ff02::2 entry to the idev->mc_tomb. Doing that over and over will
obviously result in running out of memory at some point. That list isn't
cleared until removing an interface.

Thanks Rafał, this info is very useful. When we set interface up, we will
call ipv6_add_dev() and add in6addr_linklocal_allrouters to the mcast list.
But we only remove it in ipv6_mc_destroy_dev(). This make the link down save
the list and link up add a new one.

Maybe we should remove the list in ipv6_mc_down(). like:

Or maybe we just remove the list in addrconf_ifdown(), as opposite of
ipv6_add_dev(), which looks more clear.

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 164c71c54b5c..4369087b8b74 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -3841,6 +3841,12 @@ static int addrconf_ifdown(struct net_device *dev, int how)
                  ipv6_ac_destroy_dev(idev);
                  ipv6_mc_destroy_dev(idev);
          } else {
+               ipv6_dev_mc_dec(dev, &in6addr_interfacelocal_allnodes);
+               ipv6_dev_mc_dec(dev, &in6addr_linklocal_allnodes);
+
+               if (idev->cnf.forwarding && (dev->flags & IFF_MULTICAST))
+                       ipv6_dev_mc_dec(dev, &in6addr_linklocal_allrouters);
+
                  ipv6_mc_down(idev);
          }

FWIW I can confirm it fixes the problem for me!

Only one ff02::2 entry is present when removing interface:

[  105.686503] [ipv6_mc_destroy_dev] idev->dev->name:mon-phy0
[  105.692056] [ipv6_mc_down] idev->dev->name:mon-phy0
[  105.696957] [ipv6_mc_destroy_dev -> __mld_clear_delrec] kfree(pmc:c64fd880) ff02::2

Hi Rafał,

When review the code, I got confused. On the latest net code, we only
add the allrouter address to multicast list in function
1) ipv6_add_dev(), which only called when there is no idev. But link down and
    up would not re-create idev.
2) dev_forward_change(), which only be called during forward change, this
    function will handle add/remove allrouter address correctly.

Sharp eye! You're right, I tracked (with just a pr_info) all usages of
in6addr_linklocal_allrouters and none gets triggered during my testing
routine. I'm wondering if I should start blaming my OpenWrt user space
now.

So I still don't know how you could added the ff02::2 on same dev multi times.
Does just do `ip link set $dev down; ip link set $dev up` reproduce your
problem? Or did I miss something?

A bit old-fashioned with ifconfig but basically yes, that's my test:

iw phy phy0 interface add mon-phy0 type monitor
for i in $(seq 1 10); do ifconfig mon-phy0 up; ifconfig mon-phy0 down; done
iw dev mon-phy0 del