| 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: <52B09DB2.7040700@redhat.com>
Date: Tue, 17 Dec 2013 13:53:38 -0500
From: Vlad Yasevich <vyasevic@...hat.com>
To: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
"David S . Miller" <davem@...emloft.net>,
Stephen Hemminger <stephen@...workplumber.org>,
netdev@...r.kernel.org
Subject: Re: [PATCH net v2 5/9] bridge: Fix the way to check if a local fdb
entry can be deleted
On 12/17/2013 07:03 AM, Toshiaki Makita wrote:
> We should take into account the followings when deleting a local fdb
> entry.
>
> - nbp_vlan_find() can be used only when vid != 0 to check if an entry is
> deletable, because a fdb entry with vid 0 can exist at any time while
> nbp_vlan_find() always return false with vid 0.
>
> Example of problematic case:
> ip link set eth0 address 12:34:56:78:90:ab
> ip link set eth1 address 12:34:56:78:90:ab
> brctl addif br0 eth0
> brctl addif br0 eth1
> ip link set eth0 address aa:bb:cc:dd:ee:ff
> Then, the fdb entry 12:34:56:78:90:ab will be deleted even though the
> bridge port eth1 still has that address.
>
> - The port to which the bridge device is attached might needs a local entry
> if its mac address is set manually.
>
> Example of problematic case:
> ip link set eth0 address 12:34:56:78:90:ab
> brctl addif br0 eth0
> ip link set br0 address 12:34:56:78:90:ab
> ip link set eth0 address aa:bb:cc:dd:ee:ff
> Then, the fdb still must have the entry 12:34:56:78:90:ab, but it will be
> deleted.
>
> We can use br->dev->addr_assign_type to check if the address is manually
> set or not, but I propose another approach.
>
> Since we delete and insert local entries whenever changing mac address
> of the bridge device, we can change dst of the entry to NULL regardless of
> addr_assign_type when deleting an entry associated with a certain port,
> and if it is found to be unnecessary later, then delete it.
> That is, if changing mac address of a port, the entry might be changed
> to its dst being NULL first, but is eventually deleted when recalculating
> and changing bridge id.
>
> This approach is especially useful when we want to share the code with
> deleting vlan in which the bridge device might want such an entry regardless
> of addr_assign_type, and makes things easy because we don't have to consider
> if mac address of the bridge device will be changed or not at the time we
> delete a local entry of a port, which meas fdb code will not be bothered even
> if the bridge id calculating logic is changed in the future.
>
> Note that this is a slight change in behavior where the bridge device can
> receive the traffic to the old address during the short window between
> calling br_fdb_changeaddr() and br_stp_recalculate_bridge_id() in
> br_device_event(). However, it is not a problem because we still have the
> address on the bridge device.
I think you are understating the significance here a little bit. The
change is that for a short period of time after a port has been removed,
packets addressed to the MAC of that port may be delivered only to
bridge device instead of being flooded.
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
> ---
> net/bridge/br_fdb.c | 10 +++++++++-
> net/bridge/br_private.h | 6 ++++++
> net/bridge/br_vlan.c | 19 +++++++++++++++++++
> 3 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 5f1bd11..cf8b64e 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -114,12 +114,20 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
> if (op != p &&
> ether_addr_equal(op->dev->dev_addr,
> f->addr.addr) &&
> - nbp_vlan_find(op, vid)) {
> + (!vid || nbp_vlan_find(op, vid))) {
> f->dst = op;
> goto skip_delete;
> }
> }
>
> + /* maybe bridge device has same hw addr? */
> + if (ether_addr_equal(br->dev->dev_addr,
> + f->addr.addr) &&
I think this really needs a
br->dev->addr_assign_type ==
NET_ADDR_SET &&
> + (!vid || br_vlan_find(br, vid))) {
That way we'll only do this if the user actively set the bridge mac
to one be the same as one of the ports.
-vlad
> + f->dst = NULL;
> + goto skip_delete;
> + }
> +
> /* delete old one */
> fdb_delete(br, f);
> skip_delete:
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 91fb2c2..868c059 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -594,6 +594,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
> int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
> int br_vlan_delete(struct net_bridge *br, u16 vid);
> void br_vlan_flush(struct net_bridge *br);
> +bool br_vlan_find(struct net_bridge *br, u16 vid);
> int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
> int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
> int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
> @@ -675,6 +676,11 @@ static inline void br_vlan_flush(struct net_bridge *br)
> {
> }
>
> +static inline bool br_vlan_find(struct net_bridge *br, u16 vid)
> +{
> + return false;
> +}
> +
> static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags)
> {
> return -EOPNOTSUPP;
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index af5ebd1..f87ab88f 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -316,6 +316,25 @@ void br_vlan_flush(struct net_bridge *br)
> __vlan_flush(pv);
> }
>
> +bool br_vlan_find(struct net_bridge *br, u16 vid)
> +{
> + struct net_port_vlans *pv;
> + bool found = false;
> +
> + rcu_read_lock();
> + pv = rcu_dereference(br->vlan_info);
> +
> + if (!pv)
> + goto out;
> +
> + if (test_bit(vid, pv->vlan_bitmap))
> + found = true;
> +
> +out:
> + rcu_read_unlock();
> + return found;
> +}
> +
> int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
> {
> if (!rtnl_trylock())
>
--
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