diff mbox series

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

Message ID 20250306232355.93864-2-xiyou.wangcong@gmail.com (mailing list archive)
State Accepted
Commit 0c3057a5a04d07120b3d0ec9c79568fceb9c921e
Delegated to: Netdev Maintainers
Headers show
Series net_sched: Prevent creation of classes with TC_H_ROOT | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: horms@kernel.org kuba@kernel.org pabeni@redhat.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 93 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-03-07--03-00 (tests: 894)

Commit Message

Cong Wang March 6, 2025, 11:23 p.m. UTC
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(+)

Comments

Simon Horman March 11, 2025, 10:48 a.m. UTC | #1
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?`

...
Paolo Abeni March 11, 2025, 12:47 p.m. UTC | #2
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
Simon Horman March 11, 2025, 4:04 p.m. UTC | #3
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>
Cong Wang March 11, 2025, 8:07 p.m. UTC | #4
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!
diff mbox series

Patch

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)