From: Vlad Yasevich <vyasevic@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: netdev@vger.kernel.org
Subject: Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries
Date: Thu, 30 Aug 2012 10:48:57 -0400 [thread overview]
Message-ID: <503F7D59.4030906@redhat.com> (raw)
In-Reply-To: <20120830143303.GA21630@redhat.com>
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
>
next prev parent reply other threads:[~2012-08-30 14:48 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-23 19:29 [RFC PATCH bridge 0/5] Add basic VLAN support to bridges Vlad Yasevich
2012-08-23 19:29 ` [RFC PATCH bridge 1/5] bridge: Add vlan check to forwarding path Vlad Yasevich
2012-08-23 20:58 ` Nicolas de Pesloüan
2012-08-30 12:19 ` Michael S. Tsirkin
2012-08-23 19:29 ` [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries Vlad Yasevich
2012-08-23 19:39 ` Stephen Hemminger
2012-08-23 19:42 ` Vlad Yasevich
2012-08-30 14:33 ` Michael S. Tsirkin
2012-08-30 14:48 ` Vlad Yasevich [this message]
2012-08-30 15:45 ` Michael S. Tsirkin
2012-08-30 16:07 ` Vlad Yasevich
2012-08-23 19:29 ` [RFC PATCH bridge 3/5] bridge: Add vlan id to multicast groups Vlad Yasevich
2012-08-30 12:30 ` Michael S. Tsirkin
2012-08-30 12:55 ` Eric Dumazet
2012-08-23 19:29 ` [RFC PATCH bridge 4/5] bridge: Add private ioctls to configure vlans on bridge ports Vlad Yasevich
2012-08-23 19:38 ` Stephen Hemminger
2012-08-23 19:41 ` Vlad Yasevich
2012-08-24 17:56 ` Paul E. McKenney
2012-08-24 18:11 ` Stephen Hemminger
2012-08-24 18:19 ` Vlad Yasevich
2012-08-30 12:17 ` Michael S. Tsirkin
2012-08-23 19:29 ` [RFC PATCH bridge 5/5] bridge: Add sysfs interface to display VLANS Vlad Yasevich
2012-08-30 12:27 ` Michael S. Tsirkin
2012-08-30 14:05 ` Vlad Yasevich
2012-08-30 14:26 ` Michael S. Tsirkin
2012-08-30 14:36 ` Vlad Yasevich
2012-08-30 14:44 ` Michael S. Tsirkin
2012-08-30 14:51 ` Vlad Yasevich
2012-08-30 15:03 ` Michael S. Tsirkin
2012-08-30 15:07 ` Vlad Yasevich
2012-08-30 15:47 ` Michael S. Tsirkin
2012-08-30 15:52 ` Vlad Yasevich
2012-08-30 15:58 ` Michael S. Tsirkin
2012-08-23 19:41 ` [RFC PATCH bridge 0/5] Add basic VLAN support to bridges Stephen Hemminger
2012-08-23 19:53 ` Vlad Yasevich
2012-08-23 21:03 ` Nicolas de Pesloüan
2012-08-23 21:12 ` Stephen Hemminger
2012-08-24 2:52 ` Vlad Yasevich
2012-08-24 20:44 ` Stephen Hemminger
2012-08-24 21:09 ` Nicolas de Pesloüan
2012-08-30 12:37 ` Michael S. Tsirkin
2012-08-30 13:37 ` Vlad Yasevich
2012-08-30 14:34 ` Michael S. Tsirkin
2012-08-30 14:41 ` Vlad Yasevich
2012-08-30 14:46 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=503F7D59.4030906@redhat.com \
--to=vyasevic@redhat.com \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).