[RFC PATCH] macvlan: add FDB bridge ops

Subsystems: networking drivers, the rest

STALE5275d

7 messages, 3 authors, 2012-03-30 · open the first message on its own page

[RFC PATCH] macvlan: add FDB bridge ops

From: John Fastabend <hidden>
Date: 2012-03-21 00:40:20

Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.

By default the passthru mode puts the lowerdev into a
promiscuous mode to receive all packets. This behavior
is not changed by this patch. This is a bit problematic
and needs to be solved without IMHO breaking existing
mechanics. Maybe on the first add_fdb we can decrement
the promisc mode? That seems to work reasonable well and
keep existing functionality in place... but requires
an initial add to set things up which is a bit annoying
so maybe a flag is better. I haven't thought too hard
about it yet so any ideas welcome

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

Only lightly touch tested at this point.

CC: Roopa Prabhu <redacted>
Signed-off-by: John Fastabend <redacted>
---

 drivers/net/macvlan.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..86af56b 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -349,6 +349,7 @@ static int macvlan_stop(struct net_device *dev)
 		goto hash_del;
 	}
 
+	dev_uc_unsync(lowerdev, dev);
 	dev_mc_unsync(lowerdev, dev);
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(lowerdev, -1);
@@ -403,6 +404,7 @@ static void macvlan_set_multicast_list(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 +544,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(dev, addr);
+	else if (is_multicast_ether_addr(addr))
+		err = dev_mc_add(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)
 {
@@ -577,6 +616,9 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.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)

Re: [RFC PATCH] macvlan: add FDB bridge ops

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2012-03-21 08:34:11

On Tue, Mar 20, 2012 at 05:26:48PM -0700, John Fastabend wrote:
Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.

By default the passthru mode puts the lowerdev into a
promiscuous mode to receive all packets. This behavior
is not changed by this patch. This is a bit problematic
and needs to be solved without IMHO breaking existing
mechanics. Maybe on the first add_fdb we can decrement
the promisc mode? That seems to work reasonable well and
keep existing functionality in place... but requires
an initial add to set things up which is a bit annoying
so maybe a flag is better.
I think a flag is better, too.
I haven't thought too hard
about it yet so any ideas welcome

This patch is a result of Roopa Prabhu's work. Follow up
patches are needed for VEPA and VEB macvlan modes.
For bridged mode, we need to update the hash tables.
What's needed for VEPA?
quoted hunk
Only lightly touch tested at this point.

CC: Roopa Prabhu <redacted>
Signed-off-by: John Fastabend <redacted>
---

 drivers/net/macvlan.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..86af56b 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -349,6 +349,7 @@ static int macvlan_stop(struct net_device *dev)
 		goto hash_del;
 	}
 
+	dev_uc_unsync(lowerdev, dev);
 	dev_mc_unsync(lowerdev, dev);
 	if (dev->flags & IFF_ALLMULTI)
 		dev_set_allmulti(lowerdev, -1);
@@ -403,6 +404,7 @@ static void macvlan_set_multicast_list(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 +544,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(dev, addr);
+	else if (is_multicast_ether_addr(addr))
+		err = dev_mc_add(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)
 {
@@ -577,6 +616,9 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.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)

Re: [RFC PATCH] macvlan: add FDB bridge ops

From: Roopa Prabhu <hidden>
Date: 2012-03-28 15:43:59

On 3/20/12 5:26 PM, "John Fastabend" [off-list ref] wrote:
quoted hunk
Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.

By default the passthru mode puts the lowerdev into a
promiscuous mode to receive all packets. This behavior
is not changed by this patch. This is a bit problematic
and needs to be solved without IMHO breaking existing
mechanics. Maybe on the first add_fdb we can decrement
the promisc mode? That seems to work reasonable well and
keep existing functionality in place... but requires
an initial add to set things up which is a bit annoying
so maybe a flag is better. I haven't thought too hard
about it yet so any ideas welcome

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

Only lightly touch tested at this point.

CC: Roopa Prabhu <redacted>
Signed-off-by: John Fastabend <redacted>
---

 drivers/net/macvlan.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..86af56b 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -349,6 +349,7 @@ static int macvlan_stop(struct net_device *dev)
goto hash_del;
}
 
+ dev_uc_unsync(lowerdev, dev);
dev_mc_unsync(lowerdev, dev);
if (dev->flags & IFF_ALLMULTI)
dev_set_allmulti(lowerdev, -1);
@@ -403,6 +404,7 @@ static void macvlan_set_multicast_list(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 +544,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(dev, addr);
+ else if (is_multicast_ether_addr(addr))
+  err = dev_mc_add(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)
 {
@@ -577,6 +616,9 @@ static const struct net_device_ops macvlan_netdev_ops = {
.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)
Thanks John. Looks good.
I added a few things to your patch below. Yes, I think the promisc check is
required. Made an attempt to add a flag below (I did not get a chance to
think about other approaches there too). Briefly tested it with the br
command.

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..9bc70ad 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -34,6 +34,9 @@
 
 #define MACVLAN_HASH_SIZE    (1 << BITS_PER_BYTE)
 
+/* macvlan port flags */
+#define MACVLAN_FLAG_PROMISC    0x1
+
 struct macvlan_port {
     struct net_device    *dev;
     struct hlist_head    vlan_hash[MACVLAN_HASH_SIZE];
@@ -41,6 +44,7 @@ struct macvlan_port {
     struct rcu_head        rcu;
     bool             passthru;
     int            count;
+    unsigned int        flags;
 };
 
 static void macvlan_port_destroy(struct net_device *dev);
@@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev)
 
     if (vlan->port->passthru) {
         dev_set_promiscuity(lowerdev, 1);
+        vlan->port->flags |= MACVLAN_FLAG_PROMISC;
         goto hash_add;
     }
 
@@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev)
     struct net_device *lowerdev = vlan->lowerdev;
 
     if (vlan->port->passthru) {
-        dev_set_promiscuity(lowerdev, -1);
+        if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+            dev_set_promiscuity(lowerdev, -1);
+            vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+        }
         goto hash_del;
     }
 
+    dev_uc_unsync(lowerdev, dev);
     dev_mc_unsync(lowerdev, dev);
     if (dev->flags & IFF_ALLMULTI)
         dev_set_allmulti(lowerdev, -1);
@@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(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 +552,58 @@ 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+        dev_set_promiscuity (lowerdev, -1);
+        vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+    }
+
+    if (ops->ndo_fdb_add)
+        return ops->ndo_fdb_add(ndm, lowerdev, addr, flags);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_add_excl(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_add(lowerdev, 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (ops->ndo_fdb_del)
+        return ops->ndo_fdb_del(ndm, lowerdev, addr);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_del(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_del(lowerdev, addr);
+
+    return err;
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
                     struct ethtool_drvinfo *drvinfo)
 {
@@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops =
{
     .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)

Re: [RFC PATCH] macvlan: add FDB bridge ops

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2012-03-28 15:52:17

On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
On 3/20/12 5:26 PM, "John Fastabend" [off-list ref] wrote:
quoted
Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.

By default the passthru mode puts the lowerdev into a
promiscuous mode to receive all packets. This behavior
is not changed by this patch. This is a bit problematic
and needs to be solved without IMHO breaking existing
mechanics. Maybe on the first add_fdb we can decrement
the promisc mode? That seems to work reasonable well and
keep existing functionality in place... but requires
an initial add to set things up which is a bit annoying
so maybe a flag is better. I haven't thought too hard
about it yet so any ideas welcome

...
quoted hunk
Thanks John. Looks good.
I added a few things to your patch below. Yes, I think the promisc check is
required. Made an attempt to add a flag below (I did not get a chance to
think about other approaches there too). Briefly tested it with the br
command.

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..9bc70ad 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -34,6 +34,9 @@
 
 #define MACVLAN_HASH_SIZE    (1 << BITS_PER_BYTE)
 
+/* macvlan port flags */
+#define MACVLAN_FLAG_PROMISC    0x1
+
 struct macvlan_port {
     struct net_device    *dev;
     struct hlist_head    vlan_hash[MACVLAN_HASH_SIZE];
@@ -41,6 +44,7 @@ struct macvlan_port {
     struct rcu_head        rcu;
     bool             passthru;
     int            count;
+    unsigned int        flags;
 };
 
 static void macvlan_port_destroy(struct net_device *dev);
@@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev)
 
     if (vlan->port->passthru) {
         dev_set_promiscuity(lowerdev, 1);
+        vlan->port->flags |= MACVLAN_FLAG_PROMISC;
         goto hash_add;
     }
 
@@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev)
     struct net_device *lowerdev = vlan->lowerdev;
 
     if (vlan->port->passthru) {
-        dev_set_promiscuity(lowerdev, -1);
+        if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+            dev_set_promiscuity(lowerdev, -1);
+            vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+        }
         goto hash_del;
     }
 
+    dev_uc_unsync(lowerdev, dev);
     dev_mc_unsync(lowerdev, dev);
     if (dev->flags & IFF_ALLMULTI)
         dev_set_allmulti(lowerdev, -1);
@@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(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 +552,58 @@ 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+        dev_set_promiscuity (lowerdev, -1);
+        vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+    }
+
+    if (ops->ndo_fdb_add)
+        return ops->ndo_fdb_add(ndm, lowerdev, addr, flags);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_add_excl(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_add(lowerdev, 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (ops->ndo_fdb_del)
+        return ops->ndo_fdb_del(ndm, lowerdev, addr);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_del(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_del(lowerdev, addr);
+
+    return err;
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
                     struct ethtool_drvinfo *drvinfo)
 {
@@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops =
{
     .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)

So this clears the promisc on the first add which
is a bit annoying. How about a simple flag, set when
we create the macvlan?

-- 
MST

Re: [RFC PATCH] macvlan: add FDB bridge ops

From: John Fastabend <hidden>
Date: 2012-03-28 15:58:50

On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote:
On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
quoted
On 3/20/12 5:26 PM, "John Fastabend" [off-list ref] wrote:
quoted
Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.

By default the passthru mode puts the lowerdev into a
promiscuous mode to receive all packets. This behavior
is not changed by this patch. This is a bit problematic
and needs to be solved without IMHO breaking existing
mechanics. Maybe on the first add_fdb we can decrement
the promisc mode? That seems to work reasonable well and
keep existing functionality in place... but requires
an initial add to set things up which is a bit annoying
so maybe a flag is better. I haven't thought too hard
about it yet so any ideas welcome

...
quoted
Thanks John. Looks good.
I added a few things to your patch below. Yes, I think the promisc check is
required. Made an attempt to add a flag below (I did not get a chance to
think about other approaches there too). Briefly tested it with the br
command.

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..9bc70ad 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -34,6 +34,9 @@
 
 #define MACVLAN_HASH_SIZE    (1 << BITS_PER_BYTE)
 
+/* macvlan port flags */
+#define MACVLAN_FLAG_PROMISC    0x1
+
 struct macvlan_port {
     struct net_device    *dev;
     struct hlist_head    vlan_hash[MACVLAN_HASH_SIZE];
@@ -41,6 +44,7 @@ struct macvlan_port {
     struct rcu_head        rcu;
     bool             passthru;
     int            count;
+    unsigned int        flags;
 };
 
 static void macvlan_port_destroy(struct net_device *dev);
@@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev)
 
     if (vlan->port->passthru) {
         dev_set_promiscuity(lowerdev, 1);
+        vlan->port->flags |= MACVLAN_FLAG_PROMISC;
         goto hash_add;
     }
 
@@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev)
     struct net_device *lowerdev = vlan->lowerdev;
 
     if (vlan->port->passthru) {
-        dev_set_promiscuity(lowerdev, -1);
+        if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+            dev_set_promiscuity(lowerdev, -1);
+            vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+        }
         goto hash_del;
     }
 
+    dev_uc_unsync(lowerdev, dev);
     dev_mc_unsync(lowerdev, dev);
     if (dev->flags & IFF_ALLMULTI)
         dev_set_allmulti(lowerdev, -1);
@@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(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 +552,58 @@ 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+        dev_set_promiscuity (lowerdev, -1);
+        vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+    }
+
+    if (ops->ndo_fdb_add)
+        return ops->ndo_fdb_add(ndm, lowerdev, addr, flags);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_add_excl(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_add(lowerdev, 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (ops->ndo_fdb_del)
+        return ops->ndo_fdb_del(ndm, lowerdev, addr);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_del(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_del(lowerdev, addr);
+
+    return err;
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
                     struct ethtool_drvinfo *drvinfo)
 {
@@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops =
{
     .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)

So this clears the promisc on the first add which
is a bit annoying. How about a simple flag, set when
we create the macvlan?
Agreed. This probably needs a new attrib maybe IFLA_MACVLAN_FLAGS unless
there already exists a per "kind" (rtnl_link_ops) flags field we can use.
I scanned the code briefly and didn't see any such thing so likely we need
the new attribute.

.John

Re: [RFC PATCH] macvlan: add FDB bridge ops

From: Roopa Prabhu <hidden>
Date: 2012-03-29 21:26:22



On 3/28/12 8:58 AM, "John Fastabend" [off-list ref] wrote:
On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote:
quoted
On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
quoted
On 3/20/12 5:26 PM, "John Fastabend" [off-list ref] wrote:
quoted
Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.

By default the passthru mode puts the lowerdev into a
promiscuous mode to receive all packets. This behavior
is not changed by this patch. This is a bit problematic
and needs to be solved without IMHO breaking existing
mechanics. Maybe on the first add_fdb we can decrement
the promisc mode? That seems to work reasonable well and
keep existing functionality in place... but requires
an initial add to set things up which is a bit annoying
so maybe a flag is better. I haven't thought too hard
about it yet so any ideas welcome

...
quoted
Thanks John. Looks good.
I added a few things to your patch below. Yes, I think the promisc check is
required. Made an attempt to add a flag below (I did not get a chance to
think about other approaches there too). Briefly tested it with the br
command.

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..9bc70ad 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -34,6 +34,9 @@
 
 #define MACVLAN_HASH_SIZE    (1 << BITS_PER_BYTE)
 
+/* macvlan port flags */
+#define MACVLAN_FLAG_PROMISC    0x1
+
 struct macvlan_port {
     struct net_device    *dev;
     struct hlist_head    vlan_hash[MACVLAN_HASH_SIZE];
@@ -41,6 +44,7 @@ struct macvlan_port {
     struct rcu_head        rcu;
     bool             passthru;
     int            count;
+    unsigned int        flags;
 };
 
 static void macvlan_port_destroy(struct net_device *dev);
@@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev)
 
     if (vlan->port->passthru) {
         dev_set_promiscuity(lowerdev, 1);
+        vlan->port->flags |= MACVLAN_FLAG_PROMISC;
         goto hash_add;
     }
 
@@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev)
     struct net_device *lowerdev = vlan->lowerdev;
 
     if (vlan->port->passthru) {
-        dev_set_promiscuity(lowerdev, -1);
+        if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+            dev_set_promiscuity(lowerdev, -1);
+            vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+        }
         goto hash_del;
     }
 
+    dev_uc_unsync(lowerdev, dev);
     dev_mc_unsync(lowerdev, dev);
     if (dev->flags & IFF_ALLMULTI)
         dev_set_allmulti(lowerdev, -1);
@@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(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 +552,58 @@ 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (vlan->port->flags & MACVLAN_FLAG_PROMISC) {
+        dev_set_promiscuity (lowerdev, -1);
+        vlan->port->flags &= ~MACVLAN_FLAG_PROMISC;
+    }
+
+    if (ops->ndo_fdb_add)
+        return ops->ndo_fdb_add(ndm, lowerdev, addr, flags);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_add_excl(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_add(lowerdev, 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);
+    struct net_device *lowerdev = vlan->lowerdev;
+    const struct net_device_ops *ops = lowerdev->netdev_ops;
+    int err = -EINVAL;
+
+    if (!vlan->port->passthru)
+        return -EOPNOTSUPP;
+
+    if (ops->ndo_fdb_del)
+        return ops->ndo_fdb_del(ndm, lowerdev, addr);
+
+    if (is_unicast_ether_addr(addr))
+        err = dev_uc_del(lowerdev, addr);
+    else if (is_multicast_ether_addr(addr))
+        err = dev_mc_del(lowerdev, addr);
+
+    return err;
+}
+
 static void macvlan_ethtool_get_drvinfo(struct net_device *dev,
                     struct ethtool_drvinfo *drvinfo)
 {
@@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops =
{
     .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)

So this clears the promisc on the first add which
is a bit annoying. How about a simple flag, set when
we create the macvlan?
Agreed. This probably needs a new attrib maybe IFLA_MACVLAN_FLAGS unless
there already exists a per "kind" (rtnl_link_ops) flags field we can use.
I scanned the code briefly and didn't see any such thing so likely we need
the new attribute.
O ok. That makes sense. Unfortunately I am not sure if I will be able to get
back to this anytime in the near future. If anyone wants to rework this
patch please do.

Thanks.
Roopa
 

Re: [RFC PATCH] macvlan: add FDB bridge ops

From: John Fastabend <hidden>
Date: 2012-03-30 01:06:26

On 3/29/2012 2:26 PM, Roopa Prabhu wrote:


On 3/28/12 8:58 AM, "John Fastabend" [off-list ref] wrote:
quoted
On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote:
quoted
On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
quoted
On 3/20/12 5:26 PM, "John Fastabend" [off-list ref] wrote:
quoted
Add support to add/del and dump the forwarding database
for macvlan passthru mode. The macvlan driver acts like
a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru
case so adding forwarding rules is just adding the addr
to the uc or mc lists.
[...]
quoted
quoted

So this clears the promisc on the first add which
is a bit annoying. How about a simple flag, set when
we create the macvlan?
Agreed. This probably needs a new attrib maybe IFLA_MACVLAN_FLAGS unless
there already exists a per "kind" (rtnl_link_ops) flags field we can use.
I scanned the code briefly and didn't see any such thing so likely we need
the new attribute.
O ok. That makes sense. Unfortunately I am not sure if I will be able to get
back to this anytime in the near future. If anyone wants to rework this
patch please do.

Thanks.
Roopa
 
OK I'll fix it up and submit it with the rest of the FDB
patches.

.John
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help