[net-next PATCH v1 0/7] Managing the forwarding database(FDB)

[net-next PATCH v1 0/7] Managing the forwarding database(FDB)

[John Fastabend]

The following series is a submission for net-next to allow
embedded switches and other stacked devices other then the
Linux bridge to manage a forwarding database.

This was previously posted here (where it was deferred for
more review and testing)

http://lists.openwall.net/netdev/2012/03/19/26

This series adds macvlan support per discussions with Roopa
and Michael. Also ixgbe was updated to support adding multicast
addresses per request from Greg Rose.

Finally cleanups in the generic dump routines were added
for multicast to support ixgbe and macvlan use cases.

Thanks to everyone for the helpful review and comments. As
always any comments/feedback welcome.

.John

---

Greg Rose (1):
      ixgbe: UTA table incorrectly programmed

John Fastabend (6):
      macvlan: add FDB bridge ops and new macvlan mode
      ixgbe: allow RAR table to be updated in promisc mode
      ixgbe: enable FDB netdevice ops
      net: add fdb generic dump routine
      net: addr_list: add exclusive dev_uc_add and dev_mc_add
      net: add generic PF_BRIDGE:RTM_ FDB hooks


 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  121 ++++++++++----
 drivers/net/macvlan.c                         |   60 ++++++-
 include/linux/if_link.h                       |    1 
 include/linux/neighbour.h                     |    3 
 include/linux/netdevice.h                     |   28 +++
 include/linux/rtnetlink.h                     |    4 
 net/bridge/br_device.c                        |    3 
 net/bridge/br_fdb.c                           |  128 ++++-----------
 net/bridge/br_netlink.c                       |   12 -
 net/bridge/br_private.h                       |   15 +-
 net/core/dev_addr_lists.c                     |   97 ++++++++++--
 net/core/rtnetlink.c                          |  209 +++++++++++++++++++++++++
 12 files changed, 508 insertions(+), 173 deletions(-)

-- 
Signature

[net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

[John Fastabend]

This adds two new flags NTF_MASTER and NTF_SELF that can
now be used to specify where PF_BRIDGE netlink commands should
be sent. NTF_MASTER sends the commands to the 'dev->master'
device for parsing. Typically this will be the linux net/bridge,
or open-vswitch devices. Also without any flags set the command
will be handled by the master device as well so that current user
space tools continue to work as expected.

The NTF_SELF flag will push the PF_BRIDGE commands to the
device. In the basic example below the commands are then parsed
and programmed in the embedded bridge.

Note if both NTF_SELF and NTF_MASTER bits are set then the
command will be sent to both 'dev->master' and 'dev' this allows
user space to easily keep the embedded bridge and software bridge
in sync.

To support this new net device ops were added to call into
the device and the existing bridging code was refactored
to use these. There should be no required changes in user space
to support the current bridge behavior.

A basic setup with a SR-IOV enabled NIC looks like this,

          veth0  veth2
            |      |
          ------------
          |  bridge0 |   <---- software bridging
          ------------
               /
               /
  ethx.y      ethx
    VF         PF
     \         \          <---- propagate FDB entries to HW
     \         \
  --------------------
  |  Embedded Bridge |    <---- hardware offloaded switching
  --------------------

In this case the embedded bridge must be managed to allow 'veth0'
to communicate with 'ethx.y' correctly. At present drivers managing
the embedded bridge either send frames onto the network which
then get dropped by the switch OR the embedded bridge will flood
these frames. With this patch we have a mechanism to manage the
embedded bridge correctly from user space. This example is specific
to SR-IOV but replacing the VF with another PF or dropping this
into the DSA framework generates similar management issues.

Examples session using the 'br'[1] tool to add, dump and then
delete a mac address with a new "embedded" option and enabled
ixgbe driver:

# br fdb add 22:35:19:ac:60:59 dev eth3
# br fdb
port    mac addr                flags
veth0   22:35:19:ac:60:58       static
veth0   9a:5f:81:f7:f6:ec       local
eth3    00:1b:21:55:23:59       local
eth3    22:35:19:ac:60:59       static
veth0   22:35:19:ac:60:57       static
#br fdb add 22:35:19:ac:60:59 embedded dev eth3
#br fdb
port    mac addr                flags
veth0   22:35:19:ac:60:58       static
veth0   9a:5f:81:f7:f6:ec       local
eth3    00:1b:21:55:23:59       local
eth3    22:35:19:ac:60:59       static
veth0   22:35:19:ac:60:57       static
eth3    22:35:19:ac:60:59       local embedded
#br fdb del 22:35:19:ac:60:59 embedded dev eth3

I added a couple lines to 'br' to set the flags correctly is all. It
is my opinion that the merit of this patch is now embedded and SW
bridges can both be modeled correctly in user space using very nearly
the same message passing.

[1] 'br' tool was published as an RFC here and will be renamed 'bridge'
    http://patchwork.ozlabs.org/patch/117664/

Thanks to Jamal Hadi Salim, Stephen Hemminger and Ben Hutchings for
valuable feedback, suggestions, and review.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 include/linux/neighbour.h |    3 +
 include/linux/netdevice.h |   26 ++++++++
 include/linux/rtnetlink.h |    4 +
 net/bridge/br_device.c    |    3 +
 net/bridge/br_fdb.c       |  128 ++++++++++--------------------------------
 net/bridge/br_netlink.c   |   12 ----
 net/bridge/br_private.h   |   15 ++++-
 net/core/rtnetlink.c      |  138 +++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 217 insertions(+), 112 deletions(-)

diff --git a/include/linux/neighbour.h b/include/linux/neighbour.h
index b188f68..275e5d6 100644
--- a/include/linux/neighbour.h
+++ b/include/linux/neighbour.h
@@ -33,6 +33,9 @@ enum {
 #define NTF_PROXY	0x08	/* == ATF_PUBL */
 #define NTF_ROUTER	0x80
 
+#define NTF_SELF	0x02
+#define NTF_MASTER	0x04
+
 /*
  *	Neighbor Cache Entry States.
  */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1f77540..05822e5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -54,6 +54,7 @@
 #include <net/netprio_cgroup.h>
 
 #include <linux/netdev_features.h>
+#include <linux/neighbour.h>
 
 struct netpoll_info;
 struct device;
@@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
  *	feature set might be less than what was returned by ndo_fix_features()).
  *	Must return >0 or -errno if it changed dev->features itself.
  *
+ * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
+ *		      unsigned char *addr, u16 flags)
+ *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
+ *	if the dev->master FDB should be updated or the devices internal FDB.
+ * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
+ *		      unsigned char *addr)
+ *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
+ *	contains flags to indicate if the dev->master FDB should be
+ *	updated or the devices internal FDB.
+ * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
+ *		       struct net_device *dev, int idx)
+ *	Used to add FDB entries to dump requests. Implementers should add
+ *	entries to skb and update idx with the number of entries.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1002,6 +1016,18 @@ struct net_device_ops {
 						    netdev_features_t features);
 	int			(*ndo_neigh_construct)(struct neighbour *n);
 	void			(*ndo_neigh_destroy)(struct neighbour *n);
+
+	int			(*ndo_fdb_add)(struct ndmsg *ndm,
+					       struct net_device *dev,
+					       unsigned char *addr,
+					       u16 flags);
+	int			(*ndo_fdb_del)(struct ndmsg *ndm,
+					       struct net_device *dev,
+					       unsigned char *addr);
+	int			(*ndo_fdb_dump)(struct sk_buff *skb,
+						struct netlink_callback *cb,
+						struct net_device *dev,
+						int idx);
 };
 
 /*
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 577592e..2c1de89 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -801,6 +801,10 @@ rtattr_failure:
 	return table;
 }
 
+extern int ndo_dflt_fdb_dump(struct sk_buff *skb,
+			     struct netlink_callback *cb,
+			     struct net_device *dev,
+			     int idx);
 #endif /* __KERNEL__ */
 
 
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index ba829de..d6e5929 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -317,6 +317,9 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_add_slave		 = br_add_slave,
 	.ndo_del_slave		 = br_del_slave,
 	.ndo_fix_features        = br_fix_features,
+	.ndo_fdb_add		 = br_fdb_add,
+	.ndo_fdb_del		 = br_fdb_delete,
+	.ndo_fdb_dump		 = br_fdb_dump,
 };
 
 static void br_dev_free(struct net_device *dev)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 80dbce4..5945c54 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -535,44 +535,38 @@ errout:
 }
 
 /* Dump information about entries, in response to GETNEIGH */
-int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
+int br_fdb_dump(struct sk_buff *skb,
+		struct netlink_callback *cb,
+		struct net_device *dev,
+		int idx)
 {
-	struct net *net = sock_net(skb->sk);
-	struct net_device *dev;
-	int idx = 0;
-
-	rcu_read_lock();
-	for_each_netdev_rcu(net, dev) {
-		struct net_bridge *br = netdev_priv(dev);
-		int i;
-
-		if (!(dev->priv_flags & IFF_EBRIDGE))
-			continue;
+	struct net_bridge *br = netdev_priv(dev);
+	int i;
 
-		for (i = 0; i < BR_HASH_SIZE; i++) {
-			struct hlist_node *h;
-			struct net_bridge_fdb_entry *f;
+	if (!(dev->priv_flags & IFF_EBRIDGE))
+		goto out;
 
-			hlist_for_each_entry_rcu(f, h, &br->hash[i], hlist) {
-				if (idx < cb->args[0])
-					goto skip;
+	for (i = 0; i < BR_HASH_SIZE; i++) {
+		struct hlist_node *h;
+		struct net_bridge_fdb_entry *f;
 
-				if (fdb_fill_info(skb, br, f,
-						  NETLINK_CB(cb->skb).pid,
-						  cb->nlh->nlmsg_seq,
-						  RTM_NEWNEIGH,
-						  NLM_F_MULTI) < 0)
-					break;
+		hlist_for_each_entry_rcu(f, h, &br->hash[i], hlist) {
+			if (idx < cb->args[0])
+				goto skip;
+
+			if (fdb_fill_info(skb, br, f,
+					  NETLINK_CB(cb->skb).pid,
+					  cb->nlh->nlmsg_seq,
+					  RTM_NEWNEIGH,
+					  NLM_F_MULTI) < 0)
+				break;
 skip:
-				++idx;
-			}
+			++idx;
 		}
 	}
-	rcu_read_unlock();
-
-	cb->args[0] = idx;
 
-	return skb->len;
+out:
+	return idx;
 }
 
 /* Update (create or replace) forwarding database entry */
@@ -614,43 +608,11 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
 }
 
 /* Add new permanent fdb entry with RTM_NEWNEIGH */
-int br_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
+int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
+	       unsigned char *addr, u16 nlh_flags)
 {
-	struct net *net = sock_net(skb->sk);
-	struct ndmsg *ndm;
-	struct nlattr *tb[NDA_MAX+1];
-	struct net_device *dev;
 	struct net_bridge_port *p;
-	const __u8 *addr;
-	int err;
-
-	ASSERT_RTNL();
-	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL);
-	if (err < 0)
-		return err;
-
-	ndm = nlmsg_data(nlh);
-	if (ndm->ndm_ifindex == 0) {
-		pr_info("bridge: RTM_NEWNEIGH with invalid ifindex\n");
-		return -EINVAL;
-	}
-
-	dev = __dev_get_by_index(net, ndm->ndm_ifindex);
-	if (dev == NULL) {
-		pr_info("bridge: RTM_NEWNEIGH with unknown ifindex\n");
-		return -ENODEV;
-	}
-
-	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
-		pr_info("bridge: RTM_NEWNEIGH with invalid address\n");
-		return -EINVAL;
-	}
-
-	addr = nla_data(tb[NDA_LLADDR]);
-	if (!is_valid_ether_addr(addr)) {
-		pr_info("bridge: RTM_NEWNEIGH with invalid ether address\n");
-		return -EINVAL;
-	}
+	int err = 0;
 
 	if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_NOARP|NUD_REACHABLE))) {
 		pr_info("bridge: RTM_NEWNEIGH with invalid state %#x\n", ndm->ndm_state);
@@ -670,14 +632,14 @@ int br_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 		rcu_read_unlock();
 	} else {
 		spin_lock_bh(&p->br->hash_lock);
-		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh->nlmsg_flags);
+		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
 		spin_unlock_bh(&p->br->hash_lock);
 	}
 
 	return err;
 }
 
-static int fdb_delete_by_addr(struct net_bridge_port *p, const u8 *addr)
+static int fdb_delete_by_addr(struct net_bridge_port *p, u8 *addr)
 {
 	struct net_bridge *br = p->br;
 	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
@@ -692,40 +654,12 @@ static int fdb_delete_by_addr(struct net_bridge_port *p, const u8 *addr)
 }
 
 /* Remove neighbor entry with RTM_DELNEIGH */
-int br_fdb_delete(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
+int br_fdb_delete(struct ndmsg *ndm, struct net_device *dev,
+		  unsigned char *addr)
 {
-	struct net *net = sock_net(skb->sk);
-	struct ndmsg *ndm;
 	struct net_bridge_port *p;
-	struct nlattr *llattr;
-	const __u8 *addr;
-	struct net_device *dev;
 	int err;
 
-	ASSERT_RTNL();
-	if (nlmsg_len(nlh) < sizeof(*ndm))
-		return -EINVAL;
-
-	ndm = nlmsg_data(nlh);
-	if (ndm->ndm_ifindex == 0) {
-		pr_info("bridge: RTM_DELNEIGH with invalid ifindex\n");
-		return -EINVAL;
-	}
-
-	dev = __dev_get_by_index(net, ndm->ndm_ifindex);
-	if (dev == NULL) {
-		pr_info("bridge: RTM_DELNEIGH with unknown ifindex\n");
-		return -ENODEV;
-	}
-
-	llattr = nlmsg_find_attr(nlh, sizeof(*ndm), NDA_LLADDR);
-	if (llattr == NULL || nla_len(llattr) != ETH_ALEN) {
-		pr_info("bridge: RTM_DELNEIGH with invalid address\n");
-		return -EINVAL;
-	}
-
-	addr = nla_data(llattr);
-
 	p = br_port_get_rtnl(dev);
 	if (p == NULL) {
 		pr_info("bridge: RTM_DELNEIGH %s not a bridge port\n",
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 346b368..1fa0535 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -232,18 +232,6 @@ int __init br_netlink_init(void)
 			      br_rtm_setlink, NULL, NULL);
 	if (err)
 		goto err3;
-	err = __rtnl_register(PF_BRIDGE, RTM_NEWNEIGH,
-			      br_fdb_add, NULL, NULL);
-	if (err)
-		goto err3;
-	err = __rtnl_register(PF_BRIDGE, RTM_DELNEIGH,
-			      br_fdb_delete, NULL, NULL);
-	if (err)
-		goto err3;
-	err = __rtnl_register(PF_BRIDGE, RTM_GETNEIGH,
-			      NULL, br_fdb_dump, NULL);
-	if (err)
-		goto err3;
 
 	return 0;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 0b67a63..929b9f6 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -363,9 +363,18 @@ extern int br_fdb_insert(struct net_bridge *br,
 extern void br_fdb_update(struct net_bridge *br,
 			  struct net_bridge_port *source,
 			  const unsigned char *addr);
-extern int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb);
-extern int br_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg);
-extern int br_fdb_delete(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg);
+
+extern int br_fdb_delete(struct ndmsg *ndm,
+			 struct net_device *dev,
+			 unsigned char *addr);
+extern int br_fdb_add(struct ndmsg *nlh,
+		      struct net_device *dev,
+		      unsigned char *addr,
+		      u16 nlh_flags);
+extern int br_fdb_dump(struct sk_buff *skb,
+		       struct netlink_callback *cb,
+		       struct net_device *dev,
+		       int idx);
 
 /* br_forward.c */
 extern void br_deliver(const struct net_bridge_port *to,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index b76f8fa..d6ce728 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -35,7 +35,9 @@
 #include <linux/security.h>
 #include <linux/mutex.h>
 #include <linux/if_addr.h>
+#include <linux/if_bridge.h>
 #include <linux/pci.h>
+#include <linux/etherdevice.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -1979,6 +1981,138 @@ errout:
 		rtnl_set_sk_err(net, RTNLGRP_LINK, err);
 }
 
+static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
+{
+	struct net *net = sock_net(skb->sk);
+	struct ndmsg *ndm;
+	struct nlattr *tb[NDA_MAX+1];
+	struct net_device *dev;
+	u8 *addr;
+	int err;
+
+	err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL);
+	if (err < 0)
+		return err;
+
+	ndm = nlmsg_data(nlh);
+	if (ndm->ndm_ifindex == 0) {
+		pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid ifindex\n");
+		return -EINVAL;
+	}
+
+	dev = __dev_get_by_index(net, ndm->ndm_ifindex);
+	if (dev == NULL) {
+		pr_info("PF_BRIDGE: RTM_NEWNEIGH with unknown ifindex\n");
+		return -ENODEV;
+	}
+
+	if (!tb[NDA_LLADDR] || nla_len(tb[NDA_LLADDR]) != ETH_ALEN) {
+		pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid address\n");
+		return -EINVAL;
+	}
+
+	addr = nla_data(tb[NDA_LLADDR]);
+	if (!is_valid_ether_addr(addr)) {
+		pr_info("PF_BRIDGE: RTM_NEWNEIGH with invalid ether address\n");
+		return -EINVAL;
+	}
+
+	err = -EOPNOTSUPP;
+
+	/* Support fdb on master device the net/bridge default case */
+	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
+	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
+		struct net_device *master = dev->master;
+
+		if (master->netdev_ops->ndo_fdb_add)
+			err = master->netdev_ops->ndo_fdb_add(ndm, dev, addr,
+							      nlh->nlmsg_flags);
+	}
+
+	/* Embedded bridge, macvlan, and any other device support */
+	if ((ndm->ndm_flags & NTF_SELF) &&
+	    dev->netdev_ops->ndo_fdb_add)
+		err = dev->netdev_ops->ndo_fdb_add(ndm, dev, addr,
+						   nlh->nlmsg_flags);
+
+	return err;
+}
+
+static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
+{
+	struct net *net = sock_net(skb->sk);
+	struct ndmsg *ndm;
+	struct nlattr *llattr;
+	struct net_device *dev;
+	int err = -EINVAL;
+	__u8 *addr;
+
+	if (nlmsg_len(nlh) < sizeof(*ndm))
+		return -EINVAL;
+
+	ndm = nlmsg_data(nlh);
+	if (ndm->ndm_ifindex == 0) {
+		pr_info("PF_BRIDGE: RTM_DELNEIGH with invalid ifindex\n");
+		return -EINVAL;
+	}
+
+	dev = __dev_get_by_index(net, ndm->ndm_ifindex);
+	if (dev == NULL) {
+		pr_info("PF_BRIDGE: RTM_DELNEIGH with unknown ifindex\n");
+		return -ENODEV;
+	}
+
+	llattr = nlmsg_find_attr(nlh, sizeof(*ndm), NDA_LLADDR);
+	if (llattr == NULL || nla_len(llattr) != ETH_ALEN) {
+		pr_info("PF_BRIGDE: RTM_DELNEIGH with invalid address\n");
+		return -EINVAL;
+	}
+
+	addr = nla_data(llattr);
+	err = -EOPNOTSUPP;
+
+	/* Support fdb on master device the net/bridge default case */
+	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
+	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
+		struct net_device *master = dev->master;
+
+		if (master->netdev_ops->ndo_fdb_del)
+			err = master->netdev_ops->ndo_fdb_del(ndm, dev, addr);
+	}
+
+	/* Embedded bridge, macvlan, and any other device support */
+	if ((ndm->ndm_flags & NTF_SELF) &&
+	    dev->netdev_ops->ndo_fdb_del)
+		err = dev->netdev_ops->ndo_fdb_del(ndm, dev, addr);
+
+	return err;
+}
+
+static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	int idx = 0;
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
+
+	rcu_read_lock();
+	for_each_netdev_rcu(net, dev) {
+		if (dev->priv_flags & IFF_BRIDGE_PORT) {
+			struct net_device *master = dev->master;
+			const struct net_device_ops *ops = master->netdev_ops;
+
+			if (ops->ndo_fdb_dump)
+				idx = ops->ndo_fdb_dump(skb, cb, dev, idx);
+		}
+
+		if (dev->netdev_ops->ndo_fdb_dump)
+			idx = dev->netdev_ops->ndo_fdb_dump(skb, cb, dev, idx);
+	}
+	rcu_read_unlock();
+
+	cb->args[0] = idx;
+	return skb->len;
+}
+
 /* Protected by RTNL sempahore.  */
 static struct rtattr **rta_buf;
 static int rtattr_max;
@@ -2151,5 +2285,9 @@ void __init rtnetlink_init(void)
 
 	rtnl_register(PF_UNSPEC, RTM_GETADDR, NULL, rtnl_dump_all, NULL);
 	rtnl_register(PF_UNSPEC, RTM_GETROUTE, NULL, rtnl_dump_all, NULL);
+
+	rtnl_register(PF_BRIDGE, RTM_NEWNEIGH, rtnl_fdb_add, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_DELNEIGH, rtnl_fdb_del, NULL, NULL);
+	rtnl_register(PF_BRIDGE, RTM_GETNEIGH, NULL, rtnl_fdb_dump, NULL);
 }
 

[net-next PATCH v1 2/7] net: addr_list: add exclusive dev_uc_add and dev_mc_add

[John Fastabend]

This adds a dev_uc_add_excl() and dev_mc_add_excl() calls
similar to the original dev_{uc|mc}_add() except it sets
the global bit and returns -EEXIST for duplicat entires.

This is useful for drivers that support SR-IOV, macvlan
devices and any other devices that need to manage the
unicast and multicast lists.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 include/linux/netdevice.h |    2 +
 net/core/dev_addr_lists.c |   97 ++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 83 insertions(+), 16 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 05822e5..b68a326 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2572,6 +2572,7 @@ extern int dev_addr_init(struct net_device *dev);
 
 /* Functions used for unicast addresses handling */
 extern int dev_uc_add(struct net_device *dev, unsigned char *addr);
+extern int dev_uc_add_excl(struct net_device *dev, unsigned char *addr);
 extern int dev_uc_del(struct net_device *dev, unsigned char *addr);
 extern int dev_uc_sync(struct net_device *to, struct net_device *from);
 extern void dev_uc_unsync(struct net_device *to, struct net_device *from);
@@ -2581,6 +2582,7 @@ extern void dev_uc_init(struct net_device *dev);
 /* Functions used for multicast addresses handling */
 extern int dev_mc_add(struct net_device *dev, unsigned char *addr);
 extern int dev_mc_add_global(struct net_device *dev, unsigned char *addr);
+extern int dev_mc_add_excl(struct net_device *dev, unsigned char *addr);
 extern int dev_mc_del(struct net_device *dev, unsigned char *addr);
 extern int dev_mc_del_global(struct net_device *dev, unsigned char *addr);
 extern int dev_mc_sync(struct net_device *to, struct net_device *from);
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 29c07fe..5405e28 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -21,12 +21,35 @@
  * General list handling functions
  */
 
+static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
+			       unsigned char *addr, int addr_len,
+			       unsigned char addr_type, bool global)
+{
+	struct netdev_hw_addr *ha;
+	int alloc_size;
+
+	alloc_size = sizeof(*ha);
+	if (alloc_size < L1_CACHE_BYTES)
+		alloc_size = L1_CACHE_BYTES;
+	ha = kmalloc(alloc_size, GFP_ATOMIC);
+	if (!ha)
+		return -ENOMEM;
+	memcpy(ha->addr, addr, addr_len);
+	ha->type = addr_type;
+	ha->refcount = 1;
+	ha->global_use = global;
+	ha->synced = false;
+	list_add_tail_rcu(&ha->list, &list->list);
+	list->count++;
+
+	return 0;
+}
+
 static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 			    unsigned char *addr, int addr_len,
 			    unsigned char addr_type, bool global)
 {
 	struct netdev_hw_addr *ha;
-	int alloc_size;
 
 	if (addr_len > MAX_ADDR_LEN)
 		return -EINVAL;
@@ -46,21 +69,7 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 		}
 	}
 
-
-	alloc_size = sizeof(*ha);
-	if (alloc_size < L1_CACHE_BYTES)
-		alloc_size = L1_CACHE_BYTES;
-	ha = kmalloc(alloc_size, GFP_ATOMIC);
-	if (!ha)
-		return -ENOMEM;
-	memcpy(ha->addr, addr, addr_len);
-	ha->type = addr_type;
-	ha->refcount = 1;
-	ha->global_use = global;
-	ha->synced = false;
-	list_add_tail_rcu(&ha->list, &list->list);
-	list->count++;
-	return 0;
+	return __hw_addr_create_ex(list, addr, addr_len, addr_type, global);
 }
 
 static int __hw_addr_add(struct netdev_hw_addr_list *list, unsigned char *addr,
@@ -377,6 +386,34 @@ EXPORT_SYMBOL(dev_addr_del_multiple);
  */
 
 /**
+ *	dev_uc_add_excl - Add a global secondary unicast address
+ *	@dev: device
+ *	@addr: address to add
+ */
+int dev_uc_add_excl(struct net_device *dev, unsigned char *addr)
+{
+	struct netdev_hw_addr *ha;
+	int err;
+
+	netif_addr_lock_bh(dev);
+	list_for_each_entry(ha, &dev->uc.list, list) {
+		if (!memcmp(ha->addr, addr, dev->addr_len) &&
+		    ha->type == NETDEV_HW_ADDR_T_UNICAST) {
+			err = -EEXIST;
+			goto out;
+		}
+	}
+	err = __hw_addr_create_ex(&dev->uc, addr, dev->addr_len,
+				  NETDEV_HW_ADDR_T_UNICAST, true);
+	if (!err)
+		__dev_set_rx_mode(dev);
+out:
+	netif_addr_unlock_bh(dev);
+	return err;
+}
+EXPORT_SYMBOL(dev_uc_add_excl);
+
+/**
  *	dev_uc_add - Add a secondary unicast address
  *	@dev: device
  *	@addr: address to add
@@ -501,6 +538,34 @@ EXPORT_SYMBOL(dev_uc_init);
  * Multicast list handling functions
  */
 
+/**
+ *	dev_mc_add_excl - Add a global secondary multicast address
+ *	@dev: device
+ *	@addr: address to add
+ */
+int dev_mc_add_excl(struct net_device *dev, unsigned char *addr)
+{
+	struct netdev_hw_addr *ha;
+	int err;
+
+	netif_addr_lock_bh(dev);
+	list_for_each_entry(ha, &dev->mc.list, list) {
+		if (!memcmp(ha->addr, addr, dev->addr_len) &&
+		    ha->type == NETDEV_HW_ADDR_T_UNICAST) {
+			err = -EEXIST;
+			goto out;
+		}
+	}
+	err = __hw_addr_create_ex(&dev->mc, addr, dev->addr_len,
+				  NETDEV_HW_ADDR_T_UNICAST, true);
+	if (!err)
+		__dev_set_rx_mode(dev);
+out:
+	netif_addr_unlock_bh(dev);
+	return err;
+}
+EXPORT_SYMBOL(dev_mc_add_excl);
+
 static int __dev_mc_add(struct net_device *dev, unsigned char *addr,
 			bool global)
 {

[net-next PATCH v1 3/7] net: add fdb generic dump routine

[John Fastabend]

This adds a generic dump routine drivers can call. It
should be sufficient to handle any bridging model that
uses the unicast address list. This should be most SR-IOV
enabled NICs.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/rtnetlink.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d6ce728..2bb4f59 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2088,6 +2088,77 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 	return err;
 }
 
+static int nlmsg_populate_fdb(struct sk_buff *skb,
+			      struct netlink_callback *cb,
+			      struct net_device *dev,
+			      int *idx,
+			      struct netdev_hw_addr_list *list)
+{
+	struct netdev_hw_addr *ha;
+	struct ndmsg *ndm;
+	struct nlmsghdr *nlh;
+	u32 pid, seq;
+
+	pid = NETLINK_CB(cb->skb).pid;
+	seq = cb->nlh->nlmsg_seq;
+
+	list_for_each_entry(ha, &list->list, list) {
+		if (*idx < cb->args[0])
+			goto skip;
+
+		nlh = nlmsg_put(skb, pid, seq,
+				RTM_NEWNEIGH, sizeof(*ndm), NLM_F_MULTI);
+		if (!nlh)
+			break;
+
+		ndm = nlmsg_data(nlh);
+		ndm->ndm_family  = AF_BRIDGE;
+		ndm->ndm_pad1	 = 0;
+		ndm->ndm_pad2    = 0;
+		ndm->ndm_flags	 = NTF_SELF;
+		ndm->ndm_type	 = 0;
+		ndm->ndm_ifindex = dev->ifindex;
+		ndm->ndm_state   = NUD_PERMANENT;
+
+		if (nla_put(skb, NDA_LLADDR, ETH_ALEN, ha->addr))
+			goto nla_put_failure;
+
+		nlmsg_end(skb, nlh);
+skip:
+		*idx += 1;
+	}
+	return 0;
+nla_put_failure:
+	nlmsg_cancel(skb, nlh);
+	return -ENOMEM;
+}
+
+/**
+ * ndo_dflt_fdb_dump: default netdevice operation to dump an FDB table.
+ * @nlh: netlink message header
+ * @dev: netdevice
+ *
+ * Default netdevice operation to dump the existing unicast address list.
+ * Returns zero on success.
+ */
+int ndo_dflt_fdb_dump(struct sk_buff *skb,
+		      struct netlink_callback *cb,
+		      struct net_device *dev,
+		      int idx)
+{
+	int err;
+
+	netif_addr_lock_bh(dev);
+	err = nlmsg_populate_fdb(skb, cb, dev, &idx, &dev->uc);
+	if (err)
+		goto out;
+	nlmsg_populate_fdb(skb, cb, dev, &idx, &dev->mc);
+out:
+	netif_addr_unlock_bh(dev);
+	return idx;
+}
+EXPORT_SYMBOL(ndo_dflt_fdb_dump);
+
 static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	int idx = 0;

[net-next PATCH v1 4/7] ixgbe: enable FDB netdevice ops

[John Fastabend]

Enable FDB ops on ixgbe when in SR-IOV mode.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   71 +++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 3e26b1f..8b37395 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6681,6 +6681,74 @@ static int ixgbe_set_features(struct net_device *netdev,
 	return 0;
 }
 
+static int ixgbe_ndo_fdb_add(struct ndmsg *ndm,
+			     struct net_device *dev,
+			     unsigned char *addr,
+			     u16 flags)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(dev);
+	int err = -EOPNOTSUPP;
+
+	if (ndm->ndm_state & NUD_PERMANENT) {
+		pr_info("%s: FDB only supports static addresses\n",
+			ixgbe_driver_name);
+		return -EINVAL;
+	}
+
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
+		if (is_unicast_ether_addr(addr))
+			err = dev_uc_add_excl(dev, addr);
+		else if (is_multicast_ether_addr(addr))
+			err = dev_mc_add_excl(dev, addr);
+		else
+			err = -EINVAL;
+	}
+
+	/* Only return duplicate errors if NLM_F_EXCL is set */
+	if (err == -EEXIST && !(flags & NLM_F_EXCL))
+		err = 0;
+
+	return err;
+}
+
+static int ixgbe_ndo_fdb_del(struct ndmsg *ndm,
+			     struct net_device *dev,
+			     unsigned char *addr)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(dev);
+	int err = -EOPNOTSUPP;
+
+	if (ndm->ndm_state & NUD_PERMANENT) {
+		pr_info("%s: FDB only supports static addresses\n",
+			ixgbe_driver_name);
+		return -EINVAL;
+	}
+
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
+		if (is_unicast_ether_addr(addr))
+			err = dev_uc_del(dev, addr);
+		else if (is_multicast_ether_addr(addr))
+			err = dev_mc_del(dev, addr);
+		else
+			err = -EINVAL;
+	}
+
+	return err;
+}
+
+static int ixgbe_ndo_fdb_dump(struct sk_buff *skb,
+			      struct netlink_callback *cb,
+			      struct net_device *dev,
+			      int idx)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(dev);
+
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
+		idx = ndo_dflt_fdb_dump(skb, cb, dev, idx);
+
+	return idx;
+}
+
 static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_open		= ixgbe_open,
 	.ndo_stop		= ixgbe_close,
@@ -6717,6 +6785,9 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 #endif /* IXGBE_FCOE */
 	.ndo_set_features = ixgbe_set_features,
 	.ndo_fix_features = ixgbe_fix_features,
+	.ndo_fdb_add		= ixgbe_ndo_fdb_add,
+	.ndo_fdb_del		= ixgbe_ndo_fdb_del,
+	.ndo_fdb_dump		= ixgbe_ndo_fdb_dump,
 };
 
 static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,

[net-next PATCH v1 5/7] ixgbe: allow RAR table to be updated in promisc mode

[John Fastabend]

This allows RAR table updates while in promiscuous. With
SR-IOV enabled it is valuable to allow the RAR table to
be updated even when in promisc mode to configure forwarding

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8b37395..25a7ed9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3462,16 +3462,17 @@ void ixgbe_set_rx_mode(struct net_device *netdev)
 		}
 		ixgbe_vlan_filter_enable(adapter);
 		hw->addr_ctrl.user_set_promisc = false;
-		/*
-		 * Write addresses to available RAR registers, if there is not
-		 * sufficient space to store all the addresses then enable
-		 * unicast promiscuous mode
-		 */
-		count = ixgbe_write_uc_addr_list(netdev);
-		if (count < 0) {
-			fctrl |= IXGBE_FCTRL_UPE;
-			vmolr |= IXGBE_VMOLR_ROPE;
-		}
+	}
+
+	/*
+	 * Write addresses to available RAR registers, if there is not
+	 * sufficient space to store all the addresses then enable
+	 * unicast promiscuous mode
+	 */
+	count = ixgbe_write_uc_addr_list(netdev);
+	if (count < 0) {
+		fctrl |= IXGBE_FCTRL_UPE;
+		vmolr |= IXGBE_VMOLR_ROPE;
 	}
 
 	if (adapter->num_vfs) {

[net-next PATCH v1 6/7] ixgbe: UTA table incorrectly programmed

[John Fastabend]

From: Greg Rose <gregory.v.rose@intel.com>

The UTA table was being set to the functional equivalent of promiscuous
mode.  This was resulting in traffic from the virtual function being
flooded onto the wire and the PF device. This resulted in additional
overhead for VF traffic sent to the network and in the case of traffic
sent to the PF or another VF resulted in unwanted packets on the wire.

This was actually not the intended behavior. Now that we can program
the embedded switch correctly we can remove this snippit of code. Users
who want to support this should configure the FDB correctly using the
FDB ops.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   29 -------------------------
 1 files changed, 0 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 25a7ed9..10606bd 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2904,33 +2904,6 @@ static void ixgbe_configure_rscctl(struct ixgbe_adapter *adapter,
 	IXGBE_WRITE_REG(hw, IXGBE_RSCCTL(reg_idx), rscctrl);
 }
 
-/**
- *  ixgbe_set_uta - Set unicast filter table address
- *  @adapter: board private structure
- *
- *  The unicast table address is a register array of 32-bit registers.
- *  The table is meant to be used in a way similar to how the MTA is used
- *  however due to certain limitations in the hardware it is necessary to
- *  set all the hash bits to 1 and use the VMOLR ROPE bit as a promiscuous
- *  enable bit to allow vlan tag stripping when promiscuous mode is enabled
- **/
-static void ixgbe_set_uta(struct ixgbe_adapter *adapter)
-{
-	struct ixgbe_hw *hw = &adapter->hw;
-	int i;
-
-	/* The UTA table only exists on 82599 hardware and newer */
-	if (hw->mac.type < ixgbe_mac_82599EB)
-		return;
-
-	/* we only need to do this if VMDq is enabled */
-	if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
-		return;
-
-	for (i = 0; i < 128; i++)
-		IXGBE_WRITE_REG(hw, IXGBE_UTA(i), ~0);
-}
-
 #define IXGBE_MAX_RX_DESC_POLL 10
 static void ixgbe_rx_desc_queue_enable(struct ixgbe_adapter *adapter,
 				       struct ixgbe_ring *ring)
@@ -3224,8 +3197,6 @@ static void ixgbe_configure_rx(struct ixgbe_adapter *adapter)
 	/* Program registers for the distribution of queues */
 	ixgbe_setup_mrqc(adapter);
 
-	ixgbe_set_uta(adapter);
-
 	/* set_rx_buffer_len must be called before ring initialization */
 	ixgbe_set_rx_buffer_len(adapter);
 

[net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
this mode acts the same as the original passthru mode _except_
it does not set promiscuous mode on the lowerdev. Because the
lowerdev is not put in promiscuous mode any unicast or multicast
addresses the device should receive must be explicitely added
with the FDB bridge ops. In many use cases the management stack
will know the mac addresses needed (maybe negotiated via EVB/VDP)
or may require only receiving known "good" mac addresses. This
mode with the FDB ops supports this usage model.

This patch is a result of Roopa Prabhu's work. Follow up
patches are needed for VEPA and VEB macvlan modes.

CC: Roopa Prabhu <roprabhu@cisco.com>
CC: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 drivers/net/macvlan.c   |   60 ++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/if_link.h |    1 +
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index b17fc90..9892d8d 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -181,6 +181,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 					  MACVLAN_MODE_PRIVATE |
 					  MACVLAN_MODE_VEPA    |
 					  MACVLAN_MODE_PASSTHRU|
+					  MACVLAN_MODE_PASSTHRU_NOPROMISC |
 					  MACVLAN_MODE_BRIDGE);
 		else if (src->mode == MACVLAN_MODE_VEPA)
 			/* flood to everyone except source */
@@ -312,7 +313,8 @@ static int macvlan_open(struct net_device *dev)
 	int err;
 
 	if (vlan->port->passthru) {
-		dev_set_promiscuity(lowerdev, 1);
+		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
+			dev_set_promiscuity(lowerdev, 1);
 		goto hash_add;
 	}
 
@@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
 	struct macvlan_dev *vlan = netdev_priv(dev);
 	struct net_device *lowerdev = vlan->lowerdev;
 
+	dev_uc_unsync(lowerdev, dev);
+	dev_mc_unsync(lowerdev, dev);
+
 	if (vlan->port->passthru) {
-		dev_set_promiscuity(lowerdev, -1);
+		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
+			dev_set_promiscuity(lowerdev, 1);
 		goto hash_del;
 	}
 
-	dev_mc_unsync(lowerdev, dev);
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(lowerdev, -1);
 
@@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
 		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
 }
 
-static void macvlan_set_multicast_list(struct net_device *dev)
+static void macvlan_set_mac_lists(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 
+	dev_uc_sync(vlan->lowerdev, dev);
 	dev_mc_sync(vlan->lowerdev, dev);
 }
 
@@ -542,6 +548,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev,
 	return 0;
 }
 
+static int macvlan_fdb_add(struct ndmsg *ndm,
+			   struct net_device *dev,
+			   unsigned char *addr,
+			   u16 flags)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	int err = -EINVAL;
+
+	if (!vlan->port->passthru)
+		return -EOPNOTSUPP;
+
+	if (is_unicast_ether_addr(addr))
+		err = dev_uc_add_excl(dev, addr);
+	else if (is_multicast_ether_addr(addr))
+		err = dev_mc_add_excl(dev, addr);
+
+	return err;
+}
+
+static int macvlan_fdb_del(struct ndmsg *ndm,
+			   struct net_device *dev,
+			   unsigned char *addr)
+{
+	struct macvlan_dev *vlan = netdev_priv(dev);
+	int err = -EINVAL;
+
+	if (!vlan->port->passthru)
+		return -EOPNOTSUPP;
+
+	if (is_unicast_ether_addr(addr))
+		err = dev_uc_del(dev, addr);
+	else if (is_multicast_ether_addr(addr))
+		err = dev_mc_del(dev, addr);
+
+	return err;
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
 					struct ethtool_drvinfo *drvinfo)
 {
@@ -572,11 +615,14 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_change_mtu		= macvlan_change_mtu,
 	.ndo_change_rx_flags	= macvlan_change_rx_flags,
 	.ndo_set_mac_address	= macvlan_set_mac_address,
-	.ndo_set_rx_mode	= macvlan_set_multicast_list,
+	.ndo_set_rx_mode	= macvlan_set_mac_lists,
 	.ndo_get_stats64	= macvlan_dev_get_stats64,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_vlan_rx_add_vid	= macvlan_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= macvlan_vlan_rx_kill_vid,
+	.ndo_fdb_add		= macvlan_fdb_add,
+	.ndo_fdb_del		= macvlan_fdb_del,
+	.ndo_fdb_dump		= ndo_dflt_fdb_dump,
 };
 
 void macvlan_common_setup(struct net_device *dev)
@@ -648,6 +694,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
 		case MACVLAN_MODE_VEPA:
 		case MACVLAN_MODE_BRIDGE:
 		case MACVLAN_MODE_PASSTHRU:
+		case MACVLAN_MODE_PASSTHRU_NOPROMISC:
 			break;
 		default:
 			return -EINVAL;
@@ -711,7 +758,8 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_MODE])
 		vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
 
-	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
+	if ((vlan->mode == MACVLAN_MODE_PASSTHRU) ||
+	    (vlan->mode == MACVLAN_MODE_PASSTHRU_NOPROMISC)) {
 		if (port->count)
 			return -EINVAL;
 		port->passthru = true;
diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 2f4fa93..db67b9d 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -265,6 +265,7 @@ enum macvlan_mode {
 	MACVLAN_MODE_VEPA    = 2, /* talk to other ports through ext bridge */
 	MACVLAN_MODE_BRIDGE  = 4, /* talk to bridge ports directly */
 	MACVLAN_MODE_PASSTHRU = 8,/* take over the underlying device */
+	MACVLAN_MODE_PASSTHRU_NOPROMISC = 16, /* passthru without promisc */
 };
 
 /* SR-IOV virtual function management section */

Re: [net-next PATCH v1 0/7] Managing the forwarding database(FDB)

[Stephen Hemminger]

On Mon, 09 Apr 2012 15:00:11 -0700
John Fastabend <john.r.fastabend@intel.com> wrote:

> The following series is a submission for net-next to allow
> embedded switches and other stacked devices other then the
> Linux bridge to manage a forwarding database.
> 
> This was previously posted here (where it was deferred for
> more review and testing)
> 
> http://lists.openwall.net/netdev/2012/03/19/26
> 
> This series adds macvlan support per discussions with Roopa
> and Michael. Also ixgbe was updated to support adding multicast
> addresses per request from Greg Rose.
> 
> Finally cleanups in the generic dump routines were added
> for multicast to support ixgbe and macvlan use cases.
> 
> Thanks to everyone for the helpful review and comments. As
> always any comments/feedback welcome.
> 
> .John
> 
> ---
> 
> Greg Rose (1):
>       ixgbe: UTA table incorrectly programmed
> 
> John Fastabend (6):
>       macvlan: add FDB bridge ops and new macvlan mode
>       ixgbe: allow RAR table to be updated in promisc mode
>       ixgbe: enable FDB netdevice ops
>       net: add fdb generic dump routine
>       net: addr_list: add exclusive dev_uc_add and dev_mc_add
>       net: add generic PF_BRIDGE:RTM_ FDB hooks
> 
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  121 ++++++++++----
>  drivers/net/macvlan.c                         |   60 ++++++-
>  include/linux/if_link.h                       |    1 
>  include/linux/neighbour.h                     |    3 
>  include/linux/netdevice.h                     |   28 +++
>  include/linux/rtnetlink.h                     |    4 
>  net/bridge/br_device.c                        |    3 
>  net/bridge/br_fdb.c                           |  128 ++++-----------
>  net/bridge/br_netlink.c                       |   12 -
>  net/bridge/br_private.h                       |   15 +-
>  net/core/dev_addr_lists.c                     |   97 ++++++++++--
>  net/core/rtnetlink.c                          |  209 +++++++++++++++++++++++++
>  12 files changed, 508 insertions(+), 173 deletions(-)
> 

How do statistics work in this case? What if you wanted to do BRIDGE MIB?

Re: [net-next PATCH v1 0/7] Managing the forwarding database(FDB)

[John Fastabend]

On 4/9/2012 3:15 PM, Stephen Hemminger wrote:
> On Mon, 09 Apr 2012 15:00:11 -0700
> John Fastabend <john.r.fastabend@intel.com> wrote:
> 
>> The following series is a submission for net-next to allow
>> embedded switches and other stacked devices other then the
>> Linux bridge to manage a forwarding database.
>>
>> This was previously posted here (where it was deferred for
>> more review and testing)
>>
>> http://lists.openwall.net/netdev/2012/03/19/26
>>
>> This series adds macvlan support per discussions with Roopa
>> and Michael. Also ixgbe was updated to support adding multicast
>> addresses per request from Greg Rose.
>>
>> Finally cleanups in the generic dump routines were added
>> for multicast to support ixgbe and macvlan use cases.
>>
>> Thanks to everyone for the helpful review and comments. As
>> always any comments/feedback welcome.
>>
>> .John
>>
>> ---
>>
>> Greg Rose (1):
>>       ixgbe: UTA table incorrectly programmed
>>
>> John Fastabend (6):
>>       macvlan: add FDB bridge ops and new macvlan mode
>>       ixgbe: allow RAR table to be updated in promisc mode
>>       ixgbe: enable FDB netdevice ops
>>       net: add fdb generic dump routine
>>       net: addr_list: add exclusive dev_uc_add and dev_mc_add
>>       net: add generic PF_BRIDGE:RTM_ FDB hooks
>>
>>
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  121 ++++++++++----
>>  drivers/net/macvlan.c                         |   60 ++++++-
>>  include/linux/if_link.h                       |    1 
>>  include/linux/neighbour.h                     |    3 
>>  include/linux/netdevice.h                     |   28 +++
>>  include/linux/rtnetlink.h                     |    4 
>>  net/bridge/br_device.c                        |    3 
>>  net/bridge/br_fdb.c                           |  128 ++++-----------
>>  net/bridge/br_netlink.c                       |   12 -
>>  net/bridge/br_private.h                       |   15 +-
>>  net/core/dev_addr_lists.c                     |   97 ++++++++++--
>>  net/core/rtnetlink.c                          |  209 +++++++++++++++++++++++++
>>  12 files changed, 508 insertions(+), 173 deletions(-)
>>
> 
> How do statistics work in this case? What if you wanted to do BRIDGE MIB?

I expect we will need to do the same sort of thing for PF_BRIDGE:RTM_GETLINK
to dump the statistics for an embedded or stacked bridge. This series only
gets the forwarding database setup. All the other entries in the MIB still
need to be populated. What I would like is to be able to write a BRIDGE MIB
implementation and have it work with any of the bridging components in the
linux kernel. But I don't think that should block this series.

Make sense?

Based on a quick scan I'm not sure the existing net/bridge code has one place
to pull all the stats from anyways looks like you might have to read some sysfs
entries AND do some netlink commands. I would have to check that though. It is
on my todo list to create a bridge mib for some of the EVB cases. Of course if
someone else got to it that would be great.

.John

Re: [net-next PATCH v1 2/7] net: addr_list: add exclusive dev_uc_add and dev_mc_add

[Michael S. Tsirkin]

On Mon, Apr 09, 2012 at 03:00:23PM -0700, John Fastabend wrote:
> This adds a dev_uc_add_excl() and dev_mc_add_excl() calls
> similar to the original dev_{uc|mc}_add() except it sets
> the global bit and returns -EEXIST for duplicat entires.
> 
> This is useful for drivers that support SR-IOV, macvlan
> devices and any other devices that need to manage the
> unicast and multicast lists.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  include/linux/netdevice.h |    2 +
>  net/core/dev_addr_lists.c |   97 ++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 83 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 05822e5..b68a326 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2572,6 +2572,7 @@ extern int dev_addr_init(struct net_device *dev);
>  
>  /* Functions used for unicast addresses handling */
>  extern int dev_uc_add(struct net_device *dev, unsigned char *addr);
> +extern int dev_uc_add_excl(struct net_device *dev, unsigned char *addr);
>  extern int dev_uc_del(struct net_device *dev, unsigned char *addr);
>  extern int dev_uc_sync(struct net_device *to, struct net_device *from);
>  extern void dev_uc_unsync(struct net_device *to, struct net_device *from);
> @@ -2581,6 +2582,7 @@ extern void dev_uc_init(struct net_device *dev);
>  /* Functions used for multicast addresses handling */
>  extern int dev_mc_add(struct net_device *dev, unsigned char *addr);
>  extern int dev_mc_add_global(struct net_device *dev, unsigned char *addr);
> +extern int dev_mc_add_excl(struct net_device *dev, unsigned char *addr);
>  extern int dev_mc_del(struct net_device *dev, unsigned char *addr);
>  extern int dev_mc_del_global(struct net_device *dev, unsigned char *addr);
>  extern int dev_mc_sync(struct net_device *to, struct net_device *from);
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 29c07fe..5405e28 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -21,12 +21,35 @@
>   * General list handling functions
>   */
>  
> +static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
> +			       unsigned char *addr, int addr_len,
> +			       unsigned char addr_type, bool global)
> +{
> +	struct netdev_hw_addr *ha;
> +	int alloc_size;
> +
> +	alloc_size = sizeof(*ha);
> +	if (alloc_size < L1_CACHE_BYTES)
> +		alloc_size = L1_CACHE_BYTES;
> +	ha = kmalloc(alloc_size, GFP_ATOMIC);
> +	if (!ha)
> +		return -ENOMEM;
> +	memcpy(ha->addr, addr, addr_len);
> +	ha->type = addr_type;
> +	ha->refcount = 1;
> +	ha->global_use = global;
> +	ha->synced = false;
> +	list_add_tail_rcu(&ha->list, &list->list);
> +	list->count++;
> +
> +	return 0;
> +}
> +
>  static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>  			    unsigned char *addr, int addr_len,
>  			    unsigned char addr_type, bool global)
>  {
>  	struct netdev_hw_addr *ha;
> -	int alloc_size;
>  
>  	if (addr_len > MAX_ADDR_LEN)
>  		return -EINVAL;
> @@ -46,21 +69,7 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>  		}
>  	}
>  
> -
> -	alloc_size = sizeof(*ha);
> -	if (alloc_size < L1_CACHE_BYTES)
> -		alloc_size = L1_CACHE_BYTES;
> -	ha = kmalloc(alloc_size, GFP_ATOMIC);
> -	if (!ha)
> -		return -ENOMEM;
> -	memcpy(ha->addr, addr, addr_len);
> -	ha->type = addr_type;
> -	ha->refcount = 1;
> -	ha->global_use = global;
> -	ha->synced = false;
> -	list_add_tail_rcu(&ha->list, &list->list);
> -	list->count++;
> -	return 0;
> +	return __hw_addr_create_ex(list, addr, addr_len, addr_type, global);
>  }
>  
>  static int __hw_addr_add(struct netdev_hw_addr_list *list, unsigned char *addr,
> @@ -377,6 +386,34 @@ EXPORT_SYMBOL(dev_addr_del_multiple);
>   */
>  
>  /**
> + *	dev_uc_add_excl - Add a global secondary unicast address
> + *	@dev: device
> + *	@addr: address to add
> + */
> +int dev_uc_add_excl(struct net_device *dev, unsigned char *addr)
> +{
> +	struct netdev_hw_addr *ha;
> +	int err;
> +
> +	netif_addr_lock_bh(dev);
> +	list_for_each_entry(ha, &dev->uc.list, list) {
> +		if (!memcmp(ha->addr, addr, dev->addr_len) &&
> +		    ha->type == NETDEV_HW_ADDR_T_UNICAST) {
> +			err = -EEXIST;
> +			goto out;
> +		}
> +	}
> +	err = __hw_addr_create_ex(&dev->uc, addr, dev->addr_len,
> +				  NETDEV_HW_ADDR_T_UNICAST, true);
> +	if (!err)
> +		__dev_set_rx_mode(dev);
> +out:
> +	netif_addr_unlock_bh(dev);
> +	return err;
> +}
> +EXPORT_SYMBOL(dev_uc_add_excl);
> +
> +/**
>   *	dev_uc_add - Add a secondary unicast address
>   *	@dev: device
>   *	@addr: address to add
> @@ -501,6 +538,34 @@ EXPORT_SYMBOL(dev_uc_init);
>   * Multicast list handling functions
>   */
>  
> +/**
> + *	dev_mc_add_excl - Add a global secondary multicast address
> + *	@dev: device
> + *	@addr: address to add
> + */
> +int dev_mc_add_excl(struct net_device *dev, unsigned char *addr)
> +{
> +	struct netdev_hw_addr *ha;
> +	int err;
> +
> +	netif_addr_lock_bh(dev);
> +	list_for_each_entry(ha, &dev->mc.list, list) {
> +		if (!memcmp(ha->addr, addr, dev->addr_len) &&
> +		    ha->type == NETDEV_HW_ADDR_T_UNICAST) {
> +			err = -EEXIST;
> +			goto out;
> +		}
> +	}
> +	err = __hw_addr_create_ex(&dev->mc, addr, dev->addr_len,
> +				  NETDEV_HW_ADDR_T_UNICAST, true);
> +	if (!err)
> +		__dev_set_rx_mode(dev);
> +out:
> +	netif_addr_unlock_bh(dev);
> +	return err;
> +}
> +EXPORT_SYMBOL(dev_mc_add_excl);
> +

Why is it a mistake to have duplicate multicast addresses?
For example macvlan floods all multicasts so it's fine,
and assuming it didn't, it might still be fine to have to
macvlans get the same multicast, no?

>  static int __dev_mc_add(struct net_device *dev, unsigned char *addr,
>  			bool global)
>  {

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> this mode acts the same as the original passthru mode _except_
> it does not set promiscuous mode on the lowerdev. Because the
> lowerdev is not put in promiscuous mode any unicast or multicast
> addresses the device should receive must be explicitely added
> with the FDB bridge ops. In many use cases the management stack
> will know the mac addresses needed (maybe negotiated via EVB/VDP)
> or may require only receiving known "good" mac addresses. This
> mode with the FDB ops supports this usage model.


Looks good to me. Some questions below:

> This patch is a result of Roopa Prabhu's work. Follow up
> patches are needed for VEPA and VEB macvlan modes.

And bridge too?

Also, my understanding is that other modes won't need a flag
like this since they don't put the device in promisc mode initially,
so no assumptions are broken if we require all addresses
to be declared, right?

A final question: I think we'll later add a macvlan mode
that does not flood all multicasts. This would change behaviour
in an incompatible way so we'll probably need yet another
flag. Would it make sense to combine this functionality
with nopromisc so we have less modes to support?

> CC: Roopa Prabhu <roprabhu@cisco.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  drivers/net/macvlan.c   |   60 ++++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/if_link.h |    1 +
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index b17fc90..9892d8d 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -181,6 +181,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>  					  MACVLAN_MODE_PRIVATE |
>  					  MACVLAN_MODE_VEPA    |
>  					  MACVLAN_MODE_PASSTHRU|
> +					  MACVLAN_MODE_PASSTHRU_NOPROMISC |
>  					  MACVLAN_MODE_BRIDGE);
>  		else if (src->mode == MACVLAN_MODE_VEPA)
>  			/* flood to everyone except source */
> @@ -312,7 +313,8 @@ static int macvlan_open(struct net_device *dev)
>  	int err;
>  
>  	if (vlan->port->passthru) {
> -		dev_set_promiscuity(lowerdev, 1);
> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> +			dev_set_promiscuity(lowerdev, 1);
>  		goto hash_add;
>  	}
>  
> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
>  	struct macvlan_dev *vlan = netdev_priv(dev);
>  	struct net_device *lowerdev = vlan->lowerdev;
>  
> +	dev_uc_unsync(lowerdev, dev);
> +	dev_mc_unsync(lowerdev, dev);
> +
>  	if (vlan->port->passthru) {
> -		dev_set_promiscuity(lowerdev, -1);
> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> +			dev_set_promiscuity(lowerdev, 1);
>  		goto hash_del;
>  	}
>  
> -	dev_mc_unsync(lowerdev, dev);
>  	if (dev->flags & IFF_ALLMULTI)
>  		dev_set_allmulti(lowerdev, -1);
>  
> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
>  		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
>  }
>  
> -static void macvlan_set_multicast_list(struct net_device *dev)
> +static void macvlan_set_mac_lists(struct net_device *dev)
>  {
>  	struct macvlan_dev *vlan = netdev_priv(dev);
>  
> +	dev_uc_sync(vlan->lowerdev, dev);
>  	dev_mc_sync(vlan->lowerdev, dev);
>  }
>  
> @@ -542,6 +548,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev,
>  	return 0;
>  }
>  
> +static int macvlan_fdb_add(struct ndmsg *ndm,
> +			   struct net_device *dev,
> +			   unsigned char *addr,
> +			   u16 flags)
> +{
> +	struct macvlan_dev *vlan = netdev_priv(dev);
> +	int err = -EINVAL;
> +
> +	if (!vlan->port->passthru)
> +		return -EOPNOTSUPP;
> +
> +	if (is_unicast_ether_addr(addr))
> +		err = dev_uc_add_excl(dev, addr);
> +	else if (is_multicast_ether_addr(addr))
> +		err = dev_mc_add_excl(dev, addr);
> +
> +	return err;
> +}
> +
> +static int macvlan_fdb_del(struct ndmsg *ndm,
> +			   struct net_device *dev,
> +			   unsigned char *addr)
> +{
> +	struct macvlan_dev *vlan = netdev_priv(dev);
> +	int err = -EINVAL;
> +
> +	if (!vlan->port->passthru)
> +		return -EOPNOTSUPP;
> +
> +	if (is_unicast_ether_addr(addr))
> +		err = dev_uc_del(dev, addr);
> +	else if (is_multicast_ether_addr(addr))
> +		err = dev_mc_del(dev, addr);
> +
> +	return err;
> +}
> +
>  static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
>  					struct ethtool_drvinfo *drvinfo)
>  {
> @@ -572,11 +615,14 @@ static const struct net_device_ops macvlan_netdev_ops = {
>  	.ndo_change_mtu		= macvlan_change_mtu,
>  	.ndo_change_rx_flags	= macvlan_change_rx_flags,
>  	.ndo_set_mac_address	= macvlan_set_mac_address,
> -	.ndo_set_rx_mode	= macvlan_set_multicast_list,
> +	.ndo_set_rx_mode	= macvlan_set_mac_lists,
>  	.ndo_get_stats64	= macvlan_dev_get_stats64,
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_vlan_rx_add_vid	= macvlan_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid	= macvlan_vlan_rx_kill_vid,
> +	.ndo_fdb_add		= macvlan_fdb_add,
> +	.ndo_fdb_del		= macvlan_fdb_del,
> +	.ndo_fdb_dump		= ndo_dflt_fdb_dump,
>  };
>  
>  void macvlan_common_setup(struct net_device *dev)
> @@ -648,6 +694,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
>  		case MACVLAN_MODE_VEPA:
>  		case MACVLAN_MODE_BRIDGE:
>  		case MACVLAN_MODE_PASSTHRU:
> +		case MACVLAN_MODE_PASSTHRU_NOPROMISC:
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -711,7 +758,8 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
>  	if (data && data[IFLA_MACVLAN_MODE])
>  		vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
>  
> -	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> +	if ((vlan->mode == MACVLAN_MODE_PASSTHRU) ||
> +	    (vlan->mode == MACVLAN_MODE_PASSTHRU_NOPROMISC)) {
>  		if (port->count)
>  			return -EINVAL;
>  		port->passthru = true;
> diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> index 2f4fa93..db67b9d 100644
> --- a/include/linux/if_link.h
> +++ b/include/linux/if_link.h
> @@ -265,6 +265,7 @@ enum macvlan_mode {
>  	MACVLAN_MODE_VEPA    = 2, /* talk to other ports through ext bridge */
>  	MACVLAN_MODE_BRIDGE  = 4, /* talk to bridge ports directly */
>  	MACVLAN_MODE_PASSTHRU = 8,/* take over the underlying device */
> +	MACVLAN_MODE_PASSTHRU_NOPROMISC = 16, /* passthru without promisc */
>  };
>  
>  /* SR-IOV virtual function management section */

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> > This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> > this mode acts the same as the original passthru mode _except_
> > it does not set promiscuous mode on the lowerdev. Because the
> > lowerdev is not put in promiscuous mode any unicast or multicast
> > addresses the device should receive must be explicitely added
> > with the FDB bridge ops. In many use cases the management stack
> > will know the mac addresses needed (maybe negotiated via EVB/VDP)
> > or may require only receiving known "good" mac addresses. This
> > mode with the FDB ops supports this usage model.
> 
> 
> Looks good to me. Some questions below:
> 
> > This patch is a result of Roopa Prabhu's work. Follow up
> > patches are needed for VEPA and VEB macvlan modes.
> 
> And bridge too?
> 
> Also, my understanding is that other modes won't need a flag
> like this since they don't put the device in promisc mode initially,
> so no assumptions are broken if we require all addresses
> to be declared, right?
> 
> A final question: I think we'll later add a macvlan mode
> that does not flood all multicasts. This would change behaviour
> in an incompatible way so we'll probably need yet another
> flag. Would it make sense to combine this functionality
> with nopromisc so we have less modes to support?

One other question I forgot:

> > CC: Roopa Prabhu <roprabhu@cisco.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > ---
> > 
> >  drivers/net/macvlan.c   |   60 ++++++++++++++++++++++++++++++++++++++++++-----
> >  include/linux/if_link.h |    1 +
> >  2 files changed, 55 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> > index b17fc90..9892d8d 100644
> > --- a/drivers/net/macvlan.c
> > +++ b/drivers/net/macvlan.c
> > @@ -181,6 +181,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
> >  					  MACVLAN_MODE_PRIVATE |
> >  					  MACVLAN_MODE_VEPA    |
> >  					  MACVLAN_MODE_PASSTHRU|
> > +					  MACVLAN_MODE_PASSTHRU_NOPROMISC |
> >  					  MACVLAN_MODE_BRIDGE);
> >  		else if (src->mode == MACVLAN_MODE_VEPA)
> >  			/* flood to everyone except source */
> > @@ -312,7 +313,8 @@ static int macvlan_open(struct net_device *dev)
> >  	int err;
> >  
> >  	if (vlan->port->passthru) {
> > -		dev_set_promiscuity(lowerdev, 1);
> > +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> > +			dev_set_promiscuity(lowerdev, 1);
> >  		goto hash_add;
> >  	}
> >  
> > @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
> >  	struct macvlan_dev *vlan = netdev_priv(dev);
> >  	struct net_device *lowerdev = vlan->lowerdev;
> >  
> > +	dev_uc_unsync(lowerdev, dev);
> > +	dev_mc_unsync(lowerdev, dev);
> > +
> >  	if (vlan->port->passthru) {
> > -		dev_set_promiscuity(lowerdev, -1);
> > +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> > +			dev_set_promiscuity(lowerdev, 1);
> >  		goto hash_del;
> >  	}
> >  
> > -	dev_mc_unsync(lowerdev, dev);
> >  	if (dev->flags & IFF_ALLMULTI)
> >  		dev_set_allmulti(lowerdev, -1);
> >  
> > @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
> >  		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);

In the new mode, do we want to have promisc on lowerdev follow whatever
is set on the macvlan, like we do for allmulti?
I'm not sure at this point - what do others think?


> >  }
> >  
> > -static void macvlan_set_multicast_list(struct net_device *dev)
> > +static void macvlan_set_mac_lists(struct net_device *dev)
> >  {
> >  	struct macvlan_dev *vlan = netdev_priv(dev);
> >  
> > +	dev_uc_sync(vlan->lowerdev, dev);
> >  	dev_mc_sync(vlan->lowerdev, dev);
> >  }
> >  
> > @@ -542,6 +548,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev,
> >  	return 0;
> >  }
> >  
> > +static int macvlan_fdb_add(struct ndmsg *ndm,
> > +			   struct net_device *dev,
> > +			   unsigned char *addr,
> > +			   u16 flags)
> > +{
> > +	struct macvlan_dev *vlan = netdev_priv(dev);
> > +	int err = -EINVAL;
> > +
> > +	if (!vlan->port->passthru)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (is_unicast_ether_addr(addr))
> > +		err = dev_uc_add_excl(dev, addr);
> > +	else if (is_multicast_ether_addr(addr))
> > +		err = dev_mc_add_excl(dev, addr);
> > +
> > +	return err;
> > +}
> > +
> > +static int macvlan_fdb_del(struct ndmsg *ndm,
> > +			   struct net_device *dev,
> > +			   unsigned char *addr)
> > +{
> > +	struct macvlan_dev *vlan = netdev_priv(dev);
> > +	int err = -EINVAL;
> > +
> > +	if (!vlan->port->passthru)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (is_unicast_ether_addr(addr))
> > +		err = dev_uc_del(dev, addr);
> > +	else if (is_multicast_ether_addr(addr))
> > +		err = dev_mc_del(dev, addr);
> > +
> > +	return err;
> > +}
> > +
> >  static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
> >  					struct ethtool_drvinfo *drvinfo)
> >  {
> > @@ -572,11 +615,14 @@ static const struct net_device_ops macvlan_netdev_ops = {
> >  	.ndo_change_mtu		= macvlan_change_mtu,
> >  	.ndo_change_rx_flags	= macvlan_change_rx_flags,
> >  	.ndo_set_mac_address	= macvlan_set_mac_address,
> > -	.ndo_set_rx_mode	= macvlan_set_multicast_list,
> > +	.ndo_set_rx_mode	= macvlan_set_mac_lists,
> >  	.ndo_get_stats64	= macvlan_dev_get_stats64,
> >  	.ndo_validate_addr	= eth_validate_addr,
> >  	.ndo_vlan_rx_add_vid	= macvlan_vlan_rx_add_vid,
> >  	.ndo_vlan_rx_kill_vid	= macvlan_vlan_rx_kill_vid,
> > +	.ndo_fdb_add		= macvlan_fdb_add,
> > +	.ndo_fdb_del		= macvlan_fdb_del,
> > +	.ndo_fdb_dump		= ndo_dflt_fdb_dump,
> >  };
> >  
> >  void macvlan_common_setup(struct net_device *dev)
> > @@ -648,6 +694,7 @@ static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
> >  		case MACVLAN_MODE_VEPA:
> >  		case MACVLAN_MODE_BRIDGE:
> >  		case MACVLAN_MODE_PASSTHRU:
> > +		case MACVLAN_MODE_PASSTHRU_NOPROMISC:
> >  			break;
> >  		default:
> >  			return -EINVAL;
> > @@ -711,7 +758,8 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
> >  	if (data && data[IFLA_MACVLAN_MODE])
> >  		vlan->mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> >  
> > -	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> > +	if ((vlan->mode == MACVLAN_MODE_PASSTHRU) ||
> > +	    (vlan->mode == MACVLAN_MODE_PASSTHRU_NOPROMISC)) {
> >  		if (port->count)
> >  			return -EINVAL;
> >  		port->passthru = true;
> > diff --git a/include/linux/if_link.h b/include/linux/if_link.h
> > index 2f4fa93..db67b9d 100644
> > --- a/include/linux/if_link.h
> > +++ b/include/linux/if_link.h
> > @@ -265,6 +265,7 @@ enum macvlan_mode {
> >  	MACVLAN_MODE_VEPA    = 2, /* talk to other ports through ext bridge */
> >  	MACVLAN_MODE_BRIDGE  = 4, /* talk to bridge ports directly */
> >  	MACVLAN_MODE_PASSTHRU = 8,/* take over the underlying device */
> > +	MACVLAN_MODE_PASSTHRU_NOPROMISC = 16, /* passthru without promisc */
> >  };
> >  
> >  /* SR-IOV virtual function management section */

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 1:09 AM, Michael S. Tsirkin wrote:
> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
>> this mode acts the same as the original passthru mode _except_
>> it does not set promiscuous mode on the lowerdev. Because the
>> lowerdev is not put in promiscuous mode any unicast or multicast
>> addresses the device should receive must be explicitely added
>> with the FDB bridge ops. In many use cases the management stack
>> will know the mac addresses needed (maybe negotiated via EVB/VDP)
>> or may require only receiving known "good" mac addresses. This
>> mode with the FDB ops supports this usage model.
> 
> 
> Looks good to me. Some questions below:
> 
>> This patch is a result of Roopa Prabhu's work. Follow up
>> patches are needed for VEPA and VEB macvlan modes.
> 
> And bridge too?
> 

Yes I called this mode VEB here but this is defined in if_link.h
as IFLA_MACVLAN_MODE_BRIDGE. From a IEEE point of view I think
the macvlan bridge mode acts more like a 802.1Q VEB then a 802.1d
bridge.

> Also, my understanding is that other modes won't need a flag
> like this since they don't put the device in promisc mode initially,
> so no assumptions are broken if we require all addresses
> to be declared, right?
> 

correct. But requires extra work to the hash table so the forwarding
works correctly.

> A final question: I think we'll later add a macvlan mode
> that does not flood all multicasts. This would change behaviour
> in an incompatible way so we'll probably need yet another
> flag. Would it make sense to combine this functionality
> with nopromisc so we have less modes to support?
> 

For VEPA and bridge modes this makes sense to me. If you want
the flood behavior you can create it by adding the addr to all
the devices or just to a subset of them to get the non-flooding
capabilities.

.John

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 06:27:32AM -0700, John Fastabend wrote:
> On 4/10/2012 1:09 AM, Michael S. Tsirkin wrote:
> > On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> >> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> >> this mode acts the same as the original passthru mode _except_
> >> it does not set promiscuous mode on the lowerdev. Because the
> >> lowerdev is not put in promiscuous mode any unicast or multicast
> >> addresses the device should receive must be explicitely added
> >> with the FDB bridge ops. In many use cases the management stack
> >> will know the mac addresses needed (maybe negotiated via EVB/VDP)
> >> or may require only receiving known "good" mac addresses. This
> >> mode with the FDB ops supports this usage model.
> > 
> > 
> > Looks good to me. Some questions below:
> > 
> >> This patch is a result of Roopa Prabhu's work. Follow up
> >> patches are needed for VEPA and VEB macvlan modes.
> > 
> > And bridge too?
> > 
> 
> Yes I called this mode VEB here but this is defined in if_link.h
> as IFLA_MACVLAN_MODE_BRIDGE. From a IEEE point of view I think
> the macvlan bridge mode acts more like a 802.1Q VEB then a 802.1d
> bridge.

grep didn't find IFLA_MACVLAN_MODE_BRIDGE - which kernel
are you looking at?

> > Also, my understanding is that other modes won't need a flag
> > like this since they don't put the device in promisc mode initially,
> > so no assumptions are broken if we require all addresses
> > to be declared, right?
> > 
> 
> correct. But requires extra work to the hash table so the forwarding
> works correctly.
> 
> > A final question: I think we'll later add a macvlan mode
> > that does not flood all multicasts. This would change behaviour
> > in an incompatible way so we'll probably need yet another
> > flag. Would it make sense to combine this functionality
> > with nopromisc so we have less modes to support?
> > 
> 
> For VEPA and bridge modes this makes sense to me.

Hmm okay, but this would mean we should convert
MACVLAN_MODE_PASSTHRU_NOPROMISC to something
that can combined with all modes. E.g.
MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX

and document that it does not promise to flood
multicast.

> If you want
> the flood behavior you can create it by adding the addr to all
> the devices or just to a subset of them to get the non-flooding
> capabilities.
> 
> .John

BTW we seem to try to flood in pass-through too, not sure why.

-- 
MST

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 1:14 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
>> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
>>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
>>> this mode acts the same as the original passthru mode _except_
>>> it does not set promiscuous mode on the lowerdev. Because the
>>> lowerdev is not put in promiscuous mode any unicast or multicast
>>> addresses the device should receive must be explicitely added
>>> with the FDB bridge ops. In many use cases the management stack
>>> will know the mac addresses needed (maybe negotiated via EVB/VDP)
>>> or may require only receiving known "good" mac addresses. This
>>> mode with the FDB ops supports this usage model.
>>
>>
>> Looks good to me. Some questions below:
>>
>>> This patch is a result of Roopa Prabhu's work. Follow up
>>> patches are needed for VEPA and VEB macvlan modes.
>>
>> And bridge too?
>>
>> Also, my understanding is that other modes won't need a flag
>> like this since they don't put the device in promisc mode initially,
>> so no assumptions are broken if we require all addresses
>> to be declared, right?
>>
>> A final question: I think we'll later add a macvlan mode
>> that does not flood all multicasts. This would change behaviour
>> in an incompatible way so we'll probably need yet another
>> flag. Would it make sense to combine this functionality
>> with nopromisc so we have less modes to support?
> 
> One other question I forgot:
> 

[...]

>>>  
>>> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
>>>  	struct macvlan_dev *vlan = netdev_priv(dev);
>>>  	struct net_device *lowerdev = vlan->lowerdev;
>>>  
>>> +	dev_uc_unsync(lowerdev, dev);
>>> +	dev_mc_unsync(lowerdev, dev);
>>> +
>>>  	if (vlan->port->passthru) {
>>> -		dev_set_promiscuity(lowerdev, -1);
>>> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
>>> +			dev_set_promiscuity(lowerdev, 1);
>>>  		goto hash_del;
>>>  	}
>>>  
>>> -	dev_mc_unsync(lowerdev, dev);
>>>  	if (dev->flags & IFF_ALLMULTI)
>>>  		dev_set_allmulti(lowerdev, -1);
>>>  
>>> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
>>>  		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
> 
> In the new mode, do we want to have promisc on lowerdev follow whatever
> is set on the macvlan, like we do for allmulti?
> I'm not sure at this point - what do others think?
> 

Just to enumerate why you would need this: (1) socket set with
PACKET_MR_MULTICAST and (2) something like mrouted is running
on the macvlan (3) maybe some case I missed?

Don't you need CAP_NET_RAW to set these though anyways? So I
wouldn't think it would be a problem. I assume if a user has
CAP_NET_RAW or UUID 0 they really should be able to set this
up.

.John

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 6:43 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 06:27:32AM -0700, John Fastabend wrote:
>> On 4/10/2012 1:09 AM, Michael S. Tsirkin wrote:
>>> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
>>>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
>>>> this mode acts the same as the original passthru mode _except_
>>>> it does not set promiscuous mode on the lowerdev. Because the
>>>> lowerdev is not put in promiscuous mode any unicast or multicast
>>>> addresses the device should receive must be explicitely added
>>>> with the FDB bridge ops. In many use cases the management stack
>>>> will know the mac addresses needed (maybe negotiated via EVB/VDP)
>>>> or may require only receiving known "good" mac addresses. This
>>>> mode with the FDB ops supports this usage model.
>>>
>>>
>>> Looks good to me. Some questions below:
>>>
>>>> This patch is a result of Roopa Prabhu's work. Follow up
>>>> patches are needed for VEPA and VEB macvlan modes.
>>>
>>> And bridge too?
>>>
>>
>> Yes I called this mode VEB here but this is defined in if_link.h
>> as IFLA_MACVLAN_MODE_BRIDGE. From a IEEE point of view I think
>> the macvlan bridge mode acts more like a 802.1Q VEB then a 802.1d
>> bridge.
> 
> grep didn't find IFLA_MACVLAN_MODE_BRIDGE - which kernel
> are you looking at?

grr sorry typo MACVLAN_MODE_BRIDGE in if_link.h

> 
>>> Also, my understanding is that other modes won't need a flag
>>> like this since they don't put the device in promisc mode initially,
>>> so no assumptions are broken if we require all addresses
>>> to be declared, right?
>>>
>>
>> correct. But requires extra work to the hash table so the forwarding
>> works correctly.
>>
>>> A final question: I think we'll later add a macvlan mode
>>> that does not flood all multicasts. This would change behaviour
>>> in an incompatible way so we'll probably need yet another
>>> flag. Would it make sense to combine this functionality
>>> with nopromisc so we have less modes to support?
>>>
>>
>> For VEPA and bridge modes this makes sense to me.
> 
> Hmm okay, but this would mean we should convert
> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
> that can combined with all modes. E.g.
> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
> 
> and document that it does not promise to flood
> multicast.
>

How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC -> MACVLAN_MODE_NOPORMISC
for this patch. Then a follow on series can rework bridge
and VEPA to use it as well.
 
>> If you want
>> the flood behavior you can create it by adding the addr to all
>> the devices or just to a subset of them to get the non-flooding
>> capabilities.
>>
>> .John
> 
> BTW we seem to try to flood in pass-through too, not sure why.
> 

Suppose your looking at macvlan_handle_frame()? It does
seem unneeded.

.John

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 06:50:42AM -0700, John Fastabend wrote:
> On 4/10/2012 1:14 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> >> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> >>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> >>> this mode acts the same as the original passthru mode _except_
> >>> it does not set promiscuous mode on the lowerdev. Because the
> >>> lowerdev is not put in promiscuous mode any unicast or multicast
> >>> addresses the device should receive must be explicitely added
> >>> with the FDB bridge ops. In many use cases the management stack
> >>> will know the mac addresses needed (maybe negotiated via EVB/VDP)
> >>> or may require only receiving known "good" mac addresses. This
> >>> mode with the FDB ops supports this usage model.
> >>
> >>
> >> Looks good to me. Some questions below:
> >>
> >>> This patch is a result of Roopa Prabhu's work. Follow up
> >>> patches are needed for VEPA and VEB macvlan modes.
> >>
> >> And bridge too?
> >>
> >> Also, my understanding is that other modes won't need a flag
> >> like this since they don't put the device in promisc mode initially,
> >> so no assumptions are broken if we require all addresses
> >> to be declared, right?
> >>
> >> A final question: I think we'll later add a macvlan mode
> >> that does not flood all multicasts. This would change behaviour
> >> in an incompatible way so we'll probably need yet another
> >> flag. Would it make sense to combine this functionality
> >> with nopromisc so we have less modes to support?
> > 
> > One other question I forgot:
> > 
> 
> [...]
> 
> >>>  
> >>> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
> >>>  	struct macvlan_dev *vlan = netdev_priv(dev);
> >>>  	struct net_device *lowerdev = vlan->lowerdev;
> >>>  
> >>> +	dev_uc_unsync(lowerdev, dev);
> >>> +	dev_mc_unsync(lowerdev, dev);
> >>> +
> >>>  	if (vlan->port->passthru) {
> >>> -		dev_set_promiscuity(lowerdev, -1);
> >>> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> >>> +			dev_set_promiscuity(lowerdev, 1);
> >>>  		goto hash_del;
> >>>  	}
> >>>  
> >>> -	dev_mc_unsync(lowerdev, dev);
> >>>  	if (dev->flags & IFF_ALLMULTI)
> >>>  		dev_set_allmulti(lowerdev, -1);
> >>>  
> >>> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
> >>>  		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
> > 
> > In the new mode, do we want to have promisc on lowerdev follow whatever
> > is set on the macvlan, like we do for allmulti?
> > I'm not sure at this point - what do others think?
> > 
> 
> Just to enumerate why you would need this: (1) socket set with
> PACKET_MR_MULTICAST and (2) something like mrouted is running
> on the macvlan (3) maybe some case I missed?
> 
> Don't you need CAP_NET_RAW to set these though anyways? So I
> wouldn't think it would be a problem. I assume if a user has
> CAP_NET_RAW or UUID 0 they really should be able to set this
> up.
> 
> .John

I am not sure, really.
But I note that with a security mechanism such as selinux, CAP_NET_RAW
might be insufficient to change the underlying device.
So there might be value in being able to change it in
a controlled manner through macvlan.

There's also something to be said for being able to let
management deal with macvlan devices (and there are
some very complex tools for that around) while
keeping a simple script around for the physical
one and knowing that they won't disrupt each other.

-- 
MST

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
> > Hmm okay, but this would mean we should convert
> > MACVLAN_MODE_PASSTHRU_NOPROMISC to something
> > that can combined with all modes. E.g.
> > MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
> > 
> > and document that it does not promise to flood
> > multicast.
> >
> 
> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC -> MACVLAN_MODE_NOPORMISC
> for this patch. Then a follow on series can rework bridge
> and VEPA to use it as well.

Right. We probably need a better name if it's going to
affect other things besides promisc though.

> >> If you want
> >> the flood behavior you can create it by adding the addr to all
> >> the devices or just to a subset of them to get the non-flooding
> >> capabilities.
> >>
> >> .John
> > 
> > BTW we seem to try to flood in pass-through too, not sure why.
> > 
> 
> Suppose your looking at macvlan_handle_frame()? It does
> seem unneeded.
> 
> .John

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
>>> Hmm okay, but this would mean we should convert
>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
>>> that can combined with all modes. E.g.
>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
>>>
>>> and document that it does not promise to flood
>>> multicast.
>>>
>>
>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC -> MACVLAN_MODE_NOPORMISC
>> for this patch. Then a follow on series can rework bridge
>> and VEPA to use it as well.
> 
> Right. We probably need a better name if it's going to
> affect other things besides promisc though.
> 

how about MACVLAN_MODE_FDBFLAG?

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 7:33 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 06:50:42AM -0700, John Fastabend wrote:
>> On 4/10/2012 1:14 AM, Michael S. Tsirkin wrote:
>>> On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
>>>> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
>>>>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
>>>>> this mode acts the same as the original passthru mode _except_
>>>>> it does not set promiscuous mode on the lowerdev. Because the
>>>>> lowerdev is not put in promiscuous mode any unicast or multicast
>>>>> addresses the device should receive must be explicitely added
>>>>> with the FDB bridge ops. In many use cases the management stack
>>>>> will know the mac addresses needed (maybe negotiated via EVB/VDP)
>>>>> or may require only receiving known "good" mac addresses. This
>>>>> mode with the FDB ops supports this usage model.
>>>>
>>>>
>>>> Looks good to me. Some questions below:
>>>>
>>>>> This patch is a result of Roopa Prabhu's work. Follow up
>>>>> patches are needed for VEPA and VEB macvlan modes.
>>>>
>>>> And bridge too?
>>>>
>>>> Also, my understanding is that other modes won't need a flag
>>>> like this since they don't put the device in promisc mode initially,
>>>> so no assumptions are broken if we require all addresses
>>>> to be declared, right?
>>>>
>>>> A final question: I think we'll later add a macvlan mode
>>>> that does not flood all multicasts. This would change behaviour
>>>> in an incompatible way so we'll probably need yet another
>>>> flag. Would it make sense to combine this functionality
>>>> with nopromisc so we have less modes to support?
>>>
>>> One other question I forgot:
>>>
>>
>> [...]
>>
>>>>>  
>>>>> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
>>>>>  	struct macvlan_dev *vlan = netdev_priv(dev);
>>>>>  	struct net_device *lowerdev = vlan->lowerdev;
>>>>>  
>>>>> +	dev_uc_unsync(lowerdev, dev);
>>>>> +	dev_mc_unsync(lowerdev, dev);
>>>>> +
>>>>>  	if (vlan->port->passthru) {
>>>>> -		dev_set_promiscuity(lowerdev, -1);
>>>>> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
>>>>> +			dev_set_promiscuity(lowerdev, 1);
>>>>>  		goto hash_del;
>>>>>  	}
>>>>>  
>>>>> -	dev_mc_unsync(lowerdev, dev);
>>>>>  	if (dev->flags & IFF_ALLMULTI)
>>>>>  		dev_set_allmulti(lowerdev, -1);
>>>>>  
>>>>> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
>>>>>  		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
>>>
>>> In the new mode, do we want to have promisc on lowerdev follow whatever
>>> is set on the macvlan, like we do for allmulti?
>>> I'm not sure at this point - what do others think?
>>>
>>
>> Just to enumerate why you would need this: (1) socket set with
>> PACKET_MR_MULTICAST and (2) something like mrouted is running
>> on the macvlan (3) maybe some case I missed?
>>
>> Don't you need CAP_NET_RAW to set these though anyways? So I
>> wouldn't think it would be a problem. I assume if a user has
>> CAP_NET_RAW or UUID 0 they really should be able to set this
>> up.
>>
>> .John
> 
> I am not sure, really.
> But I note that with a security mechanism such as selinux, CAP_NET_RAW
> might be insufficient to change the underlying device.
> So there might be value in being able to change it in
> a controlled manner through macvlan.
> 
> There's also something to be said for being able to let
> management deal with macvlan devices (and there are
> some very complex tools for that around) while
> keeping a simple script around for the physical
> one and knowing that they won't disrupt each other.
> 

If people really _need_/_want_ this then I guess we can
add another flag. I don't think we should to tie this into
the FDB bits creating an interface with strange side effects
is probably a poor design. Much better IMHO to have an
explicit bit if and when this is needed.

.John

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
> >>> Hmm okay, but this would mean we should convert
> >>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
> >>> that can combined with all modes. E.g.
> >>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
> >>>
> >>> and document that it does not promise to flood
> >>> multicast.
> >>>
> >>
> >> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC -> MACVLAN_MODE_NOPORMISC
> >> for this patch. Then a follow on series can rework bridge
> >> and VEPA to use it as well.
> > 
> > Right. We probably need a better name if it's going to
> > affect other things besides promisc though.
> > 
> 
> how about MACVLAN_MODE_FDBFLAG?

The idea being that no one figures out what this means so
no one will make any wrong assumptions? ;)

-- 
MST

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 08:29:45AM -0700, John Fastabend wrote:
> On 4/10/2012 7:33 AM, Michael S. Tsirkin wrote:
> > On Tue, Apr 10, 2012 at 06:50:42AM -0700, John Fastabend wrote:
> >> On 4/10/2012 1:14 AM, Michael S. Tsirkin wrote:
> >>> On Tue, Apr 10, 2012 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
> >>>> On Mon, Apr 09, 2012 at 03:00:54PM -0700, John Fastabend wrote:
> >>>>> This adds a new macvlan mode MACVLAN_PASSTHRU_NOPROMISC
> >>>>> this mode acts the same as the original passthru mode _except_
> >>>>> it does not set promiscuous mode on the lowerdev. Because the
> >>>>> lowerdev is not put in promiscuous mode any unicast or multicast
> >>>>> addresses the device should receive must be explicitely added
> >>>>> with the FDB bridge ops. In many use cases the management stack
> >>>>> will know the mac addresses needed (maybe negotiated via EVB/VDP)
> >>>>> or may require only receiving known "good" mac addresses. This
> >>>>> mode with the FDB ops supports this usage model.
> >>>>
> >>>>
> >>>> Looks good to me. Some questions below:
> >>>>
> >>>>> This patch is a result of Roopa Prabhu's work. Follow up
> >>>>> patches are needed for VEPA and VEB macvlan modes.
> >>>>
> >>>> And bridge too?
> >>>>
> >>>> Also, my understanding is that other modes won't need a flag
> >>>> like this since they don't put the device in promisc mode initially,
> >>>> so no assumptions are broken if we require all addresses
> >>>> to be declared, right?
> >>>>
> >>>> A final question: I think we'll later add a macvlan mode
> >>>> that does not flood all multicasts. This would change behaviour
> >>>> in an incompatible way so we'll probably need yet another
> >>>> flag. Would it make sense to combine this functionality
> >>>> with nopromisc so we have less modes to support?
> >>>
> >>> One other question I forgot:
> >>>
> >>
> >> [...]
> >>
> >>>>>  
> >>>>> @@ -344,12 +346,15 @@ static int macvlan_stop(struct net_device *dev)
> >>>>>  	struct macvlan_dev *vlan = netdev_priv(dev);
> >>>>>  	struct net_device *lowerdev = vlan->lowerdev;
> >>>>>  
> >>>>> +	dev_uc_unsync(lowerdev, dev);
> >>>>> +	dev_mc_unsync(lowerdev, dev);
> >>>>> +
> >>>>>  	if (vlan->port->passthru) {
> >>>>> -		dev_set_promiscuity(lowerdev, -1);
> >>>>> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU)
> >>>>> +			dev_set_promiscuity(lowerdev, 1);
> >>>>>  		goto hash_del;
> >>>>>  	}
> >>>>>  
> >>>>> -	dev_mc_unsync(lowerdev, dev);
> >>>>>  	if (dev->flags & IFF_ALLMULTI)
> >>>>>  		dev_set_allmulti(lowerdev, -1);
> >>>>>  
> >>>>> @@ -399,10 +404,11 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
> >>>>>  		dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
> >>>
> >>> In the new mode, do we want to have promisc on lowerdev follow whatever
> >>> is set on the macvlan, like we do for allmulti?
> >>> I'm not sure at this point - what do others think?
> >>>
> >>
> >> Just to enumerate why you would need this: (1) socket set with
> >> PACKET_MR_MULTICAST and (2) something like mrouted is running
> >> on the macvlan (3) maybe some case I missed?
> >>
> >> Don't you need CAP_NET_RAW to set these though anyways? So I
> >> wouldn't think it would be a problem. I assume if a user has
> >> CAP_NET_RAW or UUID 0 they really should be able to set this
> >> up.
> >>
> >> .John
> > 
> > I am not sure, really.
> > But I note that with a security mechanism such as selinux, CAP_NET_RAW
> > might be insufficient to change the underlying device.
> > So there might be value in being able to change it in
> > a controlled manner through macvlan.
> > 
> > There's also something to be said for being able to let
> > management deal with macvlan devices (and there are
> > some very complex tools for that around) while
> > keeping a simple script around for the physical
> > one and knowing that they won't disrupt each other.
> > 
> 
> If people really _need_/_want_ this then I guess we can
> add another flag. I don't think we should to tie this into
> the FDB bits creating an interface with strange side effects
> is probably a poor design. Much better IMHO to have an
> explicit bit if and when this is needed.
> 
> .John

OK so with the new flag, you will also disable
the forwarding of ALLMULTI from macvlan to lowerdev?
Fair enough.

-- 
MST

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 8:30 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
>> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
>>> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
>>>>> Hmm okay, but this would mean we should convert
>>>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
>>>>> that can combined with all modes. E.g.
>>>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
>>>>>
>>>>> and document that it does not promise to flood
>>>>> multicast.
>>>>>
>>>>
>>>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC -> MACVLAN_MODE_NOPORMISC
>>>> for this patch. Then a follow on series can rework bridge
>>>> and VEPA to use it as well.
>>>
>>> Right. We probably need a better name if it's going to
>>> affect other things besides promisc though.
>>>
>>
>> how about MACVLAN_MODE_FDBFLAG?
> 
> The idea being that no one figures out what this means so
> no one will make any wrong assumptions? ;)
> 

Well its a flag to enable the FDB (forwarding database) ops
and skip dev_set_promisc() on passthru mode. Any better ideas?
Maybe MACVLAN_MODE_FDBENABLE or MACVLAN_MODE_MANAGE_FDB?

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Sridhar Samudrala]

On 4/10/2012 8:35 AM, John Fastabend wrote:
> On 4/10/2012 8:30 AM, Michael S. Tsirkin wrote:
>> On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
>>> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
>>>> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
>>>>>> Hmm okay, but this would mean we should convert
>>>>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
>>>>>> that can combined with all modes. E.g.
>>>>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
>>>>>>
>>>>>> and document that it does not promise to flood
>>>>>> multicast.
>>>>>>
>>>>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC ->  MACVLAN_MODE_NOPORMISC
>>>>> for this patch. Then a follow on series can rework bridge
>>>>> and VEPA to use it as well.
>>>> Right. We probably need a better name if it's going to
>>>> affect other things besides promisc though.
>>>>
>>> how about MACVLAN_MODE_FDBFLAG?
>> The idea being that no one figures out what this means so
>> no one will make any wrong assumptions? ;)
>>
> Well its a flag to enable the FDB (forwarding database) ops
> and skip dev_set_promisc() on passthru mode. Any better ideas?
> Maybe MACVLAN_MODE_FDBENABLE or MACVLAN_MODE_MANAGE_FDB?
Do we need to introduce another mode? I think this patch is enabling 
passthru mode without the need
to put the underlying device in promiscuous mode. So basically we can 
consider this patch as
an optimization.

Thanks
Sridhar

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/10/2012 5:46 PM, Sridhar Samudrala wrote:
> On 4/10/2012 8:35 AM, John Fastabend wrote:
>> On 4/10/2012 8:30 AM, Michael S. Tsirkin wrote:
>>> On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
>>>> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
>>>>> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
>>>>>>> Hmm okay, but this would mean we should convert
>>>>>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
>>>>>>> that can combined with all modes. E.g.
>>>>>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
>>>>>>>
>>>>>>> and document that it does not promise to flood
>>>>>>> multicast.
>>>>>>>
>>>>>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC ->  MACVLAN_MODE_NOPORMISC
>>>>>> for this patch. Then a follow on series can rework bridge
>>>>>> and VEPA to use it as well.
>>>>> Right. We probably need a better name if it's going to
>>>>> affect other things besides promisc though.
>>>>>
>>>> how about MACVLAN_MODE_FDBFLAG?
>>> The idea being that no one figures out what this means so
>>> no one will make any wrong assumptions? ;)
>>>
>> Well its a flag to enable the FDB (forwarding database) ops
>> and skip dev_set_promisc() on passthru mode. Any better ideas?
>> Maybe MACVLAN_MODE_FDBENABLE or MACVLAN_MODE_MANAGE_FDB?
> Do we need to introduce another mode? I think this patch is enabling passthru mode without the need
> to put the underlying device in promiscuous mode. So basically we can consider this patch as
> an optimization.
> 
> Thanks
> Sridhar
> 

Sridhar, Michael,

After thinking about this a bit I would propose keeping this
patch as is. Or if we prefer I can make this a flag but I don't
think that helps much. passthru mode is the only macvlan mode
that calls dev_set_promiscuity(). Either way I think this mode
or flag should _only_ toggle the call to dev_set_promiscuity().

Setting multicast dev->flag IFF_ALLMULTI seems to be a completely
separate optimization that we can work with a follow up patch.

Any thoughts?

Thanks for the feedback,
John

Re: [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

[Ben Hutchings]

On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
[...]
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 1f77540..05822e5 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
[...]
> @@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
>   *	feature set might be less than what was returned by ndo_fix_features()).
>   *	Must return >0 or -errno if it changed dev->features itself.
>   *
> + * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
> + *		      unsigned char *addr, u16 flags)
> + *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
> + *	if the dev->master FDB should be updated or the devices internal FDB.

I don't think the second sentence is helpful, as rtnl_fdb_add() will
take care of those flags.

> + * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
> + *		      unsigned char *addr)
> + *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
> + *	contains flags to indicate if the dev->master FDB should be
> + *	updated or the devices internal FDB.

Similarly here.

> + * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
> + *		       struct net_device *dev, int idx)
> + *	Used to add FDB entries to dump requests. Implementers should add
> + *	entries to skb and update idx with the number of entries.
>   */
[...
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
[...]
> +static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
> +{
[...]
> +	err = -EOPNOTSUPP;

So if NTF_MASTER and NTF_SELF are both set, we can quietly fall back to
just setting one FDB?  Not sure that's really desirable though 

> +	/* Support fdb on master device the net/bridge default case */
> +	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
> +	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
> +		struct net_device *master = dev->master;
> +
> +		if (master->netdev_ops->ndo_fdb_add)

This operation is surely going to be mandatory for bridge devices, so
this check should be omitted or changed to a BUG_ON().

> +			err = master->netdev_ops->ndo_fdb_add(ndm, dev, addr,
> +							      nlh->nlmsg_flags);

Shoudn't we return early on error?

> +	}
> +
> +	/* Embedded bridge, macvlan, and any other device support */
> +	if ((ndm->ndm_flags & NTF_SELF) &&
> +	    dev->netdev_ops->ndo_fdb_add)

Error if !dev->netdev_ops->ndo_fdb_add.

> +		err = dev->netdev_ops->ndo_fdb_add(ndm, dev, addr,
> +						   nlh->nlmsg_flags);

Wonder what we should do on error here if we've already successfully
called ndo_fdb_add on the master?  Should we try to roll back the first
addition?

> +	return err;
> +}
> +
> +static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
> +{
[...]
> +	err = -EOPNOTSUPP;
> +
> +	/* Support fdb on master device the net/bridge default case */
> +	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
> +	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
> +		struct net_device *master = dev->master;
> +
> +		if (master->netdev_ops->ndo_fdb_del)
> +			err = master->netdev_ops->ndo_fdb_del(ndm, dev, addr);
> +	}
> +
> +	/* Embedded bridge, macvlan, and any other device support */
> +	if ((ndm->ndm_flags & NTF_SELF) &&
> +	    dev->netdev_ops->ndo_fdb_del)
> +		err = dev->netdev_ops->ndo_fdb_del(ndm, dev, addr);
> +	return err;
> +}
[...]

This has the same issues.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next PATCH v1 2/7] net: addr_list: add exclusive dev_uc_add and dev_mc_add

[Ben Hutchings]

On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
> This adds a dev_uc_add_excl() and dev_mc_add_excl() calls
> similar to the original dev_{uc|mc}_add() except it sets
> the global bit and returns -EEXIST for duplicat entires.
> 
> This is useful for drivers that support SR-IOV, macvlan
> devices and any other devices that need to manage the
> unicast and multicast lists.
[...]
> +/**
> + *	dev_mc_add_excl - Add a global secondary multicast address
> + *	@dev: device
> + *	@addr: address to add
> + */
> +int dev_mc_add_excl(struct net_device *dev, unsigned char *addr)
> +{
> +	struct netdev_hw_addr *ha;
> +	int err;
> +
> +	netif_addr_lock_bh(dev);
> +	list_for_each_entry(ha, &dev->mc.list, list) {
> +		if (!memcmp(ha->addr, addr, dev->addr_len) &&
> +		    ha->type == NETDEV_HW_ADDR_T_UNICAST) {
> +			err = -EEXIST;
> +			goto out;
> +		}
> +	}
> +	err = __hw_addr_create_ex(&dev->mc, addr, dev->addr_len,
> +				  NETDEV_HW_ADDR_T_UNICAST, true);
[...]

The address types are wrong.  But do we even need this function yet?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next PATCH v1 3/7] net: add fdb generic dump routine

[Ben Hutchings]

On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
> This adds a generic dump routine drivers can call. It
> should be sufficient to handle any bridging model that
> uses the unicast address list. This should be most SR-IOV
> enabled NICs.
[...]
> +static int nlmsg_populate_fdb(struct sk_buff *skb,
> +			      struct netlink_callback *cb,
> +			      struct net_device *dev,
> +			      int *idx,
> +			      struct netdev_hw_addr_list *list)
> +{
> +	struct netdev_hw_addr *ha;
> +	struct ndmsg *ndm;
> +	struct nlmsghdr *nlh;
> +	u32 pid, seq;
> +
> +	pid = NETLINK_CB(cb->skb).pid;
> +	seq = cb->nlh->nlmsg_seq;
> +
> +	list_for_each_entry(ha, &list->list, list) {
> +		if (*idx < cb->args[0])
> +			goto skip;
> +
> +		nlh = nlmsg_put(skb, pid, seq,
> +				RTM_NEWNEIGH, sizeof(*ndm), NLM_F_MULTI);
> +		if (!nlh)
> +			break;

This break is effectively return 0, but shouldn't we return an error?
In practice this does no harm because any subsequent invocation of
nlmsg_populate_fdb() for the same skb is also going to fail here with no
change to *idx.  But it would be more obviously correct to return an
error.

[...]
> +	}
> +	return 0;
> +nla_put_failure:
> +	nlmsg_cancel(skb, nlh);
> +	return -ENOMEM;
> +}
[...]

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[Michael S. Tsirkin]

On Tue, Apr 10, 2012 at 06:42:47PM -0700, John Fastabend wrote:
> On 4/10/2012 5:46 PM, Sridhar Samudrala wrote:
> > On 4/10/2012 8:35 AM, John Fastabend wrote:
> >> On 4/10/2012 8:30 AM, Michael S. Tsirkin wrote:
> >>> On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
> >>>> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
> >>>>> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
> >>>>>>> Hmm okay, but this would mean we should convert
> >>>>>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
> >>>>>>> that can combined with all modes. E.g.
> >>>>>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
> >>>>>>>
> >>>>>>> and document that it does not promise to flood
> >>>>>>> multicast.
> >>>>>>>
> >>>>>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC ->  MACVLAN_MODE_NOPORMISC
> >>>>>> for this patch. Then a follow on series can rework bridge
> >>>>>> and VEPA to use it as well.
> >>>>> Right. We probably need a better name if it's going to
> >>>>> affect other things besides promisc though.
> >>>>>
> >>>> how about MACVLAN_MODE_FDBFLAG?
> >>> The idea being that no one figures out what this means so
> >>> no one will make any wrong assumptions? ;)
> >>>
> >> Well its a flag to enable the FDB (forwarding database) ops
> >> and skip dev_set_promisc() on passthru mode. Any better ideas?
> >> Maybe MACVLAN_MODE_FDBENABLE or MACVLAN_MODE_MANAGE_FDB?
> > Do we need to introduce another mode? I think this patch is enabling passthru mode without the need
> > to put the underlying device in promiscuous mode. So basically we can consider this patch as
> > an optimization.
> > 
> > Thanks
> > Sridhar
> > 
> 
> Sridhar, Michael,
> 
> After thinking about this a bit I would propose keeping this
> patch as is. Or if we prefer I can make this a flag but I don't
> think that helps much. passthru mode is the only macvlan mode
> that calls dev_set_promiscuity(). Either way I think this mode
> or flag should _only_ toggle the call to dev_set_promiscuity().
> 
> Setting multicast dev->flag IFF_ALLMULTI seems to be a completely
> separate optimization that we can work with a follow up patch.
> 
> Any thoughts?
> 
> Thanks for the feedback,
> John

I agree it's a separate optimization. But if we let the
number of supported modes explode
(MACVLAN_MODE_PASSTHRU_NOPROMISC MACVLAN_MODE_PASSTHRU_NOALLMULTI
....) supporting them all and combinations thereof might become a problem.

I'm looking for an interface solution that will limit this
overhead without breaking existing setups.


One idea was to have a flag that says basically "really obey device
configuration".  Yes if we do this we can then split the support to
multiple patches and consider each individual one a bugfix, though if we
put a known broken solution in 3.5 there's a danger that someone will
come to depend on the broken behaviour. Other ideas?


-- 
MST

Re: [net-next PATCH v1 7/7] macvlan: add FDB bridge ops and new macvlan mode

[John Fastabend]

On 4/11/2012 1:02 AM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 06:42:47PM -0700, John Fastabend wrote:
>> On 4/10/2012 5:46 PM, Sridhar Samudrala wrote:
>>> On 4/10/2012 8:35 AM, John Fastabend wrote:
>>>> On 4/10/2012 8:30 AM, Michael S. Tsirkin wrote:
>>>>> On Tue, Apr 10, 2012 at 08:26:21AM -0700, John Fastabend wrote:
>>>>>> On 4/10/2012 7:35 AM, Michael S. Tsirkin wrote:
>>>>>>> On Tue, Apr 10, 2012 at 07:25:58AM -0700, John Fastabend wrote:
>>>>>>>>> Hmm okay, but this would mean we should convert
>>>>>>>>> MACVLAN_MODE_PASSTHRU_NOPROMISC to something
>>>>>>>>> that can combined with all modes. E.g.
>>>>>>>>> MACVLAN_MODE_BRIDGE | MACVLAN_MODE_FLAG_XXXXX
>>>>>>>>>
>>>>>>>>> and document that it does not promise to flood
>>>>>>>>> multicast.
>>>>>>>>>
>>>>>>>> How about changing MACVLAN_MODE_PASSTHRU_NOPROMISC ->  MACVLAN_MODE_NOPORMISC
>>>>>>>> for this patch. Then a follow on series can rework bridge
>>>>>>>> and VEPA to use it as well.
>>>>>>> Right. We probably need a better name if it's going to
>>>>>>> affect other things besides promisc though.
>>>>>>>
>>>>>> how about MACVLAN_MODE_FDBFLAG?
>>>>> The idea being that no one figures out what this means so
>>>>> no one will make any wrong assumptions? ;)
>>>>>
>>>> Well its a flag to enable the FDB (forwarding database) ops
>>>> and skip dev_set_promisc() on passthru mode. Any better ideas?
>>>> Maybe MACVLAN_MODE_FDBENABLE or MACVLAN_MODE_MANAGE_FDB?
>>> Do we need to introduce another mode? I think this patch is enabling passthru mode without the need
>>> to put the underlying device in promiscuous mode. So basically we can consider this patch as
>>> an optimization.
>>>
>>> Thanks
>>> Sridhar
>>>
>>
>> Sridhar, Michael,
>>
>> After thinking about this a bit I would propose keeping this
>> patch as is. Or if we prefer I can make this a flag but I don't
>> think that helps much. passthru mode is the only macvlan mode
>> that calls dev_set_promiscuity(). Either way I think this mode
>> or flag should _only_ toggle the call to dev_set_promiscuity().
>>
>> Setting multicast dev->flag IFF_ALLMULTI seems to be a completely
>> separate optimization that we can work with a follow up patch.
>>
>> Any thoughts?
>>
>> Thanks for the feedback,
>> John
> 
> I agree it's a separate optimization. But if we let the
> number of supported modes explode
> (MACVLAN_MODE_PASSTHRU_NOPROMISC MACVLAN_MODE_PASSTHRU_NOALLMULTI
> ....) supporting them all and combinations thereof might become a problem.
> 
> I'm looking for an interface solution that will limit this
> overhead without breaking existing setups.
> 

I'm going to respin this series today. I'll try to rework this with
a flags field so that we can set nopromisc and noallmutli using flags
and not have the supported mode space explode.

> 
> One idea was to have a flag that says basically "really obey device
> configuration".  Yes if we do this we can then split the support to
> multiple patches and consider each individual one a bugfix, though if we
> put a known broken solution in 3.5 there's a danger that someone will
> come to depend on the broken behaviour. Other ideas?
> 
> 

This sound reasonable.

Thanks,
John

Re: [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

[John Fastabend]

On 4/10/2012 8:23 PM, Ben Hutchings wrote:
> On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
> [...]
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 1f77540..05822e5 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
> [...]
>> @@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
>>   *	feature set might be less than what was returned by ndo_fix_features()).
>>   *	Must return >0 or -errno if it changed dev->features itself.
>>   *
>> + * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
>> + *		      unsigned char *addr, u16 flags)
>> + *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
>> + *	if the dev->master FDB should be updated or the devices internal FDB.
> 
> I don't think the second sentence is helpful, as rtnl_fdb_add() will
> take care of those flags.
> 
>> + * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
>> + *		      unsigned char *addr)
>> + *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
>> + *	contains flags to indicate if the dev->master FDB should be
>> + *	updated or the devices internal FDB.
> 
> Similarly here.

agreed neither seem particularly helpful I'll remove them.

> 
>> + * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
>> + *		       struct net_device *dev, int idx)
>> + *	Used to add FDB entries to dump requests. Implementers should add
>> + *	entries to skb and update idx with the number of entries.
>>   */
> [...
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
> [...]
>> +static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
>> +{
> [...]
>> +	err = -EOPNOTSUPP;
> 
> So if NTF_MASTER and NTF_SELF are both set, we can quietly fall back to
> just setting one FDB?  Not sure that's really desirable though 
> 

It makes it easier to keep an embedded agent and the sw bridge in
sync if setting both flags adds the entry to both the SW bridge and
embedded bridge. But the error case gets a bit tricky as your comments
below indicate.

>> +	/* Support fdb on master device the net/bridge default case */
>> +	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>> +	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
>> +		struct net_device *master = dev->master;
>> +
>> +		if (master->netdev_ops->ndo_fdb_add)
> 
> This operation is surely going to be mandatory for bridge devices, so
> this check should be omitted or changed to a BUG_ON().

I'll just remove the check.

> 
>> +			err = master->netdev_ops->ndo_fdb_add(ndm, dev, addr,
>> +							      nlh->nlmsg_flags);
> 
> Shoudn't we return early on error?

Agreed better to return an err here.

> 
>> +	}
>> +
>> +	/* Embedded bridge, macvlan, and any other device support */
>> +	if ((ndm->ndm_flags & NTF_SELF) &&
>> +	    dev->netdev_ops->ndo_fdb_add)
> 
> Error if !dev->netdev_ops->ndo_fdb_add.
> 
>> +		err = dev->netdev_ops->ndo_fdb_add(ndm, dev, addr,
>> +						   nlh->nlmsg_flags);
> 
> Wonder what we should do on error here if we've already successfully
> called ndo_fdb_add on the master?  Should we try to roll back the first
> addition?
> 

The problem with rolling back is the table is likely already updated and
traffic may already be being forwarded. So I think in this case the user
will have to query the device to learn what failed. It seems like the
simplest way to handle this. I think it is unwanted to have traffic being
forwarded one way for a short period of time then rolled back.

The other idea I just had is we could clear the NTF_ bit in ndm_flags after
the successful add, del command. I believe the nlmsg gets sent back to the
user on error I would need to check on this.

>> +	return err;
>> +}
>> +
>> +static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
>> +{
> [...]
>> +	err = -EOPNOTSUPP;
>> +
>> +	/* Support fdb on master device the net/bridge default case */
>> +	if ((!ndm->ndm_flags || ndm->ndm_flags & NTF_MASTER) &&
>> +	    (dev->priv_flags & IFF_BRIDGE_PORT)) {
>> +		struct net_device *master = dev->master;
>> +
>> +		if (master->netdev_ops->ndo_fdb_del)
>> +			err = master->netdev_ops->ndo_fdb_del(ndm, dev, addr);
>> +	}
>> +
>> +	/* Embedded bridge, macvlan, and any other device support */
>> +	if ((ndm->ndm_flags & NTF_SELF) &&
>> +	    dev->netdev_ops->ndo_fdb_del)
>> +		err = dev->netdev_ops->ndo_fdb_del(ndm, dev, addr);
>> +	return err;
>> +}
> [...]
> 
> This has the same issues.
> 

same comment as above

> Ben.
> 

Re: [net-next PATCH v1 2/7] net: addr_list: add exclusive dev_uc_add and dev_mc_add

[John Fastabend]

On 4/10/2012 8:33 PM, Ben Hutchings wrote:
> On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
>> This adds a dev_uc_add_excl() and dev_mc_add_excl() calls
>> similar to the original dev_{uc|mc}_add() except it sets
>> the global bit and returns -EEXIST for duplicat entires.
>>
>> This is useful for drivers that support SR-IOV, macvlan
>> devices and any other devices that need to manage the
>> unicast and multicast lists.
> [...]
>> +/**
>> + *	dev_mc_add_excl - Add a global secondary multicast address
>> + *	@dev: device
>> + *	@addr: address to add
>> + */
>> +int dev_mc_add_excl(struct net_device *dev, unsigned char *addr)
>> +{
>> +	struct netdev_hw_addr *ha;
>> +	int err;
>> +
>> +	netif_addr_lock_bh(dev);
>> +	list_for_each_entry(ha, &dev->mc.list, list) {
>> +		if (!memcmp(ha->addr, addr, dev->addr_len) &&
>> +		    ha->type == NETDEV_HW_ADDR_T_UNICAST) {
>> +			err = -EEXIST;
>> +			goto out;
>> +		}
>> +	}
>> +	err = __hw_addr_create_ex(&dev->mc, addr, dev->addr_len,
>> +				  NETDEV_HW_ADDR_T_UNICAST, true);
> [...]
> 
> The address types are wrong.  But do we even need this function yet?
> 
> Ben.
> 

macvlan wants to manage multicast addresses as well. Good catch thanks.

.John

Re: [net-next PATCH v1 3/7] net: add fdb generic dump routine

[John Fastabend]

On 4/10/2012 8:45 PM, Ben Hutchings wrote:
> On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
>> This adds a generic dump routine drivers can call. It
>> should be sufficient to handle any bridging model that
>> uses the unicast address list. This should be most SR-IOV
>> enabled NICs.
> [...]
>> +static int nlmsg_populate_fdb(struct sk_buff *skb,
>> +			      struct netlink_callback *cb,
>> +			      struct net_device *dev,
>> +			      int *idx,
>> +			      struct netdev_hw_addr_list *list)
>> +{
>> +	struct netdev_hw_addr *ha;
>> +	struct ndmsg *ndm;
>> +	struct nlmsghdr *nlh;
>> +	u32 pid, seq;
>> +
>> +	pid = NETLINK_CB(cb->skb).pid;
>> +	seq = cb->nlh->nlmsg_seq;
>> +
>> +	list_for_each_entry(ha, &list->list, list) {
>> +		if (*idx < cb->args[0])
>> +			goto skip;
>> +
>> +		nlh = nlmsg_put(skb, pid, seq,
>> +				RTM_NEWNEIGH, sizeof(*ndm), NLM_F_MULTI);
>> +		if (!nlh)
>> +			break;
> 
> This break is effectively return 0, but shouldn't we return an error?
> In practice this does no harm because any subsequent invocation of
> nlmsg_populate_fdb() for the same skb is also going to fail here with no
> change to *idx.  But it would be more obviously correct to return an
> error.
> 

sure returning -EMSGSIZE seems to be inline with rtnl_fill_ifinfo and
easier to read.

> [...]
>> +	}
>> +	return 0;
>> +nla_put_failure:
>> +	nlmsg_cancel(skb, nlh);
>> +	return -ENOMEM;
>> +}
> [...]
> 

also might be better to return -EMSGSIZE here as well. It seems to be
more inline with convention.

.John

Re: [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

[Ben Hutchings]

On Wed, 2012-04-11 at 07:45 -0700, John Fastabend wrote:
> On 4/10/2012 8:23 PM, Ben Hutchings wrote:
> > On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
> > [...]
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 1f77540..05822e5 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> > [...]
> >> @@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
> >>   *	feature set might be less than what was returned by ndo_fix_features()).
> >>   *	Must return >0 or -errno if it changed dev->features itself.
> >>   *
> >> + * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
> >> + *		      unsigned char *addr, u16 flags)
> >> + *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
> >> + *	if the dev->master FDB should be updated or the devices internal FDB.
> > 
> > I don't think the second sentence is helpful, as rtnl_fdb_add() will
> > take care of those flags.
> > 
> >> + * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
> >> + *		      unsigned char *addr)
> >> + *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
> >> + *	contains flags to indicate if the dev->master FDB should be
> >> + *	updated or the devices internal FDB.
> > 
> > Similarly here.
> 
> agreed neither seem particularly helpful I'll remove them.
> 
> > 
> >> + * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
> >> + *		       struct net_device *dev, int idx)
> >> + *	Used to add FDB entries to dump requests. Implementers should add
> >> + *	entries to skb and update idx with the number of entries.
> >>   */
> > [...
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> > [...]
> >> +static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
> >> +{
> > [...]
> >> +	err = -EOPNOTSUPP;
> > 
> > So if NTF_MASTER and NTF_SELF are both set, we can quietly fall back to
> > just setting one FDB?  Not sure that's really desirable though 
> > 
> 
> It makes it easier to keep an embedded agent and the sw bridge in
> sync if setting both flags adds the entry to both the SW bridge and
> embedded bridge.

Yes, I agree with that.

[...]
> > Wonder what we should do on error here if we've already successfully
> > called ndo_fdb_add on the master?  Should we try to roll back the first
> > addition?
> > 
> 
> The problem with rolling back is the table is likely already updated and
> traffic may already be being forwarded. So I think in this case the user
> will have to query the device to learn what failed. It seems like the
> simplest way to handle this. I think it is unwanted to have traffic being
> forwarded one way for a short period of time then rolled back.
> 
> The other idea I just had is we could clear the NTF_ bit in ndm_flags after
> the successful add, del command. I believe the nlmsg gets sent back to the
> user on error I would need to check on this.
[...]

That sounds like a good way of doing it, assuming there's no
compatibility issue.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

[John Fastabend]

On 4/11/2012 9:05 AM, Ben Hutchings wrote:
> On Wed, 2012-04-11 at 07:45 -0700, John Fastabend wrote:
>> On 4/10/2012 8:23 PM, Ben Hutchings wrote:
>>> On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
>>> [...]
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 1f77540..05822e5 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>> [...]
>>>> @@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
>>>>   *	feature set might be less than what was returned by ndo_fix_features()).
>>>>   *	Must return >0 or -errno if it changed dev->features itself.
>>>>   *
>>>> + * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
>>>> + *		      unsigned char *addr, u16 flags)
>>>> + *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
>>>> + *	if the dev->master FDB should be updated or the devices internal FDB.
>>>
>>> I don't think the second sentence is helpful, as rtnl_fdb_add() will
>>> take care of those flags.
>>>
>>>> + * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
>>>> + *		      unsigned char *addr)
>>>> + *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
>>>> + *	contains flags to indicate if the dev->master FDB should be
>>>> + *	updated or the devices internal FDB.
>>>
>>> Similarly here.
>>
>> agreed neither seem particularly helpful I'll remove them.
>>
>>>
>>>> + * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
>>>> + *		       struct net_device *dev, int idx)
>>>> + *	Used to add FDB entries to dump requests. Implementers should add
>>>> + *	entries to skb and update idx with the number of entries.
>>>>   */
>>> [...
>>>> --- a/net/core/rtnetlink.c
>>>> +++ b/net/core/rtnetlink.c
>>> [...]
>>>> +static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
>>>> +{
>>> [...]
>>>> +	err = -EOPNOTSUPP;
>>>
>>> So if NTF_MASTER and NTF_SELF are both set, we can quietly fall back to
>>> just setting one FDB?  Not sure that's really desirable though 
>>>
>>
>> It makes it easier to keep an embedded agent and the sw bridge in
>> sync if setting both flags adds the entry to both the SW bridge and
>> embedded bridge.
> 
> Yes, I agree with that.
> 
> [...]
>>> Wonder what we should do on error here if we've already successfully
>>> called ndo_fdb_add on the master?  Should we try to roll back the first
>>> addition?
>>>
>>
>> The problem with rolling back is the table is likely already updated and
>> traffic may already be being forwarded. So I think in this case the user
>> will have to query the device to learn what failed. It seems like the
>> simplest way to handle this. I think it is unwanted to have traffic being
>> forwarded one way for a short period of time then rolled back.
>>
>> The other idea I just had is we could clear the NTF_ bit in ndm_flags after
>> the successful add, del command. I believe the nlmsg gets sent back to the
>> user on error I would need to check on this.
> [...]
> 
> That sounds like a good way of doing it, assuming there's no
> compatibility issue.
> 
> Ben.
> 

I don't see any compat issues. Current applications shouldn't be setting
any flags and shouldn't be expecting any flags in the ack.

Also I think I need to add some notify hooks to help management. The
bridge code has fdb_notify() so I'll add a similar rtnl_fdb_notify() hook
here. Caught this while implementing some user space daemon code.

Thanks! v2 coming soon.