diff mbox series

[net] vrf: revert "vrf: Remove unnecessary RCU-bh critical section"

Message ID 20240929061839.1175300-1-willemdebruijn.kernel@gmail.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [net] vrf: revert "vrf: Remove unnecessary RCU-bh critical section" | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: horms@kernel.org; 1 maintainers not CCed: horms@kernel.org
netdev/build_clang success Errors and warnings before: 9 this patch: 9
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 Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 12 this patch: 12
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Willem de Bruijn Sept. 29, 2024, 6:18 a.m. UTC 
From: Willem de Bruijn <willemb@google.com>

This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.

dev_queue_xmit_nit is expected to be called with BH disabled.
__dev_queue_xmit has the following:

        /* Disable soft irqs for various locks below. Also
         * stops preemption for RCU.
         */
        rcu_read_lock_bh();

VRF must follow this invariant. The referenced commit removed this
protection. Which triggered a lockdep warning:

	================================
	WARNING: inconsistent lock state
	6.11.0 #1 Tainted: G        W
	--------------------------------
	inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
	btserver/134819 [HC0[0]:SC0[0]:HE1:SE1] takes:
	ffff8882da30c118 (rlock-AF_PACKET){+.?.}-{2:2}, at: tpacket_rcv+0x863/0x3b30
	{IN-SOFTIRQ-W} state was registered at:
	  lock_acquire+0x19a/0x4f0
	  _raw_spin_lock+0x27/0x40
	  packet_rcv+0xa33/0x1320
	  __netif_receive_skb_core.constprop.0+0xcb0/0x3a90
	  __netif_receive_skb_list_core+0x2c9/0x890
	  netif_receive_skb_list_internal+0x610/0xcc0
          [...]

	other info that might help us debug this:
	 Possible unsafe locking scenario:

	       CPU0
	       ----
	  lock(rlock-AF_PACKET);
	  <Interrupt>
	    lock(rlock-AF_PACKET);

	 *** DEADLOCK ***

	Call Trace:
	 <TASK>
	 dump_stack_lvl+0x73/0xa0
	 mark_lock+0x102e/0x16b0
	 __lock_acquire+0x9ae/0x6170
	 lock_acquire+0x19a/0x4f0
	 _raw_spin_lock+0x27/0x40
	 tpacket_rcv+0x863/0x3b30
	 dev_queue_xmit_nit+0x709/0xa40
	 vrf_finish_direct+0x26e/0x340 [vrf]
	 vrf_l3_out+0x5f4/0xe80 [vrf]
	 __ip_local_out+0x51e/0x7a0
          [...]

Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
Link: https://lore.kernel.org/netdev/20240925185216.1990381-1-greearb@candelatech.com/
Reported-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
Cc: stable@vger.kernel.org
---
 drivers/net/vrf.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ido Schimmel Sept. 29, 2024, 9:11 a.m. UTC | #1 
On Sun, Sep 29, 2024 at 02:18:20AM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
> 
> dev_queue_xmit_nit is expected to be called with BH disabled.
> __dev_queue_xmit has the following:
> 
>         /* Disable soft irqs for various locks below. Also
>          * stops preemption for RCU.
>          */
>         rcu_read_lock_bh();
> 
> VRF must follow this invariant. The referenced commit removed this
> protection. Which triggered a lockdep warning:

[...]

> 
> Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
> Link: https://lore.kernel.org/netdev/20240925185216.1990381-1-greearb@candelatech.com/
> Reported-by: Ben Greear <greearb@candelatech.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>

Thanks Willem!

The reason my script from 504fc6f4f7f6 did not trigger the problem is
that it was pinging the address inside the VRF, so vrf_finish_direct()
was only called from the Rx path.

If you ping the address outside of the VRF:

ping -I vrf1 -i 0.1 -c 10 -q 192.0.2.1

Then vrf_finish_direct() is called from process context and the lockdep
warning is triggered. Tested that it does not trigger after applying the
revert.
Willem de Bruijn Sept. 29, 2024, 10:21 a.m. UTC | #2 
Ido Schimmel wrote:
> On Sun, Sep 29, 2024 at 02:18:20AM -0400, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> > 
> > This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
> > 
> > dev_queue_xmit_nit is expected to be called with BH disabled.
> > __dev_queue_xmit has the following:
> > 
> >         /* Disable soft irqs for various locks below. Also
> >          * stops preemption for RCU.
> >          */
> >         rcu_read_lock_bh();
> > 
> > VRF must follow this invariant. The referenced commit removed this
> > protection. Which triggered a lockdep warning:
> 
> [...]
> 
> > 
> > Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
> > Link: https://lore.kernel.org/netdev/20240925185216.1990381-1-greearb@candelatech.com/
> > Reported-by: Ben Greear <greearb@candelatech.com>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Tested-by: Ido Schimmel <idosch@nvidia.com>
> 
> Thanks Willem!

Thanks for testing immediately and sharing the below context, Ido!

> The reason my script from 504fc6f4f7f6 did not trigger the problem is
> that it was pinging the address inside the VRF, so vrf_finish_direct()
> was only called from the Rx path.
> 
> If you ping the address outside of the VRF:
> 
> ping -I vrf1 -i 0.1 -c 10 -q 192.0.2.1
> 
> Then vrf_finish_direct() is called from process context and the lockdep
> warning is triggered. Tested that it does not trigger after applying the
> revert.
diff mbox series

Patch

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 4d8ccaf9a2b4..4087f72f0d2b 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -608,7 +608,9 @@  static void vrf_finish_direct(struct sk_buff *skb)
 		eth_zero_addr(eth->h_dest);
 		eth->h_proto = skb->protocol;
 
+		rcu_read_lock_bh();
 		dev_queue_xmit_nit(skb, vrf_dev);
+		rcu_read_unlock_bh();
 
 		skb_pull(skb, ETH_HLEN);
 	}