Skip to content

Commit

Permalink
bridge: vlan: add per-vlan struct and move to rhashtables
Browse files Browse the repository at this point in the history
This patch changes the bridge vlan implementation to use rhashtables
instead of bitmaps. The main motivation behind this change is that we
need extensible per-vlan structures (both per-port and global) so more
advanced features can be introduced and the vlan support can be
extended. I've tried to break this up but the moment net_port_vlans is
changed and the whole API goes away, thus this is a larger patch.
A few short goals of this patch are:
- Extensible per-vlan structs stored in rhashtables and a sorted list
- Keep user-visible behaviour (compressed vlans etc)
- Keep fastpath ingress/egress logic the same (optimizations to come
  later)

Here's a brief list of some of the new features we'd like to introduce:
- per-vlan counters
- vlan ingress/egress mapping
- per-vlan igmp configuration
- vlan priorities
- avoid fdb entries replication (e.g. local fdb scaling issues)

The structure is kept single for both global and per-port entries so to
avoid code duplication where possible and also because we'll soon introduce
"port0 / aka bridge as port" which should simplify things further
(thanks to Vlad for the suggestion!).

Now we have per-vlan global rhashtable (bridge-wide) and per-vlan port
rhashtable, if an entry is added to a port it'll get a pointer to its
global context so it can be quickly accessed later. There's also a
sorted vlan list which is used for stable walks and some user-visible
behaviour such as the vlan ranges, also for error paths.
VLANs are stored in a "vlan group" which currently contains the
rhashtable, sorted vlan list and the number of "real" vlan entries.
A good side-effect of this change is that it resembles how hw keeps
per-vlan data.
One important note after this change is that if a VLAN is being looked up
in the bridge's rhashtable for filtering purposes (or to check if it's an
existing usable entry, not just a global context) then the new helper
br_vlan_should_use() needs to be used if the vlan is found. In case the
lookup is done only with a port's vlan group, then this check can be
skipped.

Things tested so far:
- basic vlan ingress/egress
- pvids
- untagged vlans
- undef CONFIG_BRIDGE_VLAN_FILTERING
- adding/deleting vlans in different scenarios (with/without global ctx,
  while transmitting traffic, in ranges etc)
- loading/removing the module while having/adding/deleting vlans
- extracting bridge vlan information (user ABI), compressed requests
- adding/deleting fdbs on vlans
- bridge mac change, promisc mode
- default pvid change
- kmemleak ON during the whole time

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Nikolay Aleksandrov authored and David S. Miller committed Sep 29, 2015
1 parent 191988e commit 2594e90
Show file tree
Hide file tree
Showing 9 changed files with 732 additions and 460 deletions.
1 change: 1 addition & 0 deletions include/uapi/linux/if_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ enum {
#define BRIDGE_VLAN_INFO_UNTAGGED (1<<2) /* VLAN egresses untagged */
#define BRIDGE_VLAN_INFO_RANGE_BEGIN (1<<3) /* VLAN is start of vlan range */
#define BRIDGE_VLAN_INFO_RANGE_END (1<<4) /* VLAN is end of vlan range */
#define BRIDGE_VLAN_INFO_BRENTRY (1<<5) /* Global bridge VLAN entry */

struct bridge_vlan_info {
__u16 flags;
Expand Down
2 changes: 1 addition & 1 deletion net/bridge/br_device.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
skb_reset_mac_header(skb);
skb_pull(skb, ETH_HLEN);

if (!br_allowed_ingress(br, br_get_vlan_info(br), skb, &vid))
if (!br_allowed_ingress(br, skb, &vid))
goto out;

if (is_broadcast_ether_addr(dest))
Expand Down
76 changes: 39 additions & 37 deletions net/bridge/br_fdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -163,22 +163,27 @@ static void fdb_delete_local(struct net_bridge *br,
struct net_bridge_fdb_entry *f)
{
const unsigned char *addr = f->addr.addr;
u16 vid = f->vlan_id;
struct net_bridge_vlan_group *vg;
const struct net_bridge_vlan *v;
struct net_bridge_port *op;
u16 vid = f->vlan_id;

/* Maybe another port has same hw addr? */
list_for_each_entry(op, &br->port_list, list) {
vg = nbp_vlan_group(op);
if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
(!vid || nbp_vlan_find(op, vid))) {
(!vid || br_vlan_find(vg, vid))) {
f->dst = op;
f->added_by_user = 0;
return;
}
}

vg = br_vlan_group(br);
v = br_vlan_find(vg, vid);
/* Maybe bridge device has same hw addr? */
if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
(!vid || br_vlan_find(br, vid))) {
(!vid || (v && br_vlan_should_use(v)))) {
f->dst = NULL;
f->added_by_user = 0;
return;
Expand All @@ -203,14 +208,14 @@ void br_fdb_find_delete_local(struct net_bridge *br,

void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
{
struct net_bridge_vlan_group *vg;
struct net_bridge *br = p->br;
struct net_port_vlans *pv = nbp_get_vlan_info(p);
bool no_vlan = !pv;
struct net_bridge_vlan *v;
int i;
u16 vid;

spin_lock_bh(&br->hash_lock);

vg = nbp_vlan_group(p);
/* Search all chains since old address/hash is unknown */
for (i = 0; i < BR_HASH_SIZE; i++) {
struct hlist_node *h;
Expand All @@ -226,7 +231,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
* configured, we can safely be done at
* this point.
*/
if (no_vlan)
if (!vg || !vg->num_vlans)
goto insert;
}
}
Expand All @@ -236,25 +241,25 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
/* insert new address, may fail if invalid address or dup. */
fdb_insert(br, p, newaddr, 0);

if (no_vlan)
if (!vg || !vg->num_vlans)
goto done;

/* Now add entries for every VLAN configured on the port.
* This function runs under RTNL so the bitmap will not change
* from under us.
*/
for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID)
fdb_insert(br, p, newaddr, vid);
list_for_each_entry(v, &vg->vlan_list, vlist)
fdb_insert(br, p, newaddr, v->vid);

done:
spin_unlock_bh(&br->hash_lock);
}

void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
{
struct net_bridge_vlan_group *vg;
struct net_bridge_fdb_entry *f;
struct net_port_vlans *pv;
u16 vid = 0;
struct net_bridge_vlan *v;

spin_lock_bh(&br->hash_lock);

Expand All @@ -264,20 +269,18 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
fdb_delete_local(br, NULL, f);

fdb_insert(br, NULL, newaddr, 0);

vg = br_vlan_group(br);
if (!vg || !vg->num_vlans)
goto out;
/* Now remove and add entries for every VLAN configured on the
* bridge. This function runs under RTNL so the bitmap will not
* change from under us.
*/
pv = br_get_vlan_info(br);
if (!pv)
goto out;

for_each_set_bit_from(vid, pv->vlan_bitmap, VLAN_N_VID) {
f = __br_fdb_get(br, br->dev->dev_addr, vid);
list_for_each_entry(v, &vg->vlan_list, vlist) {
f = __br_fdb_get(br, br->dev->dev_addr, v->vid);
if (f && f->is_local && !f->dst)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, vid);
fdb_insert(br, NULL, newaddr, v->vid);
}
out:
spin_unlock_bh(&br->hash_lock);
Expand Down Expand Up @@ -844,9 +847,10 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
struct net_device *dev,
const unsigned char *addr, u16 vid, u16 nlh_flags)
{
struct net_bridge_vlan_group *vg;
struct net_bridge_port *p;
struct net_bridge_vlan *v;
int err = 0;
struct net_port_vlans *pv;

if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_NOARP|NUD_REACHABLE))) {
pr_info("bridge: RTM_NEWNEIGH with invalid state %#x\n", ndm->ndm_state);
Expand All @@ -865,9 +869,10 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
return -EINVAL;
}

pv = nbp_get_vlan_info(p);
vg = nbp_vlan_group(p);
if (vid) {
if (!pv || !test_bit(vid, pv->vlan_bitmap)) {
v = br_vlan_find(vg, vid);
if (!v) {
pr_info("bridge: RTM_NEWNEIGH with unconfigured "
"vlan %d on port %s\n", vid, dev->name);
return -EINVAL;
Expand All @@ -877,15 +882,15 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
err = __br_fdb_add(ndm, p, addr, nlh_flags, vid);
} else {
err = __br_fdb_add(ndm, p, addr, nlh_flags, 0);
if (err || !pv)
if (err || !vg || !vg->num_vlans)
goto out;

/* We have vlans configured on this port and user didn't
* specify a VLAN. To be nice, add/update entry for every
* vlan on this port.
*/
for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
err = __br_fdb_add(ndm, p, addr, nlh_flags, vid);
list_for_each_entry(v, &vg->vlan_list, vlist) {
err = __br_fdb_add(ndm, p, addr, nlh_flags, v->vid);
if (err)
goto out;
}
Expand Down Expand Up @@ -927,9 +932,10 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
struct net_device *dev,
const unsigned char *addr, u16 vid)
{
struct net_bridge_vlan_group *vg;
struct net_bridge_port *p;
struct net_bridge_vlan *v;
int err;
struct net_port_vlans *pv;

p = br_port_get_rtnl(dev);
if (p == NULL) {
Expand All @@ -938,9 +944,10 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
return -EINVAL;
}

pv = nbp_get_vlan_info(p);
vg = nbp_vlan_group(p);
if (vid) {
if (!pv || !test_bit(vid, pv->vlan_bitmap)) {
v = br_vlan_find(vg, vid);
if (!v) {
pr_info("bridge: RTM_DELNEIGH with unconfigured "
"vlan %d on port %s\n", vid, dev->name);
return -EINVAL;
Expand All @@ -950,16 +957,11 @@ int br_fdb_delete(struct ndmsg *ndm, struct nlattr *tb[],
} else {
err = -ENOENT;
err &= __br_fdb_delete(p, addr, 0);
if (!pv)
if (!vg || !vg->num_vlans)
goto out;

/* We have vlans configured on this port and user didn't
* specify a VLAN. To be nice, add/update entry for every
* vlan on this port.
*/
for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
err &= __br_fdb_delete(p, addr, vid);
}
list_for_each_entry(v, &vg->vlan_list, vlist)
err &= __br_fdb_delete(p, addr, v->vid);
}
out:
return err;
Expand Down
15 changes: 11 additions & 4 deletions net/bridge/br_forward.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ static int deliver_clone(const struct net_bridge_port *prev,
static inline int should_deliver(const struct net_bridge_port *p,
const struct sk_buff *skb)
{
struct net_bridge_vlan_group *vg;

vg = nbp_vlan_group(p);
return ((p->flags & BR_HAIRPIN_MODE) || skb->dev != p->dev) &&
br_allowed_egress(p->br, nbp_get_vlan_info(p), skb) &&
p->state == BR_STATE_FORWARDING;
br_allowed_egress(vg, skb) && p->state == BR_STATE_FORWARDING;
}

int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
Expand Down Expand Up @@ -76,7 +78,10 @@ EXPORT_SYMBOL_GPL(br_forward_finish);

static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
{
skb = br_handle_vlan(to->br, nbp_get_vlan_info(to), skb);
struct net_bridge_vlan_group *vg;

vg = nbp_vlan_group(to);
skb = br_handle_vlan(to->br, vg, skb);
if (!skb)
return;

Expand All @@ -99,14 +104,16 @@ static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)

static void __br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
{
struct net_bridge_vlan_group *vg;
struct net_device *indev;

if (skb_warn_if_lro(skb)) {
kfree_skb(skb);
return;
}

skb = br_handle_vlan(to->br, nbp_get_vlan_info(to), skb);
vg = nbp_vlan_group(to);
skb = br_handle_vlan(to->br, vg, skb);
if (!skb)
return;

Expand Down
10 changes: 5 additions & 5 deletions net/bridge/br_input.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,28 @@ static int br_pass_frame_up(struct sk_buff *skb)
{
struct net_device *indev, *brdev = BR_INPUT_SKB_CB(skb)->brdev;
struct net_bridge *br = netdev_priv(brdev);
struct net_bridge_vlan_group *vg;
struct pcpu_sw_netstats *brstats = this_cpu_ptr(br->stats);
struct net_port_vlans *pv;

u64_stats_update_begin(&brstats->syncp);
brstats->rx_packets++;
brstats->rx_bytes += skb->len;
u64_stats_update_end(&brstats->syncp);

vg = br_vlan_group(br);
/* Bridge is just like any other port. Make sure the
* packet is allowed except in promisc modue when someone
* may be running packet capture.
*/
pv = br_get_vlan_info(br);
if (!(brdev->flags & IFF_PROMISC) &&
!br_allowed_egress(br, pv, skb)) {
!br_allowed_egress(vg, skb)) {
kfree_skb(skb);
return NET_RX_DROP;
}

indev = skb->dev;
skb->dev = brdev;
skb = br_handle_vlan(br, pv, skb);
skb = br_handle_vlan(br, vg, skb);
if (!skb)
return NET_RX_DROP;

Expand Down Expand Up @@ -140,7 +140,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
if (!p || p->state == BR_STATE_DISABLED)
goto drop;

if (!br_allowed_ingress(p->br, nbp_get_vlan_info(p), skb, &vid))
if (!nbp_allowed_ingress(p, skb, &vid))
goto out;

/* insert into forwarding database after filtering to avoid spoofing */
Expand Down
24 changes: 12 additions & 12 deletions net/bridge/br_mdb.c
Original file line number Diff line number Diff line change
Expand Up @@ -464,11 +464,11 @@ static int __br_mdb_add(struct net *net, struct net_bridge *br,
static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh)
{
struct net *net = sock_net(skb->sk);
unsigned short vid = VLAN_N_VID;
struct net_bridge_vlan_group *vg;
struct net_device *dev, *pdev;
struct br_mdb_entry *entry;
struct net_bridge_port *p;
struct net_port_vlans *pv;
struct net_bridge_vlan *v;
struct net_bridge *br;
int err;

Expand All @@ -489,10 +489,10 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh)
if (!p || p->br != br || p->state == BR_STATE_DISABLED)
return -EINVAL;

pv = nbp_get_vlan_info(p);
if (br_vlan_enabled(br) && pv && entry->vid == 0) {
for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
entry->vid = vid;
vg = nbp_vlan_group(p);
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
err = __br_mdb_add(net, br, entry);
if (err)
break;
Expand Down Expand Up @@ -566,11 +566,11 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh)
{
struct net *net = sock_net(skb->sk);
unsigned short vid = VLAN_N_VID;
struct net_bridge_vlan_group *vg;
struct net_device *dev, *pdev;
struct br_mdb_entry *entry;
struct net_bridge_port *p;
struct net_port_vlans *pv;
struct net_bridge_vlan *v;
struct net_bridge *br;
int err;

Expand All @@ -591,10 +591,10 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh)
if (!p || p->br != br || p->state == BR_STATE_DISABLED)
return -EINVAL;

pv = nbp_get_vlan_info(p);
if (br_vlan_enabled(br) && pv && entry->vid == 0) {
for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) {
entry->vid = vid;
vg = nbp_vlan_group(p);
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, &vg->vlan_list, vlist) {
entry->vid = v->vid;
err = __br_mdb_del(br, entry);
if (!err)
__br_mdb_notify(dev, entry, RTM_DELMDB);
Expand Down
Loading

0 comments on commit 2594e90

Please sign in to comment.