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

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:   Wed, 4 Mar 2020 11:07:36 +0100
From:   Rafał Miłecki <zajec5@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Felix Fietkau <nbd@....name>, John Crispin <john@...ozen.org>,
        Jo-Philipp Wich <jo@...n.io>
Subject: Re: Regression: net/ipv6/mld running system out of memory (not a
 leak)

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?