linux-kernel - Re: [PATCH] net/sched: netem: account for backlog updates from child qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce09216d-ccb2-4cf3-8c68-4de468411db5@fau.de>
Date: Wed, 4 Dec 2024 12:47:15 +0100
From: Martin Ottens <martin.ottens@....de>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Stephen Hemminger <stephen@...workplumber.org>,
 Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
 Jiri Pirko <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net/sched: netem: account for backlog updates from child
 qdisc

On 03.12.24 04:13, Jakub Kicinski wrote:
> I don't understand why we need to perform packet accounting 
> in a separate new member (t_len). You seem to fix qlen accounting,
> anyway, and I think sch->limit should apply to the qdisc and all
> its children. Not just qdisc directly (since most classful qdiscs
> don't hold packets).

Netem is a classful qdisc but different from others because it holds 
packets in its internal tfifo and optional additionally in a child 
qdisc. However, sch->limit currently only considers the packets in 
the tfifo and not the packets hold by a child, but child qdiscs 
expect this value to refer to the number of packets that are in 
netem and all its children together. If the children change this 
value (using 'qdisc_tree_reduce_backlog'), then the number of 
packets in the tfifo no longer matches sch->limit.
By adding t_len, the number of packets in the tfifo will be tracked 
independently from sch->limit therefore sch->limit can be changes 
by children without unwanted behavior. t_len is required, because 
currently the limit option of netem refers to the maximum number 
of packets in the tfifo - therefore the behavior of netem is not
changed by this patch.

> I'm not a qdisc expert, so if you feel confident about this code you
> need to explain the thinking in the commit message..

With the patch I try to fix the error without changing the behavior 
of netem (e.g., change the meaning of the limit option to apply to 
the tfifo length and the packets in the child qdisc). Maybe there 
are even better approaches - I am happy about any feedback. I will 
revise the explanation in the patch to make this clearer and 
resubmit it.