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.
