[PATCH v2] net: core: SIOCADDMULTI/SIOCDELMULTI distinguish between uc and mc

[PATCH v2] net: core: SIOCADDMULTI/SIOCDELMULTI distinguish between uc and mc

[Denys Zagorui]

SIOCADDMULTI API allows adding multicast/unicast mac addresses but
doesn't deferentiate them so if someone tries to add secondary
unicast mac addr it will be added to multicast netdev list which is
confusing. There is at least one user that allows adding secondary
unicast through this API.
(2f41f3358672 i40e/i40evf: fix unicast mac address add)

This patch adds check whether passed mac addr is uc or mc and adds
this mac addr to the corresponding list. Add 'global' variant for
adding/removing uc addresses similarly to mc.

Signed-off-by: Denys Zagorui <dzagorui@cisco.com>
---
 include/linux/netdevice.h    |  2 +
 include/uapi/linux/sockios.h |  2 +-
 net/core/dev_addr_lists.c    | 75 +++++++++++++++++++++++++++---------
 net/core/dev_ioctl.c         | 13 ++++++-
 4 files changed, 71 insertions(+), 21 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b0e303f6603f..9394f369be33 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4345,8 +4345,10 @@ int dev_addr_init(struct net_device *dev);
 
 /* Functions used for unicast addresses handling */
 int dev_uc_add(struct net_device *dev, const unsigned char *addr);
+int dev_uc_add_global(struct net_device *dev, const unsigned char *addr);
 int dev_uc_add_excl(struct net_device *dev, const unsigned char *addr);
 int dev_uc_del(struct net_device *dev, const unsigned char *addr);
+int dev_uc_del_global(struct net_device *dev, const unsigned char *addr);
 int dev_uc_sync(struct net_device *to, struct net_device *from);
 int dev_uc_sync_multiple(struct net_device *to, struct net_device *from);
 void dev_uc_unsync(struct net_device *to, struct net_device *from);
diff --git a/include/uapi/linux/sockios.h b/include/uapi/linux/sockios.h
index 7d1bccbbef78..f41b152b0268 100644
--- a/include/uapi/linux/sockios.h
+++ b/include/uapi/linux/sockios.h
@@ -80,7 +80,7 @@
 #define SIOCGIFHWADDR	0x8927		/* Get hardware address		*/
 #define SIOCGIFSLAVE	0x8929		/* Driver slaving support	*/
 #define SIOCSIFSLAVE	0x8930
-#define SIOCADDMULTI	0x8931		/* Multicast address lists	*/
+#define SIOCADDMULTI	0x8931		/* Mac address lists	*/
 #define SIOCDELMULTI	0x8932
 #define SIOCGIFINDEX	0x8933		/* name -> if_index mapping	*/
 #define SIOGIFINDEX	SIOCGIFINDEX	/* misprint compatibility :-)	*/
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 54cd568e7c2f..d150c2d84df4 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -573,6 +573,20 @@ int dev_uc_add_excl(struct net_device *dev, const unsigned char *addr)
 }
 EXPORT_SYMBOL(dev_uc_add_excl);
 
+static int __dev_uc_add(struct net_device *dev, const unsigned char *addr,
+			bool global)
+{
+	int err;
+
+	netif_addr_lock_bh(dev);
+	err = __hw_addr_add_ex(&dev->uc, addr, dev->addr_len,
+			       NETDEV_HW_ADDR_T_UNICAST, global, false, 0);
+	if (!err)
+		__dev_set_rx_mode(dev);
+	netif_addr_unlock_bh(dev);
+	return err;
+}
+
 /**
  *	dev_uc_add - Add a secondary unicast address
  *	@dev: device
@@ -583,18 +597,37 @@ EXPORT_SYMBOL(dev_uc_add_excl);
  */
 int dev_uc_add(struct net_device *dev, const unsigned char *addr)
 {
-	int err;
-
-	netif_addr_lock_bh(dev);
-	err = __hw_addr_add(&dev->uc, addr, dev->addr_len,
-			    NETDEV_HW_ADDR_T_UNICAST);
-	if (!err)
-		__dev_set_rx_mode(dev);
-	netif_addr_unlock_bh(dev);
-	return err;
+	return __dev_uc_add(dev, addr, false);
 }
 EXPORT_SYMBOL(dev_uc_add);
 
+/**
+ *	dev_uc_add_global - Add a global unicast address
+ *	@dev: device
+ *	@addr: address to add
+ *
+ *	Add a global unicast address to the device.
+ */
+int dev_uc_add_global(struct net_device *dev, const unsigned char *addr)
+{
+	return __dev_uc_add(dev, addr, true);
+}
+EXPORT_SYMBOL(dev_uc_add_global);
+
+static int __dev_uc_del(struct net_device *dev, const unsigned char *addr,
+			bool global)
+{
+	int err;
+
+	netif_addr_lock_bh(dev);
+	err = __hw_addr_del_ex(&dev->uc, addr, dev->addr_len,
+			       NETDEV_HW_ADDR_T_UNICAST, global, false);
+	if (!err)
+		__dev_set_rx_mode(dev);
+	netif_addr_unlock_bh(dev);
+	return err;
+}
+
 /**
  *	dev_uc_del - Release secondary unicast address.
  *	@dev: device
@@ -605,18 +638,24 @@ EXPORT_SYMBOL(dev_uc_add);
  */
 int dev_uc_del(struct net_device *dev, const unsigned char *addr)
 {
-	int err;
-
-	netif_addr_lock_bh(dev);
-	err = __hw_addr_del(&dev->uc, addr, dev->addr_len,
-			    NETDEV_HW_ADDR_T_UNICAST);
-	if (!err)
-		__dev_set_rx_mode(dev);
-	netif_addr_unlock_bh(dev);
-	return err;
+	return __dev_uc_del(dev, addr, false);
 }
 EXPORT_SYMBOL(dev_uc_del);
 
+/**
+ *	dev_uc_del_global - Delete a global unicast address.
+ *	@dev: device
+ *	@addr: address to delete
+ *
+ *	Release reference to a unicast address and remove it
+ *	from the device if the reference count drops to zero.
+ */
+int dev_uc_del_global(struct net_device *dev, const unsigned char *addr)
+{
+	return __dev_uc_del(dev, addr, true);
+}
+EXPORT_SYMBOL(dev_uc_del_global);
+
 /**
  *	dev_uc_sync - Synchronize device's unicast list to another device
  *	@to: destination device
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b2cf9b7bb7b8..7883bfd920fd 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -7,6 +7,7 @@
 #include <linux/wireless.h>
 #include <net/dsa.h>
 #include <net/wext.h>
+#include <linux/if_arp.h>
 
 /*
  *	Map an interface index to its name (SIOCGIFNAME)
@@ -299,7 +300,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 			return -EINVAL;
 		if (!netif_device_present(dev))
 			return -ENODEV;
-		return dev_mc_add_global(dev, ifr->ifr_hwaddr.sa_data);
+		if (dev->type == ARPHRD_ETHER &&
+		    is_unicast_ether_addr(ifr->ifr_hwaddr.sa_data))
+			return dev_uc_add_global(dev, ifr->ifr_hwaddr.sa_data);
+		else
+			return dev_mc_add_global(dev, ifr->ifr_hwaddr.sa_data);
 
 	case SIOCDELMULTI:
 		if (!ops->ndo_set_rx_mode ||
@@ -307,7 +312,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 			return -EINVAL;
 		if (!netif_device_present(dev))
 			return -ENODEV;
-		return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
+		if (dev->type == ARPHRD_ETHER &&
+		    is_unicast_ether_addr(ifr->ifr_hwaddr.sa_data))
+			return dev_uc_del_global(dev, ifr->ifr_hwaddr.sa_data);
+		else
+			return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
 
 	case SIOCSIFTXQLEN:
 		if (ifr->ifr_qlen < 0)
-- 
2.19.1

Re: [PATCH v2] net: core: SIOCADDMULTI/SIOCDELMULTI distinguish between uc and mc

[David Miller]

From: Denys Zagorui <dzagorui@cisco.com>
Date: Mon, 17 Aug 2020 10:52:24 -0700

> SIOCADDMULTI API allows adding multicast/unicast mac addresses but
> doesn't deferentiate them so if someone tries to add secondary
> unicast mac addr it will be added to multicast netdev list which is
> confusing. There is at least one user that allows adding secondary
> unicast through this API.
> (2f41f3358672 i40e/i40evf: fix unicast mac address add)

This doesn't seem appropriate at all.  If anything UC addresses
should be blocked and the Intel driver change reverted.  We have
a well defined way to add secondary UC addresses and the MC interfaces
are not it.

Furthermore, even if this was appropriate, "fixing" this only for
ethernet is definitely not appropriate.  The fix would need to be able
to handle any address type.  Having a generic interface work
inconsistently for one link type vs. another is a non-starter.

Thanks.

Re: [PATCH v2] net: core: SIOCADDMULTI/SIOCDELMULTI distinguish between uc and mc

[Denys Zagorui -X (dzagorui - GLOBALLOGIC INC at Cisco)]

>This doesn't seem appropriate at all.  If anything UC addresses
>should be blocked and the Intel driver change reverted.  We have
>a well defined way to add secondary UC addresses and the MC interfaces
>are not it.

As I understand by ‘well defined way’ you mean macvlan feature. But macvlan 
is more virtualisation thing where an additional netdevice is created and for each
skb passed to linux stack hash_lookup is made to find a netdevice to which this
skb belongs to based on mac_dst. In case if someone want to have a network
protocol on top of physical layer (AF_PACKET) where NIC should allow several uc
mac addr without enabling promiscuous mode i am not sure that for this simple
task macvlan is needed.

>Furthermore, even if this was appropriate, "fixing" this only for
>ethernet is definitely not appropriate.  The fix would need to be able
>to handle any address type.  Having a generic interface work
>inconsistently for one link type vs. another is a non-starter.

Maybe overusing multicast api isn’t the best choice for this purpose, but i noticed
that it is already overused by i40 for the same purpose, that is why i suggested
this diff. Anyway i can add separate ioctls for add/del secondary uc, if this is worth it.