Re: [PATCH net-next] bridge: Synchronize unicast filtering with FDB

[Toshiaki Makita]

On 2016/06/04 2:35, Nikolay Aleksandrov wrote:
...
>>> void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char 
>>> *newaddr)
>>> @@ -288,6 +296,95 @@ out:
>>> spin_unlock_bh(>hash_lock);
>>> }
>>>
>>> +void br_fdb_sync_uc(struct net_bridge *br)
>>> +{
>>> +   struct net_bridge_vlan_group *vg;
>>> +   struct netdev_hw_addr *ha;
>>> +   int i;
>>> +
>>> +   spin_lock_bh(>hash_lock);
>>> +
>>> +   for (i = 0; i < BR_HASH_SIZE; i++) {
>>> +   struct hlist_node *h;
>>> +
>>> +   hlist_for_each(h, >hash[i]) {
>>> +   struct net_bridge_fdb_entry *f;
>>> +
>>> +   f = hlist_entry(h, struct net_bridge_fdb_entry, hlist);
>>> +   if (!f->dst && f->is_local && !f->added_by_user &&
>>> +   !ether_addr_equal(f->addr.addr, br->dev->dev_addr)) 
>>> {
>>> +   /* delete old one */
>>> +   fdb_delete_local(br, NULL, f);
>>> +   }
>>> +   }
>>> +   }
>>> +
>>> +   vg = br_vlan_group(br);
>>> +
>>> +   /* insert new address,  may fail if invalid address or dup. */
>>> +   netdev_for_each_uc_addr(ha, br->dev) {
>>> +   struct net_bridge_vlan *v;
>>> +
>>> +   fdb_insert(br, NULL, ha->addr, 0);
>>> +
>>> +   if (!vg || !vg->num_vlans)
>>> +   continue;
>>> +
>>> +   list_for_each_entry(v, >vlan_list, vlist)
>>> +   fdb_insert(br, NULL, ha->addr, v->vid);
>>
>> Since here you’re walking over the bridge’s vlan list, you should test the 
>> vlans with br_vlan_should_use()
>> because it can be a global context holder if the vlan was configured only on 
>> ports.

Thank you for your feedback.
will fix in v2.

I actually thought that this is the same logic as
br_fdb_change_mac_address() so assumed it should be all right.
Does br_fdb_change_mac_address() need br_vlan_should_use() as well?

Toshiaki Makita

Re: [PATCH net-next] bridge: Synchronize unicast filtering with FDB

[Nikolay Aleksandrov]

> On Jun 3, 2016, at 5:54 PM, Nikolay Aleksandrov  
> wrote:
> 
>> 
>> On Jun 3, 2016, at 11:33 AM, Toshiaki Makita  
>> wrote:
>> 
>> Patrick Schaaf reported that flooding due to a missing fdb entry of
>> the address of macvlan on the bridge device caused high CPU
>> consumption of an openvpn process behind a tap bridge port.
>> Adding an fdb entry of the macvlan address can suppress flooding
>> and avoid this problem.
>> 
>> This change makes bridge able to synchronize unicast filtering with
>> fdb automatically so admin do not need to manually add an fdb entry.
>> This effectively supports IFF_UNICAST_FLT in bridge, thus adding an
>> macvlan device would not place bridge into promiscuous mode as well.
>> 
>> Reported-by: Patrick Schaaf 
>> Signed-off-by: Toshiaki Makita 
>> ---
>> net/bridge/br_device.c  |   7 +--
>> net/bridge/br_fdb.c | 117 
>> +++-
>> net/bridge/br_private.h |   7 +--
>> net/bridge/br_vlan.c|  11 +++--
>> 4 files changed, 120 insertions(+), 22 deletions(-)
>> 
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 2c8095a..9b56802 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -123,8 +123,9 @@ static int br_dev_open(struct net_device *dev)
>>  return 0;
>> }
>> 
>> -static void br_dev_set_multicast_list(struct net_device *dev)
>> +static void br_dev_set_rx_mode(struct net_device *dev)
>> {
>> +br_fdb_sync_uc(netdev_priv(dev));
>> }
>> 
>> static void br_dev_change_rx_flags(struct net_device *dev, int change)
>> @@ -329,7 +330,7 @@ static const struct net_device_ops br_netdev_ops = {
>>  .ndo_start_xmit  = br_dev_xmit,
>>  .ndo_get_stats64 = br_get_stats64,
>>  .ndo_set_mac_address = br_set_mac_address,
>> -.ndo_set_rx_mode = br_dev_set_multicast_list,
>> +.ndo_set_rx_mode = br_dev_set_rx_mode,
>>  .ndo_change_rx_flags = br_dev_change_rx_flags,
>>  .ndo_change_mtu  = br_change_mtu,
>>  .ndo_do_ioctl= br_dev_ioctl,
>> @@ -373,7 +374,7 @@ void br_dev_setup(struct net_device *dev)
>>  dev->destructor = br_dev_free;
>>  dev->ethtool_ops = _ethtool_ops;
>>  SET_NETDEV_DEVTYPE(dev, _type);
>> -dev->priv_flags = IFF_EBRIDGE | IFF_NO_QUEUE;
>> +dev->priv_flags = IFF_EBRIDGE | IFF_NO_QUEUE | IFF_UNICAST_FLT;
>> 
>>  dev->features = COMMON_FEATURES | NETIF_F_LLTX | NETIF_F_NETNS_LOCAL |
>>  NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index dcea4f4..bc082e6 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -184,28 +184,36 @@ static void fdb_delete_local(struct net_bridge *br,
>>  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 || (v && br_vlan_should_use(v {
>> -f->dst = NULL;
>> -f->added_by_user = 0;
>> -return;
>> +if (p && (!vid || (v && br_vlan_should_use(v {
>> +struct netdev_hw_addr *ha;
>> +
>> +if (ether_addr_equal(br->dev->dev_addr, addr)) {
>> +f->dst = NULL;
>> +f->added_by_user = 0;
>> +return;
>> +}
>> +netdev_for_each_uc_addr(ha, br->dev) {
> 
> I think you need either netif_addr_lock or RCU in order to walk safely over 
> the uc list.

Hmm, actually rtnl looks enough and it is held in all paths that the list is 
walked here so
please don’t mind this comment, just the one about the bridge vlan entries.

Thanks,
 Nik

> 
>> +if (ether_addr_equal(ha->addr, addr)) {
>> +f->dst = NULL;
>> +f->added_by_user = 0;
>> +return;
>> +}
>> +}
>>  }
>> 
>>  fdb_delete(br, f);
>> }
>> 
>> -void br_fdb_find_delete_local(struct net_bridge *br,
>> -  const struct net_bridge_port *p,
>> -  const unsigned char *addr, u16 vid)
>> +static void fdb_find_delete_local(struct net_bridge *br,
>> +  const struct net_bridge_port *p,
>> +  const unsigned char *addr, u16 vid)
>> {
>>  struct hlist_head *head = >hash[br_mac_hash(addr, vid)];
>>  struct net_bridge_fdb_entry *f;
>> 
>> -spin_lock_bh(>hash_lock);
>>  f = fdb_find(head, addr, vid);
>>  if (f && f->is_local && !f->added_by_user && f->dst == p)
>>  fdb_delete_local(br, p, f);
>> -spin_unlock_bh(>hash_lock);
>> }
>> 
>> void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char 
>> *newaddr)
>> @@ -288,6 +296,95 @@ out:
>>  

Re: [PATCH net-next] bridge: Synchronize unicast filtering with FDB

[Nikolay Aleksandrov]

> On Jun 3, 2016, at 11:33 AM, Toshiaki Makita  
> wrote:
> 
> Patrick Schaaf reported that flooding due to a missing fdb entry of
> the address of macvlan on the bridge device caused high CPU
> consumption of an openvpn process behind a tap bridge port.
> Adding an fdb entry of the macvlan address can suppress flooding
> and avoid this problem.
> 
> This change makes bridge able to synchronize unicast filtering with
> fdb automatically so admin do not need to manually add an fdb entry.
> This effectively supports IFF_UNICAST_FLT in bridge, thus adding an
> macvlan device would not place bridge into promiscuous mode as well.
> 
> Reported-by: Patrick Schaaf 
> Signed-off-by: Toshiaki Makita 
> ---
> net/bridge/br_device.c  |   7 +--
> net/bridge/br_fdb.c | 117 +++-
> net/bridge/br_private.h |   7 +--
> net/bridge/br_vlan.c|  11 +++--
> 4 files changed, 120 insertions(+), 22 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 2c8095a..9b56802 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -123,8 +123,9 @@ static int br_dev_open(struct net_device *dev)
>   return 0;
> }
> 
> -static void br_dev_set_multicast_list(struct net_device *dev)
> +static void br_dev_set_rx_mode(struct net_device *dev)
> {
> + br_fdb_sync_uc(netdev_priv(dev));
> }
> 
> static void br_dev_change_rx_flags(struct net_device *dev, int change)
> @@ -329,7 +330,7 @@ static const struct net_device_ops br_netdev_ops = {
>   .ndo_start_xmit  = br_dev_xmit,
>   .ndo_get_stats64 = br_get_stats64,
>   .ndo_set_mac_address = br_set_mac_address,
> - .ndo_set_rx_mode = br_dev_set_multicast_list,
> + .ndo_set_rx_mode = br_dev_set_rx_mode,
>   .ndo_change_rx_flags = br_dev_change_rx_flags,
>   .ndo_change_mtu  = br_change_mtu,
>   .ndo_do_ioctl= br_dev_ioctl,
> @@ -373,7 +374,7 @@ void br_dev_setup(struct net_device *dev)
>   dev->destructor = br_dev_free;
>   dev->ethtool_ops = _ethtool_ops;
>   SET_NETDEV_DEVTYPE(dev, _type);
> - dev->priv_flags = IFF_EBRIDGE | IFF_NO_QUEUE;
> + dev->priv_flags = IFF_EBRIDGE | IFF_NO_QUEUE | IFF_UNICAST_FLT;
> 
>   dev->features = COMMON_FEATURES | NETIF_F_LLTX | NETIF_F_NETNS_LOCAL |
>   NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index dcea4f4..bc082e6 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -184,28 +184,36 @@ static void fdb_delete_local(struct net_bridge *br,
>   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 || (v && br_vlan_should_use(v {
> - f->dst = NULL;
> - f->added_by_user = 0;
> - return;
> + if (p && (!vid || (v && br_vlan_should_use(v {
> + struct netdev_hw_addr *ha;
> +
> + if (ether_addr_equal(br->dev->dev_addr, addr)) {
> + f->dst = NULL;
> + f->added_by_user = 0;
> + return;
> + }
> + netdev_for_each_uc_addr(ha, br->dev) {

I think you need either netif_addr_lock or RCU in order to walk safely over the 
uc list.

> + if (ether_addr_equal(ha->addr, addr)) {
> + f->dst = NULL;
> + f->added_by_user = 0;
> + return;
> + }
> + }
>   }
> 
>   fdb_delete(br, f);
> }
> 
> -void br_fdb_find_delete_local(struct net_bridge *br,
> -   const struct net_bridge_port *p,
> -   const unsigned char *addr, u16 vid)
> +static void fdb_find_delete_local(struct net_bridge *br,
> +   const struct net_bridge_port *p,
> +   const unsigned char *addr, u16 vid)
> {
>   struct hlist_head *head = >hash[br_mac_hash(addr, vid)];
>   struct net_bridge_fdb_entry *f;
> 
> - spin_lock_bh(>hash_lock);
>   f = fdb_find(head, addr, vid);
>   if (f && f->is_local && !f->added_by_user && f->dst == p)
>   fdb_delete_local(br, p, f);
> - spin_unlock_bh(>hash_lock);
> }
> 
> void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char 
> *newaddr)
> @@ -288,6 +296,95 @@ out:
>   spin_unlock_bh(>hash_lock);
> }
> 
> +void br_fdb_sync_uc(struct net_bridge *br)
> +{
> + struct net_bridge_vlan_group *vg;
> + struct netdev_hw_addr *ha;
> + int i;
> +
> + spin_lock_bh(>hash_lock);
> +
> + for (i = 0; i < BR_HASH_SIZE; i++) {
> + struct hlist_node *h;
> +
> + hlist_for_each(h, >hash[i]) {
>