[RFC PATCH] macvlan: add FDB bridge ops

[RFC PATCH] macvlan: add FDB bridge ops

[John Fastabend]

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 <roprabhu@cisco.com>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 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

[Michael S. Tsirkin]

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?

> Only lightly touch tested at this point.
> 
> CC: Roopa Prabhu <roprabhu@cisco.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  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

[Roopa Prabhu]

On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> 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 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 <roprabhu@cisco.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  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

[Michael S. Tsirkin]

On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> 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 haven't thought too hard
> > about it yet so any ideas welcome


...

> 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

[John Fastabend]

On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
>> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> 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 haven't thought too hard
>>> about it yet so any ideas welcome
> 
> 
> ...
> 
>> 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

[Roopa Prabhu]

On 3/28/12 8:58 AM, "John Fastabend" <john.r.fastabend@intel.com> wrote:

> On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote:
>> On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
>>> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> 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 haven't thought too hard
>>>> about it yet so any ideas welcome
>> 
>> 
>> ...
>> 
>>> 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

[John Fastabend]

On 3/29/2012 2:26 PM, Roopa Prabhu wrote:
> 
> 
> 
> On 3/28/12 8:58 AM, "John Fastabend" <john.r.fastabend@intel.com> wrote:
> 
>> On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
>>>> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> 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.
>>>>>

[...]

>>>
>>>
>>> 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