| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <503F8FB8.8010609@redhat.com>
Date: Thu, 30 Aug 2012 12:07:20 -0400
From: Vlad Yasevich <vyasevic@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
CC: netdev@...r.kernel.org
Subject: Re: [RFC PATCH bridge 2/5] bridge: Add vlan to unicast fdb entries
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@...hat.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@...r.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@...r.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists