* [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
@ 2024-09-25 18:52 greearb
2024-09-26 8:24 ` Greg KH
2024-09-26 9:03 ` Willem de Bruijn
0 siblings, 2 replies; 7+ messages in thread
From: greearb @ 2024-09-25 18:52 UTC (permalink / raw)
To: netdev; +Cc: stable, Ben Greear
From: Ben Greear <greearb@candelatech.com>
This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
dev_queue_xmit_nit needs to run with bh locking, otherwise
it conflicts with packets coming in from a nic in softirq
context and packets being transmitted from user context.
================================
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
napi_complete_done+0x1c0/0x7c0
igb_poll+0x1dbb/0x57e0 [igb]
__napi_poll.constprop.0+0x99/0x430
net_rx_action+0x8e7/0xe10
handle_softirqs+0x1b7/0x800
__irq_exit_rcu+0x91/0xc0
irq_exit_rcu+0x5/0x10
[snip]
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
[snip]
Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
Signed-off-by: Ben Greear <greearb@candelatech.com>
---
v2: Edit patch description.
drivers/net/vrf.c | 2 ++
net/core/dev.c | 1 +
2 files changed, 3 insertions(+)
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);
}
diff --git a/net/core/dev.c b/net/core/dev.c
index cd479f5f22f6..566e69a38eed 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
/*
* Support routine. Sends outgoing frames to any network
* taps currently in use.
+ * BH must be disabled before calling this.
*/
void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
--
2.42.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
2024-09-25 18:52 [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section" greearb
@ 2024-09-26 8:24 ` Greg KH
2024-09-26 9:03 ` Willem de Bruijn
1 sibling, 0 replies; 7+ messages in thread
From: Greg KH @ 2024-09-26 8:24 UTC (permalink / raw)
To: greearb; +Cc: netdev, stable
On Wed, Sep 25, 2024 at 11:52:16AM -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
>
> dev_queue_xmit_nit needs to run with bh locking, otherwise
> it conflicts with packets coming in from a nic in softirq
> context and packets being transmitted from user context.
>
> ================================
> 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
> napi_complete_done+0x1c0/0x7c0
> igb_poll+0x1dbb/0x57e0 [igb]
> __napi_poll.constprop.0+0x99/0x430
> net_rx_action+0x8e7/0xe10
> handle_softirqs+0x1b7/0x800
> __irq_exit_rcu+0x91/0xc0
> irq_exit_rcu+0x5/0x10
> [snip]
>
> 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
> [snip]
>
> Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
> Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>
> v2: Edit patch description.
>
> drivers/net/vrf.c | 2 ++
> net/core/dev.c | 1 +
> 2 files changed, 3 insertions(+)
>
> 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);
> }
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cd479f5f22f6..566e69a38eed 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
> /*
> * Support routine. Sends outgoing frames to any network
> * taps currently in use.
> + * BH must be disabled before calling this.
> */
>
> void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
> --
> 2.42.0
>
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
2024-09-25 18:52 [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section" greearb
2024-09-26 8:24 ` Greg KH
@ 2024-09-26 9:03 ` Willem de Bruijn
2024-09-26 18:19 ` Ben Greear
1 sibling, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2024-09-26 9:03 UTC (permalink / raw)
To: greearb, netdev; +Cc: stable, Ben Greear
greearb@ wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
>
> dev_queue_xmit_nit needs to run with bh locking, otherwise
> it conflicts with packets coming in from a nic in softirq
> context and packets being transmitted from user context.
>
> ================================
> 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
> napi_complete_done+0x1c0/0x7c0
> igb_poll+0x1dbb/0x57e0 [igb]
> __napi_poll.constprop.0+0x99/0x430
> net_rx_action+0x8e7/0xe10
> handle_softirqs+0x1b7/0x800
> __irq_exit_rcu+0x91/0xc0
> irq_exit_rcu+0x5/0x10
> [snip]
>
> 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
> [snip]
>
> Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
> Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
Please Cc: all previous reviewers and folks who participated in the
discussion. I entirely missed this. No need to add as Cc tags, just
--cc in git send-email will do.
> ---
>
> v2: Edit patch description.
>
> drivers/net/vrf.c | 2 ++
> net/core/dev.c | 1 +
> 2 files changed, 3 insertions(+)
>
> 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);
> }
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cd479f5f22f6..566e69a38eed 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
> /*
> * Support routine. Sends outgoing frames to any network
> * taps currently in use.
> + * BH must be disabled before calling this.
Unnecessary. Especially patches for stable should be minimal to
reduce the chance of conflicts.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
2024-09-26 9:03 ` Willem de Bruijn
@ 2024-09-26 18:19 ` Ben Greear
2024-09-27 6:19 ` Greg KH
2024-09-27 9:35 ` Willem de Bruijn
0 siblings, 2 replies; 7+ messages in thread
From: Ben Greear @ 2024-09-26 18:19 UTC (permalink / raw)
To: Willem de Bruijn, netdev; +Cc: stable
On 9/26/24 02:03, Willem de Bruijn wrote:
> greearb@ wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
>>
>> dev_queue_xmit_nit needs to run with bh locking, otherwise
>> it conflicts with packets coming in from a nic in softirq
>> context and packets being transmitted from user context.
>>
>> ================================
>> 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
>> napi_complete_done+0x1c0/0x7c0
>> igb_poll+0x1dbb/0x57e0 [igb]
>> __napi_poll.constprop.0+0x99/0x430
>> net_rx_action+0x8e7/0xe10
>> handle_softirqs+0x1b7/0x800
>> __irq_exit_rcu+0x91/0xc0
>> irq_exit_rcu+0x5/0x10
>> [snip]
>>
>> 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
>> [snip]
>>
>> Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
>> Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>
> Please Cc: all previous reviewers and folks who participated in the
> discussion. I entirely missed this. No need to add as Cc tags, just
> --cc in git send-email will do.
>
>> ---
>>
>> v2: Edit patch description.
>>
>> drivers/net/vrf.c | 2 ++
>> net/core/dev.c | 1 +
>> 2 files changed, 3 insertions(+)
>>
>> 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);
>> }
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index cd479f5f22f6..566e69a38eed 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
>> /*
>> * Support routine. Sends outgoing frames to any network
>> * taps currently in use.
>> + * BH must be disabled before calling this.
>
> Unnecessary. Especially patches for stable should be minimal to
> reduce the chance of conflicts.
I was asked to add this, and also asked to CC stable.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
2024-09-26 18:19 ` Ben Greear
@ 2024-09-27 6:19 ` Greg KH
2024-09-27 9:35 ` Willem de Bruijn
1 sibling, 0 replies; 7+ messages in thread
From: Greg KH @ 2024-09-27 6:19 UTC (permalink / raw)
To: Ben Greear; +Cc: Willem de Bruijn, netdev, stable
On Thu, Sep 26, 2024 at 11:19:53AM -0700, Ben Greear wrote:
> On 9/26/24 02:03, Willem de Bruijn wrote:
> > greearb@ wrote:
> > > From: Ben Greear <greearb@candelatech.com>
> > >
> > > This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
> > >
> > > dev_queue_xmit_nit needs to run with bh locking, otherwise
> > > it conflicts with packets coming in from a nic in softirq
> > > context and packets being transmitted from user context.
> > >
> > > ================================
> > > 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
> > > napi_complete_done+0x1c0/0x7c0
> > > igb_poll+0x1dbb/0x57e0 [igb]
> > > __napi_poll.constprop.0+0x99/0x430
> > > net_rx_action+0x8e7/0xe10
> > > handle_softirqs+0x1b7/0x800
> > > __irq_exit_rcu+0x91/0xc0
> > > irq_exit_rcu+0x5/0x10
> > > [snip]
> > >
> > > 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
> > > [snip]
> > >
> > > Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
> > > Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
> > >
> > > Signed-off-by: Ben Greear <greearb@candelatech.com>
> >
> > Please Cc: all previous reviewers and folks who participated in the
> > discussion. I entirely missed this. No need to add as Cc tags, just
> > --cc in git send-email will do.
> >
> > > ---
> > >
> > > v2: Edit patch description.
> > >
> > > drivers/net/vrf.c | 2 ++
> > > net/core/dev.c | 1 +
> > > 2 files changed, 3 insertions(+)
> > >
> > > 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);
> > > }
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index cd479f5f22f6..566e69a38eed 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
> > > /*
> > > * Support routine. Sends outgoing frames to any network
> > > * taps currently in use.
> > > + * BH must be disabled before calling this.
> >
> > Unnecessary. Especially patches for stable should be minimal to
> > reduce the chance of conflicts.
>
> I was asked to add this, and also asked to CC stable.
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
2024-09-26 18:19 ` Ben Greear
2024-09-27 6:19 ` Greg KH
@ 2024-09-27 9:35 ` Willem de Bruijn
2024-09-27 13:39 ` Ben Greear
1 sibling, 1 reply; 7+ messages in thread
From: Willem de Bruijn @ 2024-09-27 9:35 UTC (permalink / raw)
To: Ben Greear, Willem de Bruijn, netdev; +Cc: stable
Ben Greear wrote:
> On 9/26/24 02:03, Willem de Bruijn wrote:
> > greearb@ wrote:
> >> From: Ben Greear <greearb@candelatech.com>
> >>
> >> This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
> >>
> >> dev_queue_xmit_nit needs to run with bh locking, otherwise
> >> it conflicts with packets coming in from a nic in softirq
> >> context and packets being transmitted from user context.
> >>
> >> ================================
> >> 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
> >> napi_complete_done+0x1c0/0x7c0
> >> igb_poll+0x1dbb/0x57e0 [igb]
> >> __napi_poll.constprop.0+0x99/0x430
> >> net_rx_action+0x8e7/0xe10
> >> handle_softirqs+0x1b7/0x800
> >> __irq_exit_rcu+0x91/0xc0
> >> irq_exit_rcu+0x5/0x10
> >> [snip]
> >>
> >> 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
> >> [snip]
> >>
> >> Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
> >> Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
> >>
> >> Signed-off-by: Ben Greear <greearb@candelatech.com>
> >
> > Please Cc: all previous reviewers and folks who participated in the
> > discussion. I entirely missed this. No need to add as Cc tags, just
> > --cc in git send-email will do.
> >
> >> ---
> >>
> >> v2: Edit patch description.
> >>
> >> drivers/net/vrf.c | 2 ++
> >> net/core/dev.c | 1 +
> >> 2 files changed, 3 insertions(+)
> >>
> >> 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);
> >> }
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index cd479f5f22f6..566e69a38eed 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
> >> /*
> >> * Support routine. Sends outgoing frames to any network
> >> * taps currently in use.
> >> + * BH must be disabled before calling this.
> >
> > Unnecessary. Especially patches for stable should be minimal to
> > reduce the chance of conflicts.
>
> I was asked to add this, and also asked to CC stable.
Conflicting feedback is annoying, but I disagree with the other
comment.
Not only does it potentially complicate backporting, it also is no
longer a strict revert. In which case it must not be labeled as such.
Easier is to keep the revert unmodified, and add the comment to the
commit message.
Most important is the Link to our earlier thread that explains the
history.
If the process appears byzantine, if you prefer I can also send it.
Thanks,
Willem
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section"
2024-09-27 9:35 ` Willem de Bruijn
@ 2024-09-27 13:39 ` Ben Greear
0 siblings, 0 replies; 7+ messages in thread
From: Ben Greear @ 2024-09-27 13:39 UTC (permalink / raw)
To: Willem de Bruijn, netdev
On 9/27/24 02:35, Willem de Bruijn wrote:
> Ben Greear wrote:
>> On 9/26/24 02:03, Willem de Bruijn wrote:
>>> greearb@ wrote:
>>>> From: Ben Greear <greearb@candelatech.com>
>>>>
>>>> This reverts commit 504fc6f4f7f681d2a03aa5f68aad549d90eab853.
>>>>
>>>> dev_queue_xmit_nit needs to run with bh locking, otherwise
>>>> it conflicts with packets coming in from a nic in softirq
>>>> context and packets being transmitted from user context.
>>>>
>>>> ================================
>>>> 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
>>>> napi_complete_done+0x1c0/0x7c0
>>>> igb_poll+0x1dbb/0x57e0 [igb]
>>>> __napi_poll.constprop.0+0x99/0x430
>>>> net_rx_action+0x8e7/0xe10
>>>> handle_softirqs+0x1b7/0x800
>>>> __irq_exit_rcu+0x91/0xc0
>>>> irq_exit_rcu+0x5/0x10
>>>> [snip]
>>>>
>>>> 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
>>>> [snip]
>>>>
>>>> Fixes: 504fc6f4f7f6 ("vrf: Remove unnecessary RCU-bh critical section")
>>>> Link: https://lore.kernel.org/netdev/05765015-f727-2f30-58da-2ad6fa7ea99f@candelatech.com/T/
>>>>
>>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>>
>>> Please Cc: all previous reviewers and folks who participated in the
>>> discussion. I entirely missed this. No need to add as Cc tags, just
>>> --cc in git send-email will do.
>>>
>>>> ---
>>>>
>>>> v2: Edit patch description.
>>>>
>>>> drivers/net/vrf.c | 2 ++
>>>> net/core/dev.c | 1 +
>>>> 2 files changed, 3 insertions(+)
>>>>
>>>> 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);
>>>> }
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index cd479f5f22f6..566e69a38eed 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -2285,6 +2285,7 @@ EXPORT_SYMBOL_GPL(dev_nit_active);
>>>> /*
>>>> * Support routine. Sends outgoing frames to any network
>>>> * taps currently in use.
>>>> + * BH must be disabled before calling this.
>>>
>>> Unnecessary. Especially patches for stable should be minimal to
>>> reduce the chance of conflicts.
>>
>> I was asked to add this, and also asked to CC stable.
>
> Conflicting feedback is annoying, but I disagree with the other
> comment.
>
> Not only does it potentially complicate backporting, it also is no
> longer a strict revert. In which case it must not be labeled as such.
>
> Easier is to keep the revert unmodified, and add the comment to the
> commit message.
>
> Most important is the Link to our earlier thread that explains the
> history.
>
> If the process appears byzantine, if you prefer I can also send it.
I would appreciate it if you can send it. As long as it functions,
I will be happy.
Thanks,
Ben
>
> Thanks,
>
> Willem
>
>
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-27 13:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-25 18:52 [PATCH v2] Revert "vrf: Remove unnecessary RCU-bh critical section" greearb
2024-09-26 8:24 ` Greg KH
2024-09-26 9:03 ` Willem de Bruijn
2024-09-26 18:19 ` Ben Greear
2024-09-27 6:19 ` Greg KH
2024-09-27 9:35 ` Willem de Bruijn
2024-09-27 13:39 ` Ben Greear
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).