[Patch net 0/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Patch net 0/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Cong Wang]

This patchset contains a bug fix and its TDC test case.

---

Cong Wang (2):
  net_sched: Prevent creation of classes with TC_H_ROOT
  selftests/tc-testing: Add a test case for DRR class with TC_H_ROOT

 net/sched/sch_api.c                           |  6 +++++
 .../tc-testing/tc-tests/qdiscs/drr.json       | 25 +++++++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.34.1

[Patch net 1/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Cong Wang]

The function qdisc_tree_reduce_backlog() uses TC_H_ROOT as a termination
condition when traversing up the qdisc tree to update parent backlog
counters. However, if a class is created with classid TC_H_ROOT, the
traversal terminates prematurely at this class instead of reaching the
actual root qdisc, causing parent statistics to be incorrectly maintained.
In case of DRR, this could lead to a crash as reported by Mingi Cho.

Prevent the creation of any Qdisc class with classid TC_H_ROOT
(0xFFFFFFFF) across all qdisc types, as suggested by Jamal.

Reported-by: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_api.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e3e91cf867eb..6c625dcd0651 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -2254,6 +2254,12 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
 		return -EOPNOTSUPP;
 	}
 
+	/* Prevent creation of traffic classes with classid TC_H_ROOT */
+	if (clid == TC_H_ROOT) {
+		NL_SET_ERR_MSG(extack, "Cannot create traffic class with classid TC_H_ROOT");
+		return -EINVAL;
+	}
+
 	new_cl = cl;
 	err = -EOPNOTSUPP;
 	if (cops->change)
-- 
2.34.1

Re: [Patch net 1/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Simon Horman]

On Thu, Mar 06, 2025 at 03:23:54PM -0800, Cong Wang wrote:
> The function qdisc_tree_reduce_backlog() uses TC_H_ROOT as a termination
> condition when traversing up the qdisc tree to update parent backlog
> counters. However, if a class is created with classid TC_H_ROOT, the
> traversal terminates prematurely at this class instead of reaching the
> actual root qdisc, causing parent statistics to be incorrectly maintained.
> In case of DRR, this could lead to a crash as reported by Mingi Cho.
> 
> Prevent the creation of any Qdisc class with classid TC_H_ROOT
> (0xFFFFFFFF) across all qdisc types, as suggested by Jamal.
> 
> Reported-by: Mingi Cho <mincho@theori.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Hi Cong,

This change looks good to me.
But could we get a fixes tag?`

...

Re: [Patch net 1/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Paolo Abeni]

On 3/11/25 11:48 AM, Simon Horman wrote:
> On Thu, Mar 06, 2025 at 03:23:54PM -0800, Cong Wang wrote:
>> The function qdisc_tree_reduce_backlog() uses TC_H_ROOT as a termination
>> condition when traversing up the qdisc tree to update parent backlog
>> counters. However, if a class is created with classid TC_H_ROOT, the
>> traversal terminates prematurely at this class instead of reaching the
>> actual root qdisc, causing parent statistics to be incorrectly maintained.
>> In case of DRR, this could lead to a crash as reported by Mingi Cho.
>>
>> Prevent the creation of any Qdisc class with classid TC_H_ROOT
>> (0xFFFFFFFF) across all qdisc types, as suggested by Jamal.
>>
>> Reported-by: Mingi Cho <mincho@theori.io>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> 
> Hi Cong,
> 
> This change looks good to me.
> But could we get a fixes tag?`
> 
> ...

Should be:

Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix
qdisc_tree_decrease_qlen() loop")

right?

/P

Re: [Patch net 1/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Simon Horman]

On Tue, Mar 11, 2025 at 01:47:32PM +0100, Paolo Abeni wrote:
> 
> 
> On 3/11/25 11:48 AM, Simon Horman wrote:
> > On Thu, Mar 06, 2025 at 03:23:54PM -0800, Cong Wang wrote:
> >> The function qdisc_tree_reduce_backlog() uses TC_H_ROOT as a termination
> >> condition when traversing up the qdisc tree to update parent backlog
> >> counters. However, if a class is created with classid TC_H_ROOT, the
> >> traversal terminates prematurely at this class instead of reaching the
> >> actual root qdisc, causing parent statistics to be incorrectly maintained.
> >> In case of DRR, this could lead to a crash as reported by Mingi Cho.
> >>
> >> Prevent the creation of any Qdisc class with classid TC_H_ROOT
> >> (0xFFFFFFFF) across all qdisc types, as suggested by Jamal.
> >>
> >> Reported-by: Mingi Cho <mincho@theori.io>
> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > 
> > Hi Cong,
> > 
> > This change looks good to me.
> > But could we get a fixes tag?`
> > 
> > ...
> 
> Should be:
> 
> Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix
> qdisc_tree_decrease_qlen() loop")

Thanks.

Looking at that, I might have gone for the following commit,
which is a fix for the above one. But either way is fine by me.

commit 2e95c4384438 ("net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT")

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [Patch net 1/2] net_sched: Prevent creation of classes with TC_H_ROOT

[Cong Wang]

On Tue, Mar 11, 2025 at 05:04:49PM +0100, Simon Horman wrote:
> On Tue, Mar 11, 2025 at 01:47:32PM +0100, Paolo Abeni wrote:
> > 
> > 
> > On 3/11/25 11:48 AM, Simon Horman wrote:
> > > On Thu, Mar 06, 2025 at 03:23:54PM -0800, Cong Wang wrote:
> > >> The function qdisc_tree_reduce_backlog() uses TC_H_ROOT as a termination
> > >> condition when traversing up the qdisc tree to update parent backlog
> > >> counters. However, if a class is created with classid TC_H_ROOT, the
> > >> traversal terminates prematurely at this class instead of reaching the
> > >> actual root qdisc, causing parent statistics to be incorrectly maintained.
> > >> In case of DRR, this could lead to a crash as reported by Mingi Cho.
> > >>
> > >> Prevent the creation of any Qdisc class with classid TC_H_ROOT
> > >> (0xFFFFFFFF) across all qdisc types, as suggested by Jamal.
> > >>
> > >> Reported-by: Mingi Cho <mincho@theori.io>
> > >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > > 
> > > Hi Cong,
> > > 
> > > This change looks good to me.
> > > But could we get a fixes tag?`
> > > 
> > > ...
> > 
> > Should be:
> > 
> > Fixes: 066a3b5b2346 ("[NET_SCHED] sch_api: fix
> > qdisc_tree_decrease_qlen() loop")
> 
> Thanks.
> 
> Looking at that, I might have gone for the following commit,
> which is a fix for the above one. But either way is fine by me.
> 
> commit 2e95c4384438 ("net/sched: stop qdisc_tree_reduce_backlog on TC_H_ROOT")

Indeed it is. Sorry for forgetting about it.

Thanks a lot!

[Patch net 2/2] selftests/tc-testing: Add a test case for DRR class with TC_H_ROOT

[Cong Wang]

Integrate the reproduer from Mingi to TDC.

All test results:

1..4
ok 1 0385 - Create DRR with default setting
ok 2 2375 - Delete DRR with handle
ok 3 3092 - Show DRR class
ok 4 4009 - Reject creation of DRR class with classid TC_H_ROOT

Cc: Mingi Cho <mincho@theori.io>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/qdiscs/drr.json       | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json
index 7126ec3485cb..2b61d8d79bde 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/drr.json
@@ -61,5 +61,30 @@
         "teardown": [
             "$TC qdisc del dev $DUMMY handle 1: root"
         ]
+    },
+    {
+        "id": "4009",
+        "name": "Reject creation of DRR class with classid TC_H_ROOT",
+        "category": [
+            "qdisc",
+            "drr"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$TC qdisc add dev $DUMMY root handle ffff: drr",
+            "$TC filter add dev $DUMMY parent ffff: basic classid ffff:1",
+            "$TC class add dev $DUMMY parent ffff: classid ffff:1 drr",
+            "$TC filter add dev $DUMMY parent ffff: prio 1 u32 match u16 0x0000 0xfe00 at 2 flowid ffff:ffff"
+        ],
+        "cmdUnderTest": "$TC class add dev $DUMMY parent ffff: classid ffff:ffff drr",
+        "expExitCode": "2",
+        "verifyCmd": "$TC class show dev $DUMMY",
+        "matchPattern": "class drr ffff:ffff",
+        "matchCount": "0",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY root"
+        ]
     }
 ]
-- 
2.34.1

Re: [Patch net 2/2] selftests/tc-testing: Add a test case for DRR class with TC_H_ROOT

[Simon Horman]

On Thu, Mar 06, 2025 at 03:23:55PM -0800, Cong Wang wrote:
> Integrate the reproduer from Mingi to TDC.
> 
> All test results:
> 
> 1..4
> ok 1 0385 - Create DRR with default setting
> ok 2 2375 - Delete DRR with handle
> ok 3 3092 - Show DRR class
> ok 4 4009 - Reject creation of DRR class with classid TC_H_ROOT
> 
> Cc: Mingi Cho <mincho@theori.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [Patch net 0/2] net_sched: Prevent creation of classes with TC_H_ROOT

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  6 Mar 2025 15:23:53 -0800 you wrote:
> This patchset contains a bug fix and its TDC test case.
> 
> ---
> 
> Cong Wang (2):
>   net_sched: Prevent creation of classes with TC_H_ROOT
>   selftests/tc-testing: Add a test case for DRR class with TC_H_ROOT
> 
> [...]

Here is the summary with links:
  - [net,1/2] net_sched: Prevent creation of classes with TC_H_ROOT
    https://git.kernel.org/netdev/net/c/0c3057a5a04d
  - [net,2/2] selftests/tc-testing: Add a test case for DRR class with TC_H_ROOT
    https://git.kernel.org/netdev/net/c/bb7737de5f59

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html