* [PATCH] net/sched: Set the flushing flags to false to prevent an infinite loop
@ 2023-06-06 14:45 renmingshuai
2023-06-06 16:59 ` Pedro Tammela
0 siblings, 1 reply; 8+ messages in thread
From: renmingshuai @ 2023-06-06 14:45 UTC (permalink / raw)
To: netdev, linux-kernel, jhs, xiyou.wangcong, jiri, davem, edumazet,
kuba, pabeni
Cc: liaichun, caowangbao, yanan, liubo335
When a new chain is added by using tc, one soft lockup alarm will be
generated after delete the prio 0 filter of the chain. To reproduce
the problem, perform the following steps:
(1) tc qdisc add dev eth0 root handle 1: htb default 1
(2) tc chain add dev eth0
(3) tc filter del dev eth0 chain 0 parent 1: prio 0
(4) tc filter add dev eth0 chain 0 parent 1:
The refcnt of the chain added by step 2 is equal to 1. After step 3,
the flushing flag of the chain is set to true in the tcf_chain_flush()
called by tc_del_tfilter() because the prio is 0. In this case, if
we add a new filter to this chain, it will never succeed and try again
and again because the refresh flash is always true and refcnt is 1.
A soft lock alarm is generated 20 seconds later.
The stack is show as below:
Kernel panic - not syncing: softlockup: hung tasks
CPU: 2 PID: 3321861 Comm: tc Kdump: loaded Tainted: G
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
Call Trace:
<IRQ>
dump_stack+0x57/0x6e
panic+0x196/0x3ec
watchdog_timer_fn.cold+0x16/0x5c
__run_hrtimer+0x5e/0x190
__hrtimer_run_queues+0x8a/0xe0
hrtimer_interrupt+0x110/0x2c0
? irqtime_account_irq+0x49/0xf0
__sysvec_apic_timer_interrupt+0x5f/0xe0
asm_call_irq_on_stack+0x12/0x20
</IRQ>
sysvec_apic_timer_interrupt+0x72/0x80
asm_sysvec_apic_timer_interrupt+0x12/0x20
RIP: 0010:mutex_lock+0x24/0x70
RSP: 0018:ffffa836004ab9a8 EFLAGS: 00000246
RAX: 0000000000000000 RBX: ffff95bb02d76700 RCX: 0000000000000000
RDX: ffff95bb27462100 RSI: 0000000000000000 RDI: ffff95ba5b527000
RBP: ffff95ba5b527000 R08: 0000000000000001 R09: ffffa836004abbb8
R10: 000000000000000f R11: 0000000000000000 R12: 0000000000000000
R13: ffff95ba5b527000 R14: ffffa836004abbb8 R15: 0000000000000001
__tcf_chain_put+0x27/0x200
tc_new_tfilter+0x5e8/0x810
? tc_setup_cb_add+0x210/0x210
rtnetlink_rcv_msg+0x2e3/0x380
? rtnl_calcit.isra.0+0x120/0x120
netlink_rcv_skb+0x50/0x100
netlink_unicast+0x12d/0x1d0
netlink_sendmsg+0x286/0x490
sock_sendmsg+0x62/0x70
____sys_sendmsg+0x24c/0x2c0
? import_iovec+0x17/0x20
? sendmsg_copy_msghdr+0x80/0xa0
___sys_sendmsg+0x75/0xc0
? do_fault_around+0x118/0x160
? do_read_fault+0x68/0xf0
? __handle_mm_fault+0x3f9/0x6f0
__sys_sendmsg+0x59/0xa0
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x61/0xc6
RIP: 0033:0x7f96705b8247
RSP: 002b:00007ffe552e9dc8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f96705b8247
RDX: 0000000000000000 RSI: 00007ffe552e9e40 RDI: 0000000000000003
RBP: 0000000000000001 R08: 0000000000000001 R09: 0000558113f678b0
R10: 00007f967069ab00 R11: 0000000000000246 R12: 00000000647ea089
R13: 00007ffe552e9f30 R14: 0000000000000001 R15: 0000558113175f00
To avoid this case, set chain->flushing to be false if the chain->refcnt
is 1 after flushing the chain when prio is 0.
Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
Signed-off-by: Ren Mingshuai <renmingshuai@huawei.com>
---
net/sched/cls_api.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2621550bfddc..68be55d75831 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tfilter_notify_chain(net, skb, block, q, parent, n,
chain, RTM_DELTFILTER, extack);
tcf_chain_flush(chain, rtnl_held);
+ /* Set the flushing flags to false to prevent an infinite loop
+ * when a new filter is added.
+ */
+ mutex_lock(&chain->filter_chain_lock);
+ if (chain->refcnt == 1)
+ chain->flushing = false;
+ mutex_unlock(&chain->filter_chain_lock);
err = 0;
goto errout;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net/sched: Set the flushing flags to false to prevent an infinite loop
2023-06-06 14:45 [PATCH] net/sched: Set the flushing flags to false to prevent an infinite loop renmingshuai
@ 2023-06-06 16:59 ` Pedro Tammela
2023-06-07 4:19 ` renmingshuai
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Tammela @ 2023-06-06 16:59 UTC (permalink / raw)
To: renmingshuai, netdev, linux-kernel, jhs, xiyou.wangcong, jiri,
davem, edumazet, kuba, pabeni
Cc: liaichun, caowangbao, yanan, liubo335
On 06/06/2023 11:45, renmingshuai wrote:
> When a new chain is added by using tc, one soft lockup alarm will be
> generated after delete the prio 0 filter of the chain. To reproduce
> the problem, perform the following steps:
> (1) tc qdisc add dev eth0 root handle 1: htb default 1
> (2) tc chain add dev eth0
> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
> (4) tc filter add dev eth0 chain 0 parent 1:
This seems like it could be added to tdc or 3 and 4 must be run in parallel?
>
>
> The refcnt of the chain added by step 2 is equal to 1. After step 3,
> the flushing flag of the chain is set to true in the tcf_chain_flush()
> called by tc_del_tfilter() because the prio is 0. In this case, if
> we add a new filter to this chain, it will never succeed and try again
> and again because the refresh flash is always true and refcnt is 1.
> A soft lock alarm is generated 20 seconds later.
> The stack is show as below:
>
> Kernel panic - not syncing: softlockup: hung tasks
> CPU: 2 PID: 3321861 Comm: tc Kdump: loaded Tainted: G
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> Call Trace:
> <IRQ>
> dump_stack+0x57/0x6e
> panic+0x196/0x3ec
> watchdog_timer_fn.cold+0x16/0x5c
> __run_hrtimer+0x5e/0x190
> __hrtimer_run_queues+0x8a/0xe0
> hrtimer_interrupt+0x110/0x2c0
> ? irqtime_account_irq+0x49/0xf0
> __sysvec_apic_timer_interrupt+0x5f/0xe0
> asm_call_irq_on_stack+0x12/0x20
> </IRQ>
> sysvec_apic_timer_interrupt+0x72/0x80
> asm_sysvec_apic_timer_interrupt+0x12/0x20
> RIP: 0010:mutex_lock+0x24/0x70
> RSP: 0018:ffffa836004ab9a8 EFLAGS: 00000246
> RAX: 0000000000000000 RBX: ffff95bb02d76700 RCX: 0000000000000000
> RDX: ffff95bb27462100 RSI: 0000000000000000 RDI: ffff95ba5b527000
> RBP: ffff95ba5b527000 R08: 0000000000000001 R09: ffffa836004abbb8
> R10: 000000000000000f R11: 0000000000000000 R12: 0000000000000000
> R13: ffff95ba5b527000 R14: ffffa836004abbb8 R15: 0000000000000001
> __tcf_chain_put+0x27/0x200
> tc_new_tfilter+0x5e8/0x810
> ? tc_setup_cb_add+0x210/0x210
> rtnetlink_rcv_msg+0x2e3/0x380
> ? rtnl_calcit.isra.0+0x120/0x120
> netlink_rcv_skb+0x50/0x100
> netlink_unicast+0x12d/0x1d0
> netlink_sendmsg+0x286/0x490
> sock_sendmsg+0x62/0x70
> ____sys_sendmsg+0x24c/0x2c0
> ? import_iovec+0x17/0x20
> ? sendmsg_copy_msghdr+0x80/0xa0
> ___sys_sendmsg+0x75/0xc0
> ? do_fault_around+0x118/0x160
> ? do_read_fault+0x68/0xf0
> ? __handle_mm_fault+0x3f9/0x6f0
> __sys_sendmsg+0x59/0xa0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x61/0xc6
> RIP: 0033:0x7f96705b8247
> RSP: 002b:00007ffe552e9dc8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f96705b8247
> RDX: 0000000000000000 RSI: 00007ffe552e9e40 RDI: 0000000000000003
> RBP: 0000000000000001 R08: 0000000000000001 R09: 0000558113f678b0
> R10: 00007f967069ab00 R11: 0000000000000246 R12: 00000000647ea089
> R13: 00007ffe552e9f30 R14: 0000000000000001 R15: 0000558113175f00
>
> To avoid this case, set chain->flushing to be false if the chain->refcnt
> is 1 after flushing the chain when prio is 0.
>
> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
> Signed-off-by: Ren Mingshuai <renmingshuai@huawei.com>
> ---
> net/sched/cls_api.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 2621550bfddc..68be55d75831 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
> tfilter_notify_chain(net, skb, block, q, parent, n,
> chain, RTM_DELTFILTER, extack);
> tcf_chain_flush(chain, rtnl_held);
> + /* Set the flushing flags to false to prevent an infinite loop
> + * when a new filter is added.
> + */
> + mutex_lock(&chain->filter_chain_lock);
> + if (chain->refcnt == 1)
> + chain->flushing = false;
> + mutex_unlock(&chain->filter_chain_lock);
> err = 0;
> goto errout;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] net/sched: Set the flushing flags to false to prevent an infinite loop
2023-06-06 16:59 ` Pedro Tammela
@ 2023-06-07 4:19 ` renmingshuai
2023-06-07 15:02 ` Pedro Tammela
0 siblings, 1 reply; 8+ messages in thread
From: renmingshuai @ 2023-06-07 4:19 UTC (permalink / raw)
To: pctammela
Cc: caowangbao, davem, edumazet, jhs, jiri, kuba, liaichun,
linux-kernel, liubo335, netdev, pabeni, renmingshuai,
xiyou.wangcong, yanan
>On 06/06/2023 11:45, renmingshuai wrote:
>> When a new chain is added by using tc, one soft lockup alarm will be
>> generated after delete the prio 0 filter of the chain. To reproduce
>> the problem, perform the following steps:
>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
>> (2) tc chain add dev eth0
>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
>> (4) tc filter add dev eth0 chain 0 parent 1:
>
>This seems like it could be added to tdc or 3 and 4 must be run in parallel?
3 and 4 do not need to be run inparallel. When a new chain is added by the
way as step 1 and the step 3 is completed, this problem always occurs
whenever step 4 is run.
>>
>>
>> The refcnt of the chain added by step 2 is equal to 1. After step 3,
>> the flushing flag of the chain is set to true in the tcf_chain_flush()
>> called by tc_del_tfilter() because the prio is 0. In this case, if
>> we add a new filter to this chain, it will never succeed and try again
>> and again because the refresh flash is always true and refcnt is 1.
>> A soft lock alarm is generated 20 seconds later.
>> The stack is show as below:
>>
>> Kernel panic - not syncing: softlockup: hung tasks
>> CPU: 2 PID: 3321861 Comm: tc Kdump: loaded Tainted: G
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>> Call Trace:
>> <IRQ>
>> dump_stack+0x57/0x6e
>> panic+0x196/0x3ec
>> watchdog_timer_fn.cold+0x16/0x5c
>> __run_hrtimer+0x5e/0x190
>> __hrtimer_run_queues+0x8a/0xe0
>> hrtimer_interrupt+0x110/0x2c0
>> ? irqtime_account_irq+0x49/0xf0
>> __sysvec_apic_timer_interrupt+0x5f/0xe0
>> asm_call_irq_on_stack+0x12/0x20
>> </IRQ>
>> sysvec_apic_timer_interrupt+0x72/0x80
>> asm_sysvec_apic_timer_interrupt+0x12/0x20
>> RIP: 0010:mutex_lock+0x24/0x70
>> RSP: 0018:ffffa836004ab9a8 EFLAGS: 00000246
>> RAX: 0000000000000000 RBX: ffff95bb02d76700 RCX: 0000000000000000
>> RDX: ffff95bb27462100 RSI: 0000000000000000 RDI: ffff95ba5b527000
>> RBP: ffff95ba5b527000 R08: 0000000000000001 R09: ffffa836004abbb8
>> R10: 000000000000000f R11: 0000000000000000 R12: 0000000000000000
>> R13: ffff95ba5b527000 R14: ffffa836004abbb8 R15: 0000000000000001
>> __tcf_chain_put+0x27/0x200
>> tc_new_tfilter+0x5e8/0x810
>> ? tc_setup_cb_add+0x210/0x210
>> rtnetlink_rcv_msg+0x2e3/0x380
>> ? rtnl_calcit.isra.0+0x120/0x120
>> netlink_rcv_skb+0x50/0x100
>> netlink_unicast+0x12d/0x1d0
>> netlink_sendmsg+0x286/0x490
>> sock_sendmsg+0x62/0x70
>> ____sys_sendmsg+0x24c/0x2c0
>> ? import_iovec+0x17/0x20
>> ? sendmsg_copy_msghdr+0x80/0xa0
>> ___sys_sendmsg+0x75/0xc0
>> ? do_fault_around+0x118/0x160
>> ? do_read_fault+0x68/0xf0
>> ? __handle_mm_fault+0x3f9/0x6f0
>> __sys_sendmsg+0x59/0xa0
>> do_syscall_64+0x33/0x40
>> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>> RIP: 0033:0x7f96705b8247
>> RSP: 002b:00007ffe552e9dc8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f96705b8247
>> RDX: 0000000000000000 RSI: 00007ffe552e9e40 RDI: 0000000000000003
>> RBP: 0000000000000001 R08: 0000000000000001 R09: 0000558113f678b0
>> R10: 00007f967069ab00 R11: 0000000000000246 R12: 00000000647ea089
>> R13: 00007ffe552e9f30 R14: 0000000000000001 R15: 0000558113175f00
>>
>> To avoid this case, set chain->flushing to be false if the chain->refcnt
>> is 1 after flushing the chain when prio is 0.
>>
>> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
>> Signed-off-by: Ren Mingshuai <renmingshuai@huawei.com>
>> ---
>> net/sched/cls_api.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 2621550bfddc..68be55d75831 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>> tfilter_notify_chain(net, skb, block, q, parent, n,
>> chain, RTM_DELTFILTER, extack);
>> tcf_chain_flush(chain, rtnl_held);
>> + /* Set the flushing flags to false to prevent an infinite loop
>> + * when a new filter is added.
>> + */
>> + mutex_lock(&chain->filter_chain_lock);
>> + if (chain->refcnt == 1)
>> + chain->flushing = false;
>> + mutex_unlock(&chain->filter_chain_lock);
>> err = 0;
>> goto errout;
>> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net/sched: Set the flushing flags to false to prevent an infinite loop
2023-06-07 4:19 ` renmingshuai
@ 2023-06-07 15:02 ` Pedro Tammela
2023-06-08 12:32 ` [PATCH v2] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc renmingshuai
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Tammela @ 2023-06-07 15:02 UTC (permalink / raw)
To: renmingshuai
Cc: caowangbao, davem, edumazet, jhs, jiri, kuba, liaichun,
linux-kernel, liubo335, netdev, pabeni, xiyou.wangcong, yanan
On 07/06/2023 01:19, renmingshuai wrote:
>> On 06/06/2023 11:45, renmingshuai wrote:
>>> When a new chain is added by using tc, one soft lockup alarm will be
>>> generated after delete the prio 0 filter of the chain. To reproduce
>>> the problem, perform the following steps:
>>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
>>> (2) tc chain add dev eth0
>>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
>>> (4) tc filter add dev eth0 chain 0 parent 1:
>>
>> This seems like it could be added to tdc or 3 and 4 must be run in parallel?
> 3 and 4 do not need to be run inparallel. When a new chain is added by the
> way as step 1 and the step 3 is completed, this problem always occurs
> whenever step 4 is run.
Got it,
The test still hangs with the provided patch.
+ tc qdisc add dev lo root handle 1: htb default 1
+ tc chain add dev lo
+ tc filter del dev lo chain 0 parent 1: prio 0
[ 68.790030][ T6704] [+]
[ 68.790060][ T6704] chain refcnt 2
[ 68.790951][ T6704] [-]
+ tc filter add dev lo chain 0 parent 1:
<hangs>
Also please add this test to tdc, it should be straightforward.
>>>
>>>
>>> The refcnt of the chain added by step 2 is equal to 1. After step 3,
>>> the flushing flag of the chain is set to true in the tcf_chain_flush()
>>> called by tc_del_tfilter() because the prio is 0. In this case, if
>>> we add a new filter to this chain, it will never succeed and try again
>>> and again because the refresh flash is always true and refcnt is 1.
>>> A soft lock alarm is generated 20 seconds later.
>>> The stack is show as below:
>>>
>>> Kernel panic - not syncing: softlockup: hung tasks
>>> CPU: 2 PID: 3321861 Comm: tc Kdump: loaded Tainted: G
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>> Call Trace:
>>> <IRQ>
>>> dump_stack+0x57/0x6e
>>> panic+0x196/0x3ec
>>> watchdog_timer_fn.cold+0x16/0x5c
>>> __run_hrtimer+0x5e/0x190
>>> __hrtimer_run_queues+0x8a/0xe0
>>> hrtimer_interrupt+0x110/0x2c0
>>> ? irqtime_account_irq+0x49/0xf0
>>> __sysvec_apic_timer_interrupt+0x5f/0xe0
>>> asm_call_irq_on_stack+0x12/0x20
>>> </IRQ>
>>> sysvec_apic_timer_interrupt+0x72/0x80
>>> asm_sysvec_apic_timer_interrupt+0x12/0x20
>>> RIP: 0010:mutex_lock+0x24/0x70
>>> RSP: 0018:ffffa836004ab9a8 EFLAGS: 00000246
>>> RAX: 0000000000000000 RBX: ffff95bb02d76700 RCX: 0000000000000000
>>> RDX: ffff95bb27462100 RSI: 0000000000000000 RDI: ffff95ba5b527000
>>> RBP: ffff95ba5b527000 R08: 0000000000000001 R09: ffffa836004abbb8
>>> R10: 000000000000000f R11: 0000000000000000 R12: 0000000000000000
>>> R13: ffff95ba5b527000 R14: ffffa836004abbb8 R15: 0000000000000001
>>> __tcf_chain_put+0x27/0x200
>>> tc_new_tfilter+0x5e8/0x810
>>> ? tc_setup_cb_add+0x210/0x210
>>> rtnetlink_rcv_msg+0x2e3/0x380
>>> ? rtnl_calcit.isra.0+0x120/0x120
>>> netlink_rcv_skb+0x50/0x100
>>> netlink_unicast+0x12d/0x1d0
>>> netlink_sendmsg+0x286/0x490
>>> sock_sendmsg+0x62/0x70
>>> ____sys_sendmsg+0x24c/0x2c0
>>> ? import_iovec+0x17/0x20
>>> ? sendmsg_copy_msghdr+0x80/0xa0
>>> ___sys_sendmsg+0x75/0xc0
>>> ? do_fault_around+0x118/0x160
>>> ? do_read_fault+0x68/0xf0
>>> ? __handle_mm_fault+0x3f9/0x6f0
>>> __sys_sendmsg+0x59/0xa0
>>> do_syscall_64+0x33/0x40
>>> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>>> RIP: 0033:0x7f96705b8247
>>> RSP: 002b:00007ffe552e9dc8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>>> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f96705b8247
>>> RDX: 0000000000000000 RSI: 00007ffe552e9e40 RDI: 0000000000000003
>>> RBP: 0000000000000001 R08: 0000000000000001 R09: 0000558113f678b0
>>> R10: 00007f967069ab00 R11: 0000000000000246 R12: 00000000647ea089
>>> R13: 00007ffe552e9f30 R14: 0000000000000001 R15: 0000558113175f00
>>>
>>> To avoid this case, set chain->flushing to be false if the chain->refcnt
>>> is 1 after flushing the chain when prio is 0.
>>>
>>> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
>>> Signed-off-by: Ren Mingshuai <renmingshuai@huawei.com>
>>> ---
>>> net/sched/cls_api.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>> index 2621550bfddc..68be55d75831 100644
>>> --- a/net/sched/cls_api.c
>>> +++ b/net/sched/cls_api.c
>>> @@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>> tfilter_notify_chain(net, skb, block, q, parent, n,
>>> chain, RTM_DELTFILTER, extack);
>>> tcf_chain_flush(chain, rtnl_held);
>>> + /* Set the flushing flags to false to prevent an infinite loop
>>> + * when a new filter is added.
>>> + */
>>> + mutex_lock(&chain->filter_chain_lock);
>>> + if (chain->refcnt == 1)
>>> + chain->flushing = false;
>>> + mutex_unlock(&chain->filter_chain_lock);
>>> err = 0;
>>> goto errout;
>>> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc
2023-06-07 15:02 ` Pedro Tammela
@ 2023-06-08 12:32 ` renmingshuai
2023-06-08 16:08 ` Pedro Tammela
0 siblings, 1 reply; 8+ messages in thread
From: renmingshuai @ 2023-06-08 12:32 UTC (permalink / raw)
To: pctammela
Cc: caowangbao, davem, edumazet, jhs, jiri, kuba, liaichun,
linux-kernel, liubo335, netdev, pabeni, renmingshuai,
xiyou.wangcong, yanan
>On 07/06/2023 01:19, renmingshuai wrote:
>>> On 06/06/2023 11:45, renmingshuai wrote:
>>>> When a new chain is added by using tc, one soft lockup alarm will
>>>> be
>>>> generated after delete the prio 0 filter of the chain. To
>>>> reproduce
>>>> the problem, perform the following steps:
>>>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
>>>> (2) tc chain add dev eth0
>>>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
>>>> (4) tc filter add dev eth0 chain 0 parent 1:
>>>
>>> This seems like it could be added to tdc or 3 and 4 must be run in
>>> parallel?
>> 3 and 4 do not need to be run inparallel. When a new chain is added
>> by the
>> way as step 1 and the step 3 is completed, this problem always
>> occurs
>> whenever step 4 is run.
>
>Got it,
>The test still hangs with the provided patch.
>
>+ tc qdisc add dev lo root handle 1: htb default 1
>+ tc chain add dev lo
>+ tc filter del dev lo chain 0 parent 1: prio 0
>[ 68.790030][ T6704] [+]
>[ 68.790060][ T6704] chain refcnt 2
>[ 68.790951][ T6704] [-]
>+ tc filter add dev lo chain 0 parent 1:
><hangs>
>
>Also please add this test to tdc, it should be straightforward.
>
Sorry for not testing before. I forgot that the chain->refcnt was
increased by 1 when tcf_chain_get() is called in tc_del_tfilter().
The value of chain->refcnt is 2 after chain flush. The test
result is as follows:
[root@localhost ~]# tc qdisc add dev eth2 root handle 1: htb default 1
[root@localhost ~]# tc chain add dev eth2
[root@localhost ~]# tc filter del dev eth2 chain 0 parent 1: prio 0
[root@localhost ~]# tc filter add dev eth2 chain 0 parent 1:
Error: Filter kind and protocol must be specified.
We have an error talking to the kernel
And I have add this test to tdc:
[root@localhost tc-testing]# ./tdc.py -f tc-tests/filters/tests.json
ok 7 c2b4 - Adding a new fiter after deleting a filter in a chain does
not cause an infinite loop
Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
Signed-off-by: renmingshuai <renmingshuai@huawei.com>
---
net/sched/cls_api.c | 7 +++++
.../tc-testing/tc-tests/filters/tests.json | 26 ++++++++++++++++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2621550bfddc..3ea054e03fbf 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tfilter_notify_chain(net, skb, block, q, parent, n,
chain, RTM_DELTFILTER, extack);
tcf_chain_flush(chain, rtnl_held);
+ /* Set the flushing flags to false to prevent an infinite loop
+ * when a new filter is added.
+ */
+ mutex_lock(&chain->filter_chain_lock);
+ if (chain->refcnt == 2)
+ chain->flushing = false;
+ mutex_unlock(&chain->filter_chain_lock);
err = 0;
goto errout;
}
diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
index 361235ad574b..f165ca091109 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
@@ -125,5 +125,25 @@
"teardown": [
"$TC qdisc del dev $DEV2 ingress"
]
+ },
+ {
+ "id": "c2b4",
+ "name": "Adding a new fiter after deleting a filter in a chain does not cause an infinite loop",
+ "category": [
+ "filter",
+ "prio"
+ ],
+ "setup": [
+ "$TC qdisc add dev $DEV1 root handle 1: htb default 1",
+ "$TC chain add dev $DEV1"
+ ],
+ "cmdUnderTest": "$TC filter del dev $DEV1 chain 0 parent 1: prio 0",
+ "expExitCode": "0",
+ "verifyCmd": "$TC filter add dev $DEV1 chain 0 parent 1:",
+ "matchPattern": "Error: Filter kind and protocol must be specified.",
+ "matchCount": "1",
+ "teardown": [
+ "$TC qdisc del dev $DEV1 root handle 1: htb default 1"
+ ]
}
]
--
2.27.0
>>>>
>>>>
>>>> The refcnt of the chain added by step 2 is equal to 1. After step
>>>> 3,
>>>> the flushing flag of the chain is set to true in the
>>>> tcf_chain_flush()
>>>> called by tc_del_tfilter() because the prio is 0. In this case,
>>>> if
>>>> we add a new filter to this chain, it will never succeed and try
>>>> again
>>>> and again because the refresh flash is always true and refcnt is
>>>> 1.
>>>> A soft lock alarm is generated 20 seconds later.
>>>> The stack is show as below:
>>>>
>>>> Kernel panic - not syncing: softlockup: hung tasks
>>>> CPU: 2 PID: 3321861 Comm: tc Kdump: loaded Tainted: G
>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>>> Call Trace:
>>>> <IRQ>
>>>> dump_stack+0x57/0x6e
>>>> panic+0x196/0x3ec
>>>> watchdog_timer_fn.cold+0x16/0x5c
>>>> __run_hrtimer+0x5e/0x190
>>>> __hrtimer_run_queues+0x8a/0xe0
>>>> hrtimer_interrupt+0x110/0x2c0
>>>> ? irqtime_account_irq+0x49/0xf0
>>>> __sysvec_apic_timer_interrupt+0x5f/0xe0
>>>> asm_call_irq_on_stack+0x12/0x20
>>>> </IRQ>
>>>> sysvec_apic_timer_interrupt+0x72/0x80
>>>> asm_sysvec_apic_timer_interrupt+0x12/0x20
>>>> RIP: 0010:mutex_lock+0x24/0x70
>>>> RSP: 0018:ffffa836004ab9a8 EFLAGS: 00000246
>>>> RAX: 0000000000000000 RBX: ffff95bb02d76700 RCX: 0000000000000000
>>>> RDX: ffff95bb27462100 RSI: 0000000000000000 RDI: ffff95ba5b527000
>>>> RBP: ffff95ba5b527000 R08: 0000000000000001 R09: ffffa836004abbb8
>>>> R10: 000000000000000f R11: 0000000000000000 R12: 0000000000000000
>>>> R13: ffff95ba5b527000 R14: ffffa836004abbb8 R15: 0000000000000001
>>>> __tcf_chain_put+0x27/0x200
>>>> tc_new_tfilter+0x5e8/0x810
>>>> ? tc_setup_cb_add+0x210/0x210
>>>> rtnetlink_rcv_msg+0x2e3/0x380
>>>> ? rtnl_calcit.isra.0+0x120/0x120
>>>> netlink_rcv_skb+0x50/0x100
>>>> netlink_unicast+0x12d/0x1d0
>>>> netlink_sendmsg+0x286/0x490
>>>> sock_sendmsg+0x62/0x70
>>>> ____sys_sendmsg+0x24c/0x2c0
>>>> ? import_iovec+0x17/0x20
>>>> ? sendmsg_copy_msghdr+0x80/0xa0
>>>> ___sys_sendmsg+0x75/0xc0
>>>> ? do_fault_around+0x118/0x160
>>>> ? do_read_fault+0x68/0xf0
>>>> ? __handle_mm_fault+0x3f9/0x6f0
>>>> __sys_sendmsg+0x59/0xa0
>>>> do_syscall_64+0x33/0x40
>>>> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>>>> RIP: 0033:0x7f96705b8247
>>>> RSP: 002b:00007ffe552e9dc8 EFLAGS: 00000246 ORIG_RAX:
>>>> 000000000000002e
>>>> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f96705b8247
>>>> RDX: 0000000000000000 RSI: 00007ffe552e9e40 RDI: 0000000000000003
>>>> RBP: 0000000000000001 R08: 0000000000000001 R09: 0000558113f678b0
>>>> R10: 00007f967069ab00 R11: 0000000000000246 R12: 00000000647ea089
>>>> R13: 00007ffe552e9f30 R14: 0000000000000001 R15: 0000558113175f00
>>>>
>>>> To avoid this case, set chain->flushing to be false if the
>>>> chain->refcnt
>>>> is 1 after flushing the chain when prio is 0.
>>>>
>>>> Fixes: 726d061286ce ("net: sched: prevent insertion of new
>>>> classifiers during chain flush")
>>>> Signed-off-by: Ren Mingshuai <renmingshuai@huawei.com>
>>>> ---
>>>> net/sched/cls_api.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>>> index 2621550bfddc..68be55d75831 100644
>>>> --- a/net/sched/cls_api.c
>>>> +++ b/net/sched/cls_api.c
>>>> @@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff
>>>> *skb, struct nlmsghdr *n,
>>>> tfilter_notify_chain(net, skb, block, q, parent,
>>>> n,
>>>> chain, RTM_DELTFILTER,
>>>> extack);
>>>> tcf_chain_flush(chain, rtnl_held);
>>>> + /* Set the flushing flags to false to prevent an
>>>> infinite loop
>>>> + * when a new filter is added.
>>>> + */
>>>> + mutex_lock(&chain->filter_chain_lock);
>>>> + if (chain->refcnt == 1)
>>>> + chain->flushing = false;
>>>> + mutex_unlock(&chain->filter_chain_lock);
>>>> err = 0;
>>>> goto errout;
>>>> }
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc
2023-06-08 12:32 ` [PATCH v2] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc renmingshuai
@ 2023-06-08 16:08 ` Pedro Tammela
2023-06-09 3:31 ` [PATCH v3] " renmingshuai
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Tammela @ 2023-06-08 16:08 UTC (permalink / raw)
To: renmingshuai
Cc: caowangbao, davem, edumazet, jhs, jiri, kuba, liaichun,
linux-kernel, liubo335, netdev, pabeni, xiyou.wangcong, yanan
On 08/06/2023 09:32, renmingshuai wrote:
>> On 07/06/2023 01:19, renmingshuai wrote:
>>>> On 06/06/2023 11:45, renmingshuai wrote:
>>>>> When a new chain is added by using tc, one soft lockup alarm will
>>>>> be
>>>>> generated after delete the prio 0 filter of the chain. To
>>>>> reproduce
>>>>> the problem, perform the following steps:
>>>>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
>>>>> (2) tc chain add dev eth0
>>>>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
>>>>> (4) tc filter add dev eth0 chain 0 parent 1:
>>>>
>>>> This seems like it could be added to tdc or 3 and 4 must be run in
>>>> parallel?
>>> 3 and 4 do not need to be run inparallel. When a new chain is added
>>> by the
>>> way as step 1 and the step 3 is completed, this problem always
>>> occurs
>>> whenever step 4 is run.
>>
>> Got it,
>> The test still hangs with the provided patch.
>>
>> + tc qdisc add dev lo root handle 1: htb default 1
>> + tc chain add dev lo
>> + tc filter del dev lo chain 0 parent 1: prio 0
>> [ 68.790030][ T6704] [+]
>> [ 68.790060][ T6704] chain refcnt 2
>> [ 68.790951][ T6704] [-]
>> + tc filter add dev lo chain 0 parent 1:
>> <hangs>
>>
>> Also please add this test to tdc, it should be straightforward.
>>
> Sorry for not testing before. I forgot that the chain->refcnt was
> increased by 1 when tcf_chain_get() is called in tc_del_tfilter().
> The value of chain->refcnt is 2 after chain flush. The test
> result is as follows:
> [root@localhost ~]# tc qdisc add dev eth2 root handle 1: htb default 1
> [root@localhost ~]# tc chain add dev eth2
> [root@localhost ~]# tc filter del dev eth2 chain 0 parent 1: prio 0
> [root@localhost ~]# tc filter add dev eth2 chain 0 parent 1:
> Error: Filter kind and protocol must be specified.
> We have an error talking to the kernel
>
> And I have add this test to tdc:
> [root@localhost tc-testing]# ./tdc.py -f tc-tests/filters/tests.json
> ok 7 c2b4 - Adding a new fiter after deleting a filter in a chain does
> not cause an infinite loop
>
> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
> Signed-off-by: renmingshuai <renmingshuai@huawei.com>
Please respin with the following applied:
diff --git
a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
index c759c3db9a37..361235ad574b 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
@@ -125,25 +125,5 @@
"teardown": [
"$TC qdisc del dev $DEV2 ingress"
]
- },
- {
- "id": "c2b4",
- "name": "Adding a new fiter after deleting a filter in a chain
does not cause an infinite loop",
- "category": [
- "filter",
- "prio"
- ],
- "setup": [
- "$TC qdisc add dev $DEV1 root handle 1: htb default 1",
- "$TC chain add dev $DEV1"
- ],
- "cmdUnderTest": "$TC filter del dev $DEV1 chain 0 parent 1:
prio 0",
- "expExitCode": "0",
- "verifyCmd": "$TC filter add dev $DEV1 chain 0 parent 1:",
- "matchPattern": "Error: Filter kind and protocol must be
specified.",
- "matchCount": "1",
- "teardown": [
- "$TC qdisc del dev $DEV1 root handle 1: htb default 1"
- ]
}
]
diff --git
a/tools/testing/selftests/tc-testing/tc-tests/infra/filters.json
b/tools/testing/selftests/tc-testing/tc-tests/infra/filters.
json
new file mode 100644
index 000000000000..55d6f209c388
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/filters.json
@@ -0,0 +1,24 @@
+[
+ {
+ "id": "c2b4",
+ "name": "Adding a new filter after flushing empty chain doesnt
cause an infinite loop",
+ "category": [
+ "filter",
+ "chain"
+ ],
+ "setup": [
+ "$IP link add dev $DUMMY type dummy || /bin/true",
+ "$TC qdisc add dev $DUMMY root handle 1: htb default 1",
+ "$TC chain add dev $DUMMY",
+ "$TC filter del dev $DUMMY chain 0 parent 1: prio 0"
+ ],
+ "cmdUnderTest": "$TC filter add dev $DUMMY chain 0 parent 1:",
+ "expExitCode": "2",
+ "verifyCmd": "$TC chain ls dev $DUMMY",
+ "matchPattern": "chain parent 1: chain 0",
+ "matchCount": "1",
+ "teardown": [
+ "$TC qdisc del dev $DUMMY root handle 1: htb default 1"
+ ]
+ }
+]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc
2023-06-08 16:08 ` Pedro Tammela
@ 2023-06-09 3:31 ` renmingshuai
2023-06-09 8:24 ` Simon Horman
0 siblings, 1 reply; 8+ messages in thread
From: renmingshuai @ 2023-06-09 3:31 UTC (permalink / raw)
To: pctammela
Cc: caowangbao, davem, edumazet, jhs, jiri, kuba, liaichun,
linux-kernel, liubo335, netdev, pabeni, renmingshuai,
xiyou.wangcong, yanan
>On 08/06/2023 09:32, renmingshuai wrote:
>>> On 07/06/2023 01:19, renmingshuai wrote:
>>>>> On 06/06/2023 11:45, renmingshuai wrote:
>>>>>> When a new chain is added by using tc, one soft lockup alarm will
>>>>>> be
>>>>>> generated after delete the prio 0 filter of the chain. To
>>>>>> reproduce
>>>>>> the problem, perform the following steps:
>>>>>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
>>>>>> (2) tc chain add dev eth0
>>>>>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
>>>>>> (4) tc filter add dev eth0 chain 0 parent 1:
>>>>>
>>>>> This seems like it could be added to tdc or 3 and 4 must be run in
>>>>> parallel?
>>>> 3 and 4 do not need to be run inparallel. When a new chain is added
>>>> by the
>>>> way as step 1 and the step 3 is completed, this problem always
>>>> occurs
>>>> whenever step 4 is run.
>>>
>>> Got it,
>>> The test still hangs with the provided patch.
>>>
>>> + tc qdisc add dev lo root handle 1: htb default 1
>>> + tc chain add dev lo
>>> + tc filter del dev lo chain 0 parent 1: prio 0
>>> [ 68.790030][ T6704] [+]
>>> [ 68.790060][ T6704] chain refcnt 2
>>> [ 68.790951][ T6704] [-]
>>> + tc filter add dev lo chain 0 parent 1:
>>> <hangs>
>>>
>>> Also please add this test to tdc, it should be straightforward.
>>>
>> Sorry for not testing before. I forgot that the chain->refcnt was
>> increased by 1 when tcf_chain_get() is called in tc_del_tfilter().
>> The value of chain->refcnt is 2 after chain flush. The test
>> result is as follows:
>> [root@localhost ~]# tc qdisc add dev eth2 root handle 1: htb default 1
>> [root@localhost ~]# tc chain add dev eth2
>> [root@localhost ~]# tc filter del dev eth2 chain 0 parent 1: prio 0
>> [root@localhost ~]# tc filter add dev eth2 chain 0 parent 1:
>> Error: Filter kind and protocol must be specified.
>> We have an error talking to the kernel
>>
>> And I have add this test to tdc:
>> [root@localhost tc-testing]# ./tdc.py -f tc-tests/filters/tests.json
>> ok 7 c2b4 - Adding a new fiter after deleting a filter in a chain does
>> not cause an infinite loop
>>
>> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
>> Signed-off-by: renmingshuai <renmingshuai@huawei.com>
>
>Please respin with the following applied:
>
>diff --git
>a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
>b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
>index c759c3db9a37..361235ad574b 100644
>--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
>+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
>@@ -125,25 +125,5 @@
> "teardown": [
> "$TC qdisc del dev $DEV2 ingress"
> ]
>- },
>- {
>- "id": "c2b4",
>- "name": "Adding a new fiter after deleting a filter in a chain
>does not cause an infinite loop",
>- "category": [
>- "filter",
>- "prio"
>- ],
>- "setup": [
>- "$TC qdisc add dev $DEV1 root handle 1: htb default 1",
>- "$TC chain add dev $DEV1"
>- ],
>- "cmdUnderTest": "$TC filter del dev $DEV1 chain 0 parent 1:
>prio 0",
>- "expExitCode": "0",
>- "verifyCmd": "$TC filter add dev $DEV1 chain 0 parent 1:",
>- "matchPattern": "Error: Filter kind and protocol must be
>specified.",
>- "matchCount": "1",
>- "teardown": [
>- "$TC qdisc del dev $DEV1 root handle 1: htb default 1"
>- ]
> }
> ]
>diff --git
>a/tools/testing/selftests/tc-testing/tc-tests/infra/filters.json
>b/tools/testing/selftests/tc-testing/tc-tests/infra/filters.
>json
>new file mode 100644
>index 000000000000..55d6f209c388
>--- /dev/null
>+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/filters.json
>@@ -0,0 +1,24 @@
>+[
>+ {
>+ "id": "c2b4",
>+ "name": "Adding a new filter after flushing empty chain doesnt
>cause an infinite loop",
>+ "category": [
>+ "filter",
>+ "chain"
>+ ],
>+ "setup": [
>+ "$IP link add dev $DUMMY type dummy || /bin/true",
>+ "$TC qdisc add dev $DUMMY root handle 1: htb default 1",
>+ "$TC chain add dev $DUMMY",
>+ "$TC filter del dev $DUMMY chain 0 parent 1: prio 0"
>+ ],
>+ "cmdUnderTest": "$TC filter add dev $DUMMY chain 0 parent 1:",
>+ "expExitCode": "2",
>+ "verifyCmd": "$TC chain ls dev $DUMMY",
>+ "matchPattern": "chain parent 1: chain 0",
>+ "matchCount": "1",
>+ "teardown": [
>+ "$TC qdisc del dev $DUMMY root handle 1: htb default 1"
>+ ]
>+ }
>+]
Ok. The new test is passed.
[root@localhost tc-testing]# ./tdc.py -f tc-tests/infra/filter.json
Test c2b4: Adding a new filter after flushing empty chain doesn't cause an infinite loop
All test results:
1..1
ok 1 c2b4 - Adding a new filter after flushing empty chain doesn't cause an infinite loop
Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
Signed-off-by: renmingshuai <renmingshuai@huawei.com>
---
net/sched/cls_api.c | 7 ++++++
.../tc-testing/tc-tests/infra/filter.json | 25 +++++++++++++++++++
2 files changed, 32 insertions(+)
create mode 100644 tools/testing/selftests/tc-testing/tc-tests/infra/filter.json
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2621550bfddc..3ea054e03fbf 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2442,6 +2442,13 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tfilter_notify_chain(net, skb, block, q, parent, n,
chain, RTM_DELTFILTER, extack);
tcf_chain_flush(chain, rtnl_held);
+ /* Set the flushing flags to false to prevent an infinite loop
+ * when a new filter is added.
+ */
+ mutex_lock(&chain->filter_chain_lock);
+ if (chain->refcnt == 2)
+ chain->flushing = false;
+ mutex_unlock(&chain->filter_chain_lock);
err = 0;
goto errout;
}
diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json b/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json
new file mode 100644
index 000000000000..db3b42aaa4fa
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/filter.json
@@ -0,0 +1,25 @@
+[
+ {
+ "id": "c2b4",
+ "name": "Adding a new filter after flushing empty chain doesn't cause an infinite loop",
+ "category": [
+ "filter",
+ "chain"
+ ],
+ "setup": [
+ "$IP link add dev $DUMMY type dummy || /bin/true",
+ "$TC qdisc add dev $DUMMY root handle 1: htb default 1",
+ "$TC chain add dev $DUMMY",
+ "$TC filter del dev $DUMMY chain 0 parent 1: prio 0"
+ ],
+ "cmdUnderTest": "$TC filter add dev $DUMMY chain 0 parent 1:",
+ "expExitCode": "2",
+ "verifyCmd": "$TC chain ls dev $DUMMY",
+ "matchPattern": "chain parent 1: chain 0",
+ "matchCount": "1",
+ "teardown": [
+ "$TC qdisc del dev $DUMMY root handle 1: htb default 1",
+ "$IP link del dev $DUMMY type dummy"
+ ]
+ }
+]
--
2.27.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc
2023-06-09 3:31 ` [PATCH v3] " renmingshuai
@ 2023-06-09 8:24 ` Simon Horman
0 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2023-06-09 8:24 UTC (permalink / raw)
To: renmingshuai
Cc: pctammela, caowangbao, davem, edumazet, jhs, jiri, kuba,
liaichun, linux-kernel, liubo335, netdev, pabeni, xiyou.wangcong,
yanan
On Fri, Jun 09, 2023 at 11:31:15AM +0800, renmingshuai wrote:
> >On 08/06/2023 09:32, renmingshuai wrote:
> >>> On 07/06/2023 01:19, renmingshuai wrote:
> >>>>> On 06/06/2023 11:45, renmingshuai wrote:
> >>>>>> When a new chain is added by using tc, one soft lockup alarm will
> >>>>>> be
> >>>>>> generated after delete the prio 0 filter of the chain. To
> >>>>>> reproduce
> >>>>>> the problem, perform the following steps:
> >>>>>> (1) tc qdisc add dev eth0 root handle 1: htb default 1
> >>>>>> (2) tc chain add dev eth0
> >>>>>> (3) tc filter del dev eth0 chain 0 parent 1: prio 0
> >>>>>> (4) tc filter add dev eth0 chain 0 parent 1:
> >>>>>
> >>>>> This seems like it could be added to tdc or 3 and 4 must be run in
> >>>>> parallel?
> >>>> 3 and 4 do not need to be run inparallel. When a new chain is added
> >>>> by the
> >>>> way as step 1 and the step 3 is completed, this problem always
> >>>> occurs
> >>>> whenever step 4 is run.
> >>>
> >>> Got it,
> >>> The test still hangs with the provided patch.
> >>>
> >>> + tc qdisc add dev lo root handle 1: htb default 1
> >>> + tc chain add dev lo
> >>> + tc filter del dev lo chain 0 parent 1: prio 0
> >>> [ 68.790030][ T6704] [+]
> >>> [ 68.790060][ T6704] chain refcnt 2
> >>> [ 68.790951][ T6704] [-]
> >>> + tc filter add dev lo chain 0 parent 1:
> >>> <hangs>
> >>>
> >>> Also please add this test to tdc, it should be straightforward.
> >>>
> >> Sorry for not testing before. I forgot that the chain->refcnt was
> >> increased by 1 when tcf_chain_get() is called in tc_del_tfilter().
> >> The value of chain->refcnt is 2 after chain flush. The test
> >> result is as follows:
> >> [root@localhost ~]# tc qdisc add dev eth2 root handle 1: htb default 1
> >> [root@localhost ~]# tc chain add dev eth2
> >> [root@localhost ~]# tc filter del dev eth2 chain 0 parent 1: prio 0
> >> [root@localhost ~]# tc filter add dev eth2 chain 0 parent 1:
> >> Error: Filter kind and protocol must be specified.
> >> We have an error talking to the kernel
> >>
> >> And I have add this test to tdc:
> >> [root@localhost tc-testing]# ./tdc.py -f tc-tests/filters/tests.json
> >> ok 7 c2b4 - Adding a new fiter after deleting a filter in a chain does
> >> not cause an infinite loop
> >>
> >> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
> >> Signed-off-by: renmingshuai <renmingshuai@huawei.com>
> >
> >Please respin with the following applied:
> >
> >diff --git
> >a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> >b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> >index c759c3db9a37..361235ad574b 100644
> >--- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> >+++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json
> >@@ -125,25 +125,5 @@
> > "teardown": [
> > "$TC qdisc del dev $DEV2 ingress"
> > ]
> >- },
> >- {
> >- "id": "c2b4",
> >- "name": "Adding a new fiter after deleting a filter in a chain
> >does not cause an infinite loop",
> >- "category": [
> >- "filter",
> >- "prio"
> >- ],
> >- "setup": [
> >- "$TC qdisc add dev $DEV1 root handle 1: htb default 1",
> >- "$TC chain add dev $DEV1"
> >- ],
> >- "cmdUnderTest": "$TC filter del dev $DEV1 chain 0 parent 1:
> >prio 0",
> >- "expExitCode": "0",
> >- "verifyCmd": "$TC filter add dev $DEV1 chain 0 parent 1:",
> >- "matchPattern": "Error: Filter kind and protocol must be
> >specified.",
> >- "matchCount": "1",
> >- "teardown": [
> >- "$TC qdisc del dev $DEV1 root handle 1: htb default 1"
> >- ]
> > }
> > ]
> >diff --git
> >a/tools/testing/selftests/tc-testing/tc-tests/infra/filters.json
> >b/tools/testing/selftests/tc-testing/tc-tests/infra/filters.
> >json
> >new file mode 100644
> >index 000000000000..55d6f209c388
> >--- /dev/null
> >+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/filters.json
> >@@ -0,0 +1,24 @@
> >+[
> >+ {
> >+ "id": "c2b4",
> >+ "name": "Adding a new filter after flushing empty chain doesnt
> >cause an infinite loop",
> >+ "category": [
> >+ "filter",
> >+ "chain"
> >+ ],
> >+ "setup": [
> >+ "$IP link add dev $DUMMY type dummy || /bin/true",
> >+ "$TC qdisc add dev $DUMMY root handle 1: htb default 1",
> >+ "$TC chain add dev $DUMMY",
> >+ "$TC filter del dev $DUMMY chain 0 parent 1: prio 0"
> >+ ],
> >+ "cmdUnderTest": "$TC filter add dev $DUMMY chain 0 parent 1:",
> >+ "expExitCode": "2",
> >+ "verifyCmd": "$TC chain ls dev $DUMMY",
> >+ "matchPattern": "chain parent 1: chain 0",
> >+ "matchCount": "1",
> >+ "teardown": [
> >+ "$TC qdisc del dev $DUMMY root handle 1: htb default 1"
> >+ ]
> >+ }
> >+]
>
> Ok. The new test is passed.
> [root@localhost tc-testing]# ./tdc.py -f tc-tests/infra/filter.json
> Test c2b4: Adding a new filter after flushing empty chain doesn't cause an infinite loop
> All test results:
> 1..1
> ok 1 c2b4 - Adding a new filter after flushing empty chain doesn't cause an infinite loop
>
> Fixes: 726d061286ce ("net: sched: prevent insertion of new classifiers during chain flush")
> Signed-off-by: renmingshuai <renmingshuai@huawei.com>
Hi renmingshuai,
the above text, in it's entirety, does not meet the needs
of a patch submission for the Linux kernel. In particular:
* All the context from previous discussion (the bits prefixed by '>')
should be removed.
* The patch description should consist of just that, a description of
the patch. Any relevant test results are also welcome here.
* Information about changes from previous versions should be provided
in the form of a changelog, typically below the scissors (---).
A recent example is here:
- https://lore.kernel.org/all/20230607162353.3631199-1-mtottenh@akamai.com/
[PATCH v3] net/sched: act_pedit: Parse L3 Header for L4 offset
Also, please consider using spaces and capital letters
in your name in the from and signed-off parts of your submission.
e.g.: Mingshuai Ren <...> or Ren Mingshuai <...>
...
--
pw-bot: cr
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-09 8:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-06 14:45 [PATCH] net/sched: Set the flushing flags to false to prevent an infinite loop renmingshuai
2023-06-06 16:59 ` Pedro Tammela
2023-06-07 4:19 ` renmingshuai
2023-06-07 15:02 ` Pedro Tammela
2023-06-08 12:32 ` [PATCH v2] net/sched: Set the flushing flags to false to prevent an infinite loop and add one test to tdc renmingshuai
2023-06-08 16:08 ` Pedro Tammela
2023-06-09 3:31 ` [PATCH v3] " renmingshuai
2023-06-09 8:24 ` Simon Horman
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).