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  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:   Fri, 6 Mar 2020 12:14:08 +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)

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@...il.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

Powered by blists - more mailing lists