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

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

[Rafał Miłecki]

Hi, I need some help with my devices running out of memory. I've
debugging skills but I don't know net subsystem.

I run Linux based OpenWrt distribution on home wireless devices (ARM
routers and access points with brcmfmac wireless driver). I noticed
that using wireless monitor mode interface results in my devices (128
MiB RAM) running out of memory in about 2 days. This is NOT a memory
leak as putting wireless down brings back all the memory.

Interestingly this memory drain requires at least one of:
net.ipv6.conf.default.forwarding=1
net.ipv6.conf.all.forwarding=1
to be set. OpenWrt happens to use both by default.

This regression was introduced by the commit 1666d49e1d41 ("mld: do
not remove mld souce list info when set link down") - first appeared
in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.

Can you look at possible cause/fix of this problem, please? Is there
anything I can test or is there more info I can provide?

I'm not sure why this issue appears only when using monitor mode.
Using wireless __ap mode interface (with hostapd) won't expose this
issue. I guess it may be a matter of monitor interfaces not being
bridged?

-- 
Rafał

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

[Eric Dumazet]

On 2/11/20 11:37 PM, Rafał Miłecki wrote:
> Hi, I need some help with my devices running out of memory. I've
> debugging skills but I don't know net subsystem.
> 
> I run Linux based OpenWrt distribution on home wireless devices (ARM
> routers and access points with brcmfmac wireless driver). I noticed
> that using wireless monitor mode interface results in my devices (128
> MiB RAM) running out of memory in about 2 days. This is NOT a memory
> leak as putting wireless down brings back all the memory.
> 
> Interestingly this memory drain requires at least one of:
> net.ipv6.conf.default.forwarding=1
> net.ipv6.conf.all.forwarding=1
> to be set. OpenWrt happens to use both by default.
> 
> This regression was introduced by the commit 1666d49e1d41 ("mld: do
> not remove mld souce list info when set link down") - first appeared
> in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
> Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.
> 
> Can you look at possible cause/fix of this problem, please? Is there
> anything I can test or is there more info I can provide?
> 
> I'm not sure why this issue appears only when using monitor mode.
> Using wireless __ap mode interface (with hostapd) won't expose this
> issue. I guess it may be a matter of monitor interfaces not being
> bridged?
> 

This commit had few fixes, are you sure they were applied to your kernel ?

9c8bb163ae784be4f79ae504e78c862806087c54 igmp, mld: Fix memory leak in igmpv3/mld_del_delrec()
08d3ffcc0cfaba36f6b86fd568cc3bc773061fa6 multicast: do not restore deleted record source filter mode to new one
a84d016479896b5526a2cc54784e6ffc41c9d6f6 mld: fix memory leak in mld_del_delrec()

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

[Hangbin Liu]

On Wed, Feb 12, 2020 at 08:37:31AM +0100, Rafał Miłecki wrote:
> Hi, I need some help with my devices running out of memory. I've
> debugging skills but I don't know net subsystem.
> 
> I run Linux based OpenWrt distribution on home wireless devices (ARM
> routers and access points with brcmfmac wireless driver). I noticed
> that using wireless monitor mode interface results in my devices (128
> MiB RAM) running out of memory in about 2 days. This is NOT a memory
> leak as putting wireless down brings back all the memory.
> 
> Interestingly this memory drain requires at least one of:
> net.ipv6.conf.default.forwarding=1
> net.ipv6.conf.all.forwarding=1
> to be set. OpenWrt happens to use both by default.
> 
> This regression was introduced by the commit 1666d49e1d41 ("mld: do
> not remove mld souce list info when set link down") - first appeared
> in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
> Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.
> 
> Can you look at possible cause/fix of this problem, please? Is there
> anything I can test or is there more info I can provide?

Hi Rafał,

Thanks for the report. Although you said this is not a memory leak. Maybe
you can try a84d01647989 ("mld: fix memory leak in mld_del_delrec()").

Thanks
Hangbin

> 
> I'm not sure why this issue appears only when using monitor mode.
> Using wireless __ap mode interface (with hostapd) won't expose this
> issue. I guess it may be a matter of monitor interfaces not being
> bridged?
> 
> -- 
> Rafał

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

[Rafał Miłecki]

On 12.02.2020 08:59, Eric Dumazet wrote:
> On 2/11/20 11:37 PM, Rafał Miłecki wrote:
>> Hi, I need some help with my devices running out of memory. I've
>> debugging skills but I don't know net subsystem.
>>
>> I run Linux based OpenWrt distribution on home wireless devices (ARM
>> routers and access points with brcmfmac wireless driver). I noticed
>> that using wireless monitor mode interface results in my devices (128
>> MiB RAM) running out of memory in about 2 days. This is NOT a memory
>> leak as putting wireless down brings back all the memory.
>>
>> Interestingly this memory drain requires at least one of:
>> net.ipv6.conf.default.forwarding=1
>> net.ipv6.conf.all.forwarding=1
>> to be set. OpenWrt happens to use both by default.
>>
>> This regression was introduced by the commit 1666d49e1d41 ("mld: do
>> not remove mld souce list info when set link down") - first appeared
>> in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
>> Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.
>>
>> Can you look at possible cause/fix of this problem, please? Is there
>> anything I can test or is there more info I can provide?
>>
>> I'm not sure why this issue appears only when using monitor mode.
>> Using wireless __ap mode interface (with hostapd) won't expose this
>> issue. I guess it may be a matter of monitor interfaces not being
>> bridged?
>>
> 
> This commit had few fixes, are you sure they were applied to your kernel ?

Thanks for looking at this! Unfortunately I already have all listed
patches so it has to be yet another issue.


> 9c8bb163ae784be4f79ae504e78c862806087c54 igmp, mld: Fix memory leak in igmpv3/mld_del_delrec()

First included in v4.10-rc8, so it's present in my 4.14.169.


> 08d3ffcc0cfaba36f6b86fd568cc3bc773061fa6 multicast: do not restore deleted record source filter mode to new one
First included in v4.18-rc8, so it's present in my 4.14.169.


> a84d016479896b5526a2cc54784e6ffc41c9d6f6 mld: fix memory leak in mld_del_delrec()
Backported to linux-4.14.y as df9c0f8a15c283b3339ef636642d3769f8fbc434
and first included in v4.14.143, so it's present in my 4.14.169.

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

[Rafał Miłecki]

On 12.02.2020 09:24, Hangbin Liu wrote:
> On Wed, Feb 12, 2020 at 08:37:31AM +0100, Rafał Miłecki wrote:
>> Hi, I need some help with my devices running out of memory. I've
>> debugging skills but I don't know net subsystem.
>>
>> I run Linux based OpenWrt distribution on home wireless devices (ARM
>> routers and access points with brcmfmac wireless driver). I noticed
>> that using wireless monitor mode interface results in my devices (128
>> MiB RAM) running out of memory in about 2 days. This is NOT a memory
>> leak as putting wireless down brings back all the memory.
>>
>> Interestingly this memory drain requires at least one of:
>> net.ipv6.conf.default.forwarding=1
>> net.ipv6.conf.all.forwarding=1
>> to be set. OpenWrt happens to use both by default.
>>
>> This regression was introduced by the commit 1666d49e1d41 ("mld: do
>> not remove mld souce list info when set link down") - first appeared
>> in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
>> Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.
>>
>> Can you look at possible cause/fix of this problem, please? Is there
>> anything I can test or is there more info I can provide?
> 
> Hi Rafał,
> 
> Thanks for the report. Although you said this is not a memory leak. Maybe
> you can try a84d01647989 ("mld: fix memory leak in mld_del_delrec()").

Thanks, that commit was also pointed by Eric and I verified it was
backported as:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.9.y&id=df9c0f8a15c283b3339ef636642d3769f8fbc434

So it must be some other bug affecting me.

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

[Hangbin Liu]

On Wed, Feb 12, 2020 at 09:49:02AM +0100, Rafał Miłecki wrote:
> On 12.02.2020 09:24, Hangbin Liu wrote:
> > On Wed, Feb 12, 2020 at 08:37:31AM +0100, Rafał Miłecki wrote:
> > > Hi, I need some help with my devices running out of memory. I've
> > > debugging skills but I don't know net subsystem.
> > > 
> > > I run Linux based OpenWrt distribution on home wireless devices (ARM
> > > routers and access points with brcmfmac wireless driver). I noticed
> > > that using wireless monitor mode interface results in my devices (128
> > > MiB RAM) running out of memory in about 2 days. This is NOT a memory
> > > leak as putting wireless down brings back all the memory.
> > > 
> > > Interestingly this memory drain requires at least one of:
> > > net.ipv6.conf.default.forwarding=1
> > > net.ipv6.conf.all.forwarding=1
> > > to be set. OpenWrt happens to use both by default.
> > > 
> > > This regression was introduced by the commit 1666d49e1d41 ("mld: do
> > > not remove mld souce list info when set link down") - first appeared
> > > in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
> > > Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.
> > > 
> > > Can you look at possible cause/fix of this problem, please? Is there
> > > anything I can test or is there more info I can provide?
> > 
> > Hi Rafał,
> > 
> > Thanks for the report. Although you said this is not a memory leak. Maybe
> > you can try a84d01647989 ("mld: fix memory leak in mld_del_delrec()").
> 
> Thanks, that commit was also pointed by Eric and I verified it was
> backported as:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.9.y&id=df9c0f8a15c283b3339ef636642d3769f8fbc434
> 
> So it must be some other bug affecting me.

Hmm, I'm surprised that IGMP works for you, as it requires enable IPv6
forwarding. Do you have a lot IPv6 multicast groups on your device?
What dose `ip maddr list` show?

Thanks
Hangbin

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

[Rafał Miłecki]

I'm sorry for a late reply, I spent that time for switching my devices
to some newer kernel. I wanted to make sure we are not chasing a bug
that's long time fixed now.

This problem still exists in the 5.4.18.

On Wed, 12 Feb 2020 at 11:08, Hangbin Liu <liuhangbin@gmail.com> wrote:
> On Wed, Feb 12, 2020 at 09:49:02AM +0100, Rafał Miłecki wrote:
> > On 12.02.2020 09:24, Hangbin Liu wrote:
> > > Thanks for the report. Although you said this is not a memory leak. Maybe
> > > you can try a84d01647989 ("mld: fix memory leak in mld_del_delrec()").
> >
> > Thanks, that commit was also pointed by Eric and I verified it was
> > backported as:
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.9.y&id=df9c0f8a15c283b3339ef636642d3769f8fbc434
> >
> > So it must be some other bug affecting me.
>
> Hmm, I'm surprised that IGMP works for you, as it requires enable IPv6
> forwarding. Do you have a lot IPv6 multicast groups on your device?

The thing is I don't really use IPv6. There are some single IPv6 packets
in my network (e.g. MDNS packets) but nothing significant.

For my testing purposes I access my access points using ssh and it's the
only real traffic. There are no wireless devices connected to my testing
devices. They are just running monitor mode interfaces without any real
traffic.

Monitor interfaces have
	type = ARPHRD_IEEE80211_RADIOTAP
and skbs have
	skb->pkt_type = PACKET_OTHERHOST
	skb->protocol = htons(ETH_P_802_2)
that also shouldn't trigger any IPv6 traffic in the kernel AFAIU.


 > What dose `ip maddr list` show?

# ip maddr list
1:      lo
         inet  224.0.0.1
         inet6 ff02::1
         inet6 ff01::1
2:      eth0
         link  33:33:00:00:00:01
         link  33:33:00:00:00:02 users 2
         link  01:00:5e:00:00:01 users 2
         link  33:33:ff:7a:fc:80
         link  33:33:ff:00:00:00
         inet  224.0.0.1
         inet6 ff02::1:ff00:0
         inet6 ff02::1:ff7a:fc80
         inet6 ff05::2
         inet6 ff01::2
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
3:      eth1
         link  33:33:00:00:00:01
         link  33:33:00:00:00:02 users 2
         inet6 ff05::2
         inet6 ff01::2
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
4:      eth2
         link  33:33:00:00:00:01
         link  33:33:00:00:00:02 users 2
         inet6 ff05::2
         inet6 ff01::2
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
5:      wlan0
         link  01:00:5e:00:00:01
         link  33:33:00:00:00:02 users 2
         link  33:33:00:00:00:01
         link  33:33:ff:7a:fc:81
         link  33:33:ff:00:00:00
         inet  224.0.0.1
         inet6 ff02::1:ff00:0
         inet6 ff02::1:ff7a:fc81
         inet6 ff05::2
         inet6 ff01::2
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
6:      wlan1
         link  01:00:5e:00:00:01
         link  33:33:00:00:00:02 users 2
         link  33:33:00:00:00:01
         link  33:33:ff:7a:fc:88
         link  33:33:ff:00:00:00
         inet  224.0.0.1
         inet6 ff02::1:ff00:0
         inet6 ff02::1:ff7a:fc88
         inet6 ff05::2
         inet6 ff01::2
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
7:      br-lan
         link  33:33:00:00:00:01
         link  33:33:00:00:00:02
         link  01:00:5e:00:00:01
         link  33:33:ff:7a:fc:80
         link  33:33:ff:00:00:00
         inet  224.0.0.1
         inet6 ff02::1:ff00:0
         inet6 ff02::1:ff7a:fc80
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
8:      eth0.1
         link  01:00:5e:00:00:01 users 2
         inet  224.0.0.1
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
9:      mon-phy0
         inet  224.0.0.1
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1
10:     mon-phy1
         inet6 ff02::2
         inet6 ff02::1
         inet6 ff01::1

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

[Rafał Miłecki]

On Wed, 12 Feb 2020 at 08:37, Rafał Miłecki <zajec5@gmail.com> wrote:
> I run Linux based OpenWrt distribution on home wireless devices (ARM
> routers and access points with brcmfmac wireless driver). I noticed
> that using wireless monitor mode interface results in my devices (128
> MiB RAM) running out of memory in about 2 days. This is NOT a memory
> leak as putting wireless down brings back all the memory.
>
> Interestingly this memory drain requires at least one of:
> net.ipv6.conf.default.forwarding=1
> net.ipv6.conf.all.forwarding=1
> to be set. OpenWrt happens to use both by default.
>
> This regression was introduced by the commit 1666d49e1d41 ("mld: do
> not remove mld souce list info when set link down") - first appeared
> in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
> Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.

Thinking about meaning of that commit ("do not remove mld souce list
info when set link down") made me realize one more thing.

My app accessing monitor mode brings it up and down repeatedly:
while (1) {
  ifconfig X up (ifr_flags |= IFF_UP | IFF_RUNNING)
  select(...)
  recv(...)
  ifconfig X down (ifr_flags &= ~(IFF_UP | IFF_RUNNING))
  sleep(...)
}

So maybe that bug running device out of memory was there since ever?
Maybe before 1666d49e1d41 ("mld: do not remove mld souce list info
when set link down") I just didn't notice it as every "ifconfig X
down" was flushing list. Flushing it every few seconds didn't let list
grow too big and eat all my memory?

I'm going to test some old kernel now using monitor mode all time,
without putting its interface down. Is there some of debugging
mld/ipv6 kernel lists to see if there are indeed growing huge?

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

[Hangbin Liu]

On Tue, Feb 18, 2020 at 07:55:36AM +0100, Rafał Miłecki wrote:
> I'm sorry for a late reply, I spent that time for switching my devices
> to some newer kernel. I wanted to make sure we are not chasing a bug
> that's long time fixed now.
> 
> This problem still exists in the 5.4.18.

OK, bad news...

> > 
> > Hmm, I'm surprised that IGMP works for you, as it requires enable IPv6
> > forwarding. Do you have a lot IPv6 multicast groups on your device?
> 
> The thing is I don't really use IPv6. There are some single IPv6 packets
> in my network (e.g. MDNS packets) but nothing significant.
> 
> For my testing purposes I access my access points using ssh and it's the
> only real traffic. There are no wireless devices connected to my testing
> devices. They are just running monitor mode interfaces without any real
> traffic.
> 
> > What dose `ip maddr list` show?
> 
> # ip maddr list
> 1:      lo
>         inet  224.0.0.1
>         inet6 ff02::1
>         inet6 ff01::1
> 7:      br-lan
>         link  33:33:00:00:00:01
>         link  33:33:00:00:00:02
>         link  01:00:5e:00:00:01
>         link  33:33:ff:7a:fc:80
>         link  33:33:ff:00:00:00
>         inet  224.0.0.1
>         inet6 ff02::1:ff00:0
>         inet6 ff02::1:ff7a:fc80
>         inet6 ff02::2
>         inet6 ff02::1
>         inet6 ff01::1

No much ipv6 traffic, no much IPv6 multicast groups. Only occurred with
IPv6 forwarding enabled...Any possibility(although unlikely) that there
is a loop for ipv6 multicast traffic under br-lan?

Maybe we can use perf kmem to trace kernel memory statistics.

Thanks
Hangbin

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

[Hangbin Liu]

On Tue, Feb 18, 2020 at 09:08:54AM +0100, Rafał Miłecki wrote:
> 
> Thinking about meaning of that commit ("do not remove mld souce list
> info when set link down") made me realize one more thing.
> 
> My app accessing monitor mode brings it up and down repeatedly:
> while (1) {
>   ifconfig X up (ifr_flags |= IFF_UP | IFF_RUNNING)
>   select(...)
>   recv(...)
>   ifconfig X down (ifr_flags &= ~(IFF_UP | IFF_RUNNING))
>   sleep(...)
> }
> 
> So maybe that bug running device out of memory was there since ever?
> Maybe before 1666d49e1d41 ("mld: do not remove mld souce list info
> when set link down") I just didn't notice it as every "ifconfig X
> down" was flushing list. Flushing it every few seconds didn't let list
> grow too big and eat all my memory?

Maybe. before that patch we will remove all the list when link down.
After that we will store the list and restore it back when link up.
> 
> I'm going to test some old kernel now using monitor mode all time,
> without putting its interface down. Is there some of debugging
> mld/ipv6 kernel lists to see if there are indeed growing huge?

I will just try test with link up/down(with forwarding enabled) and see
if I can reproduce.

Thanks
Hangbin

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

[Rafał Miłecki]

On 12.02.2020 08:37, Rafał Miłecki wrote:
> I run Linux based OpenWrt distribution on home wireless devices (ARM
> routers and access points with brcmfmac wireless driver). I noticed
> that using wireless monitor mode interface results in my devices (128
> MiB RAM) running out of memory in about 2 days. This is NOT a memory
> leak as putting wireless down brings back all the memory.
> 
> Interestingly this memory drain requires at least one of:
> net.ipv6.conf.default.forwarding=1
> net.ipv6.conf.all.forwarding=1
> to be set. OpenWrt happens to use both by default.
> 
> This regression was introduced by the commit 1666d49e1d41 ("mld: do
> not remove mld souce list info when set link down") - first appeared
> in 4.10 and then backported. This bug exists in 4.9.14 and 4.14.169.
> Reverting that commit from 4.9.14 and 4.14.169 /fixes/ the problem.

I have some interesting debugging results to share. I decided to log all
kfree() operations in mcast.c. Logging mld_clear_delrec() provided some
promising info.

With kernel 4.14.169 (affected by regression) bringing monitor ifaces
down doesn't result in freeing any memory in mld_clear_delrec():

down
[   73.487846] [ipv6_mc_down] idev->dev->name:mon-phy0
[   73.917823] [ipv6_mc_down] idev->dev->name:mon-phy1

down
[   76.767781] [ipv6_mc_down] idev->dev->name:mon-phy0
[   77.157790] [ipv6_mc_down] idev->dev->name:mon-phy1

down
[   79.658260] [ipv6_mc_down] idev->dev->name:mon-phy0
[   80.047805] [ipv6_mc_down] idev->dev->name:mon-phy1

down
[   83.067773] [ipv6_mc_down] idev->dev->name:mon-phy0
[   83.447778] [ipv6_mc_down] idev->dev->name:mon-phy1

down
[   86.555700] [ipv6_mc_down] idev->dev->name:mon-phy0
[   86.985706] [ipv6_mc_down] idev->dev->name:mon-phy1

However removing interfaces reveals there were duplicated entries in
idev->mc_tomb lists:

remove
[   89.694038] [ipv6_mc_down] idev->dev->name:wlan1
[   89.717953] [ipv6_mc_destroy_dev] idev->dev->name:mon-phy1
[   89.723433] [ipv6_mc_down] idev->dev->name:mon-phy1
[   89.728375] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c6e2c600) ff02::2
[   89.736013] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b100) ff02::2
[   89.743643] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b900) ff02::2
[   89.751275] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b680) ff02::2
[   89.758901] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b580) ff02::2
[   89.868542] [ipv6_mc_down] idev->dev->name:wlan0
[   89.878203] [ipv6_mc_destroy_dev] idev->dev->name:wlan1
[   89.883479] [ipv6_mc_down] idev->dev->name:wlan1
[   89.888208] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c6e2ca80) ff02::2
[   89.895901] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c6e2ca00) ff02::1:ff10:e018
[   89.904412] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c6e2c980) ff02::1:ff00:0
[   89.937564] [ipv6_mc_destroy_dev] idev->dev->name:mon-phy0
[   89.943062] [ipv6_mc_down] idev->dev->name:mon-phy0
[   89.948070] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c6e2c300) ff02::2
[   89.955728] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b780) ff02::2
[   89.963392] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b980) ff02::2
[   89.971035] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b700) ff02::2
[   89.978688] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620b080) ff02::2
[   90.068433] [ipv6_mc_destroy_dev] idev->dev->name:wlan0
[   90.073672] [ipv6_mc_down] idev->dev->name:wlan0
[   90.078357] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620bf00) ff02::2
[   90.085992] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620be80) ff02::1:ff0e:5018
[   90.094530] [ipv6_mc_destroy_dev -> mld_clear_delrec] kfree(pmc:c620bd80) ff02::1:ff00:0

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.

I searched and found ff02::2 to be described as "all routers" address.
Any idea why is it being added over and over?



Just to make my debugging complete I tried the same logging with the
kernel 4.4.194 (before regression). It clears idev->mc_tomb list on
every interface down so that list never grows so big.

down
[  119.241112] [ipv6_mc_down] idev->dev->name:mon-phy0
[  119.246025] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c80b40) ff02::2
[  119.649975] [ipv6_mc_down] idev->dev->name:mon-phy1
[  119.654873] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c80300) ff02::2

down
[  125.220060] [ipv6_mc_down] idev->dev->name:mon-phy0
[  125.224969] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c80540) ff02::2
[  125.580000] [ipv6_mc_down] idev->dev->name:mon-phy1
[  125.584900] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c80180) ff02::2

down
[  128.520013] [ipv6_mc_down] idev->dev->name:mon-phy0
[  128.524921] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c800c0) ff02::2
[  128.879994] [ipv6_mc_down] idev->dev->name:mon-phy1
[  128.884899] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c80e40) ff02::2

down
[  131.820028] [ipv6_mc_down] idev->dev->name:mon-phy0
[  131.824934] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca53c0) ff02::2
[  132.179992] [ipv6_mc_down] idev->dev->name:mon-phy1
[  132.184894] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca5240) ff02::2

down
[  134.759991] [ipv6_mc_down] idev->dev->name:mon-phy0
[  134.764901] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca5540) ff02::2
[  135.119995] [ipv6_mc_down] idev->dev->name:mon-phy1
[  135.124897] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca5a80) ff02::2

remove
[  140.746744] [ipv6_mc_down] idev->dev->name:wlan1
[  140.751432] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca5480) ff02::2
[  140.758457] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c800c0) ff02::1:ff10:e018
[  140.766367] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6c80e40) ff02::1:ff00:0
[  140.791016] [ipv6_mc_down] idev->dev->name:wlan0
[  140.795686] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca5c00) ff02::2
[  140.802814] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca59c0) ff02::1:ff0e:5018
[  140.810818] [ipv6_mc_down -> mld_clear_delrec] kfree(pmc:c6ca50c0) ff02::1:ff00:0
[  140.820225] [ipv6_mc_destroy_dev] idev->dev->name:mon-phy1
[  140.825849] [ipv6_mc_down] idev->dev->name:mon-phy1
[  140.830785] [ipv6_mc_destroy_dev] igmp6_group_dropped(c6c80840)
[  140.902121] [ipv6_mc_destroy_dev] idev->dev->name:mon-phy0
[  140.907622] [ipv6_mc_down] idev->dev->name:mon-phy0
[  140.912622] [ipv6_mc_destroy_dev] igmp6_group_dropped(c6cc4a80)
[  140.983259] [ipv6_mc_destroy_dev] idev->dev->name:wlan1
[  140.988472] [ipv6_mc_down] idev->dev->name:wlan1
[  140.993151] [ipv6_mc_destroy_dev] igmp6_group_dropped(c72f6780)
[  141.013696] [ipv6_mc_destroy_dev] idev->dev->name:wlan0
[  141.018933] [ipv6_mc_down] idev->dev->name:wlan0
[  141.023722] [ipv6_mc_destroy_dev] igmp6_group_dropped(c5c4da80)

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

[Hangbin Liu]

On Tue, Mar 03, 2020 at 07:16:44AM +0100, Rafał Miłecki wrote:
> 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:

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index eaa4c2cc2fbb..786352ff7704 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -2533,6 +2533,18 @@ void ipv6_mc_down(struct inet6_dev *idev)
 {
        struct ifmcaddr6 *i;

+       /* Delete all-nodes address. */
+       /* We cannot call ipv6_dev_mc_dec() directly, our caller in
+        * addrconf.c has NULL'd out dev->ip6_ptr so in6_dev_get() will
+        * fail.
+        */
+       __ipv6_dev_mc_dec(idev, &in6addr_interfacelocal_allnodes);
+       __ipv6_dev_mc_dec(idev, &in6addr_linklocal_allnodes);
+
+       if (idev->cnf.forwarding)
+               __ipv6_dev_mc_dec(idev, &in6addr_linklocal_allrouters);
+
+
        /* Withdraw multicast list */

        read_lock_bh(&idev->lock);
@@ -2603,16 +2615,6 @@ void ipv6_mc_destroy_dev(struct inet6_dev *idev)
        ipv6_mc_down(idev);
        mld_clear_delrec(idev);

-       /* Delete all-nodes address. */
-       /* We cannot call ipv6_dev_mc_dec() directly, our caller in
-        * addrconf.c has NULL'd out dev->ip6_ptr so in6_dev_get() will
-        * fail.
-        */
-       __ipv6_dev_mc_dec(idev, &in6addr_linklocal_allnodes);
-
-       if (idev->cnf.forwarding)
-               __ipv6_dev_mc_dec(idev, &in6addr_linklocal_allrouters);
-
        write_lock_bh(&idev->lock);
        while ((i = idev->mc_list) != NULL) {
                idev->mc_list = i->next;


Thanks
Hangbin

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

[Hangbin Liu]

On Tue, Mar 03, 2020 at 05:00:35PM +0800, Hangbin Liu wrote:
> On Tue, Mar 03, 2020 at 07:16:44AM +0100, Rafał Miłecki wrote:
> > 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);
        }

Thanks
Hangbin

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

[Rafał Miłecki]

On 03.03.2020 10:11, Hangbin Liu wrote:
> On Tue, Mar 03, 2020 at 05:00:35PM +0800, Hangbin Liu wrote:
>> On Tue, Mar 03, 2020 at 07:16:44AM +0100, Rafał Miłecki wrote:
>>> 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

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

[Hangbin Liu]

On Tue, Mar 03, 2020 at 10:23:12AM +0100, Rafał Miłecki wrote:
> > 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

Cool, thanks for the quick test. I will post the fix then.

Thanks
Hangbin

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

[Hangbin Liu]

On Tue, Mar 03, 2020 at 10:23:12AM +0100, Rafał Miłecki wrote:
> On 03.03.2020 10:11, Hangbin Liu wrote:
> > On Tue, Mar 03, 2020 at 05:00:35PM +0800, Hangbin Liu wrote:
> > > On Tue, Mar 03, 2020 at 07:16:44AM +0100, Rafał Miłecki wrote:
> > > > 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.

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?

Thanks
Hangbin

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

[Rafał Miłecki]

On 04.03.2020 07:45, Hangbin Liu wrote:
> On Tue, Mar 03, 2020 at 10:23:12AM +0100, Rafał Miłecki wrote:
>> On 03.03.2020 10:11, Hangbin Liu wrote:
>>> On Tue, Mar 03, 2020 at 05:00:35PM +0800, Hangbin Liu wrote:
>>>> On Tue, Mar 03, 2020 at 07:16:44AM +0100, Rafał Miłecki wrote:
>>>>> 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

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

[Hangbin Liu]

On Wed, Mar 04, 2020 at 08:44:29AM +0100, Rafał Miłecki wrote:
> > 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.

Yeah...Hope you could help dig more.
> 
> 
> > 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

What does this step do? Is there a corresponding command for ip link?

> for i in $(seq 1 10); do ifconfig mon-phy0 up; ifconfig mon-phy0 down; done
> iw dev mon-phy0 del

The other cmd looks normal. BTW, I have create a new patch. The previous one
will leak ff01::1, ff02::1, ff01::2 as the link up will not re-add them.

The new patch will join and leave all node/route address when link up/down
instead of store them. But I don't think it could resolve your issue as the
code logic is not changed. If you like, you can have a try.

Thanks
Hangbin

From 9cf504dc30198133e470ea783c3fa53a8f56a20a Mon Sep 17 00:00:00 2001
From: Hangbin Liu <liuhangbin@gmail.com>
Date: Tue, 3 Mar 2020 20:58:39 +0800
Subject: [PATCH] ipv6/mcast: join and leave allnodes, allrouters addresses
 when link up/down

This patch fixes two issues:

1) When link up with IPv6 forwarding enabled, we forgot to join
sitelocal_allrouters(ff01::2) and interfacelocal_allrouters(ff05::2),
like what we do in function dev_forward_change()

2) When link down, there is no listener for the allnodes, allrouters
addresses. So we should remove them instead of storing the info in
idev->mc_tomb.

To fix these two issue, I add two new functions ipv6_mc_join_localaddr()
and ipv6_mc_leave_localaddr() to handle these addresses. And just join
leave the group in ipv6_mc_{up, down}.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/ipv6/addrconf.c | 22 +++++++---------------
 net/ipv6/mcast.c    | 45 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 164c71c54b5c..8e39f569beaa 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -449,16 +449,6 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 	/* protected by rtnl_lock */
 	rcu_assign_pointer(dev->ip6_ptr, ndev);
 
-	/* Join interface-local all-node multicast group */
-	ipv6_dev_mc_inc(dev, &in6addr_interfacelocal_allnodes);
-
-	/* Join all-node multicast group */
-	ipv6_dev_mc_inc(dev, &in6addr_linklocal_allnodes);
-
-	/* Join all-router multicast group if forwarding is set */
-	if (ndev->cnf.forwarding && (dev->flags & IFF_MULTICAST))
-		ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
-
 	return ndev;
 
 err_release:
@@ -481,7 +471,7 @@ static struct inet6_dev *ipv6_find_idev(struct net_device *dev)
 			return idev;
 	}
 
-	if (dev->flags&IFF_UP)
+	if (dev->flags & IFF_UP && dev->flags & IFF_MULTICAST)
 		ipv6_mc_up(idev);
 	return idev;
 }
@@ -795,7 +785,7 @@ static void dev_forward_change(struct inet6_dev *idev)
 	dev = idev->dev;
 	if (idev->cnf.forwarding)
 		dev_disable_lro(dev);
-	if (dev->flags & IFF_MULTICAST) {
+	if (dev->flags & IFF_UP && dev->flags & IFF_MULTICAST) {
 		if (idev->cnf.forwarding) {
 			ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
 			ipv6_dev_mc_inc(dev, &in6addr_interfacelocal_allrouters);
@@ -3554,7 +3544,8 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			}
 
 			if (!IS_ERR_OR_NULL(idev)) {
-				if (idev->if_flags & IF_READY) {
+				if (idev->if_flags & IF_READY &&
+				    dev->flags & IFF_MULTICAST) {
 					/* device is already configured -
 					 * but resend MLD reports, we might
 					 * have roamed and need to update
@@ -3839,8 +3830,9 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 	/* Step 5: Discard anycast and multicast list */
 	if (how) {
 		ipv6_ac_destroy_dev(idev);
-		ipv6_mc_destroy_dev(idev);
-	} else {
+		if (dev->flags & IFF_MULTICAST)
+			ipv6_mc_destroy_dev(idev);
+	} else if (dev->flags & IFF_MULTICAST){
 		ipv6_mc_down(idev);
 	}
 
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index eaa4c2cc2fbb..1512ca906b36 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -2529,11 +2529,44 @@ void ipv6_mc_remap(struct inet6_dev *idev)
 
 /* Device going down */
 
+static void ipv6_mc_join_localaddr(struct inet6_dev *idev)
+{
+	struct net_device *dev;
+
+	dev = idev->dev;
+
+	/* Join interface-local all-node multicast group */
+	ipv6_dev_mc_inc(dev, &in6addr_interfacelocal_allnodes);
+
+	/* Join all-node multicast group */
+	ipv6_dev_mc_inc(dev, &in6addr_linklocal_allnodes);
+
+	/* Join all-router multicast group if forwarding is set */
+	if (idev->cnf.forwarding) {
+		ipv6_dev_mc_inc(dev, &in6addr_interfacelocal_allrouters);
+		ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
+		ipv6_dev_mc_inc(dev, &in6addr_sitelocal_allrouters);
+	}
+}
+
+static void ipv6_mc_leave_localaddr(struct inet6_dev *idev)
+{
+	__ipv6_dev_mc_dec(idev, &in6addr_interfacelocal_allnodes);
+	__ipv6_dev_mc_dec(idev, &in6addr_linklocal_allnodes);
+
+	if (idev->cnf.forwarding) {
+		__ipv6_dev_mc_dec(idev, &in6addr_interfacelocal_allrouters);
+		__ipv6_dev_mc_dec(idev, &in6addr_linklocal_allrouters);
+		__ipv6_dev_mc_dec(idev, &in6addr_sitelocal_allrouters);
+	}
+}
+
 void ipv6_mc_down(struct inet6_dev *idev)
 {
 	struct ifmcaddr6 *i;
 
 	/* Withdraw multicast list */
+	ipv6_mc_leave_localaddr(idev);
 
 	read_lock_bh(&idev->lock);
 
@@ -2564,7 +2597,7 @@ void ipv6_mc_up(struct inet6_dev *idev)
 {
 	struct ifmcaddr6 *i;
 
-	/* Install multicast list, except for all-nodes (already installed) */
+	ipv6_mc_join_localaddr(idev);
 
 	read_lock_bh(&idev->lock);
 	ipv6_mc_reset(idev);
@@ -2603,16 +2636,6 @@ void ipv6_mc_destroy_dev(struct inet6_dev *idev)
 	ipv6_mc_down(idev);
 	mld_clear_delrec(idev);
 
-	/* Delete all-nodes address. */
-	/* We cannot call ipv6_dev_mc_dec() directly, our caller in
-	 * addrconf.c has NULL'd out dev->ip6_ptr so in6_dev_get() will
-	 * fail.
-	 */
-	__ipv6_dev_mc_dec(idev, &in6addr_linklocal_allnodes);
-
-	if (idev->cnf.forwarding)
-		__ipv6_dev_mc_dec(idev, &in6addr_linklocal_allrouters);
-
 	write_lock_bh(&idev->lock);
 	while ((i = idev->mc_list) != NULL) {
 		idev->mc_list = i->next;
-- 
2.19.2

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

[Rafał Miłecki]

On 04.03.2020 10:07, Hangbin Liu wrote:
> On Wed, Mar 04, 2020 at 08:44:29AM +0100, Rafał Miłecki wrote:
>>> 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.
> 
> Yeah...Hope you could help dig more.

I disabled all relevant OpenWrt deamons (netifd, odhcpcd, etc.) but
issue remains.

I did some extra debugging and I believe I found the real problem!

When creating interface, ipv6_add_dev() gets executed and it calls:
ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);

That results in allocating "struct ifmcaddr6" with ff02::2 and adding
it to the idev->mc_list.

On "ifconfig $dev up" nothing interesting happens.

On "ifconfig $dev down" ipv6_mc_down() gets executed. It iterates over
idev->mc_list (in my case: ff02::2, ff02::1, ff01::1) and calls
igmp6_group_dropped() for each entry.

In case of ff02::2 igmp6_group_dropped() calls igmp6_leave_group() which
calls mld_add_delrec(). *That* is when another "struct ifmcaddr6" gets
allocated and gets added to the idev->mc_tomb.

BINGO.

Summary:
Every "ifconfig $dev down" results in:
ipv6_mc_down() → igmp6_group_dropped() → igmp6_leave_group() → mld_add_delrec()
& allocating & adding "struct ifmcaddr6" (ff02::2) to the idev->mc_tomb.


>>> 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
> 
> What does this step do? Is there a corresponding command for ip link?

A single wireless device can support multiple interfaces. That is what
lets you handle following example cases on a single radio chip:
1. Multiple AP interfaces (different SSIDs)
2. STA (client) interface + monitor interface (raw 802.11 frames)
3. STA (client) interface + AP insterface - wireless bridging

That iw command simply creates network interface for a given radio.


>> for i in $(seq 1 10); do ifconfig mon-phy0 up; ifconfig mon-phy0 down; done
>> iw dev mon-phy0 del
> 
> The other cmd looks normal. BTW, I have create a new patch. The previous one
> will leak ff01::1, ff02::1, ff01::2 as the link up will not re-add them.
> 
> The new patch will join and leave all node/route address when link up/down
> instead of store them. But I don't think it could resolve your issue as the
> code logic is not changed. If you like, you can have a try.

Should I still try it given my above debugging results?

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

[Hangbin Liu]

On Wed, Mar 04, 2020 at 11:07:36AM +0100, Rafał Miłecki wrote:
> BINGO.
> 
> Summary:
> Every "ifconfig $dev down" results in:
> ipv6_mc_down() → igmp6_group_dropped() → igmp6_leave_group() → mld_add_delrec()
> & allocating & adding "struct ifmcaddr6" (ff02::2) to the idev->mc_tomb.

Yes, when link down, we store the pmc info in idev->mc_tomb via
mld_add_delrec(), but later when link up, we didn't create new pmc,
but just copy the pmc info in idev->mc_tomb to current idev via
mld_del_delrec() and free the tomb pmc.

> Should I still try it given my above debugging results?

In the new patch, I removed the "ff02::2" address in ipv6_mc_down()
and re-added it in ipv6_mc_up(). I would appreciate if you could help
try it and see if we really did wrong in mld_add_delrec().

Thanks
Hangbin

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

[Rafał Miłecki]

Hi Hangbin,

I took a moment to note down what exactly happens in the IPv6 code so
we can be sure things work as expected. I compared kernel behavior
without your patch and with it for two different network interfaces.


On 04.03.2020 10:07, Hangbin Liu wrote:
>  From 9cf504dc30198133e470ea783c3fa53a8f56a20a Mon Sep 17 00:00:00 2001
> From: Hangbin Liu <liuhangbin@gmail.com>
> Date: Tue, 3 Mar 2020 20:58:39 +0800
> Subject: [PATCH] ipv6/mcast: join and leave allnodes, allrouters addresses
>   when link up/down
> 
> This patch fixes two issues:
> 
> 1) When link up with IPv6 forwarding enabled, we forgot to join
> sitelocal_allrouters(ff01::2) and interfacelocal_allrouters(ff05::2),
> like what we do in function dev_forward_change()
> 
> 2) When link down, there is no listener for the allnodes, allrouters
> addresses. So we should remove them instead of storing the info in
> idev->mc_tomb.
> 
> To fix these two issue, I add two new functions ipv6_mc_join_localaddr()
> and ipv6_mc_leave_localaddr() to handle these addresses. And just join
> leave the group in ipv6_mc_{up, down}.

********************
  WITHOUT YOUR PATCH
********************

The problem we're dealing with seems to be specific to non-Ethernen
devices. For ARPHRD_IEEE80211_RADIOTAP:
1. Multicast addresses get added normally - as for any Ethernet device
2. addrconf_dev_config() returns without calling addrconf_add_dev()

That means wireless monitor interface gets mcast addresses added but
not removed (like it happens for Ethernet devices).

For ARPHRD_ETHER things seem to work correctly.

#
# wlan0 (ARPHRD_ETHER)
#

addrconf_notify(NETDEV_REGISTER)
	ipv6_add_dev
		ipv6_dev_mc_inc(ff01::1)
		ipv6_dev_mc_inc(ff02::1)
		ipv6_dev_mc_inc(ff02::2)

addrconf_notify(NETDEV_UP)
	addrconf_dev_config
		addrconf_add_dev
			ipv6_find_idev
				ipv6_mc_up
					mld_del_delrec(ff02::2)
					igmp6_group_added(ff02::2)
					mld_del_delrec(ff02::1)
					igmp6_group_added(ff02::1)
					mld_del_delrec(ff01::1)
					igmp6_group_added(ff01::1)

addrconf_notify(NETDEV_DOWN)
	addrconf_ifdown
		ipv6_mc_down
			igmp6_group_dropped(ff02::2)
				igmp6_leave_group(ff02::2)
					mld_add_delrec(ff02::2)
			igmp6_group_dropped(ff02::1)
			igmp6_group_dropped(ff01::1)

ipv6_mc_destroy_dev
	ipv6_mc_down
		igmp6_group_dropped(ff02::2)
		igmp6_group_dropped(ff02::1)
		igmp6_group_dropped(ff01::1)
	mld_clear_delrec
	__ipv6_dev_mc_dec(ff02::1)
	__ipv6_dev_mc_dec(ff02::2)


#
# mon-phy0 (ARPHRD_IEEE80211_RADIOTAP) ***
#

addrconf_notify(NETDEV_REGISTER)
	ipv6_add_dev
		ipv6_dev_mc_inc(ff01::1)
		ipv6_dev_mc_inc(ff02::1)
		ipv6_dev_mc_inc(ff02::2)

addrconf_notify(NETDEV_UP)
	addrconf_dev_config
		/* Alas, we support only Ethernet autoconfiguration. */
		return;

addrconf_notify(NETDEV_DOWN)
	addrconf_ifdown
		ipv6_mc_down
			igmp6_group_dropped(ff02::2)
				igmp6_leave_group(ff02::2)
					mld_add_delrec(ff02::2)
			igmp6_group_dropped(ff02::1)
			igmp6_group_dropped(ff01::1)

ipv6_mc_destroy_dev
	ipv6_mc_down
		igmp6_group_dropped(ff02::2)
		igmp6_group_dropped(ff02::1)
		igmp6_group_dropped(ff01::1)
	mld_clear_delrec
	__ipv6_dev_mc_dec(ff02::1)
	__ipv6_dev_mc_dec(ff02::2)

*****************
  WITH YOUR PATCH
*****************

Things work OK - with your changes all calls like:
ipv6_dev_mc_inc(ff01::1)
ipv6_dev_mc_inc(ff02::1)
ipv6_dev_mc_inc(ff02::2)
are now part of ipv6_mc_up() which gets never called for the
ARPHRD_IEEE80211_RADIOTAP.

I got one more question though:

Should we really call ipv6_mc_down() for ARPHRD_IEEE80211_RADIOTAP?

We don't call ipv6_mc_up() so maybe ipv6_mc_down() should be avoided
too? It seems like asking for more problems in the future. Even now
we call ipv6_mc_leave_localaddr() without ipv6_mc_join_localaddr()
called first which seems unintuitive.

#
# wlan0 (ARPHRD_ETHER)
#

addrconf_notify(NETDEV_REGISTER)
	ipv6_add_dev

addrconf_notify(NETDEV_UP)
	addrconf_dev_config
		addrconf_add_dev
			ipv6_find_idev
				ipv6_mc_up
					ipv6_mc_join_localaddr
						ipv6_dev_mc_inc(ff01::1)
						ipv6_dev_mc_inc(ff02::1)
						ipv6_dev_mc_inc(ff01::2)
						ipv6_dev_mc_inc(ff02::2)
						ipv6_dev_mc_inc(ff05::2)
					mld_del_delrec(ff05::2)
					igmp6_group_added(ff05::2)
					mld_del_delrec(ff02::2)
					igmp6_group_added(ff02::2)
					mld_del_delrec(ff01::2)
					igmp6_group_added(ff01::2)
					mld_del_delrec(ff02::1)
					igmp6_group_added(ff02::1)
					mld_del_delrec(ff01::1)
					igmp6_group_added(ff01::1)

addrconf_notify(NETDEV_DOWN)
	addrconf_ifdown
		ipv6_mc_down
			ipv6_mc_leave_localaddr
				__ipv6_dev_mc_dec(ff01::1)
				__ipv6_dev_mc_dec(ff02::1)
				__ipv6_dev_mc_dec(ff01::2)
				__ipv6_dev_mc_dec(ff02::2)
					igmp6_group_dropped(ff02::2)
				__ipv6_dev_mc_dec(ff05::2)
					igmp6_group_dropped(ff05::2)

ipv6_mc_destroy_dev
	ipv6_mc_down
		ipv6_mc_leave_localaddr
			__ipv6_dev_mc_dec(ff01::1)
			__ipv6_dev_mc_dec(ff02::1)
			__ipv6_dev_mc_dec(ff01::2)
			__ipv6_dev_mc_dec(ff02::2)
			__ipv6_dev_mc_dec(ff05::2)
	mld_clear_delrec

#
# mon-phy0 (ARPHRD_IEEE80211_RADIOTAP)
#

addrconf_notify(NETDEV_REGISTER)
	ipv6_add_dev

addrconf_notify(NETDEV_UP)
	addrconf_dev_config
		/* Alas, we support only Ethernet autoconfiguration. */
		return;

addrconf_notify(NETDEV_DOWN)
	addrconf_ifdown
		ipv6_mc_down
			ipv6_mc_leave_localaddr
				__ipv6_dev_mc_dec(ff01::1)
				__ipv6_dev_mc_dec(ff02::1)
				__ipv6_dev_mc_dec(ff01::2)
				__ipv6_dev_mc_dec(ff02::2)
				__ipv6_dev_mc_dec(ff05::2)

ipv6_mc_destroy_dev
	ipv6_mc_down
		ipv6_mc_leave_localaddr
			__ipv6_dev_mc_dec(ff01::1)
			__ipv6_dev_mc_dec(ff02::1)
			__ipv6_dev_mc_dec(ff01::2)
			__ipv6_dev_mc_dec(ff02::2)
			__ipv6_dev_mc_dec(ff05::2)
	mld_clear_delrec

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

[Hangbin Liu]

Hi Rafał,
On Fri, Mar 06, 2020 at 12:14:08PM +0100, Rafał Miłecki wrote:
> ********************
>  WITHOUT YOUR PATCH
> ********************
> 
> The problem we're dealing with seems to be specific to non-Ethernen
> devices. For ARPHRD_IEEE80211_RADIOTAP:
> 1. Multicast addresses get added normally - as for any Ethernet device
> 2. addrconf_dev_config() returns without calling addrconf_add_dev()
> 
> That means wireless monitor interface gets mcast addresses added but
> not removed (like it happens for Ethernet devices).
> #
> # mon-phy0 (ARPHRD_IEEE80211_RADIOTAP) ***
> #
> 
> addrconf_notify(NETDEV_REGISTER)
> 	ipv6_add_dev
> 		ipv6_dev_mc_inc(ff01::1)
> 		ipv6_dev_mc_inc(ff02::1)
> 		ipv6_dev_mc_inc(ff02::2)
> 
> addrconf_notify(NETDEV_UP)
> 	addrconf_dev_config
> 		/* Alas, we support only Ethernet autoconfiguration. */
> 		return;
> 
> addrconf_notify(NETDEV_DOWN)
> 	addrconf_ifdown
> 		ipv6_mc_down
> 			igmp6_group_dropped(ff02::2)
> 				igmp6_leave_group(ff02::2)
> 					mld_add_delrec(ff02::2)
> 			igmp6_group_dropped(ff02::1)
> 			igmp6_group_dropped(ff01::1)
> 

I'm very appreciate for your analyze. This makes me know why this issue
happens and why I couldn't reproduce it.

Yes, with ARPHRD_IEEE80211_RADIOTAP, we called mld_add_delrec() every time
when ipv6_mc_down(), but we never called mld_del_delrec() as ipv6_mc_up() was
not called. This makes the idev->mc_tomb bigger and bigger.

> *****************
>  WITH YOUR PATCH
> *****************
> 
> Things work OK - with your changes all calls like:
> ipv6_dev_mc_inc(ff01::1)
> ipv6_dev_mc_inc(ff02::1)
> ipv6_dev_mc_inc(ff02::2)
> are now part of ipv6_mc_up() which gets never called for the
> ARPHRD_IEEE80211_RADIOTAP.
> 
> I got one more question though:
> 
> Should we really call ipv6_mc_down() for ARPHRD_IEEE80211_RADIOTAP?
> 
> We don't call ipv6_mc_up() so maybe ipv6_mc_down() should be avoided
> too? It seems like asking for more problems in the future. Even now


Yes, for me there are actually two questions.

1. Should we avoid call ipv6_mc_down() as we don't call ipv6_mc_up() for
non-Ethernen dev. I think the answer is yes, we could. But on the
other hand, it seems IPv4 doesn't check the dev type and calls ip_mc_up()
directly in inetdev_event() NETDEV_UP.

2. Should we move ipv6_dev_mc_inc() from ipv6_add_dev() to ipv6_mc_up()?
I don't know yet, this dependents on whether we could add multicast address
on non-Ethernen dev.

> we call ipv6_mc_leave_localaddr() without ipv6_mc_join_localaddr()
> called first which seems unintuitive.

This doesn't matter much yet. As we will check if we have the address
in __ipv6_dev_mc_dec(), if not, we just return. But yes, form logic, this
looks asymmetric.

Thanks
Hangbin

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

[Rafał Miłecki]

On 09.03.2020 09:33, Hangbin Liu wrote:
> I'm very appreciate for your analyze. This makes me know why this issue
> happens and why I couldn't reproduce it.
> 
> Yes, with ARPHRD_IEEE80211_RADIOTAP, we called mld_add_delrec() every time
> when ipv6_mc_down(), but we never called mld_del_delrec() as ipv6_mc_up() was
> not called. This makes the idev->mc_tomb bigger and bigger.

Thanks, I'm happy to hear that was helpful.


>> *****************
>>   WITH YOUR PATCH
>> *****************
>>
>> Things work OK - with your changes all calls like:
>> ipv6_dev_mc_inc(ff01::1)
>> ipv6_dev_mc_inc(ff02::1)
>> ipv6_dev_mc_inc(ff02::2)
>> are now part of ipv6_mc_up() which gets never called for the
>> ARPHRD_IEEE80211_RADIOTAP.
>>
>> I got one more question though:
>>
>> Should we really call ipv6_mc_down() for ARPHRD_IEEE80211_RADIOTAP?
>>
>> We don't call ipv6_mc_up() so maybe ipv6_mc_down() should be avoided
>> too? It seems like asking for more problems in the future. Even now
> 
> 
> Yes, for me there are actually two questions.
> 
> 1. Should we avoid call ipv6_mc_down() as we don't call ipv6_mc_up() for
> non-Ethernen dev. I think the answer is yes, we could. But on the
> other hand, it seems IPv4 doesn't check the dev type and calls ip_mc_up()
> directly in inetdev_event() NETDEV_UP.
> 
> 2. Should we move ipv6_dev_mc_inc() from ipv6_add_dev() to ipv6_mc_up()?
> I don't know yet, this dependents on whether we could add multicast address
> on non-Ethernen dev.

I'm not the one to answer them surely with my limited net subsystem
understanding :( Any idea how to proceed with this? I assume your patch
is still the right step, do you think you can send it officially now?


>> we call ipv6_mc_leave_localaddr() without ipv6_mc_join_localaddr()
>> called first which seems unintuitive.
> 
> This doesn't matter much yet. As we will check if we have the address
> in __ipv6_dev_mc_dec(), if not, we just return. But yes, form logic, this
> looks asymmetric.

Right, just slightly unintuitive asymmetric code.

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

[Hangbin Liu]

On Mon, Mar 09, 2020 at 01:31:16PM +0100, Rafał Miłecki wrote:
> > 2. Should we move ipv6_dev_mc_inc() from ipv6_add_dev() to ipv6_mc_up()?
> > I don't know yet, this dependents on whether we could add multicast address
> > on non-Ethernen dev.
> 
> I'm not the one to answer them surely with my limited net subsystem
> understanding :( Any idea how to proceed with this? I assume your patch
> is still the right step, do you think you can send it officially now?
> 
> 
> > > we call ipv6_mc_leave_localaddr() without ipv6_mc_join_localaddr()
> > > called first which seems unintuitive.
> > 
> > This doesn't matter much yet. As we will check if we have the address
> > in __ipv6_dev_mc_dec(), if not, we just return. But yes, form logic, this
> > looks asymmetric.
> 
> Right, just slightly unintuitive asymmetric code.

Hi Rafał,

I investigated this issue and am going to enable ipv6_mc_up for non-Ethernet
interface. The patch I sent you before will be post as a RFC for net-next.

Thanks
Hangbin