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 |
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?` ...
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
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>
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 --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)
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(+)