[PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Davide Caratti]

Shuang reported sch_ets test-case [1] crashing in ets_class_qlen_notify()
after recent changes from Lion [2]. The problem is: in ets_qdisc_change()
we purge unused DWRR queues; the value of 'q->nbands' is the new one, and
the cleanup should be done with the old one. The problem is here since my
first attempts to fix ets_qdisc_change(), but it surfaced again after the
recent qdisc len accounting fixes. Fix it purging idle DWRR queues before
assigning a new value of 'q->nbands', so that all purge operations find a
consistent configuration:

 - old 'q->nbands' because it's needed by ets_class_find()
 - old 'q->nstrict' because it's needed by ets_class_is_strict()

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: Oops: 0000 [#1] SMP NOPTI
 CPU: 62 UID: 0 PID: 39457 Comm: tc Kdump: loaded Not tainted 6.12.0-116.el10.x86_64 #1 PREEMPT(voluntary)
 Hardware name: Dell Inc. PowerEdge R640/06DKY5, BIOS 2.12.2 07/09/2021
 RIP: 0010:__list_del_entry_valid_or_report+0x4/0x80
 Code: ff 4c 39 c7 0f 84 39 19 8e ff b8 01 00 00 00 c3 cc cc cc cc 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa <48> 8b 17 48 8b 4f 08 48 85 d2 0f 84 56 19 8e ff 48 85 c9 0f 84 ab
 RSP: 0018:ffffba186009f400 EFLAGS: 00010202
 RAX: 00000000000000d6 RBX: 0000000000000000 RCX: 0000000000000004
 RDX: ffff9f0fa29b69c0 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: ffffffffc12c2400 R08: 0000000000000008 R09: 0000000000000004
 R10: ffffffffffffffff R11: 0000000000000004 R12: 0000000000000000
 R13: ffff9f0f8cfe0000 R14: 0000000000100005 R15: 0000000000000000
 FS:  00007f2154f37480(0000) GS:ffff9f269c1c0000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000000000000 CR3: 00000001530be001 CR4: 00000000007726f0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 PKRU: 55555554
 Call Trace:
  <TASK>
  ets_class_qlen_notify+0x65/0x90 [sch_ets]
  qdisc_tree_reduce_backlog+0x74/0x110
  ets_qdisc_change+0x630/0xa40 [sch_ets]
  __tc_modify_qdisc.constprop.0+0x216/0x7f0
  tc_modify_qdisc+0x7c/0x120
  rtnetlink_rcv_msg+0x145/0x3f0
  netlink_rcv_skb+0x53/0x100
  netlink_unicast+0x245/0x390
  netlink_sendmsg+0x21b/0x470
  ____sys_sendmsg+0x39d/0x3d0
  ___sys_sendmsg+0x9a/0xe0
  __sys_sendmsg+0x7a/0xd0
  do_syscall_64+0x7d/0x160
  entry_SYSCALL_64_after_hwframe+0x76/0x7e
 RIP: 0033:0x7f2155114084
 Code: 89 02 b8 ff ff ff ff eb bb 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 80 3d 25 f0 0c 00 00 74 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 89 54 24 1c 48 89
 RSP: 002b:00007fff1fd7a988 EFLAGS: 00000202 ORIG_RAX: 000000000000002e
 RAX: ffffffffffffffda RBX: 0000560ec063e5e0 RCX: 00007f2155114084
 RDX: 0000000000000000 RSI: 00007fff1fd7a9f0 RDI: 0000000000000003
 RBP: 00007fff1fd7aa60 R08: 0000000000000010 R09: 000000000000003f
 R10: 0000560ee9b3a010 R11: 0000000000000202 R12: 00007fff1fd7aae0
 R13: 000000006891ccde R14: 0000560ec063e5e0 R15: 00007fff1fd7aad0
  </TASK>

 [1] https://lore.kernel.org/netdev/e08c7f4a6882f260011909a868311c6e9b54f3e4.1639153474.git.dcaratti@redhat.com/
 [2] https://lore.kernel.org/netdev/d912cbd7-193b-4269-9857-525bee8bbb6a@gmail.com/

Fixes: 103406b38c60 ("net/sched: Always pass notifications when child class becomes empty")
Fixes: c062f2a0b04d ("net/sched: sch_ets: don't remove idle classes from the round-robin list")
Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
Reported-by: Li Shuang <shuali@redhat.com>
Closes: https://issues.redhat.com/browse/RHEL-108026
Co-developed-by: Ivan Vecera <ivecera@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/sch_ets.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index 037f764822b9..82635dd2cfa5 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -651,6 +651,12 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
 
 	sch_tree_lock(sch);
 
+	for (i = nbands; i < oldbands; i++) {
+		if (i >= q->nstrict && q->classes[i].qdisc->q.qlen)
+			list_del_init(&q->classes[i].alist);
+		qdisc_purge_queue(q->classes[i].qdisc);
+	}
+
 	WRITE_ONCE(q->nbands, nbands);
 	for (i = nstrict; i < q->nstrict; i++) {
 		if (q->classes[i].qdisc->q.qlen) {
@@ -658,11 +664,6 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
 			q->classes[i].deficit = quanta[i];
 		}
 	}
-	for (i = q->nbands; i < oldbands; i++) {
-		if (i >= q->nstrict && q->classes[i].qdisc->q.qlen)
-			list_del_init(&q->classes[i].alist);
-		qdisc_purge_queue(q->classes[i].qdisc);
-	}
 	WRITE_ONCE(q->nstrict, nstrict);
 	memcpy(q->prio2band, priomap, sizeof(priomap));
 
-- 
2.47.0

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Petr Machata]

Davide Caratti <dcaratti@redhat.com> writes:

> Shuang reported sch_ets test-case [1] crashing in ets_class_qlen_notify()
> after recent changes from Lion [2]. The problem is: in ets_qdisc_change()
> we purge unused DWRR queues; the value of 'q->nbands' is the new one, and
> the cleanup should be done with the old one. The problem is here since my
> first attempts to fix ets_qdisc_change(), but it surfaced again after the
> recent qdisc len accounting fixes. Fix it purging idle DWRR queues before
> assigning a new value of 'q->nbands', so that all purge operations find a
> consistent configuration:

First let me just note: that is one sharp paragraph edge!

>  - old 'q->nbands' because it's needed by ets_class_find()

Comes from qdisc_purge_queue() calls qdisc_tree_reduce_backlog() calls
Qdisc_class_ops.find, which is ets_class_find().

>  - old 'q->nstrict' because it's needed by ets_class_is_strict()

From qdisc_purge_queue() calls qdisc_tree_reduce_backlog() calls
Qdisc_class_ops.qlen_notify, which is ets_class_qlen_notify() calls
ets_class_is_strict().

Also qdisc_purge_queue() calls qdisc_reset() calls Qdisc_ops.reset which
is ets_qdisc_reset(), and that accesses both fields.

>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  PGD 0 P4D 0
>  Oops: Oops: 0000 [#1] SMP NOPTI
>  CPU: 62 UID: 0 PID: 39457 Comm: tc Kdump: loaded Not tainted 6.12.0-116.el10.x86_64 #1 PREEMPT(voluntary)
>  Hardware name: Dell Inc. PowerEdge R640/06DKY5, BIOS 2.12.2 07/09/2021
>  RIP: 0010:__list_del_entry_valid_or_report+0x4/0x80
>  Code: ff 4c 39 c7 0f 84 39 19 8e ff b8 01 00 00 00 c3 cc cc cc cc 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa <48> 8b 17 48 8b 4f 08 48 85 d2 0f 84 56 19 8e ff 48 85 c9 0f 84 ab
>  RSP: 0018:ffffba186009f400 EFLAGS: 00010202
>  RAX: 00000000000000d6 RBX: 0000000000000000 RCX: 0000000000000004
>  RDX: ffff9f0fa29b69c0 RSI: 0000000000000000 RDI: 0000000000000000
>  RBP: ffffffffc12c2400 R08: 0000000000000008 R09: 0000000000000004
>  R10: ffffffffffffffff R11: 0000000000000004 R12: 0000000000000000
>  R13: ffff9f0f8cfe0000 R14: 0000000000100005 R15: 0000000000000000
>  FS:  00007f2154f37480(0000) GS:ffff9f269c1c0000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000000000000 CR3: 00000001530be001 CR4: 00000000007726f0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  PKRU: 55555554
>  Call Trace:
>   <TASK>
>   ets_class_qlen_notify+0x65/0x90 [sch_ets]
>   qdisc_tree_reduce_backlog+0x74/0x110
>   ets_qdisc_change+0x630/0xa40 [sch_ets]
>   __tc_modify_qdisc.constprop.0+0x216/0x7f0
>   tc_modify_qdisc+0x7c/0x120
>   rtnetlink_rcv_msg+0x145/0x3f0
>   netlink_rcv_skb+0x53/0x100
>   netlink_unicast+0x245/0x390
>   netlink_sendmsg+0x21b/0x470
>   ____sys_sendmsg+0x39d/0x3d0
>   ___sys_sendmsg+0x9a/0xe0
>   __sys_sendmsg+0x7a/0xd0
>   do_syscall_64+0x7d/0x160
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>  RIP: 0033:0x7f2155114084
>  Code: 89 02 b8 ff ff ff ff eb bb 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 80 3d 25 f0 0c 00 00 74 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 89 54 24 1c 48 89
>  RSP: 002b:00007fff1fd7a988 EFLAGS: 00000202 ORIG_RAX: 000000000000002e
>  RAX: ffffffffffffffda RBX: 0000560ec063e5e0 RCX: 00007f2155114084
>  RDX: 0000000000000000 RSI: 00007fff1fd7a9f0 RDI: 0000000000000003
>  RBP: 00007fff1fd7aa60 R08: 0000000000000010 R09: 000000000000003f
>  R10: 0000560ee9b3a010 R11: 0000000000000202 R12: 00007fff1fd7aae0
>  R13: 000000006891ccde R14: 0000560ec063e5e0 R15: 00007fff1fd7aad0
>   </TASK>
>
>  [1] https://lore.kernel.org/netdev/e08c7f4a6882f260011909a868311c6e9b54f3e4.1639153474.git.dcaratti@redhat.com/
>  [2] https://lore.kernel.org/netdev/d912cbd7-193b-4269-9857-525bee8bbb6a@gmail.com/
>
> Fixes: 103406b38c60 ("net/sched: Always pass notifications when child class becomes empty")
> Fixes: c062f2a0b04d ("net/sched: sch_ets: don't remove idle classes from the round-robin list")
> Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
> Reported-by: Li Shuang <shuali@redhat.com>
> Closes: https://issues.redhat.com/browse/RHEL-108026
> Co-developed-by: Ivan Vecera <ivecera@redhat.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>

Reviewed-by: Petr Machata <petrm@nvidia.com>

> ---
>  net/sched/sch_ets.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index 037f764822b9..82635dd2cfa5 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c
> @@ -651,6 +651,12 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
>  
>  	sch_tree_lock(sch);
>  
> +	for (i = nbands; i < oldbands; i++) {
> +		if (i >= q->nstrict && q->classes[i].qdisc->q.qlen)
> +			list_del_init(&q->classes[i].alist);
> +		qdisc_purge_queue(q->classes[i].qdisc);
> +	}
> +
>  	WRITE_ONCE(q->nbands, nbands);
>  	for (i = nstrict; i < q->nstrict; i++) {
>  		if (q->classes[i].qdisc->q.qlen) {
> @@ -658,11 +664,6 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
>  			q->classes[i].deficit = quanta[i];
>  		}
>  	}
> -	for (i = q->nbands; i < oldbands; i++) {
> -		if (i >= q->nstrict && q->classes[i].qdisc->q.qlen)
> -			list_del_init(&q->classes[i].alist);
> -		qdisc_purge_queue(q->classes[i].qdisc);
> -	}
>  	WRITE_ONCE(q->nstrict, nstrict);
>  	memcpy(q->prio2band, priomap, sizeof(priomap));

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Victor Nogueira]

On 8/7/25 12:48, Davide Caratti wrote:
> Shuang reported sch_ets test-case [1] crashing in ets_class_qlen_notify()
> after recent changes from Lion [2]. The problem is: in ets_qdisc_change()
> we purge unused DWRR queues; the value of 'q->nbands' is the new one, and
> the cleanup should be done with the old one. The problem is here since my
> first attempts to fix ets_qdisc_change(), but it surfaced again after the
> recent qdisc len accounting fixes. Fix it purging idle DWRR queues before
> assigning a new value of 'q->nbands', so that all purge operations find a
> consistent configuration:
> 
>   - old 'q->nbands' because it's needed by ets_class_find()
>   - old 'q->nstrict' because it's needed by ets_class_is_strict()
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   #PF: supervisor read access in kernel mode
>   #PF: error_code(0x0000) - not-present page
>   PGD 0 P4D 0
>   Oops: Oops: 0000 [#1] SMP NOPTI
>   CPU: 62 UID: 0 PID: 39457 Comm: tc Kdump: loaded Not tainted 6.12.0-116.el10.x86_64 #1 PREEMPT(voluntary)
>   Hardware name: Dell Inc. PowerEdge R640/06DKY5, BIOS 2.12.2 07/09/2021
>   RIP: 0010:__list_del_entry_valid_or_report+0x4/0x80
>   Code: ff 4c 39 c7 0f 84 39 19 8e ff b8 01 00 00 00 c3 cc cc cc cc 66 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa <48> 8b 17 48 8b 4f 08 48 85 d2 0f 84 56 19 8e ff 48 85 c9 0f 84 ab
>   RSP: 0018:ffffba186009f400 EFLAGS: 00010202
>   RAX: 00000000000000d6 RBX: 0000000000000000 RCX: 0000000000000004
>   RDX: ffff9f0fa29b69c0 RSI: 0000000000000000 RDI: 0000000000000000
>   RBP: ffffffffc12c2400 R08: 0000000000000008 R09: 0000000000000004
>   R10: ffffffffffffffff R11: 0000000000000004 R12: 0000000000000000
>   R13: ffff9f0f8cfe0000 R14: 0000000000100005 R15: 0000000000000000
>   FS:  00007f2154f37480(0000) GS:ffff9f269c1c0000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 0000000000000000 CR3: 00000001530be001 CR4: 00000000007726f0
>   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>   DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>   PKRU: 55555554
>   Call Trace:
>    <TASK>
>    ets_class_qlen_notify+0x65/0x90 [sch_ets]
>    qdisc_tree_reduce_backlog+0x74/0x110
>    ets_qdisc_change+0x630/0xa40 [sch_ets]
>    __tc_modify_qdisc.constprop.0+0x216/0x7f0
>    tc_modify_qdisc+0x7c/0x120
>    rtnetlink_rcv_msg+0x145/0x3f0
>    netlink_rcv_skb+0x53/0x100
>    netlink_unicast+0x245/0x390
>    netlink_sendmsg+0x21b/0x470
>    ____sys_sendmsg+0x39d/0x3d0
>    ___sys_sendmsg+0x9a/0xe0
>    __sys_sendmsg+0x7a/0xd0
>    do_syscall_64+0x7d/0x160
>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>   RIP: 0033:0x7f2155114084
>   Code: 89 02 b8 ff ff ff ff eb bb 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 80 3d 25 f0 0c 00 00 74 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 89 54 24 1c 48 89
>   RSP: 002b:00007fff1fd7a988 EFLAGS: 00000202 ORIG_RAX: 000000000000002e
>   RAX: ffffffffffffffda RBX: 0000560ec063e5e0 RCX: 00007f2155114084
>   RDX: 0000000000000000 RSI: 00007fff1fd7a9f0 RDI: 0000000000000003
>   RBP: 00007fff1fd7aa60 R08: 0000000000000010 R09: 000000000000003f
>   R10: 0000560ee9b3a010 R11: 0000000000000202 R12: 00007fff1fd7aae0
>   R13: 000000006891ccde R14: 0000560ec063e5e0 R15: 00007fff1fd7aad0
>    </TASK>
> 
>   [1] https://lore.kernel.org/netdev/e08c7f4a6882f260011909a868311c6e9b54f3e4.1639153474.git.dcaratti@redhat.com/
>   [2] https://lore.kernel.org/netdev/d912cbd7-193b-4269-9857-525bee8bbb6a@gmail.com/
> 
> Fixes: 103406b38c60 ("net/sched: Always pass notifications when child class becomes empty")
> Fixes: c062f2a0b04d ("net/sched: sch_ets: don't remove idle classes from the round-robin list")
> Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
> Reported-by: Li Shuang <shuali@redhat.com>
> Closes: https://issues.redhat.com/browse/RHEL-108026
> Co-developed-by: Ivan Vecera <ivecera@redhat.com>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>

Can you submit a tdc test case for this bug?

cheers,
Victor

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Davide Caratti]

On Fri, Aug 08, 2025 at 03:15:13PM -0300, Victor Nogueira wrote:
> On 8/7/25 12:48, Davide Caratti wrote:

[...]
 
> > Fixes: 103406b38c60 ("net/sched: Always pass notifications when child class becomes empty")
> > Fixes: c062f2a0b04d ("net/sched: sch_ets: don't remove idle classes from the round-robin list")
> > Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
> > Reported-by: Li Shuang <shuali@redhat.com>
> > Closes: https://issues.redhat.com/browse/RHEL-108026
> > Co-developed-by: Ivan Vecera <ivecera@redhat.com>
> > Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> 
> Can you submit a tdc test case for this bug?

hello Victor,

Thanks for looking at this!

At a first look: TDC is not the correct tool here, because it doesn't
allow changing the qdisc tree while the scapy plugin emits traffic.
Maybe it's better to extend sch_ets.sh from net/forwarding instead?

If so, I can follow-up on net-next with a patch that adds a new
test-case that includes the 3-lines in [1] - while this patch can go
as-is in 'net' (and eventually in stable). In alternative, I can
investigate on TDC adding "sch_plug" to the qdisc tree in a way
that DWRR never deplete, and the crash would then happen with "verifyCmd".


WDYT?

-- 
davide

[1] https://lore.kernel.org/netdev/e08c7f4a6882f260011909a868311c6e9b54f3e4.1639153474.git.dcaratti@redhat.com/
 

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Victor Nogueira]

On 8/11/25 04:49, Davide Caratti wrote:
> On Fri, Aug 08, 2025 at 03:15:13PM -0300, Victor Nogueira wrote:
>> On 8/7/25 12:48, Davide Caratti wrote:
> 
> [...]
>   
>>> Fixes: 103406b38c60 ("net/sched: Always pass notifications when child class becomes empty")
>>> Fixes: c062f2a0b04d ("net/sched: sch_ets: don't remove idle classes from the round-robin list")
>>> Fixes: dcc68b4d8084 ("net: sch_ets: Add a new Qdisc")
>>> Reported-by: Li Shuang <shuali@redhat.com>
>>> Closes: https://issues.redhat.com/browse/RHEL-108026
>>> Co-developed-by: Ivan Vecera <ivecera@redhat.com>
>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
>>
>> Can you submit a tdc test case for this bug?
> 
> hello Victor,
> 
> Thanks for looking at this!
> 
> At a first look: TDC is not the correct tool here, because it doesn't
> allow changing the qdisc tree while the scapy plugin emits traffic.

I see.

> Maybe it's better to extend sch_ets.sh from net/forwarding instead?
> If so, I can follow-up on net-next with a patch that adds a new
> test-case that includes the 3-lines in [1] - while this patch can go
> as-is in 'net' (and eventually in stable). In alternative, I can
> investigate on TDC adding "sch_plug" to the qdisc tree in a way
> that DWRR never deplete, and the crash would then happen with "verifyCmd".
> 
> WDYT?

That works for me as well.

cheers,
Victor

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Victor Nogueira]

On 8/11/25 06:53, Victor Nogueira wrote:
> On 8/11/25 04:49, Davide Caratti wrote:
>> Maybe it's better to extend sch_ets.sh from net/forwarding instead?
>> If so, I can follow-up on net-next with a patch that adds a new
>> test-case that includes the 3-lines in [1] - while this patch can go
>> as-is in 'net' (and eventually in stable). In alternative, I can
>> investigate on TDC adding "sch_plug" to the qdisc tree in a way
>> that DWRR never deplete, and the crash would then happen with 
>> "verifyCmd".
>>
>> WDYT?
> 
> That works for me as well.

Sorry, should've been more specific.
I meant that the net/forwarding approach you suggested
seems ok. The tdc approach would be a lot of work and
I don't believe it's worth it.

cheers,
Victor

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Davide Caratti]

On Mon, Aug 11, 2025 at 10:52:08AM -0300, Victor Nogueira wrote:
> On 8/11/25 06:53, Victor Nogueira wrote:
> > On 8/11/25 04:49, Davide Caratti wrote:
> > > Maybe it's better to extend sch_ets.sh from net/forwarding instead?
> > > If so, I can follow-up on net-next with a patch that adds a new
> > > test-case that includes the 3-lines in [1] - while this patch can go
> > > as-is in 'net' (and eventually in stable). In alternative, I can
> > > investigate on TDC adding "sch_plug" to the qdisc tree in a way
> > > that DWRR never deplete, and the crash would then happen with
> > > "verifyCmd".
> > > 
> > > WDYT?
> > 
> > That works for me as well.
> 
> Sorry, should've been more specific.
> I meant that the net/forwarding approach you suggested
> seems ok. The tdc approach would be a lot of work and
> I don't believe it's worth it.

I was more of the idea of avoiding a non-deterministic kselftest, because
with mausezahn running in the background we have to be "lucky" enough to
see the tc qdisc change command executed while the packet socket is
still emitting packets. And the sch_plug approach I mentioned this morning
looks doable: I just reproduced the NULL dereference on unpatched kernel
using something like:

 # ip link add name ddd0 type dummy
 # tc qdisc add dev ddd0 root handle 1: ets bands 4 strict 2 priomap 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3
 # tc qdisc add dev ddd0 handle 10: parent 1:4 plug 
 # ip link set dev ddd0 up
 # tc qdisc change dev ddd0 handle 10: plug limit 5
 # mausezahn ddd0 -A 10.10.10.1 -B 10.10.10.2 -c 0 -a own -c 5 00:c1:a0:c1:a0:00 -t udp
 # printf "press enter to crash..."
 # read -r _
 # tc qdisc change dev ddd0 handle 1: ets bands 2 strict 0

so, including "plug" children in the tree should make kselftest feasible either with 'net/forwarding'
or with TDC + scapy plugin superpowers.

@Victor + @Jakub, can we apply this patch to 'net', so that regression is fixed ASAP, and then I post
the kselftest in a separate submission for net-next?

-- 
davide

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Victor Nogueira]

On 8/11/25 13:09, Davide Caratti wrote:
> On Mon, Aug 11, 2025 at 10:52:08AM -0300, Victor Nogueira wrote:
>> On 8/11/25 06:53, Victor Nogueira wrote:
>>> On 8/11/25 04:49, Davide Caratti wrote:
>>>> Maybe it's better to extend sch_ets.sh from net/forwarding instead?
>>>> If so, I can follow-up on net-next with a patch that adds a new
>>>> test-case that includes the 3-lines in [1] - while this patch can go
>>>> as-is in 'net' (and eventually in stable). In alternative, I can
>>>> investigate on TDC adding "sch_plug" to the qdisc tree in a way
>>>> that DWRR never deplete, and the crash would then happen with
>>>> "verifyCmd".
>>>>
>>>> WDYT?
>>>
>>> That works for me as well.
>>
>> Sorry, should've been more specific.
>> I meant that the net/forwarding approach you suggested
>> seems ok. The tdc approach would be a lot of work and
>> I don't believe it's worth it.
> 
> I was more of the idea of avoiding a non-deterministic kselftest, because
> with mausezahn running in the background we have to be "lucky" enough to
> see the tc qdisc change command executed while the packet socket is
> still emitting packets. And the sch_plug approach I mentioned this morning
> looks doable: I just reproduced the NULL dereference on unpatched kernel
> using something like:
> 
>   # ip link add name ddd0 type dummy
>   # tc qdisc add dev ddd0 root handle 1: ets bands 4 strict 2 priomap 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3
>   # tc qdisc add dev ddd0 handle 10: parent 1:4 plug
>   # ip link set dev ddd0 up
>   # tc qdisc change dev ddd0 handle 10: plug limit 5
>   # mausezahn ddd0 -A 10.10.10.1 -B 10.10.10.2 -c 0 -a own -c 5 00:c1:a0:c1:a0:00 -t udp
>   # printf "press enter to crash..."
>   # read -r _
>   # tc qdisc change dev ddd0 handle 1: ets bands 2 strict 0
> 
> so, including "plug" children in the tree should make kselftest feasible either with 'net/forwarding'
> or with TDC + scapy plugin superpowers.

I see, so I think it would be better to use the 'net/forwarding' 
approach with
"plug" children mainly because it looks simpler.

> @Victor + @Jakub, can we apply this patch to 'net', so that regression is fixed ASAP, and then I post
> the kselftest in a separate submission for net-next?

 From my side, that's ok.

cheers,
Victor

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Jakub Kicinski]

On Mon, 11 Aug 2025 14:35:50 -0300 Victor Nogueira wrote:
> > @Victor + @Jakub, can we apply this patch to 'net', so that regression is fixed ASAP, and then I post
> > the kselftest in a separate submission for net-next?  
> 
>  From my side, that's ok.

Fine in principle, but is the urgency really that high?
Feels like crashes in qdiscs with net-admin are dime a dozen,
we can wait a day or two for a test and merge them together..

Re: [PATCH net] net/sched: ets: use old 'nbands' while purging unused classes

[Davide Caratti]

On Mon, Aug 11, 2025 at 02:35:50PM -0300, Victor Nogueira wrote:
> On 8/11/25 13:09, Davide Caratti wrote:
> > On Mon, Aug 11, 2025 at 10:52:08AM -0300, Victor Nogueira wrote:
> > > On 8/11/25 06:53, Victor Nogueira wrote:
> > > > On 8/11/25 04:49, Davide Caratti wrote:

[...]
  
> > so, including "plug" children in the tree should make kselftest feasible either with 'net/forwarding'
> > or with TDC + scapy plugin superpowers.
> 
> I see, so I think it would be better to use the 'net/forwarding' approach
> with
> "plug" children mainly because it looks simpler.

AFAIS the problem with TDC scapy plugin is: it doesn't work well with nsPlugin.
At the moment it can only inject packets in $DEV0 transmit, assuming that $DEV1
will do something with received packets. So, some TC mirred trickery is needed to
test qdiscs (or alternatively, qdiscs with traffic should be tested in the main
namespace using $DEV0 - but this is not friendly to other tests)

(some mirred trickery be like: adding 1 tc-mirred line at the end of "setup")

    {
        "id": "1027",
        "name": "purge DWRR classes with non-empty backlog",
        "category": [
            "qdisc",
            "ets"
        ],
        "plugins": {
            "requires": [
                "nsPlugin",
                "scapyPlugin"
            ]
        },
        "setup": [
            "$TC qdisc add dev $DUMMY root handle 1: ets bands 4 strict 2 priomap 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3 3",
            "$TC qdisc add dev $DUMMY handle 10: parent 1:4 plug",
            "$TC qdisc add dev $DEV1 clsact",
            "$TC filter add dev $DEV1 ingress protocol ip matchall action mirred egress redirect dev $DUMMY"
        ],
        "scapy": [
            {
                "iface": "$DEV0",
                "count": 1,
                "packet": "Ether(type=0x800)/IP(src='10.10.10.1',dst='10.10.10.2')/UDP(sport=5000,dport=10)"
            }
        ],
        "cmdUnderTest": "true",
        "expExitCode": "0",
        "verifyCmd": "$TC qdisc change dev $DUMMY handle 1: ets bands 2 strict 0",
        "matchPattern": "match_pattern_not_relevant",
        "matchCount": "0",
        "teardown": []
    }


yes, net/forwarding should take less lines: will post a patch in the next hours. thanks,

-- 
davide