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

From: "Michael S. Tsirkin" <[email-masked]>
Date: Wed, 28 Mar 2012 17:52:28 +0200

On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote:
> On 3/20/12 5:26 PM, "John Fastabend" <[email-masked]> 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