[RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[Vladimir Oltean]

ipv4 devinet calls ip_mc_down(), and ipv6 calls addrconf_ifdown(), and
both of these eventually result in calls to dev_mc_del(), either through
igmp_group_dropped() or igmp6_group_dropped().

The problem is that dev_mc_del() does call __dev_set_rx_mode(), but this
will not propagate all the way to the ndo_set_rx_mode() of the device,
because of this check:

	/* dev_open will call this function so the list will stay sane. */
	if (!(dev->flags&IFF_UP))
		return;

and the NETDEV_DOWN notifier is emitted while the interface is already
down. OTOH we have NETDEV_GOING_DOWN which is emitted a bit earlier -
see:

dev_close_many()
-> __dev_close_many()
   -> call_netdevice_notifiers(NETDEV_GOING_DOWN, dev);
   -> dev->flags &= ~IFF_UP;
-> call_netdevice_notifiers(NETDEV_DOWN, dev);

Normally this oversight is easy to miss, because the addresses aren't
lost, just not synced to the device until the next up event.

DSA does some processing in its dsa_slave_set_rx_mode(), and assumes
that all addresses that were synced are also unsynced by the time the
device is unregistered. Due to that assumption not being satisfied,
the WARN_ON(!list_empty(&dp->mdbs)); from dsa_switch_release_ports()
triggers, and we leak memory corresponding to the multicast addresses
that were never synced.

Minimal reproducer:
ip link set swp0 up
ip link set swp0 down
echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind

The proposal is to respond to that slightly earlier notifier with the
IGMP address deletion, so that the ndo_set_rx_mode() of the device does
actually get called. I am not familiar with the details of these layers,
but it appeared to me that NETDEV_DOWN needed to be replaced everywhere
with NETDEV_GOING_DOWN, so I blindly did that and it worked.

Fixes: 5e8a1e03aa4d ("net: dsa: install secondary unicast and multicast addresses as host FDB/MDB")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Obviously DSA is not the only affected driver, but the extent to which
other drivers are impacted is not obvious to me. At least in DSA, there
is a WARN_ON() and a memory leak, so this is why I chose that Fixes tag.

 net/ipv4/devinet.c  |  7 ++++---
 net/ipv6/addrconf.c | 12 ++++++------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 5deac0517ef7..95690d16d651 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -392,7 +392,8 @@ static void __inet_del_ifa(struct in_device *in_dev,
 
 				rtmsg_ifa(RTM_DELADDR, ifa, nlh, portid);
 				blocking_notifier_call_chain(&inetaddr_chain,
-						NETDEV_DOWN, ifa);
+							     NETDEV_GOING_DOWN,
+							     ifa);
 				inet_free_ifa(ifa);
 			} else {
 				promote = ifa;
@@ -429,7 +430,7 @@ static void __inet_del_ifa(struct in_device *in_dev,
 	   So that, this order is correct.
 	 */
 	rtmsg_ifa(RTM_DELADDR, ifa1, nlh, portid);
-	blocking_notifier_call_chain(&inetaddr_chain, NETDEV_DOWN, ifa1);
+	blocking_notifier_call_chain(&inetaddr_chain, NETDEV_GOING_DOWN, ifa1);
 
 	if (promote) {
 		struct in_ifaddr *next_sec;
@@ -1588,7 +1589,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 		/* Send gratuitous ARP to notify of link change */
 		inetdev_send_gratuitous_arp(dev, in_dev);
 		break;
-	case NETDEV_DOWN:
+	case NETDEV_GOING_DOWN:
 		ip_mc_down(in_dev);
 		break;
 	case NETDEV_PRE_TYPE_CHANGE:
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3797917237d0..9e484f829f1c 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1307,7 +1307,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 
 	ipv6_ifa_notify(RTM_DELADDR, ifp);
 
-	inet6addr_notifier_call_chain(NETDEV_DOWN, ifp);
+	inet6addr_notifier_call_chain(NETDEV_GOING_DOWN, ifp);
 
 	if (action != CLEANUP_PREFIX_RT_NOP) {
 		cleanup_prefix_route(ifp, expires,
@@ -3670,12 +3670,12 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 		}
 		break;
 
-	case NETDEV_DOWN:
+	case NETDEV_GOING_DOWN:
 	case NETDEV_UNREGISTER:
 		/*
 		 *	Remove all addresses from this interface.
 		 */
-		addrconf_ifdown(dev, event != NETDEV_DOWN);
+		addrconf_ifdown(dev, event != NETDEV_GOING_DOWN);
 		break;
 
 	case NETDEV_CHANGENAME:
@@ -3741,7 +3741,7 @@ static bool addr_is_local(const struct in6_addr *addr)
 
 static int addrconf_ifdown(struct net_device *dev, bool unregister)
 {
-	unsigned long event = unregister ? NETDEV_UNREGISTER : NETDEV_DOWN;
+	unsigned long event = unregister ? NETDEV_UNREGISTER : NETDEV_GOING_DOWN;
 	struct net *net = dev_net(dev);
 	struct inet6_dev *idev;
 	struct inet6_ifaddr *ifa;
@@ -3877,7 +3877,7 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
 
 		if (state != INET6_IFADDR_STATE_DEAD) {
 			__ipv6_ifa_notify(RTM_DELADDR, ifa);
-			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
+			inet6addr_notifier_call_chain(NETDEV_GOING_DOWN, ifa);
 		} else {
 			if (idev->cnf.forwarding)
 				addrconf_leave_anycast(ifa);
@@ -6252,7 +6252,7 @@ static void dev_disable_change(struct inet6_dev *idev)
 
 	netdev_notifier_info_init(&info, idev->dev);
 	if (idev->cnf.disable_ipv6)
-		addrconf_notify(NULL, NETDEV_DOWN, &info);
+		addrconf_notify(NULL, NETDEV_GOING_DOWN, &info);
 	else
 		addrconf_notify(NULL, NETDEV_UP, &info);
 }
-- 
2.34.1

Re: [RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[David Ahern]

[ cc Ido in case such a change has implications to mlxsw ]

On 4/6/23 5:30 PM, Vladimir Oltean wrote:
> ipv4 devinet calls ip_mc_down(), and ipv6 calls addrconf_ifdown(), and
> both of these eventually result in calls to dev_mc_del(), either through
> igmp_group_dropped() or igmp6_group_dropped().
> 
> The problem is that dev_mc_del() does call __dev_set_rx_mode(), but this
> will not propagate all the way to the ndo_set_rx_mode() of the device,
> because of this check:
> 
> 	/* dev_open will call this function so the list will stay sane. */
> 	if (!(dev->flags&IFF_UP))
> 		return;
> 
> and the NETDEV_DOWN notifier is emitted while the interface is already
> down. OTOH we have NETDEV_GOING_DOWN which is emitted a bit earlier -
> see:
> 
> dev_close_many()
> -> __dev_close_many()
>    -> call_netdevice_notifiers(NETDEV_GOING_DOWN, dev);
>    -> dev->flags &= ~IFF_UP;
> -> call_netdevice_notifiers(NETDEV_DOWN, dev);
> 
> Normally this oversight is easy to miss, because the addresses aren't
> lost, just not synced to the device until the next up event.
> 
> DSA does some processing in its dsa_slave_set_rx_mode(), and assumes
> that all addresses that were synced are also unsynced by the time the
> device is unregistered. Due to that assumption not being satisfied,
> the WARN_ON(!list_empty(&dp->mdbs)); from dsa_switch_release_ports()
> triggers, and we leak memory corresponding to the multicast addresses
> that were never synced.
> 
> Minimal reproducer:
> ip link set swp0 up
> ip link set swp0 down
> echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind
> 
> The proposal is to respond to that slightly earlier notifier with the
> IGMP address deletion, so that the ndo_set_rx_mode() of the device does
> actually get called. I am not familiar with the details of these layers,
> but it appeared to me that NETDEV_DOWN needed to be replaced everywhere
> with NETDEV_GOING_DOWN, so I blindly did that and it worked.
> 
> Fixes: 5e8a1e03aa4d ("net: dsa: install secondary unicast and multicast addresses as host FDB/MDB")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Obviously DSA is not the only affected driver, but the extent to which
> other drivers are impacted is not obvious to me. At least in DSA, there
> is a WARN_ON() and a memory leak, so this is why I chose that Fixes tag.
> 
>  net/ipv4/devinet.c  |  7 ++++---
>  net/ipv6/addrconf.c | 12 ++++++------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 5deac0517ef7..95690d16d651 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -392,7 +392,8 @@ static void __inet_del_ifa(struct in_device *in_dev,
>  
>  				rtmsg_ifa(RTM_DELADDR, ifa, nlh, portid);
>  				blocking_notifier_call_chain(&inetaddr_chain,
> -						NETDEV_DOWN, ifa);
> +							     NETDEV_GOING_DOWN,
> +							     ifa);
>  				inet_free_ifa(ifa);
>  			} else {
>  				promote = ifa;
> @@ -429,7 +430,7 @@ static void __inet_del_ifa(struct in_device *in_dev,
>  	   So that, this order is correct.
>  	 */
>  	rtmsg_ifa(RTM_DELADDR, ifa1, nlh, portid);
> -	blocking_notifier_call_chain(&inetaddr_chain, NETDEV_DOWN, ifa1);
> +	blocking_notifier_call_chain(&inetaddr_chain, NETDEV_GOING_DOWN, ifa1);
>  
>  	if (promote) {
>  		struct in_ifaddr *next_sec;
> @@ -1588,7 +1589,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
>  		/* Send gratuitous ARP to notify of link change */
>  		inetdev_send_gratuitous_arp(dev, in_dev);
>  		break;
> -	case NETDEV_DOWN:
> +	case NETDEV_GOING_DOWN:
>  		ip_mc_down(in_dev);
>  		break;
>  	case NETDEV_PRE_TYPE_CHANGE:
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 3797917237d0..9e484f829f1c 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1307,7 +1307,7 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
>  
>  	ipv6_ifa_notify(RTM_DELADDR, ifp);
>  
> -	inet6addr_notifier_call_chain(NETDEV_DOWN, ifp);
> +	inet6addr_notifier_call_chain(NETDEV_GOING_DOWN, ifp);
>  
>  	if (action != CLEANUP_PREFIX_RT_NOP) {
>  		cleanup_prefix_route(ifp, expires,
> @@ -3670,12 +3670,12 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
>  		}
>  		break;
>  
> -	case NETDEV_DOWN:
> +	case NETDEV_GOING_DOWN:
>  	case NETDEV_UNREGISTER:
>  		/*
>  		 *	Remove all addresses from this interface.
>  		 */
> -		addrconf_ifdown(dev, event != NETDEV_DOWN);
> +		addrconf_ifdown(dev, event != NETDEV_GOING_DOWN);
>  		break;
>  
>  	case NETDEV_CHANGENAME:
> @@ -3741,7 +3741,7 @@ static bool addr_is_local(const struct in6_addr *addr)
>  
>  static int addrconf_ifdown(struct net_device *dev, bool unregister)
>  {
> -	unsigned long event = unregister ? NETDEV_UNREGISTER : NETDEV_DOWN;
> +	unsigned long event = unregister ? NETDEV_UNREGISTER : NETDEV_GOING_DOWN;
>  	struct net *net = dev_net(dev);
>  	struct inet6_dev *idev;
>  	struct inet6_ifaddr *ifa;
> @@ -3877,7 +3877,7 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
>  
>  		if (state != INET6_IFADDR_STATE_DEAD) {
>  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
> -			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
> +			inet6addr_notifier_call_chain(NETDEV_GOING_DOWN, ifa);
>  		} else {
>  			if (idev->cnf.forwarding)
>  				addrconf_leave_anycast(ifa);
> @@ -6252,7 +6252,7 @@ static void dev_disable_change(struct inet6_dev *idev)
>  
>  	netdev_notifier_info_init(&info, idev->dev);
>  	if (idev->cnf.disable_ipv6)
> -		addrconf_notify(NULL, NETDEV_DOWN, &info);
> +		addrconf_notify(NULL, NETDEV_GOING_DOWN, &info);
>  	else
>  		addrconf_notify(NULL, NETDEV_UP, &info);
>  }

Re: [RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[Ido Schimmel]

On Sun, Apr 09, 2023 at 08:08:01PM -0600, David Ahern wrote:
> [ cc Ido in case such a change has implications to mlxsw ]

Thanks

> 
> On 4/6/23 5:30 PM, Vladimir Oltean wrote:
> > ipv4 devinet calls ip_mc_down(), and ipv6 calls addrconf_ifdown(), and
> > both of these eventually result in calls to dev_mc_del(), either through
> > igmp_group_dropped() or igmp6_group_dropped().
> > 
> > The problem is that dev_mc_del() does call __dev_set_rx_mode(), but this
> > will not propagate all the way to the ndo_set_rx_mode() of the device,
> > because of this check:
> > 
> > 	/* dev_open will call this function so the list will stay sane. */
> > 	if (!(dev->flags&IFF_UP))
> > 		return;
> > 
> > and the NETDEV_DOWN notifier is emitted while the interface is already
> > down. OTOH we have NETDEV_GOING_DOWN which is emitted a bit earlier -
> > see:
> > 
> > dev_close_many()
> > -> __dev_close_many()
> >    -> call_netdevice_notifiers(NETDEV_GOING_DOWN, dev);
> >    -> dev->flags &= ~IFF_UP;
> > -> call_netdevice_notifiers(NETDEV_DOWN, dev);
> > 
> > Normally this oversight is easy to miss, because the addresses aren't
> > lost, just not synced to the device until the next up event.
> > 
> > DSA does some processing in its dsa_slave_set_rx_mode(), and assumes
> > that all addresses that were synced are also unsynced by the time the
> > device is unregistered. Due to that assumption not being satisfied,
> > the WARN_ON(!list_empty(&dp->mdbs)); from dsa_switch_release_ports()
> > triggers, and we leak memory corresponding to the multicast addresses
> > that were never synced.
> > 
> > Minimal reproducer:
> > ip link set swp0 up
> > ip link set swp0 down
> > echo 0000:00:00.5 > /sys/bus/pci/drivers/mscc_felix/unbind
> > 
> > The proposal is to respond to that slightly earlier notifier with the
> > IGMP address deletion, so that the ndo_set_rx_mode() of the device does
> > actually get called. I am not familiar with the details of these layers,
> > but it appeared to me that NETDEV_DOWN needed to be replaced everywhere
> > with NETDEV_GOING_DOWN, so I blindly did that and it worked.

I think there is a confusion here between the netdev notifier and
inetaddr notifiers. They all use "NETDEV_DOWN", but in the inetaddr
notifiers it means that an address is being deleted. Changing the event
to "NETDEV_GOING_DOWN" is going to break a lot of users since none of
the inetaddr listeners respond to "NETDEV_GOING_DOWN".

IOW, I believe you only need this change for IPv4 (and similarly for
IPv6):

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 5deac0517ef7..679c9819f25b 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1588,7 +1588,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 		/* Send gratuitous ARP to notify of link change */
 		inetdev_send_gratuitous_arp(dev, in_dev);
 		break;
-	case NETDEV_DOWN:
+	case NETDEV_GOING_DOWN:
 		ip_mc_down(in_dev);
 		break;
 	case NETDEV_PRE_TYPE_CHANGE:

Re: [RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[Vladimir Oltean]

Hi Ido,

On Mon, Apr 10, 2023 at 10:55:02AM +0300, Ido Schimmel wrote:
> > > The proposal is to respond to that slightly earlier notifier with the
> > > IGMP address deletion, so that the ndo_set_rx_mode() of the device does
> > > actually get called. I am not familiar with the details of these layers,
> > > but it appeared to me that NETDEV_DOWN needed to be replaced everywhere
> > > with NETDEV_GOING_DOWN, so I blindly did that and it worked.
> 
> I think there is a confusion here between the netdev notifier and
> inetaddr notifiers. They all use "NETDEV_DOWN", but in the inetaddr
> notifiers it means that an address is being deleted. Changing the event
> to "NETDEV_GOING_DOWN" is going to break a lot of users since none of
> the inetaddr listeners respond to "NETDEV_GOING_DOWN".
> 
> IOW, I believe you only need this change for IPv4 (and similarly for
> IPv6):
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 5deac0517ef7..679c9819f25b 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1588,7 +1588,7 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
>  		/* Send gratuitous ARP to notify of link change */
>  		inetdev_send_gratuitous_arp(dev, in_dev);
>  		break;
> -	case NETDEV_DOWN:
> +	case NETDEV_GOING_DOWN:
>  		ip_mc_down(in_dev);
>  		break;
>  	case NETDEV_PRE_TYPE_CHANGE:

You are correct, only that portion is needed for IPv4. When I open my
eyes, I see it too :)

Although it would have been a lot less confusing for someone looking at
the code for the first time if the inetaddr and inet6addr notifiers did
not use events from the same NETDEV_ namespace as the netdev notifiers...

So, how do you think I should proceed with this? One patch or two
(for IPv4 and IPv6)? Is the Fixes: tag ok?

Re: [RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[Ido Schimmel]

On Mon, Apr 10, 2023 at 01:09:58PM +0300, Vladimir Oltean wrote:
> So, how do you think I should proceed with this? One patch or two
> (for IPv4 and IPv6)? Is the Fixes: tag ok?

Fixes tag looks OK and one patch is fine by me. However, given the
problem is the check you mentioned in __dev_set_rx_mode(), wouldn't it
be better to simply remove it? From the comment above this check it
seems to assume that there is no need to update the Rx filters of the
device when it's down because they will be synced when it's put back up,
but it fails to consider the case where one wants to clear the filters
of the device as part of device dismantle.

Re: [RFC PATCH net] net: ipv4/ipv6 addrconf: call igmp{,6}_group_dropped() while dev is still up

[Vladimir Oltean]

On Mon, Apr 10, 2023 at 02:43:43PM +0300, Ido Schimmel wrote:
> On Mon, Apr 10, 2023 at 01:09:58PM +0300, Vladimir Oltean wrote:
> > So, how do you think I should proceed with this? One patch or two
> > (for IPv4 and IPv6)? Is the Fixes: tag ok?
> 
> Fixes tag looks OK and one patch is fine by me. However, given the
> problem is the check you mentioned in __dev_set_rx_mode(), wouldn't it
> be better to simply remove it? From the comment above this check it
> seems to assume that there is no need to update the Rx filters of the
> device when it's down because they will be synced when it's put back up,
> but it fails to consider the case where one wants to clear the filters
> of the device as part of device dismantle.

Yes, I can do this.