[RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Vlad Yasevich]

This series of patches provides an ability to add VLAN IDs to the bridge
ports.  This is similar to what can be found in most switches.  The bridge
port may have any number of VLANs added to it including vlan 0 for untagged
traffic.  When vlans are added to the port, only traffic tagged with particular
vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
entries and become part of the lookup.  This way we correctly identify the FDB
entry.

There are still pieces missing.  I don't yet support adding a static fdb entry
with a particular vlan.  There is no netlink support for carrying a vlan id.

I'd like to hear thoughts of whether this is usufull and something we should
persue.

The default behavior ofthe bridge is unchanged if no vlans have been
configured.

Vlad Yasevich (5):
  bridge: Add vlan check to forwarding path
  bridge: Add vlan to unicast fdb entries
  bridge: Add vlan id to multicast groups
  bridge: Add private ioctls to configure vlans on bridge ports
  bridge: Add sysfs interface to display VLANS

 include/linux/if_bridge.h |    4 ++
 net/bridge/br_device.c    |    2 +-
 net/bridge/br_fdb.c       |   71 ++++++++++++++++++-------------
 net/bridge/br_forward.c   |   17 +++++++-
 net/bridge/br_if.c        |  103 +++++++++++++++++++++++++++++++++++++++++++++
 net/bridge/br_input.c     |   18 +++++++-
 net/bridge/br_ioctl.c     |   31 +++++++++++++
 net/bridge/br_multicast.c |   64 +++++++++++++++++++---------
 net/bridge/br_private.h   |   14 +++++-
 net/bridge/br_sysfs_if.c  |   28 ++++++++++++
 10 files changed, 296 insertions(+), 56 deletions(-)

-- 
1.7.7.6

[RFC PATCH bridge 1/5] bridge: Add vlan check to forwarding path

[Vlad Yasevich]

When forwarding packets make sure vlan matches any configured vlan for
the port.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_forward.c |   17 ++++++++++++++++-
 net/bridge/br_input.c   |    8 ++++++++
 net/bridge/br_private.h |    2 ++
 3 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index e9466d4..ab81954 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -26,11 +26,26 @@ static int deliver_clone(const struct net_bridge_port *prev,
 			 void (*__packet_hook)(const struct net_bridge_port *p,
 					       struct sk_buff *skb));
 
-/* Don't forward packets to originating port or forwarding diasabled */
+/* check to see that the vlan is allowed to be forwarded on this interface */
+static inline int vlan_match(const struct net_bridge_port *p,
+			     const struct sk_buff *skb)
+{
+	unsigned long *vlan_map = rcu_dereference(p->vlan_map);
+	unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
+
+	/* The map keeps the vlans off by 1 so adjust for that */
+	return (vlan_map && vlan_tx_tag_present(skb) &&
+		test_bit(vid+1, vlan_map));
+}
+
+/* Don't forward packets to originating port or forwarding diasabled.
+ */
 static inline int should_deliver(const struct net_bridge_port *p,
 				 const struct sk_buff *skb)
 {
+
 	return (((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
+		vlan_match(p, skb) &&
 		p->state == BR_STATE_FORWARDING);
 }
 
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 76f15fd..b7ca3b3 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -53,10 +53,18 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	struct net_bridge_fdb_entry *dst;
 	struct net_bridge_mdb_entry *mdst;
 	struct sk_buff *skb2;
+	unsigned long *vlan_map;
 
 	if (!p || p->state == BR_STATE_DISABLED)
 		goto drop;
 
+	/* If VLAN filter is configured on the port, make sure we accept
+	 * only traffic matching the VLAN filter.
+	 */
+	vlan_map = rcu_dereference(p->vlan_map);
+	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
+		goto drop;
+
 	/* insert into forwarding database after filtering to avoid spoofing */
 	br = p->br;
 	br_fdb_update(br, p, eth_hdr(skb)->h_source);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a768b24..8da90e8 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -152,6 +152,8 @@ struct net_bridge_port
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	struct netpoll			*np;
 #endif
+	unsigned long __rcu		*vlan_map;
+
 };
 
 #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
-- 
1.7.7.6

Re: [RFC PATCH bridge 1/5] bridge: Add vlan check to forwarding path

[Nicolas de Pesloüan]

Le 23/08/2012 21:29, Vlad Yasevich a écrit :
> -/* Don't forward packets to originating port or forwarding diasabled */
> +/* Don't forward packets to originating port or forwarding diasabled.

While you work on this, feel free to fix the typo in disabled ^^^^^^^^^

	Nicolas.

Re: [RFC PATCH bridge 1/5] bridge: Add vlan check to forwarding path

[Michael S. Tsirkin]

On Thu, Aug 23, 2012 at 03:29:51PM -0400, Vlad Yasevich wrote:
> When forwarding packets make sure vlan matches any configured vlan for
> the port.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  net/bridge/br_forward.c |   17 ++++++++++++++++-
>  net/bridge/br_input.c   |    8 ++++++++
>  net/bridge/br_private.h |    2 ++
>  3 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index e9466d4..ab81954 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -26,11 +26,26 @@ static int deliver_clone(const struct net_bridge_port *prev,
>  			 void (*__packet_hook)(const struct net_bridge_port *p,
>  					       struct sk_buff *skb));
>  
> -/* Don't forward packets to originating port or forwarding diasabled */
> +/* check to see that the vlan is allowed to be forwarded on this interface */
> +static inline int vlan_match(const struct net_bridge_port *p,
> +			     const struct sk_buff *skb)
> +{
> +	unsigned long *vlan_map = rcu_dereference(p->vlan_map);
> +	unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
> +
> +	/* The map keeps the vlans off by 1 so adjust for that */
> +	return (vlan_map && vlan_tx_tag_present(skb) &&
> +		test_bit(vid+1, vlan_map));

It looks better if you put spaces around +,* etc:
vid + 1 This applies to all patches and many places so I am not
noting it everywhere - could you fnd such instances and fix up?

Also, this off by 1 map logic is all over this patch,
one hopes we did not forget is somewhere.
Would be better to have a wrapper.

Also do not put return value in () please - it gets us nothing
at all. Again applies everywhere.

> +}
> +
> +/* Don't forward packets to originating port or forwarding diasabled.
> + */
>  static inline int should_deliver(const struct net_bridge_port *p,
>  				 const struct sk_buff *skb)
>  {
> +

extra empty line - no needed here.

>  	return (((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
> +		vlan_match(p, skb) &&
>  		p->state == BR_STATE_FORWARDING);
>  }
>  
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 76f15fd..b7ca3b3 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -53,10 +53,18 @@ int br_handle_frame_finish(struct sk_buff *skb)
>  	struct net_bridge_fdb_entry *dst;
>  	struct net_bridge_mdb_entry *mdst;
>  	struct sk_buff *skb2;
> +	unsigned long *vlan_map;
>  
>  	if (!p || p->state == BR_STATE_DISABLED)
>  		goto drop;
>  
> +	/* If VLAN filter is configured on the port, make sure we accept
> +	 * only traffic matching the VLAN filter.
> +	 */
> +	vlan_map = rcu_dereference(p->vlan_map);
> +	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))

() not needed here around parameters.

> +		goto drop;
> +
>  	/* insert into forwarding database after filtering to avoid spoofing */
>  	br = p->br;
>  	br_fdb_update(br, p, eth_hdr(skb)->h_source);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index a768b24..8da90e8 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -152,6 +152,8 @@ struct net_bridge_port
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	struct netpoll			*np;
>  #endif
> +	unsigned long __rcu		*vlan_map;
> +

Empty line not needed.

>  };
>  
>  #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT)
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Vlad Yasevich]

This patch adds vlan to unicast fdb entries that are created for
learned addresses (not the manually configured ones).  It adds
vlan id into the hash mix and uses vlan as an addditional parameter
for an entry match.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/if_bridge.h |    1 +
 net/bridge/br_device.c    |    2 +-
 net/bridge/br_fdb.c       |   71 ++++++++++++++++++++++++++------------------
 net/bridge/br_input.c     |   14 ++++++---
 net/bridge/br_private.h   |    7 +++-
 5 files changed, 58 insertions(+), 37 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index dd3f201..288ff10 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -95,6 +95,7 @@ struct __fdb_entry {
 	__u8 port_hi;
 	__u8 pad0;
 	__u16 unused;
+#define fdb_vid unused
 };
 
 #ifdef __KERNEL__
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3334845..e321b9d 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -66,7 +66,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			br_multicast_deliver(mdst, skb);
 		else
 			br_flood_deliver(br, skb);
-	} else if ((dst = __br_fdb_get(br, dest)) != NULL)
+	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) != NULL)
 		br_deliver(dst->dst, skb);
 	else
 		br_flood_deliver(br, skb);
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d21f323..478ca1d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/atomic.h>
 #include <asm/unaligned.h>
+#include <linux/if_vlan.h>
 #include "br_private.h"
 
 static struct kmem_cache *br_fdb_cache __read_mostly;
@@ -67,11 +68,12 @@ static inline int has_expired(const struct net_bridge *br,
 		time_before_eq(fdb->updated + hold_time(br), jiffies);
 }
 
-static inline int br_mac_hash(const unsigned char *mac)
+static inline int br_mac_hash(const unsigned char *mac, __u16 vlan_tci)
 {
-	/* use 1 byte of OUI cnd 3 bytes of NIC */
+	/* use 1 byte of OUI and 3 bytes of NIC */
 	u32 key = get_unaligned((u32 *)(mac + 2));
-	return jhash_1word(key, fdb_salt) & (BR_HASH_SIZE - 1);
+	return jhash_2words(key, (vlan_tci & VLAN_VID_MASK),
+				fdb_salt) & (BR_HASH_SIZE - 1);
 }
 
 static void fdb_rcu_free(struct rcu_head *head)
@@ -132,7 +134,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 	struct net_bridge_fdb_entry *f;
 
 	/* If old entry was unassociated with any port, then delete it. */
-	f = __br_fdb_get(br, br->dev->dev_addr);
+	f = __br_fdb_get(br, br->dev->dev_addr, 0);
 	if (f && f->is_local && !f->dst)
 		fdb_delete(br, f);
 
@@ -231,13 +233,16 @@ void br_fdb_delete_by_port(struct net_bridge *br,
 
 /* No locking or refcounting, assumes caller has rcu_read_lock */
 struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
-					  const unsigned char *addr)
+					  const unsigned char *addr,
+					  __u16 vlan_tci)
 {
 	struct hlist_node *h;
 	struct net_bridge_fdb_entry *fdb;
 
-	hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
-		if (ether_addr_equal(fdb->addr.addr, addr)) {
+	hlist_for_each_entry_rcu(fdb, h,
+				&br->hash[br_mac_hash(addr, vlan_tci)], hlist) {
+		if (ether_addr_equal(fdb->addr.addr, addr) &&
+		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK) ) {
 			if (unlikely(has_expired(br, fdb)))
 				break;
 			return fdb;
@@ -261,7 +266,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
 	if (!port)
 		ret = 0;
 	else {
-		fdb = __br_fdb_get(port->br, addr);
+		fdb = __br_fdb_get(port->br, addr, 0);
 		ret = fdb && fdb->dst && fdb->dst->dev != dev &&
 			fdb->dst->state == BR_STATE_FORWARDING;
 	}
@@ -313,6 +318,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
 			fe->is_local = f->is_local;
 			if (!f->is_static)
 				fe->ageing_timer_value = jiffies_to_clock_t(jiffies - f->updated);
+			fe->fdb_vid = f->vlan_id;
 			++fe;
 			++num;
 		}
@@ -325,26 +331,30 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
 }
 
 static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
-					     const unsigned char *addr)
+					     const unsigned char *addr,
+					     __u16 vlan_tci)
 {
 	struct hlist_node *h;
 	struct net_bridge_fdb_entry *fdb;
 
 	hlist_for_each_entry(fdb, h, head, hlist) {
-		if (ether_addr_equal(fdb->addr.addr, addr))
+		if (ether_addr_equal(fdb->addr.addr, addr) &&
+		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
 			return fdb;
 	}
 	return NULL;
 }
 
 static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
-						 const unsigned char *addr)
+						 const unsigned char *addr,
+						 __u16 vlan_tci)
 {
 	struct hlist_node *h;
 	struct net_bridge_fdb_entry *fdb;
 
 	hlist_for_each_entry_rcu(fdb, h, head, hlist) {
-		if (ether_addr_equal(fdb->addr.addr, addr))
+		if (ether_addr_equal(fdb->addr.addr, addr) &&
+		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
 			return fdb;
 	}
 	return NULL;
@@ -352,7 +362,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
 
 static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
 					       struct net_bridge_port *source,
-					       const unsigned char *addr)
+					       const unsigned char *addr,
+					       __u16 vlan_tci)
 {
 	struct net_bridge_fdb_entry *fdb;
 
@@ -360,6 +371,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
 	if (fdb) {
 		memcpy(fdb->addr.addr, addr, ETH_ALEN);
 		fdb->dst = source;
+		fdb->vlan_id = (vlan_tci & VLAN_VID_MASK);
 		fdb->is_local = 0;
 		fdb->is_static = 0;
 		fdb->updated = fdb->used = jiffies;
@@ -371,13 +383,13 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
 static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		  const unsigned char *addr)
 {
-	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
 	struct net_bridge_fdb_entry *fdb;
 
 	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 
-	fdb = fdb_find(head, addr);
+	fdb = fdb_find(head, addr, 0);
 	if (fdb) {
 		/* it is okay to have multiple ports with same
 		 * address, just use the first one.
@@ -390,7 +402,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 		fdb_delete(br, fdb);
 	}
 
-	fdb = fdb_create(head, source, addr);
+	fdb = fdb_create(head, source, addr, 0);
 	if (!fdb)
 		return -ENOMEM;
 
@@ -412,9 +424,9 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
 }
 
 void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
-		   const unsigned char *addr)
+		   const unsigned char *addr, __u16 vlan_tci)
 {
-	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
 	struct net_bridge_fdb_entry *fdb;
 
 	/* some users want to always flood. */
@@ -426,7 +438,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 	      source->state == BR_STATE_FORWARDING))
 		return;
 
-	fdb = fdb_find_rcu(head, addr);
+	fdb = fdb_find_rcu(head, addr, vlan_tci);
 	if (likely(fdb)) {
 		/* attempt to update an entry for a local interface */
 		if (unlikely(fdb->is_local)) {
@@ -441,8 +453,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
 		}
 	} else {
 		spin_lock(&br->hash_lock);
-		if (likely(!fdb_find(head, addr))) {
-			fdb = fdb_create(head, source, addr);
+		if (likely(!fdb_find(head, addr, vlan_tci))) {
+			fdb = fdb_create(head, source, addr, vlan_tci);
 			if (fdb)
 				fdb_notify(br, fdb, RTM_NEWNEIGH);
 		}
@@ -571,18 +583,18 @@ out:
 
 /* Update (create or replace) forwarding database entry */
 static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
-			 __u16 state, __u16 flags)
+			 __u16 state, __u16 flags, __u16 vlan_tci)
 {
 	struct net_bridge *br = source->br;
-	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
 	struct net_bridge_fdb_entry *fdb;
 
-	fdb = fdb_find(head, addr);
+	fdb = fdb_find(head, addr, vlan_tci);
 	if (fdb == NULL) {
 		if (!(flags & NLM_F_CREATE))
 			return -ENOENT;
 
-		fdb = fdb_create(head, source, addr);
+		fdb = fdb_create(head, source, addr, vlan_tci);
 		if (!fdb)
 			return -ENOMEM;
 		fdb_notify(br, fdb, RTM_NEWNEIGH);
@@ -628,11 +640,12 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
 
 	if (ndm->ndm_flags & NTF_USE) {
 		rcu_read_lock();
-		br_fdb_update(p->br, p, addr);
+		br_fdb_update(p->br, p, addr, 0);
 		rcu_read_unlock();
 	} else {
 		spin_lock_bh(&p->br->hash_lock);
-		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
+		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags,
+				0);
 		spin_unlock_bh(&p->br->hash_lock);
 	}
 
@@ -642,10 +655,10 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
 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)];
+	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
 	struct net_bridge_fdb_entry *fdb;
 
-	fdb = fdb_find(head, addr);
+	fdb = fdb_find(head, addr, 0);
 	if (!fdb)
 		return -ENOENT;
 
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index b7ca3b3..a4579ee 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -62,12 +62,15 @@ int br_handle_frame_finish(struct sk_buff *skb)
 	 * only traffic matching the VLAN filter.
 	 */
 	vlan_map = rcu_dereference(p->vlan_map);
-	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
-		goto drop;
+	if (vlan_map && vlan_tx_tag_present(skb)) {
+		unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
+		if (!test_bit(vid+1, vlan_map))
+			goto drop;
+	}
 
 	/* insert into forwarding database after filtering to avoid spoofing */
 	br = p->br;
-	br_fdb_update(br, p, eth_hdr(skb)->h_source);
+	br_fdb_update(br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
 
 	if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
 	    br_multicast_rcv(br, p, skb))
@@ -102,7 +105,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
 			skb2 = skb;
 
 		br->dev->stats.multicast++;
-	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
+	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) &&
+			dst->is_local) {
 		skb2 = skb;
 		/* Do not forward the packet since it's local. */
 		skb = NULL;
@@ -131,7 +135,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
 {
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
 
-	br_fdb_update(p->br, p, eth_hdr(skb)->h_source);
+	br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
 	return 0;	 /* process further */
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 8da90e8..921b927 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -72,6 +72,7 @@ struct net_bridge_fdb_entry
 	unsigned long			updated;
 	unsigned long			used;
 	mac_addr			addr;
+	__u16				vlan_id;
 	unsigned char			is_local;
 	unsigned char			is_static;
 };
@@ -352,7 +353,8 @@ extern void br_fdb_cleanup(unsigned long arg);
 extern void br_fdb_delete_by_port(struct net_bridge *br,
 				  const struct net_bridge_port *p, int do_all);
 extern struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
-						 const unsigned char *addr);
+						 const unsigned char *addr,
+						 __u16 vlan_tci);
 extern int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
 extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
 			  unsigned long count, unsigned long off);
@@ -361,7 +363,8 @@ extern int br_fdb_insert(struct net_bridge *br,
 			 const unsigned char *addr);
 extern void br_fdb_update(struct net_bridge *br,
 			  struct net_bridge_port *source,
-			  const unsigned char *addr);
+			  const unsigned char *addr,
+			  __u16 vlan_tci);
 
 extern int br_fdb_delete(struct ndmsg *ndm,
 			 struct net_device *dev,
-- 
1.7.7.6

Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Stephen Hemminger]

On Thu, 23 Aug 2012 15:29:52 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index dd3f201..288ff10 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -95,6 +95,7 @@ struct __fdb_entry {
>  	__u8 port_hi;
>  	__u8 pad0;
>  	__u16 unused;
> +#define fdb_vid unused

That is seriously ugly. Just change the structure definition
to use the value.

Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Vlad Yasevich]

On 08/23/2012 03:39 PM, Stephen Hemminger wrote:
> On Thu, 23 Aug 2012 15:29:52 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index dd3f201..288ff10 100644
>> --- a/include/linux/if_bridge.h
>> +++ b/include/linux/if_bridge.h
>> @@ -95,6 +95,7 @@ struct __fdb_entry {
>>   	__u8 port_hi;
>>   	__u8 pad0;
>>   	__u16 unused;
>> +#define fdb_vid unused
>
> That is seriously ugly. Just change the structure definition
> to use the value.

Ok.  I did that originally, but that made it hard to detect in 
userspace.  I'll think of something.

-vlad

Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Michael S. Tsirkin]

On Thu, Aug 23, 2012 at 03:29:52PM -0400, Vlad Yasevich wrote:
> This patch adds vlan to unicast fdb entries that are created for
> learned addresses (not the manually configured ones).  It adds
> vlan id into the hash mix and uses vlan as an addditional parameter
> for an entry match.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

I wonder whether this conflicts with Eric's
05423b241311c9380b7280179295bac7794281b6
which allowed null vid.

Specifically this calls vlan_tx_tag_get without
checking vlan_tx_tag_present. Why is this OK?

> ---
>  include/linux/if_bridge.h |    1 +
>  net/bridge/br_device.c    |    2 +-
>  net/bridge/br_fdb.c       |   71 ++++++++++++++++++++++++++------------------
>  net/bridge/br_input.c     |   14 ++++++---
>  net/bridge/br_private.h   |    7 +++-
>  5 files changed, 58 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index dd3f201..288ff10 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -95,6 +95,7 @@ struct __fdb_entry {
>  	__u8 port_hi;
>  	__u8 pad0;
>  	__u16 unused;
> +#define fdb_vid unused
>  };
>  
>  #ifdef __KERNEL__
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 3334845..e321b9d 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -66,7 +66,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  			br_multicast_deliver(mdst, skb);
>  		else
>  			br_flood_deliver(br, skb);
> -	} else if ((dst = __br_fdb_get(br, dest)) != NULL)
> +	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) != NULL)
>  		br_deliver(dst->dst, skb);
>  	else
>  		br_flood_deliver(br, skb);
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index d21f323..478ca1d 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -23,6 +23,7 @@
>  #include <linux/slab.h>
>  #include <linux/atomic.h>
>  #include <asm/unaligned.h>
> +#include <linux/if_vlan.h>
>  #include "br_private.h"
>  
>  static struct kmem_cache *br_fdb_cache __read_mostly;
> @@ -67,11 +68,12 @@ static inline int has_expired(const struct net_bridge *br,
>  		time_before_eq(fdb->updated + hold_time(br), jiffies);
>  }
>  
> -static inline int br_mac_hash(const unsigned char *mac)
> +static inline int br_mac_hash(const unsigned char *mac, __u16 vlan_tci)
>  {
> -	/* use 1 byte of OUI cnd 3 bytes of NIC */
> +	/* use 1 byte of OUI and 3 bytes of NIC */
>  	u32 key = get_unaligned((u32 *)(mac + 2));
> -	return jhash_1word(key, fdb_salt) & (BR_HASH_SIZE - 1);
> +	return jhash_2words(key, (vlan_tci & VLAN_VID_MASK),
> +				fdb_salt) & (BR_HASH_SIZE - 1);
>  }
>  
>  static void fdb_rcu_free(struct rcu_head *head)
> @@ -132,7 +134,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  	struct net_bridge_fdb_entry *f;
>  
>  	/* If old entry was unassociated with any port, then delete it. */
> -	f = __br_fdb_get(br, br->dev->dev_addr);
> +	f = __br_fdb_get(br, br->dev->dev_addr, 0);
>  	if (f && f->is_local && !f->dst)
>  		fdb_delete(br, f);
>  
> @@ -231,13 +233,16 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>  
>  /* No locking or refcounting, assumes caller has rcu_read_lock */
>  struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
> -					  const unsigned char *addr)
> +					  const unsigned char *addr,
> +					  __u16 vlan_tci)
>  {
>  	struct hlist_node *h;
>  	struct net_bridge_fdb_entry *fdb;
>  
> -	hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
> -		if (ether_addr_equal(fdb->addr.addr, addr)) {
> +	hlist_for_each_entry_rcu(fdb, h,
> +				&br->hash[br_mac_hash(addr, vlan_tci)], hlist) {
> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK) ) {
>  			if (unlikely(has_expired(br, fdb)))
>  				break;
>  			return fdb;
> @@ -261,7 +266,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
>  	if (!port)
>  		ret = 0;
>  	else {
> -		fdb = __br_fdb_get(port->br, addr);
> +		fdb = __br_fdb_get(port->br, addr, 0);
>  		ret = fdb && fdb->dst && fdb->dst->dev != dev &&
>  			fdb->dst->state == BR_STATE_FORWARDING;
>  	}
> @@ -313,6 +318,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>  			fe->is_local = f->is_local;
>  			if (!f->is_static)
>  				fe->ageing_timer_value = jiffies_to_clock_t(jiffies - f->updated);
> +			fe->fdb_vid = f->vlan_id;
>  			++fe;
>  			++num;
>  		}
> @@ -325,26 +331,30 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>  }
>  
>  static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
> -					     const unsigned char *addr)
> +					     const unsigned char *addr,
> +					     __u16 vlan_tci)
>  {
>  	struct hlist_node *h;
>  	struct net_bridge_fdb_entry *fdb;
>  
>  	hlist_for_each_entry(fdb, h, head, hlist) {
> -		if (ether_addr_equal(fdb->addr.addr, addr))
> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
>  			return fdb;
>  	}
>  	return NULL;
>  }
>  
>  static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
> -						 const unsigned char *addr)
> +						 const unsigned char *addr,
> +						 __u16 vlan_tci)
>  {
>  	struct hlist_node *h;
>  	struct net_bridge_fdb_entry *fdb;
>  
>  	hlist_for_each_entry_rcu(fdb, h, head, hlist) {
> -		if (ether_addr_equal(fdb->addr.addr, addr))
> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
>  			return fdb;
>  	}
>  	return NULL;
> @@ -352,7 +362,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
>  
>  static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  					       struct net_bridge_port *source,
> -					       const unsigned char *addr)
> +					       const unsigned char *addr,
> +					       __u16 vlan_tci)
>  {
>  	struct net_bridge_fdb_entry *fdb;
>  
> @@ -360,6 +371,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  	if (fdb) {
>  		memcpy(fdb->addr.addr, addr, ETH_ALEN);
>  		fdb->dst = source;
> +		fdb->vlan_id = (vlan_tci & VLAN_VID_MASK);
>  		fdb->is_local = 0;
>  		fdb->is_static = 0;
>  		fdb->updated = fdb->used = jiffies;
> @@ -371,13 +383,13 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  		  const unsigned char *addr)
>  {
> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
>  	struct net_bridge_fdb_entry *fdb;
>  
>  	if (!is_valid_ether_addr(addr))
>  		return -EINVAL;
>  
> -	fdb = fdb_find(head, addr);
> +	fdb = fdb_find(head, addr, 0);
>  	if (fdb) {
>  		/* it is okay to have multiple ports with same
>  		 * address, just use the first one.
> @@ -390,7 +402,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  		fdb_delete(br, fdb);
>  	}
>  
> -	fdb = fdb_create(head, source, addr);
> +	fdb = fdb_create(head, source, addr, 0);
>  	if (!fdb)
>  		return -ENOMEM;
>  
> @@ -412,9 +424,9 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>  }
>  
>  void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> -		   const unsigned char *addr)
> +		   const unsigned char *addr, __u16 vlan_tci)
>  {
> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
>  	struct net_bridge_fdb_entry *fdb;
>  
>  	/* some users want to always flood. */
> @@ -426,7 +438,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>  	      source->state == BR_STATE_FORWARDING))
>  		return;
>  
> -	fdb = fdb_find_rcu(head, addr);
> +	fdb = fdb_find_rcu(head, addr, vlan_tci);
>  	if (likely(fdb)) {
>  		/* attempt to update an entry for a local interface */
>  		if (unlikely(fdb->is_local)) {
> @@ -441,8 +453,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>  		}
>  	} else {
>  		spin_lock(&br->hash_lock);
> -		if (likely(!fdb_find(head, addr))) {
> -			fdb = fdb_create(head, source, addr);
> +		if (likely(!fdb_find(head, addr, vlan_tci))) {
> +			fdb = fdb_create(head, source, addr, vlan_tci);
>  			if (fdb)
>  				fdb_notify(br, fdb, RTM_NEWNEIGH);
>  		}
> @@ -571,18 +583,18 @@ out:
>  
>  /* Update (create or replace) forwarding database entry */
>  static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
> -			 __u16 state, __u16 flags)
> +			 __u16 state, __u16 flags, __u16 vlan_tci)
>  {
>  	struct net_bridge *br = source->br;
> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
>  	struct net_bridge_fdb_entry *fdb;
>  
> -	fdb = fdb_find(head, addr);
> +	fdb = fdb_find(head, addr, vlan_tci);
>  	if (fdb == NULL) {
>  		if (!(flags & NLM_F_CREATE))
>  			return -ENOENT;
>  
> -		fdb = fdb_create(head, source, addr);
> +		fdb = fdb_create(head, source, addr, vlan_tci);
>  		if (!fdb)
>  			return -ENOMEM;
>  		fdb_notify(br, fdb, RTM_NEWNEIGH);
> @@ -628,11 +640,12 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
>  
>  	if (ndm->ndm_flags & NTF_USE) {
>  		rcu_read_lock();
> -		br_fdb_update(p->br, p, addr);
> +		br_fdb_update(p->br, p, addr, 0);
>  		rcu_read_unlock();
>  	} else {
>  		spin_lock_bh(&p->br->hash_lock);
> -		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
> +		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags,
> +				0);
>  		spin_unlock_bh(&p->br->hash_lock);
>  	}
>  
> @@ -642,10 +655,10 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
>  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)];
> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
>  	struct net_bridge_fdb_entry *fdb;
>  
> -	fdb = fdb_find(head, addr);
> +	fdb = fdb_find(head, addr, 0);
>  	if (!fdb)
>  		return -ENOENT;
>  
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index b7ca3b3..a4579ee 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -62,12 +62,15 @@ int br_handle_frame_finish(struct sk_buff *skb)
>  	 * only traffic matching the VLAN filter.
>  	 */
>  	vlan_map = rcu_dereference(p->vlan_map);
> -	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
> -		goto drop;
> +	if (vlan_map && vlan_tx_tag_present(skb)) {
> +		unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
> +		if (!test_bit(vid+1, vlan_map))
> +			goto drop;
> +	}
>  
>  	/* insert into forwarding database after filtering to avoid spoofing */
>  	br = p->br;
> -	br_fdb_update(br, p, eth_hdr(skb)->h_source);
> +	br_fdb_update(br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
>  
>  	if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
>  	    br_multicast_rcv(br, p, skb))
> @@ -102,7 +105,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
>  			skb2 = skb;
>  
>  		br->dev->stats.multicast++;
> -	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
> +	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) &&
> +			dst->is_local) {
>  		skb2 = skb;
>  		/* Do not forward the packet since it's local. */
>  		skb = NULL;
> @@ -131,7 +135,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
>  {
>  	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>  
> -	br_fdb_update(p->br, p, eth_hdr(skb)->h_source);
> +	br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
>  	return 0;	 /* process further */
>  }
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 8da90e8..921b927 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -72,6 +72,7 @@ struct net_bridge_fdb_entry
>  	unsigned long			updated;
>  	unsigned long			used;
>  	mac_addr			addr;
> +	__u16				vlan_id;
>  	unsigned char			is_local;
>  	unsigned char			is_static;
>  };
> @@ -352,7 +353,8 @@ extern void br_fdb_cleanup(unsigned long arg);
>  extern void br_fdb_delete_by_port(struct net_bridge *br,
>  				  const struct net_bridge_port *p, int do_all);
>  extern struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
> -						 const unsigned char *addr);
> +						 const unsigned char *addr,
> +						 __u16 vlan_tci);
>  extern int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
>  extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>  			  unsigned long count, unsigned long off);
> @@ -361,7 +363,8 @@ extern int br_fdb_insert(struct net_bridge *br,
>  			 const unsigned char *addr);
>  extern void br_fdb_update(struct net_bridge *br,
>  			  struct net_bridge_port *source,
> -			  const unsigned char *addr);
> +			  const unsigned char *addr,
> +			  __u16 vlan_tci);
>  
>  extern int br_fdb_delete(struct ndmsg *ndm,
>  			 struct net_device *dev,
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Vlad Yasevich]

On 08/30/2012 10:33 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 03:29:52PM -0400, Vlad Yasevich wrote:
>> This patch adds vlan to unicast fdb entries that are created for
>> learned addresses (not the manually configured ones).  It adds
>> vlan id into the hash mix and uses vlan as an addditional parameter
>> for an entry match.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>
> I wonder whether this conflicts with Eric's
> 05423b241311c9380b7280179295bac7794281b6
> which allowed null vid.
>
> Specifically this calls vlan_tx_tag_get without
> checking vlan_tx_tag_present. Why is this OK?

We ignore the PRIO bits and just look at vlan deeper in the functions.
So if the tci is set, even to NULL/0 vid, we are still OK as we'll use
just the VID bits.

Now, the other issue with this patch is that it doesn't work when VLANs 
aren't supported by the kernel and skb->vlan_tci is never set.
I am working to address that.

-vlad

>
>> ---
>>   include/linux/if_bridge.h |    1 +
>>   net/bridge/br_device.c    |    2 +-
>>   net/bridge/br_fdb.c       |   71 ++++++++++++++++++++++++++------------------
>>   net/bridge/br_input.c     |   14 ++++++---
>>   net/bridge/br_private.h   |    7 +++-
>>   5 files changed, 58 insertions(+), 37 deletions(-)
>>
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index dd3f201..288ff10 100644
>> --- a/include/linux/if_bridge.h
>> +++ b/include/linux/if_bridge.h
>> @@ -95,6 +95,7 @@ struct __fdb_entry {
>>   	__u8 port_hi;
>>   	__u8 pad0;
>>   	__u16 unused;
>> +#define fdb_vid unused
>>   };
>>
>>   #ifdef __KERNEL__
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 3334845..e321b9d 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -66,7 +66,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>>   			br_multicast_deliver(mdst, skb);
>>   		else
>>   			br_flood_deliver(br, skb);
>> -	} else if ((dst = __br_fdb_get(br, dest)) != NULL)
>> +	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) != NULL)
>>   		br_deliver(dst->dst, skb);
>>   	else
>>   		br_flood_deliver(br, skb);
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index d21f323..478ca1d 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/atomic.h>
>>   #include <asm/unaligned.h>
>> +#include <linux/if_vlan.h>
>>   #include "br_private.h"
>>
>>   static struct kmem_cache *br_fdb_cache __read_mostly;
>> @@ -67,11 +68,12 @@ static inline int has_expired(const struct net_bridge *br,
>>   		time_before_eq(fdb->updated + hold_time(br), jiffies);
>>   }
>>
>> -static inline int br_mac_hash(const unsigned char *mac)
>> +static inline int br_mac_hash(const unsigned char *mac, __u16 vlan_tci)
>>   {
>> -	/* use 1 byte of OUI cnd 3 bytes of NIC */
>> +	/* use 1 byte of OUI and 3 bytes of NIC */
>>   	u32 key = get_unaligned((u32 *)(mac + 2));
>> -	return jhash_1word(key, fdb_salt) & (BR_HASH_SIZE - 1);
>> +	return jhash_2words(key, (vlan_tci & VLAN_VID_MASK),
>> +				fdb_salt) & (BR_HASH_SIZE - 1);
>>   }
>>
>>   static void fdb_rcu_free(struct rcu_head *head)
>> @@ -132,7 +134,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>>   	struct net_bridge_fdb_entry *f;
>>
>>   	/* If old entry was unassociated with any port, then delete it. */
>> -	f = __br_fdb_get(br, br->dev->dev_addr);
>> +	f = __br_fdb_get(br, br->dev->dev_addr, 0);
>>   	if (f && f->is_local && !f->dst)
>>   		fdb_delete(br, f);
>>
>> @@ -231,13 +233,16 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>>
>>   /* No locking or refcounting, assumes caller has rcu_read_lock */
>>   struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
>> -					  const unsigned char *addr)
>> +					  const unsigned char *addr,
>> +					  __u16 vlan_tci)
>>   {
>>   	struct hlist_node *h;
>>   	struct net_bridge_fdb_entry *fdb;
>>
>> -	hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
>> -		if (ether_addr_equal(fdb->addr.addr, addr)) {
>> +	hlist_for_each_entry_rcu(fdb, h,
>> +				&br->hash[br_mac_hash(addr, vlan_tci)], hlist) {
>> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
>> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK) ) {
>>   			if (unlikely(has_expired(br, fdb)))
>>   				break;
>>   			return fdb;
>> @@ -261,7 +266,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
>>   	if (!port)
>>   		ret = 0;
>>   	else {
>> -		fdb = __br_fdb_get(port->br, addr);
>> +		fdb = __br_fdb_get(port->br, addr, 0);
>>   		ret = fdb && fdb->dst && fdb->dst->dev != dev &&
>>   			fdb->dst->state == BR_STATE_FORWARDING;
>>   	}
>> @@ -313,6 +318,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>>   			fe->is_local = f->is_local;
>>   			if (!f->is_static)
>>   				fe->ageing_timer_value = jiffies_to_clock_t(jiffies - f->updated);
>> +			fe->fdb_vid = f->vlan_id;
>>   			++fe;
>>   			++num;
>>   		}
>> @@ -325,26 +331,30 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>>   }
>>
>>   static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
>> -					     const unsigned char *addr)
>> +					     const unsigned char *addr,
>> +					     __u16 vlan_tci)
>>   {
>>   	struct hlist_node *h;
>>   	struct net_bridge_fdb_entry *fdb;
>>
>>   	hlist_for_each_entry(fdb, h, head, hlist) {
>> -		if (ether_addr_equal(fdb->addr.addr, addr))
>> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
>> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
>>   			return fdb;
>>   	}
>>   	return NULL;
>>   }
>>
>>   static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
>> -						 const unsigned char *addr)
>> +						 const unsigned char *addr,
>> +						 __u16 vlan_tci)
>>   {
>>   	struct hlist_node *h;
>>   	struct net_bridge_fdb_entry *fdb;
>>
>>   	hlist_for_each_entry_rcu(fdb, h, head, hlist) {
>> -		if (ether_addr_equal(fdb->addr.addr, addr))
>> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
>> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
>>   			return fdb;
>>   	}
>>   	return NULL;
>> @@ -352,7 +362,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
>>
>>   static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>   					       struct net_bridge_port *source,
>> -					       const unsigned char *addr)
>> +					       const unsigned char *addr,
>> +					       __u16 vlan_tci)
>>   {
>>   	struct net_bridge_fdb_entry *fdb;
>>
>> @@ -360,6 +371,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>   	if (fdb) {
>>   		memcpy(fdb->addr.addr, addr, ETH_ALEN);
>>   		fdb->dst = source;
>> +		fdb->vlan_id = (vlan_tci & VLAN_VID_MASK);
>>   		fdb->is_local = 0;
>>   		fdb->is_static = 0;
>>   		fdb->updated = fdb->used = jiffies;
>> @@ -371,13 +383,13 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>   static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>>   		  const unsigned char *addr)
>>   {
>> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
>>   	struct net_bridge_fdb_entry *fdb;
>>
>>   	if (!is_valid_ether_addr(addr))
>>   		return -EINVAL;
>>
>> -	fdb = fdb_find(head, addr);
>> +	fdb = fdb_find(head, addr, 0);
>>   	if (fdb) {
>>   		/* it is okay to have multiple ports with same
>>   		 * address, just use the first one.
>> @@ -390,7 +402,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>>   		fdb_delete(br, fdb);
>>   	}
>>
>> -	fdb = fdb_create(head, source, addr);
>> +	fdb = fdb_create(head, source, addr, 0);
>>   	if (!fdb)
>>   		return -ENOMEM;
>>
>> @@ -412,9 +424,9 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>>   }
>>
>>   void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>> -		   const unsigned char *addr)
>> +		   const unsigned char *addr, __u16 vlan_tci)
>>   {
>> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
>>   	struct net_bridge_fdb_entry *fdb;
>>
>>   	/* some users want to always flood. */
>> @@ -426,7 +438,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>>   	      source->state == BR_STATE_FORWARDING))
>>   		return;
>>
>> -	fdb = fdb_find_rcu(head, addr);
>> +	fdb = fdb_find_rcu(head, addr, vlan_tci);
>>   	if (likely(fdb)) {
>>   		/* attempt to update an entry for a local interface */
>>   		if (unlikely(fdb->is_local)) {
>> @@ -441,8 +453,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>>   		}
>>   	} else {
>>   		spin_lock(&br->hash_lock);
>> -		if (likely(!fdb_find(head, addr))) {
>> -			fdb = fdb_create(head, source, addr);
>> +		if (likely(!fdb_find(head, addr, vlan_tci))) {
>> +			fdb = fdb_create(head, source, addr, vlan_tci);
>>   			if (fdb)
>>   				fdb_notify(br, fdb, RTM_NEWNEIGH);
>>   		}
>> @@ -571,18 +583,18 @@ out:
>>
>>   /* Update (create or replace) forwarding database entry */
>>   static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>> -			 __u16 state, __u16 flags)
>> +			 __u16 state, __u16 flags, __u16 vlan_tci)
>>   {
>>   	struct net_bridge *br = source->br;
>> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
>>   	struct net_bridge_fdb_entry *fdb;
>>
>> -	fdb = fdb_find(head, addr);
>> +	fdb = fdb_find(head, addr, vlan_tci);
>>   	if (fdb == NULL) {
>>   		if (!(flags & NLM_F_CREATE))
>>   			return -ENOENT;
>>
>> -		fdb = fdb_create(head, source, addr);
>> +		fdb = fdb_create(head, source, addr, vlan_tci);
>>   		if (!fdb)
>>   			return -ENOMEM;
>>   		fdb_notify(br, fdb, RTM_NEWNEIGH);
>> @@ -628,11 +640,12 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
>>
>>   	if (ndm->ndm_flags & NTF_USE) {
>>   		rcu_read_lock();
>> -		br_fdb_update(p->br, p, addr);
>> +		br_fdb_update(p->br, p, addr, 0);
>>   		rcu_read_unlock();
>>   	} else {
>>   		spin_lock_bh(&p->br->hash_lock);
>> -		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
>> +		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags,
>> +				0);
>>   		spin_unlock_bh(&p->br->hash_lock);
>>   	}
>>
>> @@ -642,10 +655,10 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
>>   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)];
>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
>>   	struct net_bridge_fdb_entry *fdb;
>>
>> -	fdb = fdb_find(head, addr);
>> +	fdb = fdb_find(head, addr, 0);
>>   	if (!fdb)
>>   		return -ENOENT;
>>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index b7ca3b3..a4579ee 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -62,12 +62,15 @@ int br_handle_frame_finish(struct sk_buff *skb)
>>   	 * only traffic matching the VLAN filter.
>>   	 */
>>   	vlan_map = rcu_dereference(p->vlan_map);
>> -	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
>> -		goto drop;
>> +	if (vlan_map && vlan_tx_tag_present(skb)) {
>> +		unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
>> +		if (!test_bit(vid+1, vlan_map))
>> +			goto drop;
>> +	}
>>
>>   	/* insert into forwarding database after filtering to avoid spoofing */
>>   	br = p->br;
>> -	br_fdb_update(br, p, eth_hdr(skb)->h_source);
>> +	br_fdb_update(br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
>>
>>   	if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
>>   	    br_multicast_rcv(br, p, skb))
>> @@ -102,7 +105,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
>>   			skb2 = skb;
>>
>>   		br->dev->stats.multicast++;
>> -	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
>> +	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) &&
>> +			dst->is_local) {
>>   		skb2 = skb;
>>   		/* Do not forward the packet since it's local. */
>>   		skb = NULL;
>> @@ -131,7 +135,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
>>   {
>>   	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>>
>> -	br_fdb_update(p->br, p, eth_hdr(skb)->h_source);
>> +	br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
>>   	return 0;	 /* process further */
>>   }
>>
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 8da90e8..921b927 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -72,6 +72,7 @@ struct net_bridge_fdb_entry
>>   	unsigned long			updated;
>>   	unsigned long			used;
>>   	mac_addr			addr;
>> +	__u16				vlan_id;
>>   	unsigned char			is_local;
>>   	unsigned char			is_static;
>>   };
>> @@ -352,7 +353,8 @@ extern void br_fdb_cleanup(unsigned long arg);
>>   extern void br_fdb_delete_by_port(struct net_bridge *br,
>>   				  const struct net_bridge_port *p, int do_all);
>>   extern struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
>> -						 const unsigned char *addr);
>> +						 const unsigned char *addr,
>> +						 __u16 vlan_tci);
>>   extern int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
>>   extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>>   			  unsigned long count, unsigned long off);
>> @@ -361,7 +363,8 @@ extern int br_fdb_insert(struct net_bridge *br,
>>   			 const unsigned char *addr);
>>   extern void br_fdb_update(struct net_bridge *br,
>>   			  struct net_bridge_port *source,
>> -			  const unsigned char *addr);
>> +			  const unsigned char *addr,
>> +			  __u16 vlan_tci);
>>
>>   extern int br_fdb_delete(struct ndmsg *ndm,
>>   			 struct net_device *dev,
>> --
>> 1.7.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 10:48:57AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 10:33 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 23, 2012 at 03:29:52PM -0400, Vlad Yasevich wrote:
> >>This patch adds vlan to unicast fdb entries that are created for
> >>learned addresses (not the manually configured ones).  It adds
> >>vlan id into the hash mix and uses vlan as an addditional parameter
> >>for an entry match.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >
> >I wonder whether this conflicts with Eric's
> >05423b241311c9380b7280179295bac7794281b6
> >which allowed null vid.
> >
> >Specifically this calls vlan_tx_tag_get without
> >checking vlan_tx_tag_present. Why is this OK?
> 
> We ignore the PRIO bits and just look at vlan deeper in the functions.
> So if the tci is set, even to NULL/0 vid, we are still OK as we'll use
> just the VID bits.

I do not fully understand why does kernel allow NULL vid,
thus my question.
It seems that your patches treat packets with NULL vid
same as untagged packets - is this OK?

> Now, the other issue with this patch is that it doesn't work when
> VLANs aren't supported by the kernel and skb->vlan_tci is never set.
> I am working to address that.
> 
> -vlad

I guess if we special-case !vlan_tx_tag_present this will
also treat untagged packets different from tagged ones?

> >
> >>---
> >>  include/linux/if_bridge.h |    1 +
> >>  net/bridge/br_device.c    |    2 +-
> >>  net/bridge/br_fdb.c       |   71 ++++++++++++++++++++++++++------------------
> >>  net/bridge/br_input.c     |   14 ++++++---
> >>  net/bridge/br_private.h   |    7 +++-
> >>  5 files changed, 58 insertions(+), 37 deletions(-)
> >>
> >>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>index dd3f201..288ff10 100644
> >>--- a/include/linux/if_bridge.h
> >>+++ b/include/linux/if_bridge.h
> >>@@ -95,6 +95,7 @@ struct __fdb_entry {
> >>  	__u8 port_hi;
> >>  	__u8 pad0;
> >>  	__u16 unused;
> >>+#define fdb_vid unused
> >>  };
> >>
> >>  #ifdef __KERNEL__
> >>diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> >>index 3334845..e321b9d 100644
> >>--- a/net/bridge/br_device.c
> >>+++ b/net/bridge/br_device.c
> >>@@ -66,7 +66,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> >>  			br_multicast_deliver(mdst, skb);
> >>  		else
> >>  			br_flood_deliver(br, skb);
> >>-	} else if ((dst = __br_fdb_get(br, dest)) != NULL)
> >>+	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) != NULL)
> >>  		br_deliver(dst->dst, skb);
> >>  	else
> >>  		br_flood_deliver(br, skb);
> >>diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> >>index d21f323..478ca1d 100644
> >>--- a/net/bridge/br_fdb.c
> >>+++ b/net/bridge/br_fdb.c
> >>@@ -23,6 +23,7 @@
> >>  #include <linux/slab.h>
> >>  #include <linux/atomic.h>
> >>  #include <asm/unaligned.h>
> >>+#include <linux/if_vlan.h>
> >>  #include "br_private.h"
> >>
> >>  static struct kmem_cache *br_fdb_cache __read_mostly;
> >>@@ -67,11 +68,12 @@ static inline int has_expired(const struct net_bridge *br,
> >>  		time_before_eq(fdb->updated + hold_time(br), jiffies);
> >>  }
> >>
> >>-static inline int br_mac_hash(const unsigned char *mac)
> >>+static inline int br_mac_hash(const unsigned char *mac, __u16 vlan_tci)
> >>  {
> >>-	/* use 1 byte of OUI cnd 3 bytes of NIC */
> >>+	/* use 1 byte of OUI and 3 bytes of NIC */
> >>  	u32 key = get_unaligned((u32 *)(mac + 2));
> >>-	return jhash_1word(key, fdb_salt) & (BR_HASH_SIZE - 1);
> >>+	return jhash_2words(key, (vlan_tci & VLAN_VID_MASK),
> >>+				fdb_salt) & (BR_HASH_SIZE - 1);
> >>  }
> >>
> >>  static void fdb_rcu_free(struct rcu_head *head)
> >>@@ -132,7 +134,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
> >>  	struct net_bridge_fdb_entry *f;
> >>
> >>  	/* If old entry was unassociated with any port, then delete it. */
> >>-	f = __br_fdb_get(br, br->dev->dev_addr);
> >>+	f = __br_fdb_get(br, br->dev->dev_addr, 0);
> >>  	if (f && f->is_local && !f->dst)
> >>  		fdb_delete(br, f);
> >>
> >>@@ -231,13 +233,16 @@ void br_fdb_delete_by_port(struct net_bridge *br,
> >>
> >>  /* No locking or refcounting, assumes caller has rcu_read_lock */
> >>  struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
> >>-					  const unsigned char *addr)
> >>+					  const unsigned char *addr,
> >>+					  __u16 vlan_tci)
> >>  {
> >>  	struct hlist_node *h;
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>-	hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
> >>-		if (ether_addr_equal(fdb->addr.addr, addr)) {
> >>+	hlist_for_each_entry_rcu(fdb, h,
> >>+				&br->hash[br_mac_hash(addr, vlan_tci)], hlist) {
> >>+		if (ether_addr_equal(fdb->addr.addr, addr) &&
> >>+		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK) ) {
> >>  			if (unlikely(has_expired(br, fdb)))
> >>  				break;
> >>  			return fdb;
> >>@@ -261,7 +266,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
> >>  	if (!port)
> >>  		ret = 0;
> >>  	else {
> >>-		fdb = __br_fdb_get(port->br, addr);
> >>+		fdb = __br_fdb_get(port->br, addr, 0);
> >>  		ret = fdb && fdb->dst && fdb->dst->dev != dev &&
> >>  			fdb->dst->state == BR_STATE_FORWARDING;
> >>  	}
> >>@@ -313,6 +318,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
> >>  			fe->is_local = f->is_local;
> >>  			if (!f->is_static)
> >>  				fe->ageing_timer_value = jiffies_to_clock_t(jiffies - f->updated);
> >>+			fe->fdb_vid = f->vlan_id;
> >>  			++fe;
> >>  			++num;
> >>  		}
> >>@@ -325,26 +331,30 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
> >>  }
> >>
> >>  static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
> >>-					     const unsigned char *addr)
> >>+					     const unsigned char *addr,
> >>+					     __u16 vlan_tci)
> >>  {
> >>  	struct hlist_node *h;
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>  	hlist_for_each_entry(fdb, h, head, hlist) {
> >>-		if (ether_addr_equal(fdb->addr.addr, addr))
> >>+		if (ether_addr_equal(fdb->addr.addr, addr) &&
> >>+		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
> >>  			return fdb;
> >>  	}
> >>  	return NULL;
> >>  }
> >>
> >>  static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
> >>-						 const unsigned char *addr)
> >>+						 const unsigned char *addr,
> >>+						 __u16 vlan_tci)
> >>  {
> >>  	struct hlist_node *h;
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>  	hlist_for_each_entry_rcu(fdb, h, head, hlist) {
> >>-		if (ether_addr_equal(fdb->addr.addr, addr))
> >>+		if (ether_addr_equal(fdb->addr.addr, addr) &&
> >>+		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
> >>  			return fdb;
> >>  	}
> >>  	return NULL;
> >>@@ -352,7 +362,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
> >>
> >>  static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
> >>  					       struct net_bridge_port *source,
> >>-					       const unsigned char *addr)
> >>+					       const unsigned char *addr,
> >>+					       __u16 vlan_tci)
> >>  {
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>@@ -360,6 +371,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
> >>  	if (fdb) {
> >>  		memcpy(fdb->addr.addr, addr, ETH_ALEN);
> >>  		fdb->dst = source;
> >>+		fdb->vlan_id = (vlan_tci & VLAN_VID_MASK);
> >>  		fdb->is_local = 0;
> >>  		fdb->is_static = 0;
> >>  		fdb->updated = fdb->used = jiffies;
> >>@@ -371,13 +383,13 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
> >>  static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
> >>  		  const unsigned char *addr)
> >>  {
> >>-	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
> >>+	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>  	if (!is_valid_ether_addr(addr))
> >>  		return -EINVAL;
> >>
> >>-	fdb = fdb_find(head, addr);
> >>+	fdb = fdb_find(head, addr, 0);
> >>  	if (fdb) {
> >>  		/* it is okay to have multiple ports with same
> >>  		 * address, just use the first one.
> >>@@ -390,7 +402,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
> >>  		fdb_delete(br, fdb);
> >>  	}
> >>
> >>-	fdb = fdb_create(head, source, addr);
> >>+	fdb = fdb_create(head, source, addr, 0);
> >>  	if (!fdb)
> >>  		return -ENOMEM;
> >>
> >>@@ -412,9 +424,9 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
> >>  }
> >>
> >>  void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> >>-		   const unsigned char *addr)
> >>+		   const unsigned char *addr, __u16 vlan_tci)
> >>  {
> >>-	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
> >>+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>  	/* some users want to always flood. */
> >>@@ -426,7 +438,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> >>  	      source->state == BR_STATE_FORWARDING))
> >>  		return;
> >>
> >>-	fdb = fdb_find_rcu(head, addr);
> >>+	fdb = fdb_find_rcu(head, addr, vlan_tci);
> >>  	if (likely(fdb)) {
> >>  		/* attempt to update an entry for a local interface */
> >>  		if (unlikely(fdb->is_local)) {
> >>@@ -441,8 +453,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> >>  		}
> >>  	} else {
> >>  		spin_lock(&br->hash_lock);
> >>-		if (likely(!fdb_find(head, addr))) {
> >>-			fdb = fdb_create(head, source, addr);
> >>+		if (likely(!fdb_find(head, addr, vlan_tci))) {
> >>+			fdb = fdb_create(head, source, addr, vlan_tci);
> >>  			if (fdb)
> >>  				fdb_notify(br, fdb, RTM_NEWNEIGH);
> >>  		}
> >>@@ -571,18 +583,18 @@ out:
> >>
> >>  /* Update (create or replace) forwarding database entry */
> >>  static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
> >>-			 __u16 state, __u16 flags)
> >>+			 __u16 state, __u16 flags, __u16 vlan_tci)
> >>  {
> >>  	struct net_bridge *br = source->br;
> >>-	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
> >>+	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>-	fdb = fdb_find(head, addr);
> >>+	fdb = fdb_find(head, addr, vlan_tci);
> >>  	if (fdb == NULL) {
> >>  		if (!(flags & NLM_F_CREATE))
> >>  			return -ENOENT;
> >>
> >>-		fdb = fdb_create(head, source, addr);
> >>+		fdb = fdb_create(head, source, addr, vlan_tci);
> >>  		if (!fdb)
> >>  			return -ENOMEM;
> >>  		fdb_notify(br, fdb, RTM_NEWNEIGH);
> >>@@ -628,11 +640,12 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
> >>
> >>  	if (ndm->ndm_flags & NTF_USE) {
> >>  		rcu_read_lock();
> >>-		br_fdb_update(p->br, p, addr);
> >>+		br_fdb_update(p->br, p, addr, 0);
> >>  		rcu_read_unlock();
> >>  	} else {
> >>  		spin_lock_bh(&p->br->hash_lock);
> >>-		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
> >>+		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags,
> >>+				0);
> >>  		spin_unlock_bh(&p->br->hash_lock);
> >>  	}
> >>
> >>@@ -642,10 +655,10 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
> >>  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)];
> >>+	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
> >>  	struct net_bridge_fdb_entry *fdb;
> >>
> >>-	fdb = fdb_find(head, addr);
> >>+	fdb = fdb_find(head, addr, 0);
> >>  	if (!fdb)
> >>  		return -ENOENT;
> >>
> >>diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> >>index b7ca3b3..a4579ee 100644
> >>--- a/net/bridge/br_input.c
> >>+++ b/net/bridge/br_input.c
> >>@@ -62,12 +62,15 @@ int br_handle_frame_finish(struct sk_buff *skb)
> >>  	 * only traffic matching the VLAN filter.
> >>  	 */
> >>  	vlan_map = rcu_dereference(p->vlan_map);
> >>-	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
> >>-		goto drop;
> >>+	if (vlan_map && vlan_tx_tag_present(skb)) {
> >>+		unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
> >>+		if (!test_bit(vid+1, vlan_map))
> >>+			goto drop;
> >>+	}
> >>
> >>  	/* insert into forwarding database after filtering to avoid spoofing */
> >>  	br = p->br;
> >>-	br_fdb_update(br, p, eth_hdr(skb)->h_source);
> >>+	br_fdb_update(br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
> >>
> >>  	if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
> >>  	    br_multicast_rcv(br, p, skb))
> >>@@ -102,7 +105,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
> >>  			skb2 = skb;
> >>
> >>  		br->dev->stats.multicast++;
> >>-	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
> >>+	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) &&
> >>+			dst->is_local) {
> >>  		skb2 = skb;
> >>  		/* Do not forward the packet since it's local. */
> >>  		skb = NULL;
> >>@@ -131,7 +135,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
> >>  {
> >>  	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
> >>
> >>-	br_fdb_update(p->br, p, eth_hdr(skb)->h_source);
> >>+	br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
> >>  	return 0;	 /* process further */
> >>  }
> >>
> >>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>index 8da90e8..921b927 100644
> >>--- a/net/bridge/br_private.h
> >>+++ b/net/bridge/br_private.h
> >>@@ -72,6 +72,7 @@ struct net_bridge_fdb_entry
> >>  	unsigned long			updated;
> >>  	unsigned long			used;
> >>  	mac_addr			addr;
> >>+	__u16				vlan_id;
> >>  	unsigned char			is_local;
> >>  	unsigned char			is_static;
> >>  };
> >>@@ -352,7 +353,8 @@ extern void br_fdb_cleanup(unsigned long arg);
> >>  extern void br_fdb_delete_by_port(struct net_bridge *br,
> >>  				  const struct net_bridge_port *p, int do_all);
> >>  extern struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
> >>-						 const unsigned char *addr);
> >>+						 const unsigned char *addr,
> >>+						 __u16 vlan_tci);
> >>  extern int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
> >>  extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
> >>  			  unsigned long count, unsigned long off);
> >>@@ -361,7 +363,8 @@ extern int br_fdb_insert(struct net_bridge *br,
> >>  			 const unsigned char *addr);
> >>  extern void br_fdb_update(struct net_bridge *br,
> >>  			  struct net_bridge_port *source,
> >>-			  const unsigned char *addr);
> >>+			  const unsigned char *addr,
> >>+			  __u16 vlan_tci);
> >>
> >>  extern int br_fdb_delete(struct ndmsg *ndm,
> >>  			 struct net_device *dev,
> >>--
> >>1.7.7.6
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries

[Vlad Yasevich]

On 08/30/2012 11:45 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 10:48:57AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 10:33 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 23, 2012 at 03:29:52PM -0400, Vlad Yasevich wrote:
>>>> This patch adds vlan to unicast fdb entries that are created for
>>>> learned addresses (not the manually configured ones).  It adds
>>>> vlan id into the hash mix and uses vlan as an addditional parameter
>>>> for an entry match.
>>>>
>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>
>>> I wonder whether this conflicts with Eric's
>>> 05423b241311c9380b7280179295bac7794281b6
>>> which allowed null vid.
>>>
>>> Specifically this calls vlan_tx_tag_get without
>>> checking vlan_tx_tag_present. Why is this OK?
>>
>> We ignore the PRIO bits and just look at vlan deeper in the functions.
>> So if the tci is set, even to NULL/0 vid, we are still OK as we'll use
>> just the VID bits.
>
> I do not fully understand why does kernel allow NULL vid,
> thus my question.
> It seems that your patches treat packets with NULL vid
> same as untagged packets - is this OK?
>

I think they are.  Technically they do not have a tag, just a specific
priority that a switch would interpret.  So, they should be forwarded 
the same as packets without a vlan header.

-vlad

>> Now, the other issue with this patch is that it doesn't work when
>> VLANs aren't supported by the kernel and skb->vlan_tci is never set.
>> I am working to address that.
>>
>> -vlad
>
> I guess if we special-case !vlan_tx_tag_present this will
> also treat untagged packets different from tagged ones?
>
>>>
>>>> ---
>>>>   include/linux/if_bridge.h |    1 +
>>>>   net/bridge/br_device.c    |    2 +-
>>>>   net/bridge/br_fdb.c       |   71 ++++++++++++++++++++++++++------------------
>>>>   net/bridge/br_input.c     |   14 ++++++---
>>>>   net/bridge/br_private.h   |    7 +++-
>>>>   5 files changed, 58 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>> index dd3f201..288ff10 100644
>>>> --- a/include/linux/if_bridge.h
>>>> +++ b/include/linux/if_bridge.h
>>>> @@ -95,6 +95,7 @@ struct __fdb_entry {
>>>>   	__u8 port_hi;
>>>>   	__u8 pad0;
>>>>   	__u16 unused;
>>>> +#define fdb_vid unused
>>>>   };
>>>>
>>>>   #ifdef __KERNEL__
>>>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>>>> index 3334845..e321b9d 100644
>>>> --- a/net/bridge/br_device.c
>>>> +++ b/net/bridge/br_device.c
>>>> @@ -66,7 +66,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>>>>   			br_multicast_deliver(mdst, skb);
>>>>   		else
>>>>   			br_flood_deliver(br, skb);
>>>> -	} else if ((dst = __br_fdb_get(br, dest)) != NULL)
>>>> +	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) != NULL)
>>>>   		br_deliver(dst->dst, skb);
>>>>   	else
>>>>   		br_flood_deliver(br, skb);
>>>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>>>> index d21f323..478ca1d 100644
>>>> --- a/net/bridge/br_fdb.c
>>>> +++ b/net/bridge/br_fdb.c
>>>> @@ -23,6 +23,7 @@
>>>>   #include <linux/slab.h>
>>>>   #include <linux/atomic.h>
>>>>   #include <asm/unaligned.h>
>>>> +#include <linux/if_vlan.h>
>>>>   #include "br_private.h"
>>>>
>>>>   static struct kmem_cache *br_fdb_cache __read_mostly;
>>>> @@ -67,11 +68,12 @@ static inline int has_expired(const struct net_bridge *br,
>>>>   		time_before_eq(fdb->updated + hold_time(br), jiffies);
>>>>   }
>>>>
>>>> -static inline int br_mac_hash(const unsigned char *mac)
>>>> +static inline int br_mac_hash(const unsigned char *mac, __u16 vlan_tci)
>>>>   {
>>>> -	/* use 1 byte of OUI cnd 3 bytes of NIC */
>>>> +	/* use 1 byte of OUI and 3 bytes of NIC */
>>>>   	u32 key = get_unaligned((u32 *)(mac + 2));
>>>> -	return jhash_1word(key, fdb_salt) & (BR_HASH_SIZE - 1);
>>>> +	return jhash_2words(key, (vlan_tci & VLAN_VID_MASK),
>>>> +				fdb_salt) & (BR_HASH_SIZE - 1);
>>>>   }
>>>>
>>>>   static void fdb_rcu_free(struct rcu_head *head)
>>>> @@ -132,7 +134,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>>>>   	struct net_bridge_fdb_entry *f;
>>>>
>>>>   	/* If old entry was unassociated with any port, then delete it. */
>>>> -	f = __br_fdb_get(br, br->dev->dev_addr);
>>>> +	f = __br_fdb_get(br, br->dev->dev_addr, 0);
>>>>   	if (f && f->is_local && !f->dst)
>>>>   		fdb_delete(br, f);
>>>>
>>>> @@ -231,13 +233,16 @@ void br_fdb_delete_by_port(struct net_bridge *br,
>>>>
>>>>   /* No locking or refcounting, assumes caller has rcu_read_lock */
>>>>   struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
>>>> -					  const unsigned char *addr)
>>>> +					  const unsigned char *addr,
>>>> +					  __u16 vlan_tci)
>>>>   {
>>>>   	struct hlist_node *h;
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>> -	hlist_for_each_entry_rcu(fdb, h, &br->hash[br_mac_hash(addr)], hlist) {
>>>> -		if (ether_addr_equal(fdb->addr.addr, addr)) {
>>>> +	hlist_for_each_entry_rcu(fdb, h,
>>>> +				&br->hash[br_mac_hash(addr, vlan_tci)], hlist) {
>>>> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
>>>> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK) ) {
>>>>   			if (unlikely(has_expired(br, fdb)))
>>>>   				break;
>>>>   			return fdb;
>>>> @@ -261,7 +266,7 @@ int br_fdb_test_addr(struct net_device *dev, unsigned char *addr)
>>>>   	if (!port)
>>>>   		ret = 0;
>>>>   	else {
>>>> -		fdb = __br_fdb_get(port->br, addr);
>>>> +		fdb = __br_fdb_get(port->br, addr, 0);
>>>>   		ret = fdb && fdb->dst && fdb->dst->dev != dev &&
>>>>   			fdb->dst->state == BR_STATE_FORWARDING;
>>>>   	}
>>>> @@ -313,6 +318,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>>>>   			fe->is_local = f->is_local;
>>>>   			if (!f->is_static)
>>>>   				fe->ageing_timer_value = jiffies_to_clock_t(jiffies - f->updated);
>>>> +			fe->fdb_vid = f->vlan_id;
>>>>   			++fe;
>>>>   			++num;
>>>>   		}
>>>> @@ -325,26 +331,30 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>>>>   }
>>>>
>>>>   static struct net_bridge_fdb_entry *fdb_find(struct hlist_head *head,
>>>> -					     const unsigned char *addr)
>>>> +					     const unsigned char *addr,
>>>> +					     __u16 vlan_tci)
>>>>   {
>>>>   	struct hlist_node *h;
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>>   	hlist_for_each_entry(fdb, h, head, hlist) {
>>>> -		if (ether_addr_equal(fdb->addr.addr, addr))
>>>> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
>>>> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
>>>>   			return fdb;
>>>>   	}
>>>>   	return NULL;
>>>>   }
>>>>
>>>>   static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
>>>> -						 const unsigned char *addr)
>>>> +						 const unsigned char *addr,
>>>> +						 __u16 vlan_tci)
>>>>   {
>>>>   	struct hlist_node *h;
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>>   	hlist_for_each_entry_rcu(fdb, h, head, hlist) {
>>>> -		if (ether_addr_equal(fdb->addr.addr, addr))
>>>> +		if (ether_addr_equal(fdb->addr.addr, addr) &&
>>>> +		    fdb->vlan_id == (vlan_tci & VLAN_VID_MASK))
>>>>   			return fdb;
>>>>   	}
>>>>   	return NULL;
>>>> @@ -352,7 +362,8 @@ static struct net_bridge_fdb_entry *fdb_find_rcu(struct hlist_head *head,
>>>>
>>>>   static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>>>   					       struct net_bridge_port *source,
>>>> -					       const unsigned char *addr)
>>>> +					       const unsigned char *addr,
>>>> +					       __u16 vlan_tci)
>>>>   {
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>> @@ -360,6 +371,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>>>   	if (fdb) {
>>>>   		memcpy(fdb->addr.addr, addr, ETH_ALEN);
>>>>   		fdb->dst = source;
>>>> +		fdb->vlan_id = (vlan_tci & VLAN_VID_MASK);
>>>>   		fdb->is_local = 0;
>>>>   		fdb->is_static = 0;
>>>>   		fdb->updated = fdb->used = jiffies;
>>>> @@ -371,13 +383,13 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>>>>   static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>>>>   		  const unsigned char *addr)
>>>>   {
>>>> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>>>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>>   	if (!is_valid_ether_addr(addr))
>>>>   		return -EINVAL;
>>>>
>>>> -	fdb = fdb_find(head, addr);
>>>> +	fdb = fdb_find(head, addr, 0);
>>>>   	if (fdb) {
>>>>   		/* it is okay to have multiple ports with same
>>>>   		 * address, just use the first one.
>>>> @@ -390,7 +402,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>>>>   		fdb_delete(br, fdb);
>>>>   	}
>>>>
>>>> -	fdb = fdb_create(head, source, addr);
>>>> +	fdb = fdb_create(head, source, addr, 0);
>>>>   	if (!fdb)
>>>>   		return -ENOMEM;
>>>>
>>>> @@ -412,9 +424,9 @@ int br_fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
>>>>   }
>>>>
>>>>   void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>>>> -		   const unsigned char *addr)
>>>> +		   const unsigned char *addr, __u16 vlan_tci)
>>>>   {
>>>> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>>>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>>   	/* some users want to always flood. */
>>>> @@ -426,7 +438,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>>>>   	      source->state == BR_STATE_FORWARDING))
>>>>   		return;
>>>>
>>>> -	fdb = fdb_find_rcu(head, addr);
>>>> +	fdb = fdb_find_rcu(head, addr, vlan_tci);
>>>>   	if (likely(fdb)) {
>>>>   		/* attempt to update an entry for a local interface */
>>>>   		if (unlikely(fdb->is_local)) {
>>>> @@ -441,8 +453,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
>>>>   		}
>>>>   	} else {
>>>>   		spin_lock(&br->hash_lock);
>>>> -		if (likely(!fdb_find(head, addr))) {
>>>> -			fdb = fdb_create(head, source, addr);
>>>> +		if (likely(!fdb_find(head, addr, vlan_tci))) {
>>>> +			fdb = fdb_create(head, source, addr, vlan_tci);
>>>>   			if (fdb)
>>>>   				fdb_notify(br, fdb, RTM_NEWNEIGH);
>>>>   		}
>>>> @@ -571,18 +583,18 @@ out:
>>>>
>>>>   /* Update (create or replace) forwarding database entry */
>>>>   static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>>>> -			 __u16 state, __u16 flags)
>>>> +			 __u16 state, __u16 flags, __u16 vlan_tci)
>>>>   {
>>>>   	struct net_bridge *br = source->br;
>>>> -	struct hlist_head *head = &br->hash[br_mac_hash(addr)];
>>>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, vlan_tci)];
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>> -	fdb = fdb_find(head, addr);
>>>> +	fdb = fdb_find(head, addr, vlan_tci);
>>>>   	if (fdb == NULL) {
>>>>   		if (!(flags & NLM_F_CREATE))
>>>>   			return -ENOENT;
>>>>
>>>> -		fdb = fdb_create(head, source, addr);
>>>> +		fdb = fdb_create(head, source, addr, vlan_tci);
>>>>   		if (!fdb)
>>>>   			return -ENOMEM;
>>>>   		fdb_notify(br, fdb, RTM_NEWNEIGH);
>>>> @@ -628,11 +640,12 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
>>>>
>>>>   	if (ndm->ndm_flags & NTF_USE) {
>>>>   		rcu_read_lock();
>>>> -		br_fdb_update(p->br, p, addr);
>>>> +		br_fdb_update(p->br, p, addr, 0);
>>>>   		rcu_read_unlock();
>>>>   	} else {
>>>>   		spin_lock_bh(&p->br->hash_lock);
>>>> -		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags);
>>>> +		err = fdb_add_entry(p, addr, ndm->ndm_state, nlh_flags,
>>>> +				0);
>>>>   		spin_unlock_bh(&p->br->hash_lock);
>>>>   	}
>>>>
>>>> @@ -642,10 +655,10 @@ int br_fdb_add(struct ndmsg *ndm, struct net_device *dev,
>>>>   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)];
>>>> +	struct hlist_head *head = &br->hash[br_mac_hash(addr, 0)];
>>>>   	struct net_bridge_fdb_entry *fdb;
>>>>
>>>> -	fdb = fdb_find(head, addr);
>>>> +	fdb = fdb_find(head, addr, 0);
>>>>   	if (!fdb)
>>>>   		return -ENOENT;
>>>>
>>>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>>>> index b7ca3b3..a4579ee 100644
>>>> --- a/net/bridge/br_input.c
>>>> +++ b/net/bridge/br_input.c
>>>> @@ -62,12 +62,15 @@ int br_handle_frame_finish(struct sk_buff *skb)
>>>>   	 * only traffic matching the VLAN filter.
>>>>   	 */
>>>>   	vlan_map = rcu_dereference(p->vlan_map);
>>>> -	if (vlan_map && !test_bit((skb->vlan_tci & VLAN_VID_MASK), vlan_map))
>>>> -		goto drop;
>>>> +	if (vlan_map && vlan_tx_tag_present(skb)) {
>>>> +		unsigned short vid = vlan_tx_tag_get(skb) & VLAN_VID_MASK;
>>>> +		if (!test_bit(vid+1, vlan_map))
>>>> +			goto drop;
>>>> +	}
>>>>
>>>>   	/* insert into forwarding database after filtering to avoid spoofing */
>>>>   	br = p->br;
>>>> -	br_fdb_update(br, p, eth_hdr(skb)->h_source);
>>>> +	br_fdb_update(br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
>>>>
>>>>   	if (!is_broadcast_ether_addr(dest) && is_multicast_ether_addr(dest) &&
>>>>   	    br_multicast_rcv(br, p, skb))
>>>> @@ -102,7 +105,8 @@ int br_handle_frame_finish(struct sk_buff *skb)
>>>>   			skb2 = skb;
>>>>
>>>>   		br->dev->stats.multicast++;
>>>> -	} else if ((dst = __br_fdb_get(br, dest)) && dst->is_local) {
>>>> +	} else if ((dst = __br_fdb_get(br, dest, vlan_tx_tag_get(skb))) &&
>>>> +			dst->is_local) {
>>>>   		skb2 = skb;
>>>>   		/* Do not forward the packet since it's local. */
>>>>   		skb = NULL;
>>>> @@ -131,7 +135,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
>>>>   {
>>>>   	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>>>>
>>>> -	br_fdb_update(p->br, p, eth_hdr(skb)->h_source);
>>>> +	br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vlan_tx_tag_get(skb));
>>>>   	return 0;	 /* process further */
>>>>   }
>>>>
>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>> index 8da90e8..921b927 100644
>>>> --- a/net/bridge/br_private.h
>>>> +++ b/net/bridge/br_private.h
>>>> @@ -72,6 +72,7 @@ struct net_bridge_fdb_entry
>>>>   	unsigned long			updated;
>>>>   	unsigned long			used;
>>>>   	mac_addr			addr;
>>>> +	__u16				vlan_id;
>>>>   	unsigned char			is_local;
>>>>   	unsigned char			is_static;
>>>>   };
>>>> @@ -352,7 +353,8 @@ extern void br_fdb_cleanup(unsigned long arg);
>>>>   extern void br_fdb_delete_by_port(struct net_bridge *br,
>>>>   				  const struct net_bridge_port *p, int do_all);
>>>>   extern struct net_bridge_fdb_entry *__br_fdb_get(struct net_bridge *br,
>>>> -						 const unsigned char *addr);
>>>> +						 const unsigned char *addr,
>>>> +						 __u16 vlan_tci);
>>>>   extern int br_fdb_test_addr(struct net_device *dev, unsigned char *addr);
>>>>   extern int br_fdb_fillbuf(struct net_bridge *br, void *buf,
>>>>   			  unsigned long count, unsigned long off);
>>>> @@ -361,7 +363,8 @@ extern int br_fdb_insert(struct net_bridge *br,
>>>>   			 const unsigned char *addr);
>>>>   extern void br_fdb_update(struct net_bridge *br,
>>>>   			  struct net_bridge_port *source,
>>>> -			  const unsigned char *addr);
>>>> +			  const unsigned char *addr,
>>>> +			  __u16 vlan_tci);
>>>>
>>>>   extern int br_fdb_delete(struct ndmsg *ndm,
>>>>   			 struct net_device *dev,
>>>> --
>>>> 1.7.7.6
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>

[RFC PATCH bridge 3/5] bridge: Add vlan id to multicast groups

[Vlad Yasevich]

Add vlan_id to multicasts groups so that we know which vlan each group belongs
to and can correctly forward to appropriate vlan.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/bridge/br_multicast.c |   64 +++++++++++++++++++++++++++++++--------------
 net/bridge/br_private.h   |    1 +
 2 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 2417434..2976a2b 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -51,6 +51,8 @@ static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
 {
 	if (a->proto != b->proto)
 		return 0;
+	if (a->vid != b->vid)
+		return 0;
 	switch (a->proto) {
 	case htons(ETH_P_IP):
 		return a->u.ip4 == b->u.ip4;
@@ -62,16 +64,19 @@ static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
 	return 0;
 }
 
-static inline int __br_ip4_hash(struct net_bridge_mdb_htable *mdb, __be32 ip)
+static inline int __br_ip4_hash(struct net_bridge_mdb_htable *mdb, __be32 ip,
+				__u16 vid)
 {
-	return jhash_1word(mdb->secret, (__force u32)ip) & (mdb->max - 1);
+	return jhash_2words((__force u32)ip, vid, mdb->secret) & (mdb->max - 1);
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
 static inline int __br_ip6_hash(struct net_bridge_mdb_htable *mdb,
-				const struct in6_addr *ip)
+				const struct in6_addr *ip,
+				__u16 vid)
 {
-	return jhash2((__force u32 *)ip->s6_addr32, 4, mdb->secret) & (mdb->max - 1);
+	u32 addr = *(__force u32 *)ip->s6_addr32;
+	return jhash_2words(addr, vid, mdb->secret) & (mdb->max - 1);
 }
 #endif
 
@@ -80,10 +85,10 @@ static inline int br_ip_hash(struct net_bridge_mdb_htable *mdb,
 {
 	switch (ip->proto) {
 	case htons(ETH_P_IP):
-		return __br_ip4_hash(mdb, ip->u.ip4);
+		return __br_ip4_hash(mdb, ip->u.ip4, ip->vid);
 #if IS_ENABLED(CONFIG_IPV6)
 	case htons(ETH_P_IPV6):
-		return __br_ip6_hash(mdb, &ip->u.ip6);
+		return __br_ip6_hash(mdb, &ip->u.ip6, ip->vid);
 #endif
 	}
 	return 0;
@@ -113,24 +118,27 @@ static struct net_bridge_mdb_entry *br_mdb_ip_get(
 }
 
 static struct net_bridge_mdb_entry *br_mdb_ip4_get(
-	struct net_bridge_mdb_htable *mdb, __be32 dst)
+	struct net_bridge_mdb_htable *mdb, __be32 dst, __u16 vlan_tci)
 {
 	struct br_ip br_dst;
 
 	br_dst.u.ip4 = dst;
 	br_dst.proto = htons(ETH_P_IP);
+	br_dst.vid = (vlan_tci & VLAN_VID_MASK);
 
 	return br_mdb_ip_get(mdb, &br_dst);
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
 static struct net_bridge_mdb_entry *br_mdb_ip6_get(
-	struct net_bridge_mdb_htable *mdb, const struct in6_addr *dst)
+	struct net_bridge_mdb_htable *mdb, const struct in6_addr *dst,
+	__u16 vlan_tci)
 {
 	struct br_ip br_dst;
 
 	br_dst.u.ip6 = *dst;
 	br_dst.proto = htons(ETH_P_IPV6);
+	br_dst.vid = vlan_tci & VLAN_VID_MASK;
 
 	return br_mdb_ip_get(mdb, &br_dst);
 }
@@ -692,7 +700,8 @@ err:
 
 static int br_ip4_multicast_add_group(struct net_bridge *br,
 				      struct net_bridge_port *port,
-				      __be32 group)
+				      __be32 group,
+				      __u16 vlan_tci)
 {
 	struct br_ip br_group;
 
@@ -701,6 +710,7 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
 
 	br_group.u.ip4 = group;
 	br_group.proto = htons(ETH_P_IP);
+	br_group.vid = vlan_tci & VLAN_VID_MASK;
 
 	return br_multicast_add_group(br, port, &br_group);
 }
@@ -708,7 +718,8 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
 #if IS_ENABLED(CONFIG_IPV6)
 static int br_ip6_multicast_add_group(struct net_bridge *br,
 				      struct net_bridge_port *port,
-				      const struct in6_addr *group)
+				      const struct in6_addr *group,
+				      __u16 vlan_tci)
 {
 	struct br_ip br_group;
 
@@ -717,6 +728,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 
 	br_group.u.ip6 = *group;
 	br_group.proto = htons(ETH_P_IPV6);
+	br_group.vid = vlan_tci & VLAN_VID_MASK;
 
 	return br_multicast_add_group(br, port, &br_group);
 }
@@ -928,7 +940,8 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
 			continue;
 		}
 
-		err = br_ip4_multicast_add_group(br, port, group);
+		err = br_ip4_multicast_add_group(br, port, group,
+						skb->vlan_tci);
 		if (err)
 			break;
 	}
@@ -988,7 +1001,8 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 			continue;
 		}
 
-		err = br_ip6_multicast_add_group(br, port, &grec->grec_mca);
+		err = br_ip6_multicast_add_group(br, port, &grec->grec_mca,
+						skb->vlan_tci);
 		if (!err)
 			break;
 	}
@@ -1106,7 +1120,8 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 	if (!group)
 		goto out;
 
-	mp = br_mdb_ip4_get(mlock_dereference(br->mdb, br), group);
+	mp = br_mdb_ip4_get(mlock_dereference(br->mdb, br), group,
+				skb->vlan_tci);
 	if (!mp)
 		goto out;
 
@@ -1178,7 +1193,8 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 	if (!group)
 		goto out;
 
-	mp = br_mdb_ip6_get(mlock_dereference(br->mdb, br), group);
+	mp = br_mdb_ip6_get(mlock_dereference(br->mdb, br), group,
+			skb->vlan_tci);
 	if (!mp)
 		goto out;
 
@@ -1262,7 +1278,8 @@ out:
 
 static void br_ip4_multicast_leave_group(struct net_bridge *br,
 					 struct net_bridge_port *port,
-					 __be32 group)
+					 __be32 group,
+					 __u16 vlan_tci)
 {
 	struct br_ip br_group;
 
@@ -1271,6 +1288,7 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
 
 	br_group.u.ip4 = group;
 	br_group.proto = htons(ETH_P_IP);
+	br_group.vid = (vlan_tci & VLAN_VID_MASK);
 
 	br_multicast_leave_group(br, port, &br_group);
 }
@@ -1278,7 +1296,8 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
 #if IS_ENABLED(CONFIG_IPV6)
 static void br_ip6_multicast_leave_group(struct net_bridge *br,
 					 struct net_bridge_port *port,
-					 const struct in6_addr *group)
+					 const struct in6_addr *group,
+					 __u16 vlan_tci)
 {
 	struct br_ip br_group;
 
@@ -1287,6 +1306,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
 
 	br_group.u.ip6 = *group;
 	br_group.proto = htons(ETH_P_IPV6);
+	br_group.vid = (vlan_tci & VLAN_VID_MASK);
 
 	br_multicast_leave_group(br, port, &br_group);
 }
@@ -1369,7 +1389,8 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 	case IGMP_HOST_MEMBERSHIP_REPORT:
 	case IGMPV2_HOST_MEMBERSHIP_REPORT:
 		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
-		err = br_ip4_multicast_add_group(br, port, ih->group);
+		err = br_ip4_multicast_add_group(br, port, ih->group,
+						skb2->vlan_tci);
 		break;
 	case IGMPV3_HOST_MEMBERSHIP_REPORT:
 		err = br_ip4_multicast_igmp3_report(br, port, skb2);
@@ -1378,7 +1399,8 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 		err = br_ip4_multicast_query(br, port, skb2);
 		break;
 	case IGMP_HOST_LEAVE_MESSAGE:
-		br_ip4_multicast_leave_group(br, port, ih->group);
+		br_ip4_multicast_leave_group(br, port, ih->group,
+						skb2->vlan_tci);
 		break;
 	}
 
@@ -1498,7 +1520,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		}
 		mld = (struct mld_msg *)skb_transport_header(skb2);
 		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
-		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca);
+		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca,
+						 skb2->vlan_tci);
 		break;
 	    }
 	case ICMPV6_MLD2_REPORT:
@@ -1515,7 +1538,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 			goto out;
 		}
 		mld = (struct mld_msg *)skb_transport_header(skb2);
-		br_ip6_multicast_leave_group(br, port, &mld->mld_mca);
+		br_ip6_multicast_leave_group(br, port, &mld->mld_mca,
+						skb2->vlan_tci);
 	    }
 	}
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 921b927..b6c56ab 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -61,6 +61,7 @@ struct br_ip
 #endif
 	} u;
 	__be16		proto;
+	__u16		vid;
 };
 
 struct net_bridge_fdb_entry
-- 
1.7.7.6

Re: [RFC PATCH bridge 3/5] bridge: Add vlan id to multicast groups

[Michael S. Tsirkin]

On Thu, Aug 23, 2012 at 03:29:53PM -0400, Vlad Yasevich wrote:
> Add vlan_id to multicasts groups so that we know which vlan each group belongs
> to and can correctly forward to appropriate vlan.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  net/bridge/br_multicast.c |   64 +++++++++++++++++++++++++++++++--------------
>  net/bridge/br_private.h   |    1 +
>  2 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 2417434..2976a2b 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -51,6 +51,8 @@ static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
>  {
>  	if (a->proto != b->proto)
>  		return 0;
> +	if (a->vid != b->vid)
> +		return 0;
>  	switch (a->proto) {
>  	case htons(ETH_P_IP):
>  		return a->u.ip4 == b->u.ip4;
> @@ -62,16 +64,19 @@ static inline int br_ip_equal(const struct br_ip *a, const struct br_ip *b)
>  	return 0;
>  }
>  
> -static inline int __br_ip4_hash(struct net_bridge_mdb_htable *mdb, __be32 ip)
> +static inline int __br_ip4_hash(struct net_bridge_mdb_htable *mdb, __be32 ip,
> +				__u16 vid)
>  {
> -	return jhash_1word(mdb->secret, (__force u32)ip) & (mdb->max - 1);
> +	return jhash_2words((__force u32)ip, vid, mdb->secret) & (mdb->max - 1);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
>  static inline int __br_ip6_hash(struct net_bridge_mdb_htable *mdb,
> -				const struct in6_addr *ip)
> +				const struct in6_addr *ip,
> +				__u16 vid)
>  {
> -	return jhash2((__force u32 *)ip->s6_addr32, 4, mdb->secret) & (mdb->max - 1);
> +	u32 addr = *(__force u32 *)ip->s6_addr32;
> +	return jhash_2words(addr, vid, mdb->secret) & (mdb->max - 1);
>  }
>  #endif
>  
> @@ -80,10 +85,10 @@ static inline int br_ip_hash(struct net_bridge_mdb_htable *mdb,
>  {
>  	switch (ip->proto) {
>  	case htons(ETH_P_IP):
> -		return __br_ip4_hash(mdb, ip->u.ip4);
> +		return __br_ip4_hash(mdb, ip->u.ip4, ip->vid);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	case htons(ETH_P_IPV6):
> -		return __br_ip6_hash(mdb, &ip->u.ip6);
> +		return __br_ip6_hash(mdb, &ip->u.ip6, ip->vid);
>  #endif
>  	}
>  	return 0;
> @@ -113,24 +118,27 @@ static struct net_bridge_mdb_entry *br_mdb_ip_get(
>  }
>  
>  static struct net_bridge_mdb_entry *br_mdb_ip4_get(
> -	struct net_bridge_mdb_htable *mdb, __be32 dst)
> +	struct net_bridge_mdb_htable *mdb, __be32 dst, __u16 vlan_tci)
>  {
>  	struct br_ip br_dst;
>  
>  	br_dst.u.ip4 = dst;
>  	br_dst.proto = htons(ETH_P_IP);
> +	br_dst.vid = (vlan_tci & VLAN_VID_MASK);

() around value not needed.
Same in all cases below, I am not repeating
this comment.

>  
>  	return br_mdb_ip_get(mdb, &br_dst);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
>  static struct net_bridge_mdb_entry *br_mdb_ip6_get(
> -	struct net_bridge_mdb_htable *mdb, const struct in6_addr *dst)
> +	struct net_bridge_mdb_htable *mdb, const struct in6_addr *dst,
> +	__u16 vlan_tci)
>  {
>  	struct br_ip br_dst;
>  
>  	br_dst.u.ip6 = *dst;
>  	br_dst.proto = htons(ETH_P_IPV6);
> +	br_dst.vid = vlan_tci & VLAN_VID_MASK;
>  
>  	return br_mdb_ip_get(mdb, &br_dst);
>  }
> @@ -692,7 +700,8 @@ err:
>  
>  static int br_ip4_multicast_add_group(struct net_bridge *br,
>  				      struct net_bridge_port *port,
> -				      __be32 group)
> +				      __be32 group,
> +				      __u16 vlan_tci)
>  {
>  	struct br_ip br_group;
>  
> @@ -701,6 +710,7 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
>  
>  	br_group.u.ip4 = group;
>  	br_group.proto = htons(ETH_P_IP);
> +	br_group.vid = vlan_tci & VLAN_VID_MASK;
>  
>  	return br_multicast_add_group(br, port, &br_group);
>  }
> @@ -708,7 +718,8 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
>  #if IS_ENABLED(CONFIG_IPV6)
>  static int br_ip6_multicast_add_group(struct net_bridge *br,
>  				      struct net_bridge_port *port,
> -				      const struct in6_addr *group)
> +				      const struct in6_addr *group,
> +				      __u16 vlan_tci)
>  {
>  	struct br_ip br_group;
>  
> @@ -717,6 +728,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
>  
>  	br_group.u.ip6 = *group;
>  	br_group.proto = htons(ETH_P_IPV6);
> +	br_group.vid = vlan_tci & VLAN_VID_MASK;
>  
>  	return br_multicast_add_group(br, port, &br_group);
>  }
> @@ -928,7 +940,8 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
>  			continue;
>  		}
>  
> -		err = br_ip4_multicast_add_group(br, port, group);
> +		err = br_ip4_multicast_add_group(br, port, group,
> +						skb->vlan_tci);


Pls align continuation line at (, same as other
code in this file. Same in all cases below, I am not repeating
this comment.

>  		if (err)
>  			break;
>  	}
> @@ -988,7 +1001,8 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
>  			continue;
>  		}
>  
> -		err = br_ip6_multicast_add_group(br, port, &grec->grec_mca);
> +		err = br_ip6_multicast_add_group(br, port, &grec->grec_mca,
> +						skb->vlan_tci);
>  		if (!err)
>  			break;
>  	}
> @@ -1106,7 +1120,8 @@ static int br_ip4_multicast_query(struct net_bridge *br,
>  	if (!group)
>  		goto out;
>  
> -	mp = br_mdb_ip4_get(mlock_dereference(br->mdb, br), group);
> +	mp = br_mdb_ip4_get(mlock_dereference(br->mdb, br), group,
> +				skb->vlan_tci);
>  	if (!mp)
>  		goto out;
>  
> @@ -1178,7 +1193,8 @@ static int br_ip6_multicast_query(struct net_bridge *br,
>  	if (!group)
>  		goto out;
>  
> -	mp = br_mdb_ip6_get(mlock_dereference(br->mdb, br), group);
> +	mp = br_mdb_ip6_get(mlock_dereference(br->mdb, br), group,
> +			skb->vlan_tci);
>  	if (!mp)
>  		goto out;
>  
> @@ -1262,7 +1278,8 @@ out:
>  
>  static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
> -					 __be32 group)
> +					 __be32 group,
> +					 __u16 vlan_tci)
>  {
>  	struct br_ip br_group;
>  
> @@ -1271,6 +1288,7 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  
>  	br_group.u.ip4 = group;
>  	br_group.proto = htons(ETH_P_IP);
> +	br_group.vid = (vlan_tci & VLAN_VID_MASK);
>  
>  	br_multicast_leave_group(br, port, &br_group);
>  }
> @@ -1278,7 +1296,8 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  #if IS_ENABLED(CONFIG_IPV6)
>  static void br_ip6_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
> -					 const struct in6_addr *group)
> +					 const struct in6_addr *group,
> +					 __u16 vlan_tci)
>  {
>  	struct br_ip br_group;
>  
> @@ -1287,6 +1306,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
>  
>  	br_group.u.ip6 = *group;
>  	br_group.proto = htons(ETH_P_IPV6);
> +	br_group.vid = (vlan_tci & VLAN_VID_MASK);
>  
>  	br_multicast_leave_group(br, port, &br_group);
>  }
> @@ -1369,7 +1389,8 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
>  	case IGMP_HOST_MEMBERSHIP_REPORT:
>  	case IGMPV2_HOST_MEMBERSHIP_REPORT:
>  		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> -		err = br_ip4_multicast_add_group(br, port, ih->group);
> +		err = br_ip4_multicast_add_group(br, port, ih->group,
> +						skb2->vlan_tci);
>  		break;
>  	case IGMPV3_HOST_MEMBERSHIP_REPORT:
>  		err = br_ip4_multicast_igmp3_report(br, port, skb2);
> @@ -1378,7 +1399,8 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
>  		err = br_ip4_multicast_query(br, port, skb2);
>  		break;
>  	case IGMP_HOST_LEAVE_MESSAGE:
> -		br_ip4_multicast_leave_group(br, port, ih->group);
> +		br_ip4_multicast_leave_group(br, port, ih->group,
> +						skb2->vlan_tci);
>  		break;
>  	}
>  
> @@ -1498,7 +1520,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  		}
>  		mld = (struct mld_msg *)skb_transport_header(skb2);
>  		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> -		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca);
> +		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca,
> +						 skb2->vlan_tci);
>  		break;
>  	    }
>  	case ICMPV6_MLD2_REPORT:
> @@ -1515,7 +1538,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  			goto out;
>  		}
>  		mld = (struct mld_msg *)skb_transport_header(skb2);
> -		br_ip6_multicast_leave_group(br, port, &mld->mld_mca);
> +		br_ip6_multicast_leave_group(br, port, &mld->mld_mca,
> +						skb2->vlan_tci);
>  	    }
>  	}
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 921b927..b6c56ab 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -61,6 +61,7 @@ struct br_ip
>  #endif
>  	} u;
>  	__be16		proto;
> +	__u16		vid;
>  };
>  
>  struct net_bridge_fdb_entry
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 3/5] bridge: Add vlan id to multicast groups

[Eric Dumazet]

On Thu, 2012-08-23 at 15:29 -0400, Vlad Yasevich wrote:
> Add vlan_id to multicasts groups so that we know which vlan each group belongs
> to and can correctly forward to appropriate vlan.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---

>  #if IS_ENABLED(CONFIG_IPV6)
>  static inline int __br_ip6_hash(struct net_bridge_mdb_htable *mdb,
> -				const struct in6_addr *ip)
> +				const struct in6_addr *ip,
> +				__u16 vid)
>  {
> -	return jhash2((__force u32 *)ip->s6_addr32, 4, mdb->secret) & (mdb->max - 1);
> +	u32 addr = *(__force u32 *)ip->s6_addr32;
> +	return jhash_2words(addr, vid, mdb->secret) & (mdb->max - 1);
>  }
>  #endif

It seems to me this is wrong.

Hashing only the first 32bits of the IPv6 address is not enough.

We know have ipv6_addr_hash()

[RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Vlad Yasevich]

Add a private ioctl to add and remove vlan configuration on bridge port.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/if_bridge.h |    2 +
 net/bridge/br_if.c        |   69 +++++++++++++++++++++++++++++++++++++++++++++
 net/bridge/br_ioctl.c     |   31 ++++++++++++++++++++
 net/bridge/br_private.h   |    2 +
 4 files changed, 104 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 288ff10..ab750dd 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -42,6 +42,8 @@
 #define BRCTL_SET_PORT_PRIORITY 16
 #define BRCTL_SET_PATH_COST 17
 #define BRCTL_GET_FDB_ENTRIES 18
+#define BRCTL_ADD_VLAN 19
+#define BRCTL_DEL_VLAN 20
 
 #define BR_STATE_DISABLED 0
 #define BR_STATE_LISTENING 1
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index e1144e1..90c1038 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -23,6 +23,7 @@
 #include <linux/if_ether.h>
 #include <linux/slab.h>
 #include <net/sock.h>
+#include <linux/if_vlan.h>
 
 #include "br_private.h"
 
@@ -441,6 +442,74 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	return 0;
 }
 
+/* Called with RTNL */
+int br_set_port_vlan(struct net_bridge_port *p, unsigned long vlan)
+{
+	unsigned long table_size = (VLAN_N_VID/sizeof(unsigned long)) + 1;
+	unsigned long *vid_map = NULL;
+	__u16 vid = (__u16) vlan + 1;
+
+	/* We are under lock so we can check this without rcu.
+	 * The vlan map is indexed by vid+1.  This way we can store
+	 * vid 0 (untagged) into the map as well.
+	 */
+	if (!p->vlan_map) {
+		vid_map = kzalloc(table_size, GFP_KERNEL);
+		if (!vid_map)
+			return -ENOMEM;
+
+		set_bit(vid, vid_map);
+		rcu_assign_pointer(p->vlan_map, vid_map);
+
+	} else {
+		/* Map is already allocated */
+		set_bit(vid, p->vlan_map);
+	}
+
+	return 0;
+}
+
+
+/* Called with RTNL */
+int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
+{
+	unsigned long first_bit;
+	unsigned long next_bit;
+	__u16 vid = (__u16) vlan+1;
+	unsigned long tbl_len = VLAN_N_VID+1;
+
+	if (!p->vlan_map)
+		return -EINVAL;
+
+	if (!test_bit(vlan, p->vlan_map))
+		return -EINVAL;
+
+	/* Check to see if any other vlans are in this table.  If this
+	 * is the last vlan, delete the whole table.  If this is not the
+	 * last vlan, just clear the bit.
+	 */
+	first_bit = find_first_bit(p->vlan_map, tbl_len);
+	next_bit = find_next_bit(p->vlan_map, tbl_len, (tbl_len - vid));
+
+	if ((__u16)first_bit != vid || (__u16)next_bit < tbl_len) {
+		/* There are other vlans still configured.  We can simply
+		 * clear our bit and be safe.
+		 */
+		clear_bit(vid, p->vlan_map);
+	} else {
+		/* This is the last vlan we are removing.  Replace the
+		 * map with a NULL pointer and free the old map
+		 */
+		unsigned long *map = rcu_dereference(p->vlan_map);
+
+		rcu_assign_pointer(p->vlan_map, NULL);
+		synchronize_net();
+		kfree(map);
+	}
+
+	return 0;
+}
+
 void __net_exit br_net_exit(struct net *net)
 {
 	struct net_device *dev;
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index 7222fe1..3a5b1f9 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -289,6 +289,37 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	case BRCTL_GET_FDB_ENTRIES:
 		return get_fdb_entries(br, (void __user *)args[1],
 				       args[2], args[3]);
+	case BRCTL_ADD_VLAN:
+	{
+		struct net_bridge_port *p;
+
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+
+		rcu_read_lock();
+		if ((p = br_get_port(br, args[1])) == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		rcu_read_unlock();
+		return br_set_port_vlan(p, args[2]);
+	}
+
+	case BRCTL_DEL_VLAN:
+	{
+		struct net_bridge_port *p;
+
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
+
+		rcu_read_lock();
+		if ((p = br_get_port(br, args[1])) == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		rcu_read_unlock();
+		br_set_port_vlan(p, args[2]);
+	}
 	}
 
 	return -EOPNOTSUPP;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index b6c56ab..5639c1c 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -402,6 +402,8 @@ extern int br_del_if(struct net_bridge *br,
 extern int br_min_mtu(const struct net_bridge *br);
 extern netdev_features_t br_features_recompute(struct net_bridge *br,
 	netdev_features_t features);
+extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
+extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
 
 /* br_input.c */
 extern int br_handle_frame_finish(struct sk_buff *skb);
-- 
1.7.7.6

Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Stephen Hemminger]

On Thu, 23 Aug 2012 15:29:54 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> Add a private ioctl to add and remove vlan configuration on bridge port.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  include/linux/if_bridge.h |    2 +
>  net/bridge/br_if.c        |   69 +++++++++++++++++++++++++++++++++++++++++++++
>  net/bridge/br_ioctl.c     |   31 ++++++++++++++++++++
>  net/bridge/br_private.h   |    2 +
>  4 files changed, 104 insertions(+), 0 deletions(-)


Don't go down this dead end.

The ioctl interface to bridge is deprecated because private ioctl's
can not be handled when running 32 bit user space on 64 bit kernel.

Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Vlad Yasevich]

On 08/23/2012 03:38 PM, Stephen Hemminger wrote:
> On Thu, 23 Aug 2012 15:29:54 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> Add a private ioctl to add and remove vlan configuration on bridge port.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>   include/linux/if_bridge.h |    2 +
>>   net/bridge/br_if.c        |   69 +++++++++++++++++++++++++++++++++++++++++++++
>>   net/bridge/br_ioctl.c     |   31 ++++++++++++++++++++
>>   net/bridge/br_private.h   |    2 +
>>   4 files changed, 104 insertions(+), 0 deletions(-)
>
>
> Don't go down this dead end.
>
> The ioctl interface to bridge is deprecated because private ioctl's
> can not be handled when running 32 bit user space on 64 bit kernel.
>


Went down this road because is was easier to do.  The plan is to do this 
over netlink

-vlad

Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Paul E. McKenney]

On Thu, Aug 23, 2012 at 03:29:54PM -0400, Vlad Yasevich wrote:
> Add a private ioctl to add and remove vlan configuration on bridge port.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

One question below...

> ---
>  include/linux/if_bridge.h |    2 +
>  net/bridge/br_if.c        |   69 +++++++++++++++++++++++++++++++++++++++++++++
>  net/bridge/br_ioctl.c     |   31 ++++++++++++++++++++
>  net/bridge/br_private.h   |    2 +
>  4 files changed, 104 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 288ff10..ab750dd 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -42,6 +42,8 @@
>  #define BRCTL_SET_PORT_PRIORITY 16
>  #define BRCTL_SET_PATH_COST 17
>  #define BRCTL_GET_FDB_ENTRIES 18
> +#define BRCTL_ADD_VLAN 19
> +#define BRCTL_DEL_VLAN 20
> 
>  #define BR_STATE_DISABLED 0
>  #define BR_STATE_LISTENING 1
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index e1144e1..90c1038 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -23,6 +23,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/slab.h>
>  #include <net/sock.h>
> +#include <linux/if_vlan.h>
> 
>  #include "br_private.h"
> 
> @@ -441,6 +442,74 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>  	return 0;
>  }
> 
> +/* Called with RTNL */
> +int br_set_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> +{
> +	unsigned long table_size = (VLAN_N_VID/sizeof(unsigned long)) + 1;
> +	unsigned long *vid_map = NULL;
> +	__u16 vid = (__u16) vlan + 1;
> +
> +	/* We are under lock so we can check this without rcu.
> +	 * The vlan map is indexed by vid+1.  This way we can store
> +	 * vid 0 (untagged) into the map as well.
> +	 */
> +	if (!p->vlan_map) {
> +		vid_map = kzalloc(table_size, GFP_KERNEL);
> +		if (!vid_map)
> +			return -ENOMEM;
> +
> +		set_bit(vid, vid_map);
> +		rcu_assign_pointer(p->vlan_map, vid_map);
> +
> +	} else {
> +		/* Map is already allocated */
> +		set_bit(vid, p->vlan_map);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +/* Called with RTNL */
> +int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> +{
> +	unsigned long first_bit;
> +	unsigned long next_bit;
> +	__u16 vid = (__u16) vlan+1;
> +	unsigned long tbl_len = VLAN_N_VID+1;
> +
> +	if (!p->vlan_map)
> +		return -EINVAL;
> +
> +	if (!test_bit(vlan, p->vlan_map))
> +		return -EINVAL;
> +
> +	/* Check to see if any other vlans are in this table.  If this
> +	 * is the last vlan, delete the whole table.  If this is not the
> +	 * last vlan, just clear the bit.
> +	 */
> +	first_bit = find_first_bit(p->vlan_map, tbl_len);
> +	next_bit = find_next_bit(p->vlan_map, tbl_len, (tbl_len - vid));
> +
> +	if ((__u16)first_bit != vid || (__u16)next_bit < tbl_len) {
> +		/* There are other vlans still configured.  We can simply
> +		 * clear our bit and be safe.
> +		 */
> +		clear_bit(vid, p->vlan_map);
> +	} else {
> +		/* This is the last vlan we are removing.  Replace the
> +		 * map with a NULL pointer and free the old map
> +		 */
> +		unsigned long *map = rcu_dereference(p->vlan_map);
> +
> +		rcu_assign_pointer(p->vlan_map, NULL);
> +		synchronize_net();
> +		kfree(map);
> +	}
> +
> +	return 0;
> +}
> +
>  void __net_exit br_net_exit(struct net *net)
>  {
>  	struct net_device *dev;
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 7222fe1..3a5b1f9 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -289,6 +289,37 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	case BRCTL_GET_FDB_ENTRIES:
>  		return get_fdb_entries(br, (void __user *)args[1],
>  				       args[2], args[3]);
> +	case BRCTL_ADD_VLAN:
> +	{
> +		struct net_bridge_port *p;
> +
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		rcu_read_lock();
> +		if ((p = br_get_port(br, args[1])) == NULL) {
> +			rcu_read_unlock();
> +			return -EINVAL;
> +		}
> +		rcu_read_unlock();

Why is it safe to pass "p" out of the RCU read-side critical section?
I don't see that br_get_port() does anything to make this safe, at least
not in v3.5.

> +		return br_set_port_vlan(p, args[2]);
> +	}
> +
> +	case BRCTL_DEL_VLAN:
> +	{
> +		struct net_bridge_port *p;
> +
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		rcu_read_lock();
> +		if ((p = br_get_port(br, args[1])) == NULL) {
> +			rcu_read_unlock();
> +			return -EINVAL;
> +		}
> +		rcu_read_unlock();
> +		br_set_port_vlan(p, args[2]);
> +	}
>  	}
> 
>  	return -EOPNOTSUPP;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index b6c56ab..5639c1c 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -402,6 +402,8 @@ extern int br_del_if(struct net_bridge *br,
>  extern int br_min_mtu(const struct net_bridge *br);
>  extern netdev_features_t br_features_recompute(struct net_bridge *br,
>  	netdev_features_t features);
> +extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> +extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> 
>  /* br_input.c */
>  extern int br_handle_frame_finish(struct sk_buff *skb);
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Stephen Hemminger]

On Fri, 24 Aug 2012 10:56:16 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> > +			return -EPERM;
> > +
> > +		rcu_read_lock();
> > +		if ((p = br_get_port(br, args[1])) == NULL) {
> > +			rcu_read_unlock();
> > +			return -EINVAL;
> > +		}
> > +		rcu_read_unlock();  
> 
> Why is it safe to pass "p" out of the RCU read-side critical section?
> I don't see that br_get_port() does anything to make this safe, at least
> not in v3.5.

It would be accidentally protected by rtnl?

Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Vlad Yasevich]

On 08/24/2012 01:56 PM, Paul E. McKenney wrote:
> On Thu, Aug 23, 2012 at 03:29:54PM -0400, Vlad Yasevich wrote:
>> Add a private ioctl to add and remove vlan configuration on bridge port.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>
> One question below...
>
> index 7222fe1..3a5b1f9 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -289,6 +289,37 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>>   	case BRCTL_GET_FDB_ENTRIES:
>>   		return get_fdb_entries(br, (void __user *)args[1],
>>   				       args[2], args[3]);
>> +	case BRCTL_ADD_VLAN:
>> +	{
>> +		struct net_bridge_port *p;
>> +
>> +		if (!capable(CAP_NET_ADMIN))
>> +			return -EPERM;
>> +
>> +		rcu_read_lock();
>> +		if ((p = br_get_port(br, args[1])) == NULL) {
>> +			rcu_read_unlock();
>> +			return -EINVAL;
>> +		}
>> +		rcu_read_unlock();
>
> Why is it safe to pass "p" out of the RCU read-side critical section?
> I don't see that br_get_port() does anything to make this safe, at least
> not in v3.5.
>

You right.  It not really safe.  As Stephen pointed out, it is 
accidentally protected by rtnl, so it will not go away.  However, that's 
not a good excuse and I've already fixed it in my tree.

thanks
-vlad
>> +		return br_set_port_vlan(p, args[2]);
>> +	}
>> +
>> +	case BRCTL_DEL_VLAN:
>> +	{
>> +		struct net_bridge_port *p;
>> +
>> +		if (!capable(CAP_NET_ADMIN))
>> +			return -EPERM;
>> +
>> +		rcu_read_lock();
>> +		if ((p = br_get_port(br, args[1])) == NULL) {
>> +			rcu_read_unlock();
>> +			return -EINVAL;
>> +		}
>> +		rcu_read_unlock();
>> +		br_set_port_vlan(p, args[2]);
>> +	}
>>   	}
>>
>>   	return -EOPNOTSUPP;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index b6c56ab..5639c1c 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -402,6 +402,8 @@ extern int br_del_if(struct net_bridge *br,
>>   extern int br_min_mtu(const struct net_bridge *br);
>>   extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>   	netdev_features_t features);
>> +extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>> +extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>
>>   /* br_input.c */
>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>> --
>> 1.7.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports

[Michael S. Tsirkin]

On Thu, Aug 23, 2012 at 03:29:54PM -0400, Vlad Yasevich wrote:
> Add a private ioctl to add and remove vlan configuration on bridge port.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  include/linux/if_bridge.h |    2 +
>  net/bridge/br_if.c        |   69 +++++++++++++++++++++++++++++++++++++++++++++
>  net/bridge/br_ioctl.c     |   31 ++++++++++++++++++++
>  net/bridge/br_private.h   |    2 +
>  4 files changed, 104 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 288ff10..ab750dd 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -42,6 +42,8 @@
>  #define BRCTL_SET_PORT_PRIORITY 16
>  #define BRCTL_SET_PATH_COST 17
>  #define BRCTL_GET_FDB_ENTRIES 18
> +#define BRCTL_ADD_VLAN 19
> +#define BRCTL_DEL_VLAN 20
>  
>  #define BR_STATE_DISABLED 0
>  #define BR_STATE_LISTENING 1


That's not a lot of documentation especially
considering tricks such as using vlan 0
to mean untagged (an internal convention in kernel).

> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index e1144e1..90c1038 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -23,6 +23,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/slab.h>
>  #include <net/sock.h>
> +#include <linux/if_vlan.h>
>  
>  #include "br_private.h"
>  
> @@ -441,6 +442,74 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>  	return 0;
>  }
>  
> +/* Called with RTNL */
> +int br_set_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> +{
> +	unsigned long table_size = (VLAN_N_VID/sizeof(unsigned long)) + 1;

Use DECLARE_BITMAP[VLAN_N_VID + 1]?

> +	unsigned long *vid_map = NULL;
> +	__u16 vid = (__u16) vlan + 1;

Don't put space after cast: (__u16)vlan.

In fact, cast is not needed here at all.
Applies to other places in this patch that do this.


> +
> +	/* We are under lock so we can check this without rcu.
> +	 * The vlan map is indexed by vid+1.  This way we can store
> +	 * vid 0 (untagged) into the map as well.
> +	 */

Would be better to put this documentation in struct
definition where vid_map is defined.

> +	if (!p->vlan_map) {
> +		vid_map = kzalloc(table_size, GFP_KERNEL);
> +		if (!vid_map)
> +			return -ENOMEM;
> +
> +		set_bit(vid, vid_map);

What prevents vid from being > table_size * BITS_PER_LONG?

> +		rcu_assign_pointer(p->vlan_map, vid_map);
> +
> +	} else {
> +		/* Map is already allocated */
> +		set_bit(vid, p->vlan_map);

This should call rcu_dereference_protected.

> +	}
> +
> +	return 0;
> +}
> +
> +
> +/* Called with RTNL */
> +int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> +{
> +	unsigned long first_bit;
> +	unsigned long next_bit;
> +	__u16 vid = (__u16) vlan+1;
> +	unsigned long tbl_len = VLAN_N_VID+1;

Same as above.

> +
> +	if (!p->vlan_map)
> +		return -EINVAL;
> +
> +	if (!test_bit(vlan, p->vlan_map))
> +		return -EINVAL;
> +
> +	/* Check to see if any other vlans are in this table.  If this
> +	 * is the last vlan, delete the whole table.  If this is not the
> +	 * last vlan, just clear the bit.
> +	 */
> +	first_bit = find_first_bit(p->vlan_map, tbl_len);
> +	next_bit = find_next_bit(p->vlan_map, tbl_len, (tbl_len - vid));

() not needed around arguments.

> +
> +	if ((__u16)first_bit != vid || (__u16)next_bit < tbl_len) {

What are these type casts doing? How can you get a value > 0xffff?

> +		/* There are other vlans still configured.  We can simply
> +		 * clear our bit and be safe.
> +		 */
> +		clear_bit(vid, p->vlan_map);
> +	} else {
> +		/* This is the last vlan we are removing.  Replace the
> +		 * map with a NULL pointer and free the old map
> +		 */
> +		unsigned long *map = rcu_dereference(p->vlan_map);
> +
> +		rcu_assign_pointer(p->vlan_map, NULL);
> +		synchronize_net();
> +		kfree(map);
> +	}
> +
> +	return 0;
> +}
> +
>  void __net_exit br_net_exit(struct net *net)
>  {
>  	struct net_device *dev;
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 7222fe1..3a5b1f9 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -289,6 +289,37 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>  	case BRCTL_GET_FDB_ENTRIES:
>  		return get_fdb_entries(br, (void __user *)args[1],
>  				       args[2], args[3]);
> +	case BRCTL_ADD_VLAN:
> +	{
> +		struct net_bridge_port *p;
> +
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		rcu_read_lock();
> +		if ((p = br_get_port(br, args[1])) == NULL) {
> +			rcu_read_unlock();
> +			return -EINVAL;
> +		}
> +		rcu_read_unlock();
> +		return br_set_port_vlan(p, args[2]);

So this sets vlan but does not synchronize RCU.
This means if this is the 1st vlan we set,
we return to userspace and afterwards
interfaces get packets with a wrong vlan.

I am guessing all these ioctls should synchronize_net.

> +	}
> +
> +	case BRCTL_DEL_VLAN:
> +	{
> +		struct net_bridge_port *p;
> +
> +		if (!capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		rcu_read_lock();
> +		if ((p = br_get_port(br, args[1])) == NULL) {
> +			rcu_read_unlock();
> +			return -EINVAL;
> +		}
> +		rcu_read_unlock();
> +		br_set_port_vlan(p, args[2]);

Same problem as above as synchronize_net is not
invoked always.

> +	}
>  	}
>  
>  	return -EOPNOTSUPP;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index b6c56ab..5639c1c 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -402,6 +402,8 @@ extern int br_del_if(struct net_bridge *br,
>  extern int br_min_mtu(const struct net_bridge *br);
>  extern netdev_features_t br_features_recompute(struct net_bridge *br,
>  	netdev_features_t features);
> +extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> +extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>  
>  /* br_input.c */
>  extern int br_handle_frame_finish(struct sk_buff *skb);
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich]

Add a binary sysfs file that will dump out vlans currently configured on the
port.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/if_bridge.h |    1 +
 net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
 net/bridge/br_private.h   |    2 ++
 net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index ab750dd..d0f869b 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -20,6 +20,7 @@
 #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
 #define SYSFS_BRIDGE_PORT_ATTR	"brport"
 #define SYSFS_BRIDGE_PORT_LINK	"bridge"
+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
 
 #define BRCTL_VERSION 1
 
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 90c1038..3963748 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
 	return 0;
 }
 
+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
+			unsigned long max, unsigned long skip)
+{
+	unsigned long *map;
+	unsigned short *vid = (unsigned short *)buf;
+	unsigned short i;
+	int num = 0;
+
+	if (skip > (VLAN_N_VID+1))
+		return -EINVAL;
+
+	memset(buf, 0, max * sizeof(unsigned short));
+
+	rcu_read_lock();
+	map = rcu_dereference(p->vlan_map);
+	if (!map)
+		goto out;
+
+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
+		if (test_bit(i, map)) {
+			if (num > max)
+				goto out;
+
+			*vid = i-1;
+			vid++;
+			num++;
+		}
+	}
+out:
+	rcu_read_unlock();
+
+	return num*sizeof(unsigned short);
+}
+
 void __net_exit br_net_exit(struct net *net)
 {
 	struct net_device *dev;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 5639c1c..cf95cd7 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
 	netdev_features_t features);
 extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
 extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
+				unsigned long max, unsigned long skip);
 
 /* br_input.c */
 extern int br_handle_frame_finish(struct sk_buff *skb);
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 13b36bd..a81e2ef 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
 };
 
 /*
+ * Export the vlan table for a given port as a binary file.
+ * The records are unsgined shorts.
+ *
+ * Returns the number of bytes read.
+ */
+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *bin_attr,
+				char *buf, loff_t off, size_t count)
+{
+	struct net_bridge_port *p = to_brport(kobj);
+
+	return br_port_fill_vlans(p, buf,
+				count/sizeof(unsigned short),
+				off/sizeof(unsigned short));
+}
+
+static struct bin_attribute port_vlans = {
+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
+		  .mode = S_IRUGO, },
+	.read = brport_vlans_read,
+};
+
+/*
  * Add sysfs entries to ethernet device added to a bridge.
  * Creates a brport subdirectory with bridge attributes.
  * Puts symlink in bridge's brif subdirectory
@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
 			return err;
 	}
 
+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
+	if (err) {
+		return err;
+	}
+
 	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
 	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
 }
-- 
1.7.7.6

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Michael S. Tsirkin]

On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> Add a binary sysfs file that will dump out vlans currently configured on the
> port.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

So what's the format here? I could not tell.
List of vlans? Why binary? Why not make it text in that case?
This would also allow reporting "all" if filtering
is disabled and "untagged" for untagged packets.

> ---
>  include/linux/if_bridge.h |    1 +
>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>  net/bridge/br_private.h   |    2 ++
>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>  4 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index ab750dd..d0f869b 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -20,6 +20,7 @@
>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>  
>  #define BRCTL_VERSION 1
>  
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 90c1038..3963748 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>  	return 0;
>  }
>  
> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> +			unsigned long max, unsigned long skip)
> +{
> +	unsigned long *map;
> +	unsigned short *vid = (unsigned short *)buf;
> +	unsigned short i;
> +	int num = 0;
> +
> +	if (skip > (VLAN_N_VID+1))
> +		return -EINVAL;
> +
> +	memset(buf, 0, max * sizeof(unsigned short));

Isn't max is in bytes? why is this safe?

> +
> +	rcu_read_lock();
> +	map = rcu_dereference(p->vlan_map);
> +	if (!map)
> +		goto out;
> +
> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {

Isn't skip in bytes too? Why do you compare it to i which is
in dwords?

> +		if (test_bit(i, map)) {
> +			if (num > max)
> +				goto out;
> +
> +			*vid = i-1;
> +			vid++;
> +			num++;
> +		}
> +	}
> +out:
> +	rcu_read_unlock();
> +
> +	return num*sizeof(unsigned short);
> +}
> +
>  void __net_exit br_net_exit(struct net *net)
>  {
>  	struct net_device *dev;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 5639c1c..cf95cd7 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>  	netdev_features_t features);
>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> +				unsigned long max, unsigned long skip);
>  
>  /* br_input.c */
>  extern int br_handle_frame_finish(struct sk_buff *skb);
> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index 13b36bd..a81e2ef 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>  };
>  
>  /*
> + * Export the vlan table for a given port as a binary file.
> + * The records are unsgined shorts.
> + *
> + * Returns the number of bytes read.
> + */
> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> +				struct bin_attribute *bin_attr,
> +				char *buf, loff_t off, size_t count)
> +{
> +	struct net_bridge_port *p = to_brport(kobj);
> +
> +	return br_port_fill_vlans(p, buf,
> +				count/sizeof(unsigned short),
> +				off/sizeof(unsigned short));
> +}
> +
> +static struct bin_attribute port_vlans = {
> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> +		  .mode = S_IRUGO, },
> +	.read = brport_vlans_read,
> +};
> +
> +/*
>   * Add sysfs entries to ethernet device added to a bridge.
>   * Creates a brport subdirectory with bridge attributes.
>   * Puts symlink in bridge's brif subdirectory
> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>  			return err;
>  	}
>  
> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> +	if (err) {
> +		return err;
> +	}
> +
>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>  }
> -- 
> 1.7.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich]

On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
>> Add a binary sysfs file that will dump out vlans currently configured on the
>> port.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>
> So what's the format here? I could not tell.
> List of vlans? Why binary? Why not make it text in that case?
> This would also allow reporting "all" if filtering
> is disabled and "untagged" for untagged packets.

I decided to do binary because text may result in more then page worth 
of data.  The display tool will know how to display things properly.

-vlad

>
>> ---
>>   include/linux/if_bridge.h |    1 +
>>   net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>>   net/bridge/br_private.h   |    2 ++
>>   net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>>   4 files changed, 65 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index ab750dd..d0f869b 100644
>> --- a/include/linux/if_bridge.h
>> +++ b/include/linux/if_bridge.h
>> @@ -20,6 +20,7 @@
>>   #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>>   #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>>   #define SYSFS_BRIDGE_PORT_LINK	"bridge"
>> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>>
>>   #define BRCTL_VERSION 1
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 90c1038..3963748 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>>   	return 0;
>>   }
>>
>> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
>> +			unsigned long max, unsigned long skip)
>> +{
>> +	unsigned long *map;
>> +	unsigned short *vid = (unsigned short *)buf;
>> +	unsigned short i;
>> +	int num = 0;
>> +
>> +	if (skip > (VLAN_N_VID+1))
>> +		return -EINVAL;
>> +
>> +	memset(buf, 0, max * sizeof(unsigned short));
>
> Isn't max is in bytes? why is this safe?
>
>> +
>> +	rcu_read_lock();
>> +	map = rcu_dereference(p->vlan_map);
>> +	if (!map)
>> +		goto out;
>> +
>> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
>
> Isn't skip in bytes too? Why do you compare it to i which is
> in dwords?
>
>> +		if (test_bit(i, map)) {
>> +			if (num > max)
>> +				goto out;
>> +
>> +			*vid = i-1;
>> +			vid++;
>> +			num++;
>> +		}
>> +	}
>> +out:
>> +	rcu_read_unlock();
>> +
>> +	return num*sizeof(unsigned short);
>> +}
>> +
>>   void __net_exit br_net_exit(struct net *net)
>>   {
>>   	struct net_device *dev;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 5639c1c..cf95cd7 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>   	netdev_features_t features);
>>   extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>   extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
>> +				unsigned long max, unsigned long skip);
>>
>>   /* br_input.c */
>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>> index 13b36bd..a81e2ef 100644
>> --- a/net/bridge/br_sysfs_if.c
>> +++ b/net/bridge/br_sysfs_if.c
>> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>>   };
>>
>>   /*
>> + * Export the vlan table for a given port as a binary file.
>> + * The records are unsgined shorts.
>> + *
>> + * Returns the number of bytes read.
>> + */
>> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
>> +				struct bin_attribute *bin_attr,
>> +				char *buf, loff_t off, size_t count)
>> +{
>> +	struct net_bridge_port *p = to_brport(kobj);
>> +
>> +	return br_port_fill_vlans(p, buf,
>> +				count/sizeof(unsigned short),
>> +				off/sizeof(unsigned short));
>> +}
>> +
>> +static struct bin_attribute port_vlans = {
>> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
>> +		  .mode = S_IRUGO, },
>> +	.read = brport_vlans_read,
>> +};
>> +
>> +/*
>>    * Add sysfs entries to ethernet device added to a bridge.
>>    * Creates a brport subdirectory with bridge attributes.
>>    * Puts symlink in bridge's brif subdirectory
>> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>>   			return err;
>>   	}
>>
>> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
>> +	if (err) {
>> +		return err;
>> +	}
>> +
>>   	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>>   	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>>   }
>> --
>> 1.7.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>Add a binary sysfs file that will dump out vlans currently configured on the
> >>port.
> >>
> >>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >
> >So what's the format here? I could not tell.
> >List of vlans? Why binary? Why not make it text in that case?
> >This would also allow reporting "all" if filtering
> >is disabled and "untagged" for untagged packets.
> 
> I decided to do binary because text may result in more then page
> worth of data.

Well yes, you need 3 bytes to print a vid and 1 extra for a space.  But
in binary, 2 bytes per VID and there are 4K valid VIDs so binary needs
more than 1 page too?

If using binary I would go for bit per valid bit
this solves it nicely.
We can have a separate field that shows whether untagged
packets are filtered.

> The display tool will know how to display things
> properly.
> 
> -vlad

I think you know that one of the points of sysfs
is to allow use without tools.
Tools are happier using other interfaces such as
ioctl or netlink.



> >
> >>---
> >>  include/linux/if_bridge.h |    1 +
> >>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
> >>  net/bridge/br_private.h   |    2 ++
> >>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
> >>  4 files changed, 65 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>index ab750dd..d0f869b 100644
> >>--- a/include/linux/if_bridge.h
> >>+++ b/include/linux/if_bridge.h
> >>@@ -20,6 +20,7 @@
> >>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
> >>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> >>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>
> >>  #define BRCTL_VERSION 1
> >>
> >>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>index 90c1038..3963748 100644
> >>--- a/net/bridge/br_if.c
> >>+++ b/net/bridge/br_if.c
> >>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>  	return 0;
> >>  }
> >>
> >>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>+			unsigned long max, unsigned long skip)
> >>+{
> >>+	unsigned long *map;
> >>+	unsigned short *vid = (unsigned short *)buf;
> >>+	unsigned short i;
> >>+	int num = 0;
> >>+
> >>+	if (skip > (VLAN_N_VID+1))
> >>+		return -EINVAL;
> >>+
> >>+	memset(buf, 0, max * sizeof(unsigned short));
> >
> >Isn't max is in bytes? why is this safe?
> >
> >>+
> >>+	rcu_read_lock();
> >>+	map = rcu_dereference(p->vlan_map);
> >>+	if (!map)
> >>+		goto out;
> >>+
> >>+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >
> >Isn't skip in bytes too? Why do you compare it to i which is
> >in dwords?
> >
> >>+		if (test_bit(i, map)) {
> >>+			if (num > max)
> >>+				goto out;
> >>+
> >>+			*vid = i-1;
> >>+			vid++;
> >>+			num++;
> >>+		}
> >>+	}
> >>+out:
> >>+	rcu_read_unlock();
> >>+
> >>+	return num*sizeof(unsigned short);
> >>+}
> >>+
> >>  void __net_exit br_net_exit(struct net *net)
> >>  {
> >>  	struct net_device *dev;
> >>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>index 5639c1c..cf95cd7 100644
> >>--- a/net/bridge/br_private.h
> >>+++ b/net/bridge/br_private.h
> >>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>  	netdev_features_t features);
> >>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>+				unsigned long max, unsigned long skip);
> >>
> >>  /* br_input.c */
> >>  extern int br_handle_frame_finish(struct sk_buff *skb);
> >>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>index 13b36bd..a81e2ef 100644
> >>--- a/net/bridge/br_sysfs_if.c
> >>+++ b/net/bridge/br_sysfs_if.c
> >>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>  };
> >>
> >>  /*
> >>+ * Export the vlan table for a given port as a binary file.
> >>+ * The records are unsgined shorts.
> >>+ *
> >>+ * Returns the number of bytes read.
> >>+ */
> >>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>+				struct bin_attribute *bin_attr,
> >>+				char *buf, loff_t off, size_t count)
> >>+{
> >>+	struct net_bridge_port *p = to_brport(kobj);
> >>+
> >>+	return br_port_fill_vlans(p, buf,
> >>+				count/sizeof(unsigned short),
> >>+				off/sizeof(unsigned short));
> >>+}
> >>+
> >>+static struct bin_attribute port_vlans = {
> >>+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>+		  .mode = S_IRUGO, },
> >>+	.read = brport_vlans_read,
> >>+};
> >>+
> >>+/*
> >>   * Add sysfs entries to ethernet device added to a bridge.
> >>   * Creates a brport subdirectory with bridge attributes.
> >>   * Puts symlink in bridge's brif subdirectory
> >>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>  			return err;
> >>  	}
> >>
> >>+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>+	if (err) {
> >>+		return err;
> >>+	}
> >>+
> >>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>  }
> >>--
> >>1.7.7.6
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich]

On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
>>>> Add a binary sysfs file that will dump out vlans currently configured on the
>>>> port.
>>>>
>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>
>>> So what's the format here? I could not tell.
>>> List of vlans? Why binary? Why not make it text in that case?
>>> This would also allow reporting "all" if filtering
>>> is disabled and "untagged" for untagged packets.
>>
>> I decided to do binary because text may result in more then page
>> worth of data.
>
> Well yes, you need 3 bytes to print a vid and 1 extra for a space.

4 bytes per vid and 1 for space or NULL.

>  But
> in binary, 2 bytes per VID and there are 4K valid VIDs so binary needs
> more than 1 page too?
>
> If using binary I would go for bit per valid bit
> this solves it nicely.

True.  The tool reads it in chunks.  I thought about dumping the bitmap, 
but that meant that made the tool more complex as it had to know the 
bitmap construction.

> We can have a separate field that shows whether untagged
> packets are filtered.
>
>> The display tool will know how to display things
>> properly.
>>
>> -vlad
>
> I think you know that one of the points of sysfs
> is to allow use without tools.
> Tools are happier using other interfaces such as
> ioctl or netlink.
>

I initially though of creating a sysfs object per vlan.  That would have 
made it easy to see which vlans are configured without any tools.
But that could result in a lot of objects being created, so I abandoned it.

I did think about a text interface, but due to a page of output 
limitation, I didn't go that route.  The reason is that if someone cats 
the file, they may not see all the vlans configured.  So I decided on 
the binary interface, since a binary interface with a tool to read it 
could avoid the single page limitation.

-vlad
>
>
>>>
>>>> ---
>>>>   include/linux/if_bridge.h |    1 +
>>>>   net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>>>>   net/bridge/br_private.h   |    2 ++
>>>>   net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>>>>   4 files changed, 65 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>> index ab750dd..d0f869b 100644
>>>> --- a/include/linux/if_bridge.h
>>>> +++ b/include/linux/if_bridge.h
>>>> @@ -20,6 +20,7 @@
>>>>   #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>>>>   #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>>>>   #define SYSFS_BRIDGE_PORT_LINK	"bridge"
>>>> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>>>>
>>>>   #define BRCTL_VERSION 1
>>>>
>>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>>> index 90c1038..3963748 100644
>>>> --- a/net/bridge/br_if.c
>>>> +++ b/net/bridge/br_if.c
>>>> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>>>>   	return 0;
>>>>   }
>>>>
>>>> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
>>>> +			unsigned long max, unsigned long skip)
>>>> +{
>>>> +	unsigned long *map;
>>>> +	unsigned short *vid = (unsigned short *)buf;
>>>> +	unsigned short i;
>>>> +	int num = 0;
>>>> +
>>>> +	if (skip > (VLAN_N_VID+1))
>>>> +		return -EINVAL;
>>>> +
>>>> +	memset(buf, 0, max * sizeof(unsigned short));
>>>
>>> Isn't max is in bytes? why is this safe?
>>>
>>>> +
>>>> +	rcu_read_lock();
>>>> +	map = rcu_dereference(p->vlan_map);
>>>> +	if (!map)
>>>> +		goto out;
>>>> +
>>>> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
>>>
>>> Isn't skip in bytes too? Why do you compare it to i which is
>>> in dwords?
>>>
>>>> +		if (test_bit(i, map)) {
>>>> +			if (num > max)
>>>> +				goto out;
>>>> +
>>>> +			*vid = i-1;
>>>> +			vid++;
>>>> +			num++;
>>>> +		}
>>>> +	}
>>>> +out:
>>>> +	rcu_read_unlock();
>>>> +
>>>> +	return num*sizeof(unsigned short);
>>>> +}
>>>> +
>>>>   void __net_exit br_net_exit(struct net *net)
>>>>   {
>>>>   	struct net_device *dev;
>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>> index 5639c1c..cf95cd7 100644
>>>> --- a/net/bridge/br_private.h
>>>> +++ b/net/bridge/br_private.h
>>>> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>>>   	netdev_features_t features);
>>>>   extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>   extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
>>>> +				unsigned long max, unsigned long skip);
>>>>
>>>>   /* br_input.c */
>>>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>>>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>>>> index 13b36bd..a81e2ef 100644
>>>> --- a/net/bridge/br_sysfs_if.c
>>>> +++ b/net/bridge/br_sysfs_if.c
>>>> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>>>>   };
>>>>
>>>>   /*
>>>> + * Export the vlan table for a given port as a binary file.
>>>> + * The records are unsgined shorts.
>>>> + *
>>>> + * Returns the number of bytes read.
>>>> + */
>>>> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
>>>> +				struct bin_attribute *bin_attr,
>>>> +				char *buf, loff_t off, size_t count)
>>>> +{
>>>> +	struct net_bridge_port *p = to_brport(kobj);
>>>> +
>>>> +	return br_port_fill_vlans(p, buf,
>>>> +				count/sizeof(unsigned short),
>>>> +				off/sizeof(unsigned short));
>>>> +}
>>>> +
>>>> +static struct bin_attribute port_vlans = {
>>>> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
>>>> +		  .mode = S_IRUGO, },
>>>> +	.read = brport_vlans_read,
>>>> +};
>>>> +
>>>> +/*
>>>>    * Add sysfs entries to ethernet device added to a bridge.
>>>>    * Creates a brport subdirectory with bridge attributes.
>>>>    * Puts symlink in bridge's brif subdirectory
>>>> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>>>>   			return err;
>>>>   	}
>>>>
>>>> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
>>>> +	if (err) {
>>>> +		return err;
>>>> +	}
>>>> +
>>>>   	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>>>>   	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>>>>   }
>>>> --
>>>> 1.7.7.6
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>>>Add a binary sysfs file that will dump out vlans currently configured on the
> >>>>port.
> >>>>
> >>>>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>>
> >>>So what's the format here? I could not tell.
> >>>List of vlans? Why binary? Why not make it text in that case?
> >>>This would also allow reporting "all" if filtering
> >>>is disabled and "untagged" for untagged packets.
> >>
> >>I decided to do binary because text may result in more then page
> >>worth of data.
> >
> >Well yes, you need 3 bytes to print a vid and 1 extra for a space.
> 
> 4 bytes per vid and 1 for space or NULL.

Why 4? It's 12 bit so you can print in hex: 'ABC ' 
Whatever :)

> > But
> >in binary, 2 bytes per VID and there are 4K valid VIDs so binary needs
> >more than 1 page too?
> >
> >If using binary I would go for bit per valid bit
> >this solves it nicely.
> 
> True.  The tool reads it in chunks.  I thought about dumping the
> bitmap, but that meant that made the tool more complex as it had to
> know the bitmap construction.

To me, it makes more sense than knowing that vids are
zero-extended to 16 bytes and packed in an array,
and that you need to read it in chunks.
Could be just me :)

> >We can have a separate field that shows whether untagged
> >packets are filtered.
> >
> >>The display tool will know how to display things
> >>properly.
> >>
> >>-vlad
> >
> >I think you know that one of the points of sysfs
> >is to allow use without tools.
> >Tools are happier using other interfaces such as
> >ioctl or netlink.
> >
> 
> I initially though of creating a sysfs object per vlan.  That would
> have made it easy to see which vlans are configured without any
> tools.
> But that could result in a lot of objects being created, so I abandoned it.
> 
> I did think about a text interface, but due to a page of output
> limitation, I didn't go that route.  The reason is that if someone
> cats the file, they may not see all the vlans configured.  So I
> decided on the binary interface, since a binary interface with a
> tool to read it could avoid the single page limitation.
> 
> -vlad

Maybe it's not needed in sysfs then - expose it to
brctl or whatever.

> >
> >
> >>>
> >>>>---
> >>>>  include/linux/if_bridge.h |    1 +
> >>>>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
> >>>>  net/bridge/br_private.h   |    2 ++
> >>>>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
> >>>>  4 files changed, 65 insertions(+), 0 deletions(-)
> >>>>
> >>>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>>>index ab750dd..d0f869b 100644
> >>>>--- a/include/linux/if_bridge.h
> >>>>+++ b/include/linux/if_bridge.h
> >>>>@@ -20,6 +20,7 @@
> >>>>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>>>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
> >>>>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> >>>>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>>>
> >>>>  #define BRCTL_VERSION 1
> >>>>
> >>>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>>>index 90c1038..3963748 100644
> >>>>--- a/net/bridge/br_if.c
> >>>>+++ b/net/bridge/br_if.c
> >>>>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>>>  	return 0;
> >>>>  }
> >>>>
> >>>>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>>>+			unsigned long max, unsigned long skip)
> >>>>+{
> >>>>+	unsigned long *map;
> >>>>+	unsigned short *vid = (unsigned short *)buf;
> >>>>+	unsigned short i;
> >>>>+	int num = 0;
> >>>>+
> >>>>+	if (skip > (VLAN_N_VID+1))
> >>>>+		return -EINVAL;
> >>>>+
> >>>>+	memset(buf, 0, max * sizeof(unsigned short));
> >>>
> >>>Isn't max is in bytes? why is this safe?
> >>>
> >>>>+
> >>>>+	rcu_read_lock();
> >>>>+	map = rcu_dereference(p->vlan_map);
> >>>>+	if (!map)
> >>>>+		goto out;
> >>>>+
> >>>>+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >>>
> >>>Isn't skip in bytes too? Why do you compare it to i which is
> >>>in dwords?
> >>>
> >>>>+		if (test_bit(i, map)) {
> >>>>+			if (num > max)
> >>>>+				goto out;
> >>>>+
> >>>>+			*vid = i-1;
> >>>>+			vid++;
> >>>>+			num++;
> >>>>+		}
> >>>>+	}
> >>>>+out:
> >>>>+	rcu_read_unlock();
> >>>>+
> >>>>+	return num*sizeof(unsigned short);
> >>>>+}
> >>>>+
> >>>>  void __net_exit br_net_exit(struct net *net)
> >>>>  {
> >>>>  	struct net_device *dev;
> >>>>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>>>index 5639c1c..cf95cd7 100644
> >>>>--- a/net/bridge/br_private.h
> >>>>+++ b/net/bridge/br_private.h
> >>>>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>>>  	netdev_features_t features);
> >>>>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>>>+				unsigned long max, unsigned long skip);
> >>>>
> >>>>  /* br_input.c */
> >>>>  extern int br_handle_frame_finish(struct sk_buff *skb);
> >>>>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>>>index 13b36bd..a81e2ef 100644
> >>>>--- a/net/bridge/br_sysfs_if.c
> >>>>+++ b/net/bridge/br_sysfs_if.c
> >>>>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>>>  };
> >>>>
> >>>>  /*
> >>>>+ * Export the vlan table for a given port as a binary file.
> >>>>+ * The records are unsgined shorts.
> >>>>+ *
> >>>>+ * Returns the number of bytes read.
> >>>>+ */
> >>>>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>>>+				struct bin_attribute *bin_attr,
> >>>>+				char *buf, loff_t off, size_t count)
> >>>>+{
> >>>>+	struct net_bridge_port *p = to_brport(kobj);
> >>>>+
> >>>>+	return br_port_fill_vlans(p, buf,
> >>>>+				count/sizeof(unsigned short),
> >>>>+				off/sizeof(unsigned short));
> >>>>+}
> >>>>+
> >>>>+static struct bin_attribute port_vlans = {
> >>>>+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>>>+		  .mode = S_IRUGO, },
> >>>>+	.read = brport_vlans_read,
> >>>>+};
> >>>>+
> >>>>+/*
> >>>>   * Add sysfs entries to ethernet device added to a bridge.
> >>>>   * Creates a brport subdirectory with bridge attributes.
> >>>>   * Puts symlink in bridge's brif subdirectory
> >>>>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>>>  			return err;
> >>>>  	}
> >>>>
> >>>>+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>>>+	if (err) {
> >>>>+		return err;
> >>>>+	}
> >>>>+
> >>>>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>>>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>>>  }
> >>>>--
> >>>>1.7.7.6
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>>>the body of a message to majordomo@vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich]

On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
>>>> On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
>>>>>> Add a binary sysfs file that will dump out vlans currently configured on the
>>>>>> port.
>>
>> I initially though of creating a sysfs object per vlan.  That would
>> have made it easy to see which vlans are configured without any
>> tools.
>> But that could result in a lot of objects being created, so I abandoned it.
>>
>> I did think about a text interface, but due to a page of output
>> limitation, I didn't go that route.  The reason is that if someone
>> cats the file, they may not see all the vlans configured.  So I
>> decided on the binary interface, since a binary interface with a
>> tool to read it could avoid the single page limitation.
>>
>> -vlad
>
> Maybe it's not needed in sysfs then - expose it to
> brctl or whatever.
>

brctl uses sysfs for almost everything any more :)

-vlad

>>>
>>>
>>>>>
>>>>>> ---
>>>>>>   include/linux/if_bridge.h |    1 +
>>>>>>   net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>>>>>>   net/bridge/br_private.h   |    2 ++
>>>>>>   net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>>>>>>   4 files changed, 65 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>>>> index ab750dd..d0f869b 100644
>>>>>> --- a/include/linux/if_bridge.h
>>>>>> +++ b/include/linux/if_bridge.h
>>>>>> @@ -20,6 +20,7 @@
>>>>>>   #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>>>>>>   #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>>>>>>   #define SYSFS_BRIDGE_PORT_LINK	"bridge"
>>>>>> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>>>>>>
>>>>>>   #define BRCTL_VERSION 1
>>>>>>
>>>>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>>>>> index 90c1038..3963748 100644
>>>>>> --- a/net/bridge/br_if.c
>>>>>> +++ b/net/bridge/br_if.c
>>>>>> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>>>>>>   	return 0;
>>>>>>   }
>>>>>>
>>>>>> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
>>>>>> +			unsigned long max, unsigned long skip)
>>>>>> +{
>>>>>> +	unsigned long *map;
>>>>>> +	unsigned short *vid = (unsigned short *)buf;
>>>>>> +	unsigned short i;
>>>>>> +	int num = 0;
>>>>>> +
>>>>>> +	if (skip > (VLAN_N_VID+1))
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	memset(buf, 0, max * sizeof(unsigned short));
>>>>>
>>>>> Isn't max is in bytes? why is this safe?
>>>>>
>>>>>> +
>>>>>> +	rcu_read_lock();
>>>>>> +	map = rcu_dereference(p->vlan_map);
>>>>>> +	if (!map)
>>>>>> +		goto out;
>>>>>> +
>>>>>> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
>>>>>
>>>>> Isn't skip in bytes too? Why do you compare it to i which is
>>>>> in dwords?
>>>>>
>>>>>> +		if (test_bit(i, map)) {
>>>>>> +			if (num > max)
>>>>>> +				goto out;
>>>>>> +
>>>>>> +			*vid = i-1;
>>>>>> +			vid++;
>>>>>> +			num++;
>>>>>> +		}
>>>>>> +	}
>>>>>> +out:
>>>>>> +	rcu_read_unlock();
>>>>>> +
>>>>>> +	return num*sizeof(unsigned short);
>>>>>> +}
>>>>>> +
>>>>>>   void __net_exit br_net_exit(struct net *net)
>>>>>>   {
>>>>>>   	struct net_device *dev;
>>>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>>>> index 5639c1c..cf95cd7 100644
>>>>>> --- a/net/bridge/br_private.h
>>>>>> +++ b/net/bridge/br_private.h
>>>>>> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>>>>>   	netdev_features_t features);
>>>>>>   extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>>   extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
>>>>>> +				unsigned long max, unsigned long skip);
>>>>>>
>>>>>>   /* br_input.c */
>>>>>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>>>>>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>>>>>> index 13b36bd..a81e2ef 100644
>>>>>> --- a/net/bridge/br_sysfs_if.c
>>>>>> +++ b/net/bridge/br_sysfs_if.c
>>>>>> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>>>>>>   };
>>>>>>
>>>>>>   /*
>>>>>> + * Export the vlan table for a given port as a binary file.
>>>>>> + * The records are unsgined shorts.
>>>>>> + *
>>>>>> + * Returns the number of bytes read.
>>>>>> + */
>>>>>> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
>>>>>> +				struct bin_attribute *bin_attr,
>>>>>> +				char *buf, loff_t off, size_t count)
>>>>>> +{
>>>>>> +	struct net_bridge_port *p = to_brport(kobj);
>>>>>> +
>>>>>> +	return br_port_fill_vlans(p, buf,
>>>>>> +				count/sizeof(unsigned short),
>>>>>> +				off/sizeof(unsigned short));
>>>>>> +}
>>>>>> +
>>>>>> +static struct bin_attribute port_vlans = {
>>>>>> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
>>>>>> +		  .mode = S_IRUGO, },
>>>>>> +	.read = brport_vlans_read,
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>>    * Add sysfs entries to ethernet device added to a bridge.
>>>>>>    * Creates a brport subdirectory with bridge attributes.
>>>>>>    * Puts symlink in bridge's brif subdirectory
>>>>>> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>>>>>>   			return err;
>>>>>>   	}
>>>>>>
>>>>>> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
>>>>>> +	if (err) {
>>>>>> +		return err;
>>>>>> +	}
>>>>>> +
>>>>>>   	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>>>>>>   	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>>>>>>   }
>>>>>> --
>>>>>> 1.7.7.6
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 10:51:35AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> >>>>On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >>>>>On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>>>>>Add a binary sysfs file that will dump out vlans currently configured on the
> >>>>>>port.
> >>
> >>I initially though of creating a sysfs object per vlan.  That would
> >>have made it easy to see which vlans are configured without any
> >>tools.
> >>But that could result in a lot of objects being created, so I abandoned it.
> >>
> >>I did think about a text interface, but due to a page of output
> >>limitation, I didn't go that route.  The reason is that if someone
> >>cats the file, they may not see all the vlans configured.  So I
> >>decided on the binary interface, since a binary interface with a
> >>tool to read it could avoid the single page limitation.
> >>
> >>-vlad
> >
> >Maybe it's not needed in sysfs then - expose it to
> >brctl or whatever.
> >
> 
> brctl uses sysfs for almost everything any more :)
> 
> -vlad

How about a long string of 0 and 1's?
And a separate one for untagged vlans.

> >>>
> >>>
> >>>>>
> >>>>>>---
> >>>>>>  include/linux/if_bridge.h |    1 +
> >>>>>>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
> >>>>>>  net/bridge/br_private.h   |    2 ++
> >>>>>>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
> >>>>>>  4 files changed, 65 insertions(+), 0 deletions(-)
> >>>>>>
> >>>>>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>>>>>index ab750dd..d0f869b 100644
> >>>>>>--- a/include/linux/if_bridge.h
> >>>>>>+++ b/include/linux/if_bridge.h
> >>>>>>@@ -20,6 +20,7 @@
> >>>>>>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>>>>>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
> >>>>>>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> >>>>>>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>>>>>
> >>>>>>  #define BRCTL_VERSION 1
> >>>>>>
> >>>>>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>>>>>index 90c1038..3963748 100644
> >>>>>>--- a/net/bridge/br_if.c
> >>>>>>+++ b/net/bridge/br_if.c
> >>>>>>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>
> >>>>>>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>>>>>+			unsigned long max, unsigned long skip)
> >>>>>>+{
> >>>>>>+	unsigned long *map;
> >>>>>>+	unsigned short *vid = (unsigned short *)buf;
> >>>>>>+	unsigned short i;
> >>>>>>+	int num = 0;
> >>>>>>+
> >>>>>>+	if (skip > (VLAN_N_VID+1))
> >>>>>>+		return -EINVAL;
> >>>>>>+
> >>>>>>+	memset(buf, 0, max * sizeof(unsigned short));
> >>>>>
> >>>>>Isn't max is in bytes? why is this safe?
> >>>>>
> >>>>>>+
> >>>>>>+	rcu_read_lock();
> >>>>>>+	map = rcu_dereference(p->vlan_map);
> >>>>>>+	if (!map)
> >>>>>>+		goto out;
> >>>>>>+
> >>>>>>+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >>>>>
> >>>>>Isn't skip in bytes too? Why do you compare it to i which is
> >>>>>in dwords?
> >>>>>
> >>>>>>+		if (test_bit(i, map)) {
> >>>>>>+			if (num > max)
> >>>>>>+				goto out;
> >>>>>>+
> >>>>>>+			*vid = i-1;
> >>>>>>+			vid++;
> >>>>>>+			num++;
> >>>>>>+		}
> >>>>>>+	}
> >>>>>>+out:
> >>>>>>+	rcu_read_unlock();
> >>>>>>+
> >>>>>>+	return num*sizeof(unsigned short);
> >>>>>>+}
> >>>>>>+
> >>>>>>  void __net_exit br_net_exit(struct net *net)
> >>>>>>  {
> >>>>>>  	struct net_device *dev;
> >>>>>>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>>>>>index 5639c1c..cf95cd7 100644
> >>>>>>--- a/net/bridge/br_private.h
> >>>>>>+++ b/net/bridge/br_private.h
> >>>>>>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>>>>>  	netdev_features_t features);
> >>>>>>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>>>>>+				unsigned long max, unsigned long skip);
> >>>>>>
> >>>>>>  /* br_input.c */
> >>>>>>  extern int br_handle_frame_finish(struct sk_buff *skb);
> >>>>>>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>>>>>index 13b36bd..a81e2ef 100644
> >>>>>>--- a/net/bridge/br_sysfs_if.c
> >>>>>>+++ b/net/bridge/br_sysfs_if.c
> >>>>>>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>>>>>  };
> >>>>>>
> >>>>>>  /*
> >>>>>>+ * Export the vlan table for a given port as a binary file.
> >>>>>>+ * The records are unsgined shorts.
> >>>>>>+ *
> >>>>>>+ * Returns the number of bytes read.
> >>>>>>+ */
> >>>>>>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>>>>>+				struct bin_attribute *bin_attr,
> >>>>>>+				char *buf, loff_t off, size_t count)
> >>>>>>+{
> >>>>>>+	struct net_bridge_port *p = to_brport(kobj);
> >>>>>>+
> >>>>>>+	return br_port_fill_vlans(p, buf,
> >>>>>>+				count/sizeof(unsigned short),
> >>>>>>+				off/sizeof(unsigned short));
> >>>>>>+}
> >>>>>>+
> >>>>>>+static struct bin_attribute port_vlans = {
> >>>>>>+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>>>>>+		  .mode = S_IRUGO, },
> >>>>>>+	.read = brport_vlans_read,
> >>>>>>+};
> >>>>>>+
> >>>>>>+/*
> >>>>>>   * Add sysfs entries to ethernet device added to a bridge.
> >>>>>>   * Creates a brport subdirectory with bridge attributes.
> >>>>>>   * Puts symlink in bridge's brif subdirectory
> >>>>>>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>>>>>  			return err;
> >>>>>>  	}
> >>>>>>
> >>>>>>+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>>>>>+	if (err) {
> >>>>>>+		return err;
> >>>>>>+	}
> >>>>>>+
> >>>>>>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>>>>>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>>>>>  }
> >>>>>>--
> >>>>>>1.7.7.6
> >>>>>>
> >>>>>>--
> >>>>>>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>>>>>the body of a message to majordomo@vger.kernel.org
> >>>>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich]

On 08/30/2012 11:03 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 10:51:35AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
>>>> On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
>>>>>> On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
>>>>>>>> Add a binary sysfs file that will dump out vlans currently configured on the
>>>>>>>> port.
>>>>
>>>> I initially though of creating a sysfs object per vlan.  That would
>>>> have made it easy to see which vlans are configured without any
>>>> tools.
>>>> But that could result in a lot of objects being created, so I abandoned it.
>>>>
>>>> I did think about a text interface, but due to a page of output
>>>> limitation, I didn't go that route.  The reason is that if someone
>>>> cats the file, they may not see all the vlans configured.  So I
>>>> decided on the binary interface, since a binary interface with a
>>>> tool to read it could avoid the single page limitation.
>>>>
>>>> -vlad
>>>
>>> Maybe it's not needed in sysfs then - expose it to
>>> brctl or whatever.
>>>
>>
>> brctl uses sysfs for almost everything any more :)
>>
>> -vlad
>
> How about a long string of 0 and 1's?
> And a separate one for untagged vlans.

that would work too.  You really don't like the binary interface, huh?

-vlad
>
>>>>>
>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>   include/linux/if_bridge.h |    1 +
>>>>>>>>   net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>>>>>>>>   net/bridge/br_private.h   |    2 ++
>>>>>>>>   net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>>>>>>>>   4 files changed, 65 insertions(+), 0 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>>>>>> index ab750dd..d0f869b 100644
>>>>>>>> --- a/include/linux/if_bridge.h
>>>>>>>> +++ b/include/linux/if_bridge.h
>>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>>   #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>>>>>>>>   #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>>>>>>>>   #define SYSFS_BRIDGE_PORT_LINK	"bridge"
>>>>>>>> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>>>>>>>>
>>>>>>>>   #define BRCTL_VERSION 1
>>>>>>>>
>>>>>>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>>>>>>> index 90c1038..3963748 100644
>>>>>>>> --- a/net/bridge/br_if.c
>>>>>>>> +++ b/net/bridge/br_if.c
>>>>>>>> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>>>>>>>>   	return 0;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
>>>>>>>> +			unsigned long max, unsigned long skip)
>>>>>>>> +{
>>>>>>>> +	unsigned long *map;
>>>>>>>> +	unsigned short *vid = (unsigned short *)buf;
>>>>>>>> +	unsigned short i;
>>>>>>>> +	int num = 0;
>>>>>>>> +
>>>>>>>> +	if (skip > (VLAN_N_VID+1))
>>>>>>>> +		return -EINVAL;
>>>>>>>> +
>>>>>>>> +	memset(buf, 0, max * sizeof(unsigned short));
>>>>>>>
>>>>>>> Isn't max is in bytes? why is this safe?
>>>>>>>
>>>>>>>> +
>>>>>>>> +	rcu_read_lock();
>>>>>>>> +	map = rcu_dereference(p->vlan_map);
>>>>>>>> +	if (!map)
>>>>>>>> +		goto out;
>>>>>>>> +
>>>>>>>> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
>>>>>>>
>>>>>>> Isn't skip in bytes too? Why do you compare it to i which is
>>>>>>> in dwords?
>>>>>>>
>>>>>>>> +		if (test_bit(i, map)) {
>>>>>>>> +			if (num > max)
>>>>>>>> +				goto out;
>>>>>>>> +
>>>>>>>> +			*vid = i-1;
>>>>>>>> +			vid++;
>>>>>>>> +			num++;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +out:
>>>>>>>> +	rcu_read_unlock();
>>>>>>>> +
>>>>>>>> +	return num*sizeof(unsigned short);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   void __net_exit br_net_exit(struct net *net)
>>>>>>>>   {
>>>>>>>>   	struct net_device *dev;
>>>>>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>>>>>> index 5639c1c..cf95cd7 100644
>>>>>>>> --- a/net/bridge/br_private.h
>>>>>>>> +++ b/net/bridge/br_private.h
>>>>>>>> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>>>>>>>   	netdev_features_t features);
>>>>>>>>   extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>>>>   extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>>>> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
>>>>>>>> +				unsigned long max, unsigned long skip);
>>>>>>>>
>>>>>>>>   /* br_input.c */
>>>>>>>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>>>>>>>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>>>>>>>> index 13b36bd..a81e2ef 100644
>>>>>>>> --- a/net/bridge/br_sysfs_if.c
>>>>>>>> +++ b/net/bridge/br_sysfs_if.c
>>>>>>>> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>>>>>>>>   };
>>>>>>>>
>>>>>>>>   /*
>>>>>>>> + * Export the vlan table for a given port as a binary file.
>>>>>>>> + * The records are unsgined shorts.
>>>>>>>> + *
>>>>>>>> + * Returns the number of bytes read.
>>>>>>>> + */
>>>>>>>> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
>>>>>>>> +				struct bin_attribute *bin_attr,
>>>>>>>> +				char *buf, loff_t off, size_t count)
>>>>>>>> +{
>>>>>>>> +	struct net_bridge_port *p = to_brport(kobj);
>>>>>>>> +
>>>>>>>> +	return br_port_fill_vlans(p, buf,
>>>>>>>> +				count/sizeof(unsigned short),
>>>>>>>> +				off/sizeof(unsigned short));
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static struct bin_attribute port_vlans = {
>>>>>>>> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
>>>>>>>> +		  .mode = S_IRUGO, },
>>>>>>>> +	.read = brport_vlans_read,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>>    * Add sysfs entries to ethernet device added to a bridge.
>>>>>>>>    * Creates a brport subdirectory with bridge attributes.
>>>>>>>>    * Puts symlink in bridge's brif subdirectory
>>>>>>>> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>>>>>>>>   			return err;
>>>>>>>>   	}
>>>>>>>>
>>>>>>>> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
>>>>>>>> +	if (err) {
>>>>>>>> +		return err;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>>   	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>>>>>>>>   	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>>>>>>>>   }
>>>>>>>> --
>>>>>>>> 1.7.7.6
>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 11:07:24AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 11:03 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 10:51:35AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
> >>>>On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> >>>>>On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> >>>>>>On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >>>>>>>On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>>>>>>>Add a binary sysfs file that will dump out vlans currently configured on the
> >>>>>>>>port.
> >>>>
> >>>>I initially though of creating a sysfs object per vlan.  That would
> >>>>have made it easy to see which vlans are configured without any
> >>>>tools.
> >>>>But that could result in a lot of objects being created, so I abandoned it.
> >>>>
> >>>>I did think about a text interface, but due to a page of output
> >>>>limitation, I didn't go that route.  The reason is that if someone
> >>>>cats the file, they may not see all the vlans configured.  So I
> >>>>decided on the binary interface, since a binary interface with a
> >>>>tool to read it could avoid the single page limitation.
> >>>>
> >>>>-vlad
> >>>
> >>>Maybe it's not needed in sysfs then - expose it to
> >>>brctl or whatever.
> >>>
> >>
> >>brctl uses sysfs for almost everything any more :)
> >>
> >>-vlad
> >
> >How about a long string of 0 and 1's?
> >And a separate one for untagged vlans.
> 
> that would work too.  You really don't like the binary interface, huh?
> 
> -vlad

Not in sysfs.
Another possibility: you are adding netlink command
to add/del a specific vid, add one to test a specific vid.
This means you will need to scan 4K possibilities if you
want to list them all, but in real life it's
probably just debugging tools that need to do this. 

> >
> >>>>>
> >>>>>
> >>>>>>>
> >>>>>>>>---
> >>>>>>>>  include/linux/if_bridge.h |    1 +
> >>>>>>>>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
> >>>>>>>>  net/bridge/br_private.h   |    2 ++
> >>>>>>>>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
> >>>>>>>>  4 files changed, 65 insertions(+), 0 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>>>>>>>index ab750dd..d0f869b 100644
> >>>>>>>>--- a/include/linux/if_bridge.h
> >>>>>>>>+++ b/include/linux/if_bridge.h
> >>>>>>>>@@ -20,6 +20,7 @@
> >>>>>>>>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>>>>>>>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
> >>>>>>>>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> >>>>>>>>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>>>>>>>
> >>>>>>>>  #define BRCTL_VERSION 1
> >>>>>>>>
> >>>>>>>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>>>>>>>index 90c1038..3963748 100644
> >>>>>>>>--- a/net/bridge/br_if.c
> >>>>>>>>+++ b/net/bridge/br_if.c
> >>>>>>>>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>>>>>>>  	return 0;
> >>>>>>>>  }
> >>>>>>>>
> >>>>>>>>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>>>>>>>+			unsigned long max, unsigned long skip)
> >>>>>>>>+{
> >>>>>>>>+	unsigned long *map;
> >>>>>>>>+	unsigned short *vid = (unsigned short *)buf;
> >>>>>>>>+	unsigned short i;
> >>>>>>>>+	int num = 0;
> >>>>>>>>+
> >>>>>>>>+	if (skip > (VLAN_N_VID+1))
> >>>>>>>>+		return -EINVAL;
> >>>>>>>>+
> >>>>>>>>+	memset(buf, 0, max * sizeof(unsigned short));
> >>>>>>>
> >>>>>>>Isn't max is in bytes? why is this safe?
> >>>>>>>
> >>>>>>>>+
> >>>>>>>>+	rcu_read_lock();
> >>>>>>>>+	map = rcu_dereference(p->vlan_map);
> >>>>>>>>+	if (!map)
> >>>>>>>>+		goto out;
> >>>>>>>>+
> >>>>>>>>+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >>>>>>>
> >>>>>>>Isn't skip in bytes too? Why do you compare it to i which is
> >>>>>>>in dwords?
> >>>>>>>
> >>>>>>>>+		if (test_bit(i, map)) {
> >>>>>>>>+			if (num > max)
> >>>>>>>>+				goto out;
> >>>>>>>>+
> >>>>>>>>+			*vid = i-1;
> >>>>>>>>+			vid++;
> >>>>>>>>+			num++;
> >>>>>>>>+		}
> >>>>>>>>+	}
> >>>>>>>>+out:
> >>>>>>>>+	rcu_read_unlock();
> >>>>>>>>+
> >>>>>>>>+	return num*sizeof(unsigned short);
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>  void __net_exit br_net_exit(struct net *net)
> >>>>>>>>  {
> >>>>>>>>  	struct net_device *dev;
> >>>>>>>>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>>>>>>>index 5639c1c..cf95cd7 100644
> >>>>>>>>--- a/net/bridge/br_private.h
> >>>>>>>>+++ b/net/bridge/br_private.h
> >>>>>>>>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>>>>>>>  	netdev_features_t features);
> >>>>>>>>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>>>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>>>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>>>>>>>+				unsigned long max, unsigned long skip);
> >>>>>>>>
> >>>>>>>>  /* br_input.c */
> >>>>>>>>  extern int br_handle_frame_finish(struct sk_buff *skb);
> >>>>>>>>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>>>>>>>index 13b36bd..a81e2ef 100644
> >>>>>>>>--- a/net/bridge/br_sysfs_if.c
> >>>>>>>>+++ b/net/bridge/br_sysfs_if.c
> >>>>>>>>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>>>>>>>  };
> >>>>>>>>
> >>>>>>>>  /*
> >>>>>>>>+ * Export the vlan table for a given port as a binary file.
> >>>>>>>>+ * The records are unsgined shorts.
> >>>>>>>>+ *
> >>>>>>>>+ * Returns the number of bytes read.
> >>>>>>>>+ */
> >>>>>>>>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>>>>>>>+				struct bin_attribute *bin_attr,
> >>>>>>>>+				char *buf, loff_t off, size_t count)
> >>>>>>>>+{
> >>>>>>>>+	struct net_bridge_port *p = to_brport(kobj);
> >>>>>>>>+
> >>>>>>>>+	return br_port_fill_vlans(p, buf,
> >>>>>>>>+				count/sizeof(unsigned short),
> >>>>>>>>+				off/sizeof(unsigned short));
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>+static struct bin_attribute port_vlans = {
> >>>>>>>>+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>>>>>>>+		  .mode = S_IRUGO, },
> >>>>>>>>+	.read = brport_vlans_read,
> >>>>>>>>+};
> >>>>>>>>+
> >>>>>>>>+/*
> >>>>>>>>   * Add sysfs entries to ethernet device added to a bridge.
> >>>>>>>>   * Creates a brport subdirectory with bridge attributes.
> >>>>>>>>   * Puts symlink in bridge's brif subdirectory
> >>>>>>>>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>>>>>>>  			return err;
> >>>>>>>>  	}
> >>>>>>>>
> >>>>>>>>+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>>>>>>>+	if (err) {
> >>>>>>>>+		return err;
> >>>>>>>>+	}
> >>>>>>>>+
> >>>>>>>>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>>>>>>>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>>>>>>>  }
> >>>>>>>>--
> >>>>>>>>1.7.7.6
> >>>>>>>>
> >>>>>>>>--
> >>>>>>>>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>>>>>>>the body of a message to majordomo@vger.kernel.org
> >>>>>>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Vlad Yasevich]

On 08/30/2012 11:47 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 11:07:24AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 11:03 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 30, 2012 at 10:51:35AM -0400, Vlad Yasevich wrote:
>>>> On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
>>>>>> On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
>>>>>>>> On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
>>>>>>>>> On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
>>>>>>>>>> Add a binary sysfs file that will dump out vlans currently configured on the
>>>>>>>>>> port.
>>>>>>
>>>>>> I initially though of creating a sysfs object per vlan.  That would
>>>>>> have made it easy to see which vlans are configured without any
>>>>>> tools.
>>>>>> But that could result in a lot of objects being created, so I abandoned it.
>>>>>>
>>>>>> I did think about a text interface, but due to a page of output
>>>>>> limitation, I didn't go that route.  The reason is that if someone
>>>>>> cats the file, they may not see all the vlans configured.  So I
>>>>>> decided on the binary interface, since a binary interface with a
>>>>>> tool to read it could avoid the single page limitation.
>>>>>>
>>>>>> -vlad
>>>>>
>>>>> Maybe it's not needed in sysfs then - expose it to
>>>>> brctl or whatever.
>>>>>
>>>>
>>>> brctl uses sysfs for almost everything any more :)
>>>>
>>>> -vlad
>>>
>>> How about a long string of 0 and 1's?
>>> And a separate one for untagged vlans.
>>
>> that would work too.  You really don't like the binary interface, huh?
>>
>> -vlad
>
> Not in sysfs.
> Another possibility: you are adding netlink command
> to add/del a specific vid, add one to test a specific vid.
> This means you will need to scan 4K possibilities if you
> want to list them all, but in real life it's
> probably just debugging tools that need to do this.

Not really.  A show command to see what has been configured is necessary 
not of only for debugging.  It is no different then the show command to 
list fdb entries on the bridge.  There needs to be an easy way to see 
which vlans are configured on which ports.

-vlad
>
>>>
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>   include/linux/if_bridge.h |    1 +
>>>>>>>>>>   net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
>>>>>>>>>>   net/bridge/br_private.h   |    2 ++
>>>>>>>>>>   net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
>>>>>>>>>>   4 files changed, 65 insertions(+), 0 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>>>>>>>>> index ab750dd..d0f869b 100644
>>>>>>>>>> --- a/include/linux/if_bridge.h
>>>>>>>>>> +++ b/include/linux/if_bridge.h
>>>>>>>>>> @@ -20,6 +20,7 @@
>>>>>>>>>>   #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
>>>>>>>>>>   #define SYSFS_BRIDGE_PORT_ATTR	"brport"
>>>>>>>>>>   #define SYSFS_BRIDGE_PORT_LINK	"bridge"
>>>>>>>>>> +#define SYSFS_BRIDGE_PORT_VLANS "vlans"
>>>>>>>>>>
>>>>>>>>>>   #define BRCTL_VERSION 1
>>>>>>>>>>
>>>>>>>>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>>>>>>>>> index 90c1038..3963748 100644
>>>>>>>>>> --- a/net/bridge/br_if.c
>>>>>>>>>> +++ b/net/bridge/br_if.c
>>>>>>>>>> @@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
>>>>>>>>>>   	return 0;
>>>>>>>>>>   }
>>>>>>>>>>
>>>>>>>>>> +size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
>>>>>>>>>> +			unsigned long max, unsigned long skip)
>>>>>>>>>> +{
>>>>>>>>>> +	unsigned long *map;
>>>>>>>>>> +	unsigned short *vid = (unsigned short *)buf;
>>>>>>>>>> +	unsigned short i;
>>>>>>>>>> +	int num = 0;
>>>>>>>>>> +
>>>>>>>>>> +	if (skip > (VLAN_N_VID+1))
>>>>>>>>>> +		return -EINVAL;
>>>>>>>>>> +
>>>>>>>>>> +	memset(buf, 0, max * sizeof(unsigned short));
>>>>>>>>>
>>>>>>>>> Isn't max is in bytes? why is this safe?
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +	rcu_read_lock();
>>>>>>>>>> +	map = rcu_dereference(p->vlan_map);
>>>>>>>>>> +	if (!map)
>>>>>>>>>> +		goto out;
>>>>>>>>>> +
>>>>>>>>>> +	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
>>>>>>>>>
>>>>>>>>> Isn't skip in bytes too? Why do you compare it to i which is
>>>>>>>>> in dwords?
>>>>>>>>>
>>>>>>>>>> +		if (test_bit(i, map)) {
>>>>>>>>>> +			if (num > max)
>>>>>>>>>> +				goto out;
>>>>>>>>>> +
>>>>>>>>>> +			*vid = i-1;
>>>>>>>>>> +			vid++;
>>>>>>>>>> +			num++;
>>>>>>>>>> +		}
>>>>>>>>>> +	}
>>>>>>>>>> +out:
>>>>>>>>>> +	rcu_read_unlock();
>>>>>>>>>> +
>>>>>>>>>> +	return num*sizeof(unsigned short);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>   void __net_exit br_net_exit(struct net *net)
>>>>>>>>>>   {
>>>>>>>>>>   	struct net_device *dev;
>>>>>>>>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>>>>>>>>> index 5639c1c..cf95cd7 100644
>>>>>>>>>> --- a/net/bridge/br_private.h
>>>>>>>>>> +++ b/net/bridge/br_private.h
>>>>>>>>>> @@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
>>>>>>>>>>   	netdev_features_t features);
>>>>>>>>>>   extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>>>>>>   extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
>>>>>>>>>> +extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
>>>>>>>>>> +				unsigned long max, unsigned long skip);
>>>>>>>>>>
>>>>>>>>>>   /* br_input.c */
>>>>>>>>>>   extern int br_handle_frame_finish(struct sk_buff *skb);
>>>>>>>>>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>>>>>>>>>> index 13b36bd..a81e2ef 100644
>>>>>>>>>> --- a/net/bridge/br_sysfs_if.c
>>>>>>>>>> +++ b/net/bridge/br_sysfs_if.c
>>>>>>>>>> @@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
>>>>>>>>>>   };
>>>>>>>>>>
>>>>>>>>>>   /*
>>>>>>>>>> + * Export the vlan table for a given port as a binary file.
>>>>>>>>>> + * The records are unsgined shorts.
>>>>>>>>>> + *
>>>>>>>>>> + * Returns the number of bytes read.
>>>>>>>>>> + */
>>>>>>>>>> +static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
>>>>>>>>>> +				struct bin_attribute *bin_attr,
>>>>>>>>>> +				char *buf, loff_t off, size_t count)
>>>>>>>>>> +{
>>>>>>>>>> +	struct net_bridge_port *p = to_brport(kobj);
>>>>>>>>>> +
>>>>>>>>>> +	return br_port_fill_vlans(p, buf,
>>>>>>>>>> +				count/sizeof(unsigned short),
>>>>>>>>>> +				off/sizeof(unsigned short));
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static struct bin_attribute port_vlans = {
>>>>>>>>>> +	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
>>>>>>>>>> +		  .mode = S_IRUGO, },
>>>>>>>>>> +	.read = brport_vlans_read,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +/*
>>>>>>>>>>    * Add sysfs entries to ethernet device added to a bridge.
>>>>>>>>>>    * Creates a brport subdirectory with bridge attributes.
>>>>>>>>>>    * Puts symlink in bridge's brif subdirectory
>>>>>>>>>> @@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
>>>>>>>>>>   			return err;
>>>>>>>>>>   	}
>>>>>>>>>>
>>>>>>>>>> +	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
>>>>>>>>>> +	if (err) {
>>>>>>>>>> +		return err;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>>   	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
>>>>>>>>>>   	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
>>>>>>>>>>   }
>>>>>>>>>> --
>>>>>>>>>> 1.7.7.6
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>

Re: [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 11:52:31AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 11:47 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 11:07:24AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 11:03 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 30, 2012 at 10:51:35AM -0400, Vlad Yasevich wrote:
> >>>>On 08/30/2012 10:44 AM, Michael S. Tsirkin wrote:
> >>>>>On Thu, Aug 30, 2012 at 10:36:34AM -0400, Vlad Yasevich wrote:
> >>>>>>On 08/30/2012 10:26 AM, Michael S. Tsirkin wrote:
> >>>>>>>On Thu, Aug 30, 2012 at 10:05:16AM -0400, Vlad Yasevich wrote:
> >>>>>>>>On 08/30/2012 08:27 AM, Michael S. Tsirkin wrote:
> >>>>>>>>>On Thu, Aug 23, 2012 at 03:29:55PM -0400, Vlad Yasevich wrote:
> >>>>>>>>>>Add a binary sysfs file that will dump out vlans currently configured on the
> >>>>>>>>>>port.
> >>>>>>
> >>>>>>I initially though of creating a sysfs object per vlan.  That would
> >>>>>>have made it easy to see which vlans are configured without any
> >>>>>>tools.
> >>>>>>But that could result in a lot of objects being created, so I abandoned it.
> >>>>>>
> >>>>>>I did think about a text interface, but due to a page of output
> >>>>>>limitation, I didn't go that route.  The reason is that if someone
> >>>>>>cats the file, they may not see all the vlans configured.  So I
> >>>>>>decided on the binary interface, since a binary interface with a
> >>>>>>tool to read it could avoid the single page limitation.
> >>>>>>
> >>>>>>-vlad
> >>>>>
> >>>>>Maybe it's not needed in sysfs then - expose it to
> >>>>>brctl or whatever.
> >>>>>
> >>>>
> >>>>brctl uses sysfs for almost everything any more :)
> >>>>
> >>>>-vlad
> >>>
> >>>How about a long string of 0 and 1's?
> >>>And a separate one for untagged vlans.
> >>
> >>that would work too.  You really don't like the binary interface, huh?
> >>
> >>-vlad
> >
> >Not in sysfs.
> >Another possibility: you are adding netlink command
> >to add/del a specific vid, add one to test a specific vid.
> >This means you will need to scan 4K possibilities if you
> >want to list them all, but in real life it's
> >probably just debugging tools that need to do this.
> 
> Not really.  A show command to see what has been configured is
> necessary not of only for debugging.  It is no different then the
> show command to list fdb entries on the bridge.  There needs to be
> an easy way to see which vlans are configured on which ports.
> 
> -vlad

Yes. And

for (vid = 0; vid < 2096; ++vid)
	ioctl(port,vid)

is easier than parsing a binary blob.
Just less efficient but that is I think OK
for the show command.


> >
> >>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>>---
> >>>>>>>>>>  include/linux/if_bridge.h |    1 +
> >>>>>>>>>>  net/bridge/br_if.c        |   34 ++++++++++++++++++++++++++++++++++
> >>>>>>>>>>  net/bridge/br_private.h   |    2 ++
> >>>>>>>>>>  net/bridge/br_sysfs_if.c  |   28 ++++++++++++++++++++++++++++
> >>>>>>>>>>  4 files changed, 65 insertions(+), 0 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>>>>>>>>>index ab750dd..d0f869b 100644
> >>>>>>>>>>--- a/include/linux/if_bridge.h
> >>>>>>>>>>+++ b/include/linux/if_bridge.h
> >>>>>>>>>>@@ -20,6 +20,7 @@
> >>>>>>>>>>  #define SYSFS_BRIDGE_PORT_SUBDIR "brif"
> >>>>>>>>>>  #define SYSFS_BRIDGE_PORT_ATTR	"brport"
> >>>>>>>>>>  #define SYSFS_BRIDGE_PORT_LINK	"bridge"
> >>>>>>>>>>+#define SYSFS_BRIDGE_PORT_VLANS "vlans"
> >>>>>>>>>>
> >>>>>>>>>>  #define BRCTL_VERSION 1
> >>>>>>>>>>
> >>>>>>>>>>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >>>>>>>>>>index 90c1038..3963748 100644
> >>>>>>>>>>--- a/net/bridge/br_if.c
> >>>>>>>>>>+++ b/net/bridge/br_if.c
> >>>>>>>>>>@@ -510,6 +510,40 @@ int br_del_port_vlan(struct net_bridge_port *p, unsigned long vlan)
> >>>>>>>>>>  	return 0;
> >>>>>>>>>>  }
> >>>>>>>>>>
> >>>>>>>>>>+size_t br_port_fill_vlans(struct net_bridge_port *p, char* buf,
> >>>>>>>>>>+			unsigned long max, unsigned long skip)
> >>>>>>>>>>+{
> >>>>>>>>>>+	unsigned long *map;
> >>>>>>>>>>+	unsigned short *vid = (unsigned short *)buf;
> >>>>>>>>>>+	unsigned short i;
> >>>>>>>>>>+	int num = 0;
> >>>>>>>>>>+
> >>>>>>>>>>+	if (skip > (VLAN_N_VID+1))
> >>>>>>>>>>+		return -EINVAL;
> >>>>>>>>>>+
> >>>>>>>>>>+	memset(buf, 0, max * sizeof(unsigned short));
> >>>>>>>>>
> >>>>>>>>>Isn't max is in bytes? why is this safe?
> >>>>>>>>>
> >>>>>>>>>>+
> >>>>>>>>>>+	rcu_read_lock();
> >>>>>>>>>>+	map = rcu_dereference(p->vlan_map);
> >>>>>>>>>>+	if (!map)
> >>>>>>>>>>+		goto out;
> >>>>>>>>>>+
> >>>>>>>>>>+	for (i = skip + 1; i < VLAN_N_VID + 1; i++) {
> >>>>>>>>>
> >>>>>>>>>Isn't skip in bytes too? Why do you compare it to i which is
> >>>>>>>>>in dwords?
> >>>>>>>>>
> >>>>>>>>>>+		if (test_bit(i, map)) {
> >>>>>>>>>>+			if (num > max)
> >>>>>>>>>>+				goto out;
> >>>>>>>>>>+
> >>>>>>>>>>+			*vid = i-1;
> >>>>>>>>>>+			vid++;
> >>>>>>>>>>+			num++;
> >>>>>>>>>>+		}
> >>>>>>>>>>+	}
> >>>>>>>>>>+out:
> >>>>>>>>>>+	rcu_read_unlock();
> >>>>>>>>>>+
> >>>>>>>>>>+	return num*sizeof(unsigned short);
> >>>>>>>>>>+}
> >>>>>>>>>>+
> >>>>>>>>>>  void __net_exit br_net_exit(struct net *net)
> >>>>>>>>>>  {
> >>>>>>>>>>  	struct net_device *dev;
> >>>>>>>>>>diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >>>>>>>>>>index 5639c1c..cf95cd7 100644
> >>>>>>>>>>--- a/net/bridge/br_private.h
> >>>>>>>>>>+++ b/net/bridge/br_private.h
> >>>>>>>>>>@@ -404,6 +404,8 @@ extern netdev_features_t br_features_recompute(struct net_bridge *br,
> >>>>>>>>>>  	netdev_features_t features);
> >>>>>>>>>>  extern int br_set_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>>>>>  extern int br_del_port_vlan(struct net_bridge_port *p, unsigned long vid);
> >>>>>>>>>>+extern size_t br_port_fill_vlans(struct net_bridge_port *p, char *buf,
> >>>>>>>>>>+				unsigned long max, unsigned long skip);
> >>>>>>>>>>
> >>>>>>>>>>  /* br_input.c */
> >>>>>>>>>>  extern int br_handle_frame_finish(struct sk_buff *skb);
> >>>>>>>>>>diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> >>>>>>>>>>index 13b36bd..a81e2ef 100644
> >>>>>>>>>>--- a/net/bridge/br_sysfs_if.c
> >>>>>>>>>>+++ b/net/bridge/br_sysfs_if.c
> >>>>>>>>>>@@ -234,6 +234,29 @@ const struct sysfs_ops brport_sysfs_ops = {
> >>>>>>>>>>  };
> >>>>>>>>>>
> >>>>>>>>>>  /*
> >>>>>>>>>>+ * Export the vlan table for a given port as a binary file.
> >>>>>>>>>>+ * The records are unsgined shorts.
> >>>>>>>>>>+ *
> >>>>>>>>>>+ * Returns the number of bytes read.
> >>>>>>>>>>+ */
> >>>>>>>>>>+static ssize_t brport_vlans_read(struct file *filp, struct kobject *kobj,
> >>>>>>>>>>+				struct bin_attribute *bin_attr,
> >>>>>>>>>>+				char *buf, loff_t off, size_t count)
> >>>>>>>>>>+{
> >>>>>>>>>>+	struct net_bridge_port *p = to_brport(kobj);
> >>>>>>>>>>+
> >>>>>>>>>>+	return br_port_fill_vlans(p, buf,
> >>>>>>>>>>+				count/sizeof(unsigned short),
> >>>>>>>>>>+				off/sizeof(unsigned short));
> >>>>>>>>>>+}
> >>>>>>>>>>+
> >>>>>>>>>>+static struct bin_attribute port_vlans = {
> >>>>>>>>>>+	.attr = { .name = SYSFS_BRIDGE_PORT_VLANS,
> >>>>>>>>>>+		  .mode = S_IRUGO, },
> >>>>>>>>>>+	.read = brport_vlans_read,
> >>>>>>>>>>+};
> >>>>>>>>>>+
> >>>>>>>>>>+/*
> >>>>>>>>>>   * Add sysfs entries to ethernet device added to a bridge.
> >>>>>>>>>>   * Creates a brport subdirectory with bridge attributes.
> >>>>>>>>>>   * Puts symlink in bridge's brif subdirectory
> >>>>>>>>>>@@ -255,6 +278,11 @@ int br_sysfs_addif(struct net_bridge_port *p)
> >>>>>>>>>>  			return err;
> >>>>>>>>>>  	}
> >>>>>>>>>>
> >>>>>>>>>>+	err = sysfs_create_bin_file(&p->kobj, &port_vlans);
> >>>>>>>>>>+	if (err) {
> >>>>>>>>>>+		return err;
> >>>>>>>>>>+	}
> >>>>>>>>>>+
> >>>>>>>>>>  	strlcpy(p->sysfs_name, p->dev->name, IFNAMSIZ);
> >>>>>>>>>>  	return sysfs_create_link(br->ifobj, &p->kobj, p->sysfs_name);
> >>>>>>>>>>  }
> >>>>>>>>>>--
> >>>>>>>>>>1.7.7.6
> >>>>>>>>>>
> >>>>>>>>>>--
> >>>>>>>>>>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>>>>>>>>>the body of a message to majordomo@vger.kernel.org
> >>>>>>>>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>--
> >>>To unsubscribe from this list: send the line "unsubscribe netdev" in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Stephen Hemminger]

On Thu, 23 Aug 2012 15:29:50 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> This series of patches provides an ability to add VLAN IDs to the bridge
> ports.  This is similar to what can be found in most switches.  The bridge
> port may have any number of VLANs added to it including vlan 0 for untagged
> traffic.  When vlans are added to the port, only traffic tagged with particular
> vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
> entries and become part of the lookup.  This way we correctly identify the FDB
> entry.
> 
> There are still pieces missing.  I don't yet support adding a static fdb entry
> with a particular vlan.  There is no netlink support for carrying a vlan id.
> 
> I'd like to hear thoughts of whether this is usufull and something we should
> persue.
> 
> The default behavior ofthe bridge is unchanged if no vlans have been
> configured.

Initial reaction is that this is a useful. You can already do the same thing
with ebtables, and ebtables allows more flexibility. But ebtables does slow
things down, and is harder to configure.

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Vlad Yasevich]

On 08/23/2012 03:41 PM, Stephen Hemminger wrote:
> On Thu, 23 Aug 2012 15:29:50 -0400
> Vlad Yasevich <vyasevic@redhat.com> wrote:
>
>> This series of patches provides an ability to add VLAN IDs to the bridge
>> ports.  This is similar to what can be found in most switches.  The bridge
>> port may have any number of VLANs added to it including vlan 0 for untagged
>> traffic.  When vlans are added to the port, only traffic tagged with particular
>> vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
>> entries and become part of the lookup.  This way we correctly identify the FDB
>> entry.
>>
>> There are still pieces missing.  I don't yet support adding a static fdb entry
>> with a particular vlan.  There is no netlink support for carrying a vlan id.
>>
>> I'd like to hear thoughts of whether this is usufull and something we should
>> persue.
>>
>> The default behavior ofthe bridge is unchanged if no vlans have been
>> configured.
>
> Initial reaction is that this is a useful. You can already do the same thing
> with ebtables, and ebtables allows more flexibility. But ebtables does slow
> things down, and is harder to configure.

Slowness of ebtables is exactly why I thought of doing this.  This code 
works pretty well when you have guests running on different vlans.  It 
makes sure that there is no traffic leakage.

I'll write up the netlink pieces and repost.

Thanks
-vlad

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Nicolas de Pesloüan]

Le 23/08/2012 21:29, Vlad Yasevich a écrit :
> This series of patches provides an ability to add VLAN IDs to the bridge
> ports.  This is similar to what can be found in most switches.  The bridge
> port may have any number of VLANs added to it including vlan 0 for untagged
> traffic.  When vlans are added to the port, only traffic tagged with particular
> vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
> entries and become part of the lookup.  This way we correctly identify the FDB
> entry.
>
> There are still pieces missing.  I don't yet support adding a static fdb entry
> with a particular vlan.  There is no netlink support for carrying a vlan id.
>
> I'd like to hear thoughts of whether this is usufull and something we should
> persue.
>

Do you think this might allow for per VLAN spanning tree (having ports in forwarding state or 
blocking state depending on the VLAN) in the future?

	Nicolas.

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Stephen Hemminger]

On Thu, 23 Aug 2012 23:03:21 +0200
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:

> Le 23/08/2012 21:29, Vlad Yasevich a écrit :
> > This series of patches provides an ability to add VLAN IDs to the bridge
> > ports.  This is similar to what can be found in most switches.  The bridge
> > port may have any number of VLANs added to it including vlan 0 for untagged
> > traffic.  When vlans are added to the port, only traffic tagged with particular
> > vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
> > entries and become part of the lookup.  This way we correctly identify the FDB
> > entry.
> >
> > There are still pieces missing.  I don't yet support adding a static fdb entry
> > with a particular vlan.  There is no netlink support for carrying a vlan id.
> >
> > I'd like to hear thoughts of whether this is usufull and something we should
> > persue.
> >
> 
> Do you think this might allow for per VLAN spanning tree (having ports in forwarding state or 
> blocking state depending on the VLAN) in the future?
> 
> 	Nicolas.

Completely different problem

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Vlad Yasevich]

On 08/23/2012 05:03 PM, Nicolas de Pesloüan wrote:
> Le 23/08/2012 21:29, Vlad Yasevich a écrit :
>> This series of patches provides an ability to add VLAN IDs to the bridge
>> ports.  This is similar to what can be found in most switches.  The
>> bridge
>> port may have any number of VLANs added to it including vlan 0 for
>> untagged
>> traffic.  When vlans are added to the port, only traffic tagged with
>> particular
>> vlan will forwarded over this port.  Additionally, vlan ids are added
>> to FDB
>> entries and become part of the lookup.  This way we correctly identify
>> the FDB
>> entry.
>>
>> There are still pieces missing.  I don't yet support adding a static
>> fdb entry
>> with a particular vlan.  There is no netlink support for carrying a
>> vlan id.
>>
>> I'd like to hear thoughts of whether this is usufull and something we
>> should
>> persue.
>>
>
> Do you think this might allow for per VLAN spanning tree (having ports
> in forwarding state or blocking state depending on the VLAN) in the future?
>
>      Nicolas.

sure, why not.

-vlad

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Stephen Hemminger]

On Thu, 23 Aug 2012 22:52:02 -0400
Vlad Yasevich <vyasevic@redhat.com> wrote:

> On 08/23/2012 05:03 PM, Nicolas de Pesloüan wrote:
> > Le 23/08/2012 21:29, Vlad Yasevich a écrit :
> >> This series of patches provides an ability to add VLAN IDs to the bridge
> >> ports.  This is similar to what can be found in most switches.  The
> >> bridge
> >> port may have any number of VLANs added to it including vlan 0 for
> >> untagged
> >> traffic.  When vlans are added to the port, only traffic tagged with
> >> particular
> >> vlan will forwarded over this port.  Additionally, vlan ids are added
> >> to FDB
> >> entries and become part of the lookup.  This way we correctly identify
> >> the FDB
> >> entry.
> >>
> >> There are still pieces missing.  I don't yet support adding a static
> >> fdb entry
> >> with a particular vlan.  There is no netlink support for carrying a
> >> vlan id.
> >>
> >> I'd like to hear thoughts of whether this is usufull and something we
> >> should
> >> persue.
> >>
> >
> > Do you think this might allow for per VLAN spanning tree (having ports
> > in forwarding state or blocking state depending on the VLAN) in the future?
> >
> >      Nicolas.
> 
> sure, why not.

The vlan map table would be helpful, but the Spanning Tree implementation
doesn't have a clue about what it means.

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Nicolas de Pesloüan]

Le 24/08/2012 22:44, Stephen Hemminger a écrit :
<snip>

> The vlan map table would be helpful, but the Spanning Tree implementation
> doesn't have a clue about what it means.

Yes, but a possibly future userland implementation of MSTP (or PVST) would benefit from the ability 
to manage per VLAN port states.

So at least we should have this in mind while integrating VLAN into the bridge code.

	Nicolas.

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Michael S. Tsirkin]

On Thu, Aug 23, 2012 at 03:29:50PM -0400, Vlad Yasevich wrote:
> This series of patches provides an ability to add VLAN IDs to the bridge
> ports.  This is similar to what can be found in most switches.  The bridge
> port may have any number of VLANs added to it including vlan 0 for untagged
> traffic.  When vlans are added to the port, only traffic tagged with particular
> vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
> entries and become part of the lookup.  This way we correctly identify the FDB
> entry.
> 
> There are still pieces missing.  I don't yet support adding a static fdb entry
> with a particular vlan.  There is no netlink support for carrying a vlan id.
> 
> I'd like to hear thoughts of whether this is usufull and something we should
> persue.
> 
> The default behavior ofthe bridge is unchanged if no vlans have been
> configured.

Overall the feature looks good, I can think of some uses
for it - for example, it could become useful for VMs if
we add support to tap essentially stripping tags in Xmit but maybe you
could be more explicit about what you have in mind?
Do you plan to add tap support as well?
Also - what tool support do you plan?

I also found some coding style issues and some bugs in
the patchset. Sent on list.

Hope this helps.

-- 
MST

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Vlad Yasevich]

On 08/30/2012 08:37 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 03:29:50PM -0400, Vlad Yasevich wrote:
>> This series of patches provides an ability to add VLAN IDs to the bridge
>> ports.  This is similar to what can be found in most switches.  The bridge
>> port may have any number of VLANs added to it including vlan 0 for untagged
>> traffic.  When vlans are added to the port, only traffic tagged with particular
>> vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
>> entries and become part of the lookup.  This way we correctly identify the FDB
>> entry.
>>
>> There are still pieces missing.  I don't yet support adding a static fdb entry
>> with a particular vlan.  There is no netlink support for carrying a vlan id.
>>
>> I'd like to hear thoughts of whether this is usufull and something we should
>> persue.
>>
>> The default behavior ofthe bridge is unchanged if no vlans have been
>> configured.
>
> Overall the feature looks good, I can think of some uses
> for it - for example, it could become useful for VMs if
> we add support to tap essentially stripping tags in Xmit but maybe you
> could be more explicit about what you have in mind?
> Do you plan to add tap support as well?

Yes,  this is something I've thought of.  Not sure if it would be at tap
or bridge itself.  Need to work out where best to do it.

> Also - what tool support do you plan?

the patchset includes brctl to configure, but that seems to be getting 
deprecated.  I am working on iproute2 to add capability to configure this.

>
> I also found some coding style issues and some bugs in
> the patchset. Sent on list.

Thanks
-vlad

>
> Hope this helps.
>

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 09:37:17AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 08:37 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 23, 2012 at 03:29:50PM -0400, Vlad Yasevich wrote:
> >>This series of patches provides an ability to add VLAN IDs to the bridge
> >>ports.  This is similar to what can be found in most switches.  The bridge
> >>port may have any number of VLANs added to it including vlan 0 for untagged
> >>traffic.  When vlans are added to the port, only traffic tagged with particular
> >>vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
> >>entries and become part of the lookup.  This way we correctly identify the FDB
> >>entry.
> >>
> >>There are still pieces missing.  I don't yet support adding a static fdb entry
> >>with a particular vlan.  There is no netlink support for carrying a vlan id.
> >>
> >>I'd like to hear thoughts of whether this is usufull and something we should
> >>persue.
> >>
> >>The default behavior ofthe bridge is unchanged if no vlans have been
> >>configured.
> >
> >Overall the feature looks good, I can think of some uses
> >for it - for example, it could become useful for VMs if
> >we add support to tap essentially stripping tags in Xmit but maybe you
> >could be more explicit about what you have in mind?
> >Do you plan to add tap support as well?
> 
> Yes,  this is something I've thought of.  Not sure if it would be at tap
> or bridge itself.  Need to work out where best to do it.

It's certainly much easier to do in tap.
A 20 line patch should do it.
Does stripping tags seem like something bridge should do?

> >Also - what tool support do you plan?
> 
> the patchset includes brctl to configure, but that seems to be
> getting deprecated.  I am working on iproute2 to add capability to
> configure this.
> 
> >
> >I also found some coding style issues and some bugs in
> >the patchset. Sent on list.
> 
> Thanks
> -vlad
> 
> >
> >Hope this helps.
> >

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Vlad Yasevich]

On 08/30/2012 10:34 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 30, 2012 at 09:37:17AM -0400, Vlad Yasevich wrote:
>> On 08/30/2012 08:37 AM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 23, 2012 at 03:29:50PM -0400, Vlad Yasevich wrote:
>>>> This series of patches provides an ability to add VLAN IDs to the bridge
>>>> ports.  This is similar to what can be found in most switches.  The bridge
>>>> port may have any number of VLANs added to it including vlan 0 for untagged
>>>> traffic.  When vlans are added to the port, only traffic tagged with particular
>>>> vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
>>>> entries and become part of the lookup.  This way we correctly identify the FDB
>>>> entry.
>>>>
>>>> There are still pieces missing.  I don't yet support adding a static fdb entry
>>>> with a particular vlan.  There is no netlink support for carrying a vlan id.
>>>>
>>>> I'd like to hear thoughts of whether this is usufull and something we should
>>>> persue.
>>>>
>>>> The default behavior ofthe bridge is unchanged if no vlans have been
>>>> configured.
>>>
>>> Overall the feature looks good, I can think of some uses
>>> for it - for example, it could become useful for VMs if
>>> we add support to tap essentially stripping tags in Xmit but maybe you
>>> could be more explicit about what you have in mind?
>>> Do you plan to add tap support as well?
>>
>> Yes,  this is something I've thought of.  Not sure if it would be at tap
>> or bridge itself.  Need to work out where best to do it.
>
> It's certainly much easier to do in tap.
> A 20 line patch should do it.
> Does stripping tags seem like something bridge should do?

I agree.  It would be easier in tap.  There also the other side of 
adding tags for outbound traffic.  This would allow auto-access like 
functionality where the guest itself doesn't know anything about vlans,
but the bridge port will add/remove vlans as appropriate.  This is on
the list of features I want to support.

-vlad

>
>>> Also - what tool support do you plan?
>>
>> the patchset includes brctl to configure, but that seems to be
>> getting deprecated.  I am working on iproute2 to add capability to
>> configure this.
>>
>>>
>>> I also found some coding style issues and some bugs in
>>> the patchset. Sent on list.
>>
>> Thanks
>> -vlad
>>
>>>
>>> Hope this helps.
>>>

Re: [RFC PATCH bridge 0/5] Add basic VLAN support to bridges

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 10:41:00AM -0400, Vlad Yasevich wrote:
> On 08/30/2012 10:34 AM, Michael S. Tsirkin wrote:
> >On Thu, Aug 30, 2012 at 09:37:17AM -0400, Vlad Yasevich wrote:
> >>On 08/30/2012 08:37 AM, Michael S. Tsirkin wrote:
> >>>On Thu, Aug 23, 2012 at 03:29:50PM -0400, Vlad Yasevich wrote:
> >>>>This series of patches provides an ability to add VLAN IDs to the bridge
> >>>>ports.  This is similar to what can be found in most switches.  The bridge
> >>>>port may have any number of VLANs added to it including vlan 0 for untagged
> >>>>traffic.  When vlans are added to the port, only traffic tagged with particular
> >>>>vlan will forwarded over this port.  Additionally, vlan ids are added to FDB
> >>>>entries and become part of the lookup.  This way we correctly identify the FDB
> >>>>entry.
> >>>>
> >>>>There are still pieces missing.  I don't yet support adding a static fdb entry
> >>>>with a particular vlan.  There is no netlink support for carrying a vlan id.
> >>>>
> >>>>I'd like to hear thoughts of whether this is usufull and something we should
> >>>>persue.
> >>>>
> >>>>The default behavior ofthe bridge is unchanged if no vlans have been
> >>>>configured.
> >>>
> >>>Overall the feature looks good, I can think of some uses
> >>>for it - for example, it could become useful for VMs if
> >>>we add support to tap essentially stripping tags in Xmit but maybe you
> >>>could be more explicit about what you have in mind?
> >>>Do you plan to add tap support as well?
> >>
> >>Yes,  this is something I've thought of.  Not sure if it would be at tap
> >>or bridge itself.  Need to work out where best to do it.
> >
> >It's certainly much easier to do in tap.
> >A 20 line patch should do it.
> >Does stripping tags seem like something bridge should do?
> 
> I agree.  It would be easier in tap.  There also the other side of
> adding tags for outbound traffic.  This would allow auto-access like
> functionality where the guest itself doesn't know anything about
> vlans,
> but the bridge port will add/remove vlans as appropriate.  This is on
> the list of features I want to support.
> 
> -vlad

Looks like easier in tap too. You can only add 1 vlan :)

> >
> >>>Also - what tool support do you plan?
> >>
> >>the patchset includes brctl to configure, but that seems to be
> >>getting deprecated.  I am working on iproute2 to add capability to
> >>configure this.
> >>
> >>>
> >>>I also found some coding style issues and some bugs in
> >>>the patchset. Sent on list.
> >>
> >>Thanks
> >>-vlad
> >>
> >>>
> >>>Hope this helps.
> >>>