netdev - Re: [PATCH v9 net-next 01/12] bridge: Add vlan filtering infrastructure

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHXqBFJfPYwnxbeDR6AfG0-PQJ__=rKMxQWRcFJGseGiXyuYGg@mail.gmail.com>
Date:	Sat, 2 Feb 2013 02:04:09 +0100
From:	Michał Mirosław <mirqus@...il.com>
To:	Vlad Yasevich <vyasevic@...hat.com>
Cc:	shemminger@...tta.com, bridge@...ts.linux-foundation.org,
	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v9 net-next 01/12] bridge: Add vlan filtering infrastructure

2013/2/1 Vlad Yasevich <vyasevic@...hat.com>:
> Adds an optional infrustructure component to bridge that would allow
> native vlan filtering in the bridge.  Each bridge port (as well
> as the bridge device) now get a VLAN bitmap.  Each bit in the bitmap
> is associated with a vlan id.  This way if the bit corresponding to
> the vid is set in the bitmap that the packet with vid is allowed to
> enter and exit the port.
[...]
+struct net_port_vlans {
+       u16                             port_idx;
+       void                            *parent;
+       struct rcu_head                 rcu;
+       unsigned long                   vlan_bitmap[BR_VLAN_BITMAP_LEN];
+};
[later, in br_vlan.c]
+#define vlans_to_parent(type, pv) ((type *)(pv)->parent)

A really don't like this pointer casting. It's easy to get it wrong
(and you did in __vlan_del).

1. union { struct net_bridge *br; struct net_bridge_port *port; } parent;

2. inline net_port_vlans __rcu **br_vlan_info_parent_ptr(struct
net_port_vlans *v)
{
	return v->port_idx ? &v->parent.port->vlan_info : &v->parent.br->vlan_info;
}

(I'm not insisting on #2. Just think about it. ;)

> @@ -156,6 +166,7 @@ struct net_bridge_port
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>         struct netpoll                  *np;
>  #endif
> +       struct net_port_vlans __rcu     *vlan_info;
>  };

Missing #ifdef CONFIG_BRIDGE_VLAN_FILTERING?

> +static int __vlan_del(struct net_port_vlans *v, u16 vid)
> +{
> +       unsigned long first_bit;
> +       unsigned long last_bit;
> +
> +       if (!test_bit(vid, v->vlan_bitmap))
> +               return -EINVAL;
> +
> +       /* Check to see if any other vlans are in this table.  If this
> +        * is the last vlan, delete the whole structure.  If this is not the
> +        * last vlan, just clear the bit.
> +        */
> +       first_bit = find_first_bit(v->vlan_bitmap, BR_VLAN_BITMAP_LEN);
> +       last_bit = find_last_bit(v->vlan_bitmap, BR_VLAN_BITMAP_LEN);
> +
> +       if (v->port_idx && vid) {
> +               struct net_device *dev =
> +                       vlans_to_parent(struct net_bridge, v)->dev;

struct net_bridge_port (or union and v->parent.port)

> +
> +               if (dev->features & NETIF_F_HW_VLAN_FILTER)
> +                       dev->netdev_ops->ndo_vlan_rx_kill_vid(dev, vid);
> +       }
> +
> +       clear_bit(vid, v->vlan_bitmap);
> +       if (first_bit == last_bit) {

bitmap_empty() is faster than two times find_xxx_bit().

> +               if (v->port_idx) {
> +                       struct net_bridge_port *p =
> +                               vlans_to_parent(struct net_bridge_port, v);
> +                       rcu_assign_pointer(p->vlan_info, NULL);
> +               } else {
> +                       struct net_bridge *br =
> +                               vlans_to_parent(struct net_bridge, v);
> +                       rcu_assign_pointer(br->vlan_info, NULL);
> +               }

With inline function above:

rcu_assign_pointer(*br_vlan_info_parent_ptr(v), NULL);

> +               kfree_rcu(v, rcu);
> +       }
> +       return 0;
> +}
> +
> +static void __vlan_flush(struct net_port_vlans *v)
> +{
> +       bitmap_zero(v->vlan_bitmap, BR_VLAN_BITMAP_LEN);
> +       if (v->port_idx) {
> +               struct net_bridge_port *p =
> +                               vlans_to_parent(struct net_bridge_port, v);
> +               rcu_assign_pointer(p->vlan_info, NULL);
> +       } else {
> +               struct net_bridge *br =
> +                               vlans_to_parent(struct net_bridge, v);
> +               rcu_assign_pointer(br->vlan_info, NULL);
> +       }

Same here.

> +       kfree_rcu(v, rcu);
> +}

Best Regards,
Michał Mirosław
--
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