diff mbox series

[v6,net-next,05/14] bridge: Add filling forward path from port to port

Message ID 20250209111034.241571-6-ericwouds@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series bridge-fastpath and related improvements | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 48 this patch: 48
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 6 this patch: 6
netdev/checkpatch warning WARNING: line length of 88 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0

Commit Message

Eric Woudstra Feb. 9, 2025, 11:10 a.m. UTC 
If a port is passed as argument instead of the master, then:

At br_fill_forward_path(): find the master and use it to fill the
forward path.

At br_vlan_fill_forward_path_pvid(): lookup vlan group from port
instead.

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 net/bridge/br_device.c  | 19 ++++++++++++++-----
 net/bridge/br_private.h |  2 ++
 net/bridge/br_vlan.c    |  6 +++++-
 3 files changed, 21 insertions(+), 6 deletions(-)

Comments

Vladimir Oltean Feb. 11, 2025, 1:28 p.m. UTC | #1 
On Sun, Feb 09, 2025 at 12:10:25PM +0100, Eric Woudstra wrote:
> @@ -1453,7 +1454,10 @@ void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
>  	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
>  		return;
>  
> -	vg = br_vlan_group(br);
> +	if (p)
> +		vg = nbp_vlan_group(p);
> +	else
> +		vg = br_vlan_group(br);
>  
>  	if (idx >= 0 &&
>  	    ctx->vlan[idx].proto == br->vlan_proto) {

I think the original usage of br_vlan_group() here was incorrect, and so
is the new usage of nbp_vlan_group(). They should be br_vlan_group_rcu()
and nbp_vlan_group_rcu().

The lockdep annotation is important, otherwise I get this with CONFIG_PROVE_LOCKING=y:
[ 1140.931869] =============================
[ 1140.935996] WARNING: suspicious RCU usage
[ 1140.940094] 6.14.0-rc1-00224-gda8926a49ba1-dirty #2666 Not tainted
[ 1140.946371] -----------------------------
[ 1140.950520] net/bridge/br_private.h:1604 suspicious rcu_dereference_protected() usage!
[ 1140.958622]
[ 1140.958622] other info that might help us debug this:
[ 1140.958622]
[ 1140.966752]
[ 1140.966752] rcu_scheduler_active = 2, debug_locks = 1
[ 1140.973435] 2 locks held by swapper/0/0:
[ 1140.977521]  #0: ffffd9f646c333b0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire+0x4/0x40
[ 1140.986404]  #1: ffffd9f646c333b0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire+0x4/0x48
[ 1140.995170]
[ 1140.995170] stack backtrace:
[ 1140.999636] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.14.0-rc1-00224-gda8926a49ba1-dirty #2666
[ 1140.999650] Hardware name: LS1028A RDB Board (DT)
[ 1140.999656] Call trace:
[ 1140.999660]  show_stack+0x24/0x38 (C)
[ 1140.999683]  dump_stack_lvl+0x40/0xa0
[ 1140.999698]  dump_stack+0x18/0x24
[ 1140.999711]  lockdep_rcu_suspicious+0x174/0x218
[ 1140.999723]  br_vlan_fill_forward_path_pvid+0x90/0x150
[ 1140.999735]  br_fill_forward_path+0x54/0x1b0
[ 1140.999751]  dev_fill_bridge_path+0x9c/0x188
[ 1140.999766]  nft_dev_fill_bridge_path+0x2ac/0x418
[ 1140.999785]  nft_flow_offload_bridge_init+0x188/0x1c8
[ 1140.999801]  nft_flow_offload_eval+0x18c/0x300
[ 1140.999816]  nft_do_chain+0x1c8/0x538
[ 1140.999831]  nft_do_chain_bridge+0x15c/0x210
[ 1140.999846]  nf_hook_slow+0x80/0x130
[ 1140.999862]  NF_HOOK+0xd8/0x1d0
[ 1140.999871]  __br_forward+0x138/0x1a0
[ 1140.999880]  br_forward+0xd8/0x160
[ 1140.999889]  br_handle_frame_finish+0x3bc/0x5a8
[ 1140.999900]  br_nf_pre_routing_finish+0x538/0x608
[ 1140.999917]  NF_HOOK+0x254/0x298
[ 1140.999933]  br_nf_pre_routing+0x3e8/0x428
[ 1140.999949]  br_handle_frame+0x264/0x490
[ 1140.999959]  __netif_receive_skb_core+0x13c/0x1128
[ 1140.999975]  __netif_receive_skb_list_core+0xd4/0x1e8
[ 1140.999989]  netif_receive_skb_list_internal+0x224/0x338
[ 1141.000000]  napi_complete_done+0xb4/0x1d8
[ 1141.000012]  gro_cell_poll+0x94/0xb8
[ 1141.000025]  __napi_poll+0x58/0x258
[ 1141.000040]  net_rx_action+0x1f4/0x3e0
[ 1141.000055]  handle_softirqs+0x184/0x458
[ 1141.000070]  __do_softirq+0x20/0x2c
[ 1141.000079]  ____do_softirq+0x1c/0x30
[ 1141.000095]  call_on_irq_stack+0x24/0x58
[ 1141.000111]  do_softirq_own_stack+0x28/0x40
[ 1141.000127]  __irq_exit_rcu+0xd4/0x1a0
[ 1141.000139]  irq_exit_rcu+0x1c/0x40
[ 1141.000152]  el1_interrupt+0x8c/0xc0
[ 1141.000170]  el1h_64_irq_handler+0x18/0x28
[ 1141.000186]  el1h_64_irq+0x6c/0x70
[ 1141.000195]  arch_local_irq_enable+0x8/0x10 (P)
[ 1141.000213]  cpuidle_enter+0x44/0x68
[ 1141.000228]  do_idle+0x1e8/0x280
[ 1141.000238]  cpu_startup_entry+0x40/0x50
[ 1141.000248]  rest_init+0x1c4/0x1d0
[ 1141.000260]  start_kernel+0x324/0x3e8
[ 1141.000272]  __primary_switched+0x88/0x98
[ 1141.197568] ------------[ cut here ]------------
Eric Woudstra Feb. 11, 2025, 2:43 p.m. UTC | #2 
On 2/11/25 2:28 PM, Vladimir Oltean wrote:
> On Sun, Feb 09, 2025 at 12:10:25PM +0100, Eric Woudstra wrote:
>> @@ -1453,7 +1454,10 @@ void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
>>  	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
>>  		return;
>>  
>> -	vg = br_vlan_group(br);
>> +	if (p)
>> +		vg = nbp_vlan_group(p);
>> +	else
>> +		vg = br_vlan_group(br);
>>  
>>  	if (idx >= 0 &&
>>  	    ctx->vlan[idx].proto == br->vlan_proto) {
> 
> I think the original usage of br_vlan_group() here was incorrect, and so
> is the new usage of nbp_vlan_group(). They should be br_vlan_group_rcu()
> and nbp_vlan_group_rcu().
> 
> The lockdep annotation is important, otherwise I get this with CONFIG_PROVE_LOCKING=y:
> [ 1140.931869] =============================
> [ 1140.935996] WARNING: suspicious RCU usage
> [ 1140.940094] 6.14.0-rc1-00224-gda8926a49ba1-dirty #2666 Not tainted
> [ 1140.946371] -----------------------------
> [ 1140.950520] net/bridge/br_private.h:1604 suspicious rcu_dereference_protected() usage!
> [ 1140.958622]
> [ 1140.958622] other info that might help us debug this:
> [ 1140.958622]
> [ 1140.966752]
> [ 1140.966752] rcu_scheduler_active = 2, debug_locks = 1
> [ 1140.973435] 2 locks held by swapper/0/0:
> [ 1140.977521]  #0: ffffd9f646c333b0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire+0x4/0x40
> [ 1140.986404]  #1: ffffd9f646c333b0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire+0x4/0x48
> [ 1140.995170]
> [ 1140.995170] stack backtrace:
> [ 1140.999636] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.14.0-rc1-00224-gda8926a49ba1-dirty #2666
> [ 1140.999650] Hardware name: LS1028A RDB Board (DT)
> [ 1140.999656] Call trace:
> [ 1140.999660]  show_stack+0x24/0x38 (C)
> [ 1140.999683]  dump_stack_lvl+0x40/0xa0
> [ 1140.999698]  dump_stack+0x18/0x24
> [ 1140.999711]  lockdep_rcu_suspicious+0x174/0x218
> [ 1140.999723]  br_vlan_fill_forward_path_pvid+0x90/0x150
> [ 1140.999735]  br_fill_forward_path+0x54/0x1b0
> [ 1140.999751]  dev_fill_bridge_path+0x9c/0x188
> [ 1140.999766]  nft_dev_fill_bridge_path+0x2ac/0x418
> [ 1140.999785]  nft_flow_offload_bridge_init+0x188/0x1c8
> [ 1140.999801]  nft_flow_offload_eval+0x18c/0x300
> [ 1140.999816]  nft_do_chain+0x1c8/0x538
> [ 1140.999831]  nft_do_chain_bridge+0x15c/0x210
> [ 1140.999846]  nf_hook_slow+0x80/0x130
> [ 1140.999862]  NF_HOOK+0xd8/0x1d0
> [ 1140.999871]  __br_forward+0x138/0x1a0
> [ 1140.999880]  br_forward+0xd8/0x160
> [ 1140.999889]  br_handle_frame_finish+0x3bc/0x5a8
> [ 1140.999900]  br_nf_pre_routing_finish+0x538/0x608
> [ 1140.999917]  NF_HOOK+0x254/0x298
> [ 1140.999933]  br_nf_pre_routing+0x3e8/0x428
> [ 1140.999949]  br_handle_frame+0x264/0x490
> [ 1140.999959]  __netif_receive_skb_core+0x13c/0x1128
> [ 1140.999975]  __netif_receive_skb_list_core+0xd4/0x1e8
> [ 1140.999989]  netif_receive_skb_list_internal+0x224/0x338
> [ 1141.000000]  napi_complete_done+0xb4/0x1d8
> [ 1141.000012]  gro_cell_poll+0x94/0xb8
> [ 1141.000025]  __napi_poll+0x58/0x258
> [ 1141.000040]  net_rx_action+0x1f4/0x3e0
> [ 1141.000055]  handle_softirqs+0x184/0x458
> [ 1141.000070]  __do_softirq+0x20/0x2c
> [ 1141.000079]  ____do_softirq+0x1c/0x30
> [ 1141.000095]  call_on_irq_stack+0x24/0x58
> [ 1141.000111]  do_softirq_own_stack+0x28/0x40
> [ 1141.000127]  __irq_exit_rcu+0xd4/0x1a0
> [ 1141.000139]  irq_exit_rcu+0x1c/0x40
> [ 1141.000152]  el1_interrupt+0x8c/0xc0
> [ 1141.000170]  el1h_64_irq_handler+0x18/0x28
> [ 1141.000186]  el1h_64_irq+0x6c/0x70
> [ 1141.000195]  arch_local_irq_enable+0x8/0x10 (P)
> [ 1141.000213]  cpuidle_enter+0x44/0x68
> [ 1141.000228]  do_idle+0x1e8/0x280
> [ 1141.000238]  cpu_startup_entry+0x40/0x50
> [ 1141.000248]  rest_init+0x1c4/0x1d0
> [ 1141.000260]  start_kernel+0x324/0x3e8
> [ 1141.000272]  __primary_switched+0x88/0x98
> [ 1141.197568] ------------[ cut here ]------------

Thanks. I will correct both in v7.
Vladimir Oltean Feb. 11, 2025, 2:46 p.m. UTC | #3 
On Tue, Feb 11, 2025 at 03:43:22PM +0100, Eric Woudstra wrote:
> Thanks. I will correct both in v7.

Please wait a bit more for v7. I'm trying really hard to ask a pertinent
question on patch 11/14, which currently I don't understand very well
what it does and why it is needed.
Nikolay Aleksandrov Feb. 11, 2025, 4 p.m. UTC | #4 
On 2/11/25 15:28, Vladimir Oltean wrote:
> On Sun, Feb 09, 2025 at 12:10:25PM +0100, Eric Woudstra wrote:
>> @@ -1453,7 +1454,10 @@ void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
>>  	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
>>  		return;
>>  
>> -	vg = br_vlan_group(br);
>> +	if (p)
>> +		vg = nbp_vlan_group(p);
>> +	else
>> +		vg = br_vlan_group(br);
>>  
>>  	if (idx >= 0 &&
>>  	    ctx->vlan[idx].proto == br->vlan_proto) {
> 
> I think the original usage of br_vlan_group() here was incorrect, and so
> is the new usage of nbp_vlan_group(). They should be br_vlan_group_rcu()
> and nbp_vlan_group_rcu().
> 

Oops, right. Nice catch!
Eric Woudstra Feb. 11, 2025, 4:35 p.m. UTC | #5 
On 2/11/25 5:00 PM, Nikolay Aleksandrov wrote:
> On 2/11/25 15:28, Vladimir Oltean wrote:
>> On Sun, Feb 09, 2025 at 12:10:25PM +0100, Eric Woudstra wrote:
>>> @@ -1453,7 +1454,10 @@ void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
>>>  	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
>>>  		return;
>>>  
>>> -	vg = br_vlan_group(br);
>>> +	if (p)
>>> +		vg = nbp_vlan_group(p);
>>> +	else
>>> +		vg = br_vlan_group(br);
>>>  
>>>  	if (idx >= 0 &&
>>>  	    ctx->vlan[idx].proto == br->vlan_proto) {
>>
>> I think the original usage of br_vlan_group() here was incorrect, and so
>> is the new usage of nbp_vlan_group(). They should be br_vlan_group_rcu()
>> and nbp_vlan_group_rcu().
>>
> 
> Oops, right. Nice catch!
> 

Hi Nikolay,

I gather that I can include your Acked-by also in the corrected patch.
Nikolay Aleksandrov Feb. 11, 2025, 5:02 p.m. UTC | #6 
On 2/11/25 18:35, Eric Woudstra wrote:
> 
> 
> On 2/11/25 5:00 PM, Nikolay Aleksandrov wrote:
>> On 2/11/25 15:28, Vladimir Oltean wrote:
>>> On Sun, Feb 09, 2025 at 12:10:25PM +0100, Eric Woudstra wrote:
>>>> @@ -1453,7 +1454,10 @@ void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
>>>>  	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
>>>>  		return;
>>>>  
>>>> -	vg = br_vlan_group(br);
>>>> +	if (p)
>>>> +		vg = nbp_vlan_group(p);
>>>> +	else
>>>> +		vg = br_vlan_group(br);
>>>>  
>>>>  	if (idx >= 0 &&
>>>>  	    ctx->vlan[idx].proto == br->vlan_proto) {
>>>
>>> I think the original usage of br_vlan_group() here was incorrect, and so
>>> is the new usage of nbp_vlan_group(). They should be br_vlan_group_rcu()
>>> and nbp_vlan_group_rcu().
>>>
>>
>> Oops, right. Nice catch!
>>
> 
> Hi Nikolay,
> 
> I gather that I can include your Acked-by also in the corrected patch.
> 

Yes, thanks.
diff mbox series

Patch

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 0ab4613aa07a..c7646afc8b96 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -383,16 +383,25 @@  static int br_del_slave(struct net_device *dev, struct net_device *slave_dev)
 static int br_fill_forward_path(struct net_device_path_ctx *ctx,
 				struct net_device_path *path)
 {
+	struct net_bridge_port *src, *dst;
 	struct net_bridge_fdb_entry *f;
-	struct net_bridge_port *dst;
 	struct net_bridge *br;
 
-	if (netif_is_bridge_port(ctx->dev))
-		return -1;
+	if (netif_is_bridge_port(ctx->dev)) {
+		struct net_device *br_dev;
+
+		br_dev = netdev_master_upper_dev_get_rcu((struct net_device *)ctx->dev);
+		if (!br_dev)
+			return -1;
 
-	br = netdev_priv(ctx->dev);
+		src = br_port_get_rcu(ctx->dev);
+		br = netdev_priv(br_dev);
+	} else {
+		src = NULL;
+		br = netdev_priv(ctx->dev);
+	}
 
-	br_vlan_fill_forward_path_pvid(br, ctx, path);
+	br_vlan_fill_forward_path_pvid(br, src, ctx, path);
 
 	f = br_fdb_find_rcu(br, ctx->daddr, path->bridge.vlan_id);
 	if (!f)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1054b8a88edc..a0b950390a16 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1584,6 +1584,7 @@  bool br_vlan_can_enter_range(const struct net_bridge_vlan *v_curr,
 			     const struct net_bridge_vlan *range_end);
 
 void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
+				    struct net_bridge_port *p,
 				    struct net_device_path_ctx *ctx,
 				    struct net_device_path *path);
 int br_vlan_fill_forward_path_mode(struct net_bridge *br,
@@ -1753,6 +1754,7 @@  static inline int nbp_get_num_vlan_infos(struct net_bridge_port *p,
 }
 
 static inline void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
+						  struct net_bridge_port *p,
 						  struct net_device_path_ctx *ctx,
 						  struct net_device_path *path)
 {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index d9a69ec9affe..07dae3655c26 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1441,6 +1441,7 @@  int br_vlan_get_pvid_rcu(const struct net_device *dev, u16 *p_pvid)
 EXPORT_SYMBOL_GPL(br_vlan_get_pvid_rcu);
 
 void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
+				    struct net_bridge_port *p,
 				    struct net_device_path_ctx *ctx,
 				    struct net_device_path *path)
 {
@@ -1453,7 +1454,10 @@  void br_vlan_fill_forward_path_pvid(struct net_bridge *br,
 	if (!br_opt_get(br, BROPT_VLAN_ENABLED))
 		return;
 
-	vg = br_vlan_group(br);
+	if (p)
+		vg = nbp_vlan_group(p);
+	else
+		vg = br_vlan_group(br);
 
 	if (idx >= 0 &&
 	    ctx->vlan[idx].proto == br->vlan_proto) {