lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CB9A2187.4A989%roprabhu@cisco.com> Date: Thu, 29 Mar 2012 14:26:15 -0700 From: Roopa Prabhu <roprabhu@...co.com> To: John Fastabend <john.r.fastabend@...el.com>, "Michael S. Tsirkin" <mst@...hat.com> CC: <netdev@...r.kernel.org> Subject: Re: [RFC PATCH] macvlan: add FDB bridge ops On 3/28/12 8:58 AM, "John Fastabend" <john.r.fastabend@...el.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@...el.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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists