diff mbox series

[net-next,2/6] net: bridge: fdb: add support for fine-grained flushing

Message ID 20220409105857.803667-3-razor@blackwall.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: bridge: add flush filtering support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/cc_maintainers warning 1 maintainers not CCed: pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 6 this patch: 6
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 96 lines checked
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0

Commit Message

Nikolay Aleksandrov April 9, 2022, 10:58 a.m. UTC 
Add the ability to specify exactly which fdbs to be flushed. They are
described by a new structure - net_bridge_fdb_flush_desc. Currently it
can match on port/bridge ifindex, vlan id and fdb flags. It is used to
describe the existing dynamic fdb flush operation.

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 net/bridge/br_fdb.c      | 36 +++++++++++++++++++++++++++++-------
 net/bridge/br_netlink.c  |  9 +++++++--
 net/bridge/br_private.h  | 10 +++++++++-
 net/bridge/br_sysfs_br.c |  6 +++++-
 4 files changed, 50 insertions(+), 11 deletions(-)

Comments

Ido Schimmel April 11, 2022, 8:20 a.m. UTC | #1 
On Sat, Apr 09, 2022 at 01:58:53PM +0300, Nikolay Aleksandrov wrote:
> Add the ability to specify exactly which fdbs to be flushed. They are
> described by a new structure - net_bridge_fdb_flush_desc. Currently it
> can match on port/bridge ifindex, vlan id and fdb flags. It is used to
> describe the existing dynamic fdb flush operation.
> 
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>  net/bridge/br_fdb.c      | 36 +++++++++++++++++++++++++++++-------
>  net/bridge/br_netlink.c  |  9 +++++++--
>  net/bridge/br_private.h  | 10 +++++++++-
>  net/bridge/br_sysfs_br.c |  6 +++++-
>  4 files changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 6ccda68bd473..4b0bf88c4121 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -558,18 +558,40 @@ void br_fdb_cleanup(struct work_struct *work)
>  	mod_delayed_work(system_long_wq, &br->gc_work, work_delay);
>  }
>  
> -/* Completely flush all dynamic entries in forwarding database.*/
> -void br_fdb_flush(struct net_bridge *br)
> +static bool __fdb_flush_matches(const struct net_bridge *br,
> +				const struct net_bridge_fdb_entry *f,
> +				const struct net_bridge_fdb_flush_desc *desc)
> +{
> +	const struct net_bridge_port *dst = READ_ONCE(f->dst);
> +	int port_ifidx, br_ifidx = br->dev->ifindex;
> +
> +	port_ifidx = dst ? dst->dev->ifindex : 0;
> +
> +	return (!desc->vlan_id || desc->vlan_id == f->key.vlan_id) &&
> +	       (!desc->port_ifindex ||
> +		(desc->port_ifindex == port_ifidx ||
> +		 (!dst && desc->port_ifindex == br_ifidx))) &&
> +	       (!desc->flags_mask ||
> +		((f->flags & desc->flags_mask) == desc->flags));

I find this easier to read:

port_ifidx = dst ? dst->dev->ifindex : br_ifidx;

if (desc->vlan_id && desc->vlan_id != f->key.vlan_id)
	return false;
if (desc->port_ifindex && desc->port_ifindex != port_ifidx)
	return false;
if (desc->flags_mask && (f->flags & desc->flags_mask) != desc->flags)
	return false;

return true;

> +}
> +
> +/* Flush forwarding database entries matching the description */
> +void br_fdb_flush(struct net_bridge *br,
> +		  const struct net_bridge_fdb_flush_desc *desc)
>  {
>  	struct net_bridge_fdb_entry *f;
> -	struct hlist_node *tmp;
>  
> -	spin_lock_bh(&br->hash_lock);
> -	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
> -		if (!test_bit(BR_FDB_STATIC, &f->flags))
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
> +		if (!__fdb_flush_matches(br, f, desc))
> +			continue;
> +
> +		spin_lock_bh(&br->hash_lock);
> +		if (!hlist_unhashed(&f->fdb_node))
>  			fdb_delete(br, f, true);
> +		spin_unlock_bh(&br->hash_lock);
>  	}
> -	spin_unlock_bh(&br->hash_lock);
> +	rcu_read_unlock();
>  }
>  
>  /* Flush all entries referring to a specific port.
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index fe2211d4c0c7..6e6dce6880c9 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1366,8 +1366,13 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
>  		br_recalculate_fwd_mask(br);
>  	}
>  
> -	if (data[IFLA_BR_FDB_FLUSH])
> -		br_fdb_flush(br);
> +	if (data[IFLA_BR_FDB_FLUSH]) {
> +		struct net_bridge_fdb_flush_desc desc = {
> +			.flags_mask = BR_FDB_STATIC
> +		};
> +
> +		br_fdb_flush(br, &desc);

I wanted to ask why you are not doing the same for IFLA_BRPORT_FLUSH,
but then I read the implementation of br_fdb_delete_by_port() and
remembered the comment in the cover letter regarding fdb_delete vs
fdb_delete_local. Probably best to note it in the commit message

> +	}
>  
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	if (data[IFLA_BR_MCAST_ROUTER]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 6e62af2e07e9..e6930e9ee69d 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -274,6 +274,13 @@ struct net_bridge_fdb_entry {
>  	struct rcu_head			rcu;
>  };
>  
> +struct net_bridge_fdb_flush_desc {
> +	unsigned long			flags;
> +	unsigned long			flags_mask;
> +	int				port_ifindex;
> +	u16				vlan_id;
> +};
> +
>  #define MDB_PG_FLAGS_PERMANENT	BIT(0)
>  #define MDB_PG_FLAGS_OFFLOAD	BIT(1)
>  #define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
> @@ -759,7 +766,8 @@ int br_fdb_init(void);
>  void br_fdb_fini(void);
>  int br_fdb_hash_init(struct net_bridge *br);
>  void br_fdb_hash_fini(struct net_bridge *br);
> -void br_fdb_flush(struct net_bridge *br);
> +void br_fdb_flush(struct net_bridge *br,
> +		  const struct net_bridge_fdb_flush_desc *desc);
>  void br_fdb_find_delete_local(struct net_bridge *br,
>  			      const struct net_bridge_port *p,
>  			      const unsigned char *addr, u16 vid);
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 3f7ca88c2aa3..612e367fff20 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -344,7 +344,11 @@ static DEVICE_ATTR_RW(group_addr);
>  static int set_flush(struct net_bridge *br, unsigned long val,
>  		     struct netlink_ext_ack *extack)
>  {
> -	br_fdb_flush(br);
> +	struct net_bridge_fdb_flush_desc desc = {
> +		.flags_mask = BR_FDB_STATIC
> +	};
> +
> +	br_fdb_flush(br, &desc);
>  	return 0;
>  }
>  
> -- 
> 2.35.1
>
Nikolay Aleksandrov April 11, 2022, 8:54 a.m. UTC | #2 
On 11/04/2022 11:20, Ido Schimmel wrote:
> On Sat, Apr 09, 2022 at 01:58:53PM +0300, Nikolay Aleksandrov wrote:
>> Add the ability to specify exactly which fdbs to be flushed. They are
>> described by a new structure - net_bridge_fdb_flush_desc. Currently it
>> can match on port/bridge ifindex, vlan id and fdb flags. It is used to
>> describe the existing dynamic fdb flush operation.
>>
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>>  net/bridge/br_fdb.c      | 36 +++++++++++++++++++++++++++++-------
>>  net/bridge/br_netlink.c  |  9 +++++++--
>>  net/bridge/br_private.h  | 10 +++++++++-
>>  net/bridge/br_sysfs_br.c |  6 +++++-
>>  4 files changed, 50 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index 6ccda68bd473..4b0bf88c4121 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -558,18 +558,40 @@ void br_fdb_cleanup(struct work_struct *work)
>>  	mod_delayed_work(system_long_wq, &br->gc_work, work_delay);
>>  }
>>  
>> -/* Completely flush all dynamic entries in forwarding database.*/
>> -void br_fdb_flush(struct net_bridge *br)
>> +static bool __fdb_flush_matches(const struct net_bridge *br,
>> +				const struct net_bridge_fdb_entry *f,
>> +				const struct net_bridge_fdb_flush_desc *desc)
>> +{
>> +	const struct net_bridge_port *dst = READ_ONCE(f->dst);
>> +	int port_ifidx, br_ifidx = br->dev->ifindex;
>> +
>> +	port_ifidx = dst ? dst->dev->ifindex : 0;
>> +
>> +	return (!desc->vlan_id || desc->vlan_id == f->key.vlan_id) &&
>> +	       (!desc->port_ifindex ||
>> +		(desc->port_ifindex == port_ifidx ||
>> +		 (!dst && desc->port_ifindex == br_ifidx))) &&
>> +	       (!desc->flags_mask ||
>> +		((f->flags & desc->flags_mask) == desc->flags));
> 
> I find this easier to read:
> 
> port_ifidx = dst ? dst->dev->ifindex : br_ifidx;
> 
> if (desc->vlan_id && desc->vlan_id != f->key.vlan_id)
> 	return false;
> if (desc->port_ifindex && desc->port_ifindex != port_ifidx)
> 	return false;
> if (desc->flags_mask && (f->flags & desc->flags_mask) != desc->flags)
> 	return false;
> 
> return true;
> 

Ack

>> +}
>> +
>> +/* Flush forwarding database entries matching the description */
>> +void br_fdb_flush(struct net_bridge *br,
>> +		  const struct net_bridge_fdb_flush_desc *desc)
>>  {
>>  	struct net_bridge_fdb_entry *f;
>> -	struct hlist_node *tmp;
>>  
>> -	spin_lock_bh(&br->hash_lock);
>> -	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
>> -		if (!test_bit(BR_FDB_STATIC, &f->flags))
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
>> +		if (!__fdb_flush_matches(br, f, desc))
>> +			continue;
>> +
>> +		spin_lock_bh(&br->hash_lock);
>> +		if (!hlist_unhashed(&f->fdb_node))
>>  			fdb_delete(br, f, true);
>> +		spin_unlock_bh(&br->hash_lock);
>>  	}
>> -	spin_unlock_bh(&br->hash_lock);
>> +	rcu_read_unlock();
>>  }
>>  
>>  /* Flush all entries referring to a specific port.
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index fe2211d4c0c7..6e6dce6880c9 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -1366,8 +1366,13 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
>>  		br_recalculate_fwd_mask(br);
>>  	}
>>  
>> -	if (data[IFLA_BR_FDB_FLUSH])
>> -		br_fdb_flush(br);
>> +	if (data[IFLA_BR_FDB_FLUSH]) {
>> +		struct net_bridge_fdb_flush_desc desc = {
>> +			.flags_mask = BR_FDB_STATIC
>> +		};
>> +
>> +		br_fdb_flush(br, &desc);
> 
> I wanted to ask why you are not doing the same for IFLA_BRPORT_FLUSH,
> but then I read the implementation of br_fdb_delete_by_port() and
> remembered the comment in the cover letter regarding fdb_delete vs
> fdb_delete_local. Probably best to note it in the commit message
> 

Yup, good point. Will add.

>> +	}
>>  
>>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>>  	if (data[IFLA_BR_MCAST_ROUTER]) {
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 6e62af2e07e9..e6930e9ee69d 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -274,6 +274,13 @@ struct net_bridge_fdb_entry {
>>  	struct rcu_head			rcu;
>>  };
>>  
>> +struct net_bridge_fdb_flush_desc {
>> +	unsigned long			flags;
>> +	unsigned long			flags_mask;
>> +	int				port_ifindex;
>> +	u16				vlan_id;
>> +};
>> +
>>  #define MDB_PG_FLAGS_PERMANENT	BIT(0)
>>  #define MDB_PG_FLAGS_OFFLOAD	BIT(1)
>>  #define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
>> @@ -759,7 +766,8 @@ int br_fdb_init(void);
>>  void br_fdb_fini(void);
>>  int br_fdb_hash_init(struct net_bridge *br);
>>  void br_fdb_hash_fini(struct net_bridge *br);
>> -void br_fdb_flush(struct net_bridge *br);
>> +void br_fdb_flush(struct net_bridge *br,
>> +		  const struct net_bridge_fdb_flush_desc *desc);
>>  void br_fdb_find_delete_local(struct net_bridge *br,
>>  			      const struct net_bridge_port *p,
>>  			      const unsigned char *addr, u16 vid);
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 3f7ca88c2aa3..612e367fff20 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -344,7 +344,11 @@ static DEVICE_ATTR_RW(group_addr);
>>  static int set_flush(struct net_bridge *br, unsigned long val,
>>  		     struct netlink_ext_ack *extack)
>>  {
>> -	br_fdb_flush(br);
>> +	struct net_bridge_fdb_flush_desc desc = {
>> +		.flags_mask = BR_FDB_STATIC
>> +	};
>> +
>> +	br_fdb_flush(br, &desc);
>>  	return 0;
>>  }
>>  
>> -- 
>> 2.35.1
>>
diff mbox series

Patch

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6ccda68bd473..4b0bf88c4121 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -558,18 +558,40 @@  void br_fdb_cleanup(struct work_struct *work)
 	mod_delayed_work(system_long_wq, &br->gc_work, work_delay);
 }
 
-/* Completely flush all dynamic entries in forwarding database.*/
-void br_fdb_flush(struct net_bridge *br)
+static bool __fdb_flush_matches(const struct net_bridge *br,
+				const struct net_bridge_fdb_entry *f,
+				const struct net_bridge_fdb_flush_desc *desc)
+{
+	const struct net_bridge_port *dst = READ_ONCE(f->dst);
+	int port_ifidx, br_ifidx = br->dev->ifindex;
+
+	port_ifidx = dst ? dst->dev->ifindex : 0;
+
+	return (!desc->vlan_id || desc->vlan_id == f->key.vlan_id) &&
+	       (!desc->port_ifindex ||
+		(desc->port_ifindex == port_ifidx ||
+		 (!dst && desc->port_ifindex == br_ifidx))) &&
+	       (!desc->flags_mask ||
+		((f->flags & desc->flags_mask) == desc->flags));
+}
+
+/* Flush forwarding database entries matching the description */
+void br_fdb_flush(struct net_bridge *br,
+		  const struct net_bridge_fdb_flush_desc *desc)
 {
 	struct net_bridge_fdb_entry *f;
-	struct hlist_node *tmp;
 
-	spin_lock_bh(&br->hash_lock);
-	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
-		if (!test_bit(BR_FDB_STATIC, &f->flags))
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
+		if (!__fdb_flush_matches(br, f, desc))
+			continue;
+
+		spin_lock_bh(&br->hash_lock);
+		if (!hlist_unhashed(&f->fdb_node))
 			fdb_delete(br, f, true);
+		spin_unlock_bh(&br->hash_lock);
 	}
-	spin_unlock_bh(&br->hash_lock);
+	rcu_read_unlock();
 }
 
 /* Flush all entries referring to a specific port.
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index fe2211d4c0c7..6e6dce6880c9 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1366,8 +1366,13 @@  static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
 		br_recalculate_fwd_mask(br);
 	}
 
-	if (data[IFLA_BR_FDB_FLUSH])
-		br_fdb_flush(br);
+	if (data[IFLA_BR_FDB_FLUSH]) {
+		struct net_bridge_fdb_flush_desc desc = {
+			.flags_mask = BR_FDB_STATIC
+		};
+
+		br_fdb_flush(br, &desc);
+	}
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	if (data[IFLA_BR_MCAST_ROUTER]) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6e62af2e07e9..e6930e9ee69d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -274,6 +274,13 @@  struct net_bridge_fdb_entry {
 	struct rcu_head			rcu;
 };
 
+struct net_bridge_fdb_flush_desc {
+	unsigned long			flags;
+	unsigned long			flags_mask;
+	int				port_ifindex;
+	u16				vlan_id;
+};
+
 #define MDB_PG_FLAGS_PERMANENT	BIT(0)
 #define MDB_PG_FLAGS_OFFLOAD	BIT(1)
 #define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
@@ -759,7 +766,8 @@  int br_fdb_init(void);
 void br_fdb_fini(void);
 int br_fdb_hash_init(struct net_bridge *br);
 void br_fdb_hash_fini(struct net_bridge *br);
-void br_fdb_flush(struct net_bridge *br);
+void br_fdb_flush(struct net_bridge *br,
+		  const struct net_bridge_fdb_flush_desc *desc);
 void br_fdb_find_delete_local(struct net_bridge *br,
 			      const struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid);
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 3f7ca88c2aa3..612e367fff20 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -344,7 +344,11 @@  static DEVICE_ATTR_RW(group_addr);
 static int set_flush(struct net_bridge *br, unsigned long val,
 		     struct netlink_ext_ack *extack)
 {
-	br_fdb_flush(br);
+	struct net_bridge_fdb_flush_desc desc = {
+		.flags_mask = BR_FDB_STATIC
+	};
+
+	br_fdb_flush(br, &desc);
 	return 0;
 }